Add a bidirectional chat sample to demonstrate SignalR Trigger #101

zackliu · 2020-03-10T06:32:36Z

Here is the C# chat sample to demonstrate new feature: SignalR Trigger
The trigger can collaborate with SignalR Service's new upstream feature smoothly to empower customer to handle messages from service in serverless mode.

Now, client can call connection.send() or connection.invoke() just like what they usually do in default mode.
Also, in C#, Function can design there own attribute to implement Authorization. In the sample, a very simple attribute that only allow the admin to call broadcast is demonstrated.

It's not the final design, please give some suggestions. And I will add a JavaScript sample later.

…-extension into dev

JialinXin · 2020-03-11T02:36:08Z

samples/bidirectional-chat/csharp/Function.cs

+
+        [FunctionAuthorize]
+        [FunctionName("broadcast")]
+        public static async Task Broadcast([SignalRTrigger(Hub, Category.Messages, "broadcast", "message")]InvocationContext invocationContext,


broadcast [](start = 84, length = 9)

How about enum messages event as well. the functionality is limited to our supported APIs.

samples/bidirectional-chat/csharp/local.settings.sample.json

chenkennt · 2020-03-11T06:30:09Z

samples/bidirectional-chat/csharp/Authorize/FunctionAuthorizeAttribute.cs

+    {
+        private const string AdminKey = "admin";
+
+        public override Task OnExecutingAsync(FunctionExecutingContext executingContext, CancellationToken cancellationToken)


Can we extract some common logic into a base class so that developer only needs to focus on the claim validation part.

That's good. I will use another PR to do that.

chenkennt · 2020-03-11T06:43:18Z

samples/bidirectional-chat/csharp/Function.cs

+
+        [FunctionAuthorize]
+        [FunctionName("broadcast")]
+        public static async Task Broadcast([SignalRTrigger(Hub, Category.Messages, "broadcast", "message")]InvocationContext invocationContext,


Is it possible to get method name from FunctionName attribute?

The function name need to be globally unique, but method name just need to be unique in hub level

samples/bidirectional-chat/csharp/Function.cs

davidfowl · 2020-03-11T06:59:06Z

The way I see it, we need to get as close to possible to a Hub as we can on with the Azure functions infrastructure and see what that looks like in multiple languages. I have some ideas of what a nice node and C# experience looks like.

zackliu · 2020-03-11T09:10:37Z

@davidfowl I've just updated the sample and applied class base on it.

Briefly, when Functions are defined in under a class that derived from ServerlessHub, many configurations work can be simplified.

[FunctionName(nameof(SendToConnection))]
public async Task SendToConnection([SignalRTrigger]InvocationContext invocationContext,
    [SignalRParameter]string connectionId,
    [SignalRParameter]string message)
{
    await Clients.Client(connectionId).SendAsync(NewMessageTarget, new NewMessage(invocationContext, message));
}

Considered the balance of convenience and correction, [SignalRParameter] is used to make it more controllable and avoid conflict with other binding's binding data.

However, all these simplify can only be applied in C#. As the limitation of Function architecture, we can do less in other language. I will then open another PR to demonstrate trigger in nodejs.

src/SignalRServiceExtension/TriggerBindings/ServerlessHub.cs

src/SignalRServiceExtension/TriggerBindings/SignalRTriggerBindingProvider.cs

samples/bidirectional-chat/csharp/Function.cs

src/SignalRServiceExtension/TriggerBindings/SignalRTriggerBindingProvider.cs

samples/bidirectional-chat/csharp/Function.cs

samples/bidirectional-chat/csharp/Authorize/FunctionAuthorizeAttribute.cs

davidfowl · 2020-03-12T15:45:24Z

Can we get rid of [SignalRParameter] and move InvocationContext to a Property on the base class?

zackliu · 2020-03-13T03:00:58Z

@davidfowl Getting rid of [SignalRParameter] makes the trigger hand to decide which parameter the customer want to bind by signar tigger. Customers usually use other input/output bindings in Azure Function. Using [SignalRParameter] is a little tedious but clear and easy for learning.
InvocationContext is directly passed to the Function method. Seems it's hard to get it in the ctor and assign to a Property.

davidfowl · 2020-03-13T04:54:37Z

They want to bring everything that isn’t already marked. Mark the things that are non signalr explicitly.

src/SignalRServiceExtension/TriggerBindings/ServerlessHub.cs

chenkennt · 2020-03-13T06:10:50Z

samples/bidirectional-chat/csharp/Function.cs

+        private const string NewConnectionTarget = "newConnection";
+
+        [FunctionName("negotiate")]
+        public static SignalRConnectionInfo GetSignalRInfo(


Can this be moved into ServerlessHub?

That's good. But it needs some investigation. I will put it into another RP.

davidfowl · 2020-03-13T06:12:04Z

samples/bidirectional-chat/csharp/Function.cs

+        }
+
+        [FunctionName(nameof(Disconnect))]
+        public void Disconnect([SignalRTrigger]InvocationContext invocationContext)


Can you call these methods explicitly?

What's your "explicitly" means?

Can these calls come from outside the signalr service?

Can the client trigger a call to disconnect and not be disconnected?

Yes, as long as the message sent to SignalR trigger can follow our serverless spec.

If using SignalR Service, it won't happen. Because all message from client's invocation will be in the messages category. And connected/disconnected are connections category

chenkennt · 2020-03-13T06:24:16Z

src/SignalRServiceExtension/TriggerBindings/SignalRParameterAttribute.cs

+    /// </summary>
+    [AttributeUsage(AttributeTargets.Parameter)]
+    public class SignalRParameterAttribute : Attribute
+    {


Also allow specify parameter name

I think it's useless. The parameter name is used to match the actual parameter name in method. It should be exactly the name it decorates. We should avoid user using like

void Function([SignalRTrigger]InvocationContext context, [SignalRParameter("arg1")]arg0, [SignalRParameter("arg0")]arg1)

It just cause confusing.

Like in JSON serialization, you can use JsonProperty to give property a different name (for example a short name). But seems SignalR itself doesn't support this as well. So it should be ok to not support it for now.

Then do we really need this? A parameter is by default recognized as a SignalR parameter.

Only in class base. It prepared for non-class based function. It's still better than using ParameterNames directly

chenkennt · 2020-03-13T06:32:26Z

src/SignalRServiceExtension/TriggerBindings/SignalRTriggerBindingProvider.cs

+                    $"{nameof(SignalRTriggerAttribute)}.{nameof(SignalRTriggerAttribute.ParameterNames)} and {nameof(SignalRParameterAttribute)} can not be set in the same Function.");
+            }
+
+            parameterNames = parameterNamesFromAttribute.Count != 0


Should check parameterNames first? parametersNames should have higher priority than attributes.

I don't want parametersNames and attribute are set together. So, there's no priority issue.

chenkennt · 2020-03-13T06:46:46Z

samples/bidirectional-chat/content/index.html

+        data.messages.unshift(message);
+      };
+      function onNewConnection(message) {
+        data.myConnectionId = message.ConnectionId;


Do we want to do a camel case conversion for class members? (ConnectionId -> connectionId) I think SignalR by default does this conversion.

JialinXin · 2020-03-13T09:22:11Z

src/SignalRServiceExtension/TriggerBindings/SignalRTriggerBindingProvider.cs

+
+        private bool IsLegalClassBasedParameter(ParameterInfo parameter)
+        {
+            // In class based model, we treat all the parameters as a legal parameter exception the cases below


exception [](start = 86, length = 9)

typo: except

JialinXin · 2020-03-13T10:52:40Z

src/SignalRServiceExtension/TriggerBindings/SignalRIgnoreAttribute.cs

+{
+    /// <summary>
+    /// In class based model, mark the parameter explicitly not to be a SignalR parameter.
+    /// That means it won't be bind to a InvocationMessage argument.


be bind [](start = 28, length = 7)

typo: bind or be bound

JialinXin

chenkennt · 2020-03-16T03:27:27Z

samples/bidirectional-chat/csharp/Function.cs

+        }
+
+        [FunctionName(nameof(Disconnect))]
+        public void Disconnect([SignalRTrigger]InvocationContext invocationContext)


Should be disconnected?

Use another PR to do that change including onconnected

chenkennt · 2020-03-16T04:06:58Z

src/SignalRServiceExtension/TriggerBindings/ServerlessHub.cs

+            UserGroups = hubContext.UserGroups;
+        }
+
+        public IHubClients Clients { get; }


How about also add them to InvocationContext?

chenkennt · 2020-03-16T04:07:53Z

samples/bidirectional-chat/csharp/Function.cs

+        }
+
+        [FunctionName(nameof(Connected))]
+        public async Task Connected([SignalRTrigger]InvocationContext invocationContext, ILogger logger)


Can you still specify hub name in SignalRTrigger?

zackliu added 4 commits March 9, 2020 20:24

Add a bi-directional chat sample

cd4afc9

stash

be8280b

Merge branch 'dev' of github.com:Azure/azure-functions-signalrservice…

fe2c0af

…-extension into dev

Merge branch 'dev' into trigger8

2519ca3

zackliu requested review from JialinXin, anthonychu, chenkennt, davidfowl, vicancy, vwxyzh and wanlwanl March 10, 2020 06:32

JialinXin reviewed Mar 11, 2020

View reviewed changes

samples/bidirectional-chat/csharp/local.settings.sample.json Outdated Show resolved Hide resolved

Merge branch 'dev' into trigger8

18dffdc

chenkennt reviewed Mar 11, 2020

View reviewed changes

samples/bidirectional-chat/csharp/Function.cs Outdated Show resolved Hide resolved

chenkennt reviewed Mar 11, 2020

View reviewed changes

samples/bidirectional-chat/csharp/Function.cs Outdated Show resolved Hide resolved

zackliu added 2 commits March 11, 2020 16:50

Simplify trigger usage in C#

432cf6d

Hide connectionstring

2f69300

JialinXin reviewed Mar 12, 2020

View reviewed changes

src/SignalRServiceExtension/TriggerBindings/ServerlessHub.cs Show resolved Hide resolved

JialinXin reviewed Mar 12, 2020

View reviewed changes

src/SignalRServiceExtension/TriggerBindings/SignalRTriggerBindingProvider.cs Show resolved Hide resolved

JialinXin reviewed Mar 12, 2020

View reviewed changes

samples/bidirectional-chat/csharp/Function.cs Outdated Show resolved Hide resolved

JialinXin reviewed Mar 12, 2020

View reviewed changes

src/SignalRServiceExtension/TriggerBindings/SignalRTriggerBindingProvider.cs Outdated Show resolved Hide resolved

wanlwanl reviewed Mar 12, 2020

View reviewed changes

samples/bidirectional-chat/csharp/Function.cs Show resolved Hide resolved

wanlwanl reviewed Mar 12, 2020

View reviewed changes

samples/bidirectional-chat/csharp/Function.cs Outdated Show resolved Hide resolved

zackliu mentioned this pull request Mar 12, 2020

Add a JavaScript sample to demonstrate SignalR Trigger #103

Open

wanlwanl reviewed Mar 12, 2020

View reviewed changes

samples/bidirectional-chat/csharp/Authorize/FunctionAuthorizeAttribute.cs Outdated Show resolved Hide resolved

Add tests and update according to comments

31fe376

chenkennt reviewed Mar 13, 2020

View reviewed changes

src/SignalRServiceExtension/TriggerBindings/ServerlessHub.cs Outdated Show resolved Hide resolved

chenkennt reviewed Mar 13, 2020

View reviewed changes

davidfowl reviewed Mar 13, 2020

View reviewed changes

chenkennt reviewed Mar 13, 2020

View reviewed changes

Improve class based model

3cad150

JialinXin reviewed Mar 13, 2020

View reviewed changes

JialinXin approved these changes Mar 13, 2020

View reviewed changes

Fix typo

4ff7265

zackliu merged commit c1f7c46 into dev Mar 16, 2020

chenkennt reviewed Mar 16, 2020

View reviewed changes

Uh oh!

Add a bidirectional chat sample to demonstrate SignalR Trigger #101

Add a bidirectional chat sample to demonstrate SignalR Trigger #101

Uh oh!

Conversation

zackliu commented Mar 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidfowl commented Mar 11, 2020

Uh oh!

zackliu commented Mar 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidfowl commented Mar 12, 2020

Uh oh!

zackliu commented Mar 13, 2020

Uh oh!

davidfowl commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zackliu Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JialinXin left a comment

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Mar 13, 2020 •

edited

Loading

zackliu Mar 13, 2020 •

edited

Loading