Add ApplicationName to options and use it to prefix the HubName

[steverash]

Added a HubPrefix property to the options class.

This allows it to be configured to prefix every HubName with a prefix and therefore allows applications to share the same Azure SignalR service without worrying about multiple applications having the same hub names.

#348

[msftclas]

All CLA requirements met.

[vicancy]

Hi @steverash ,really appreciate your effort on improving the SDK. For this particular feature, we tend to follow the initial proposal in #348 to leverage the appName input parameter considering:

1. appName is already a required parameter for ASP.NET support, introducing that concept into the ASP.NET Core one as an optional parameter seems consistent and easy to understand.
2. We'd also like to add an appName parameter into REST API endpoint. If usingHubPrefix, users who directly call the REST API would be aware that the actual hub name is HubPrefix + HubName. The appName approach sounds comparatively straight-forward.

Please just let us know if you have any concerns about the proposal.

[kroymann]

@vicancy I'm not familiar with the appName parameter that you are referring to. Aside from being named differently than HubPrefix, how would this behavior differ from what I proposed (and what @steverash then implemented)? I'm pretty sure the only thing that both he and I care about is the ability to inject an arbitrary string into the ASRS configuration that is then used to distinguish/isolate hub connections that share the same HubName. This could be the same app running in different environments (prod/test/dev/etc), or it could be multiple different applications that happen to use the same name HubName.

[vicancy]

applicatonName is a required parameter when using the SDK for ASP.NET SignalR

azure-signalr/src/Microsoft.Azure.SignalR.AspNet/OwinExtensions.cs

Line 22 in a101b43

public static IAppBuilder MapAzureSignalR(this IAppBuilder builder, string applicationName)

Yeah, from the implementation perspective, it is similar to HubPrefix.

[steverash]

@vicancy thanks for taking the time to review this.

Is it just renaming hubPrefix to appName, but still implementing in the same way via the ServiceOptions? If so, I'm happy to go back through the code and do that?

I can see the code around the ASP.NET version, but putting in the ServiceOptions seemed like a good fit and in keeping with the ASP.NET Core patterns. The ASP.NET/Owin code seems to work in a quite different way and does not utilise the ASP.NET Core Options pattern?

I notice there is a seperate ASP.NET and Core ServiceOptions class that both implement IServiceEndpointOptions. I can see this would be confusing on the ASP.NET side as there would be 2 places in the Owin startup for AppName.

At the moment even on the ASP.NET/Owin side, which has ApplicationName, I can't see that it actually provides any isolation of the Hubs? Is ApplicationName a work in progress in that respect?

Also an important point for me is the ability to keep the signalr client routes the same whilst separating the Azure SignalR usage by the appName (hubPrefix).

If you can confirm that it is the naming needing changing (HubPrefix -> AppName) and if you'd like me to proceed, I'm happy to update the PR tomorrow.

[vicancy]

Thanks @steverash, your points make sense.

applicationName in ASP.NET is for other usage but we feel that it sounds great to promote its usage to also isolate hubs.

How about updating the PR to:

1. Rename HubPrefix to ApplicationName
2. For ASP.NET, leverage the existing applicationName

cc @chenkennt @vwxyzh @davidfowl Any comments?

[steverash]

@vicancy - I am happy with that plan.

On a slight side issue, I've just noticed that the behaviour of ASP.NET and ASP.NET Core ServiceEndpointProvider differ. On ASP.NET the client endpoints don't include the hub query string parameter, but the ASP.NET Core ones do. Is this intentional or a bug?

I'd noticed it whilst also finding that I'd failed to include the hubPrefix on ASP.NET endpoints. At the moment I've updated to only use on the server endpoints for ASP.NET.

[steverash]

I've made the change to update it to ApplicationName.

On the ASP.NET side, it was a struggle to reconcile the applicationName in the RunAzureSignalR function parameters and the new one in the options class. Given the Options class is shared between Core and ASP.NET Options.ApplicationName cannot be removed from ASP.NET easily. However, I've made the property set internal only for the ASP.NET Options.ApplicationName. I've also added an extra options property "UseHubNamePrefix" to ASP.NET. This means consumers will need to specifically opt into having the app name prefix applied on ASP.NET. I did this to preserve existing behaviour, as I was concerned that suddenly existing ASP.NET users will have hub names being unexpectedly prefixed.

Not perfect, but hopefully the best option.

[vicancy]

Looks cool, some minor comments

[steverash]

I've made the changes suggested, and I've also changed the .NET Core project to remove the ApplicationName from the ServiceEndPoint and use the IOptions to supply the ApplicationName in the ServiceEndPointProvider. I think this is better and means ApplicationName is being provided more directly from the IOptions.

[wanlwanl]

Apply ApplicationName to related tests for management sdk and fix a bug

[TimVinkemeier]

Thank you for your work, I was actually waiting for exactly this feature and have been watching this PR for a while 😃
@vicancy Any idea when it will be available via NuGet (e.g. as a preview or a nightly myget)?

[wanlwanl]

@TimVinkemeier The myget package should be the latest dev branch (we publish a new package when the dev branch changes), you can install you the package in myget. Just click the myget package link in this section, and follow the installation guide to install it.