Hub filters!

[BrennanConroy]

Fixes #5353

Late feedback welcome!

public class HubLifetimeContext
{
    public HubLifetimeContext(HubCallerContext context, IServiceProvider serviceProvider, Hub hub);

    public HubCallerContext Context { get; }

    public Hub Hub { get; }

    public IServiceProvider ServiceProvider { get; }
}

public class HubInvocationContext
{
+   [Obsolete]
    public HubInvocationContext(HubCallerContext context, string hubMethodName, object[] hubMethodArguments);

+   public HubInvocationContext(HubCallerContext context, IServiceProvider serviceProvider, Hub hub, MethodInfo hubMethod, object[] hubMethodArguments);

+   [Obsolete]
    public string HubMethodName { get; }

    public HubCallerContext Context { get; }

    public IReadOnlyList<object> HubMethodArguments { get; }

+   public Hub Hub { get; }

+   public IServiceProvider ServiceProvider { get; }

+   public MethodInfo HubMethod { get; }
}

public static class HubOptionsExtensions
{
    public static void AddFilter(this HubOptions options, IHubFilter hubFilter);

    public static void AddFilter<TFilter>(this HubOptions options) where TFilter : IHubFilter;

    public static void AddFilter(this HubOptions options, Type filterType);
}

public interface IHubFilter
{
    ValueTask<object> InvokeMethodAsync(HubInvocationContext invocationContext, Func<HubInvocationContext, ValueTask<object>> next);

    Task OnConnectedAsync(HubLifetimeContext context, Func<HubLifetimeContext, Task> next) => next(context);

    Task OnDisconnectedAsync(HubLifetimeContext context, Exception exception, Func<HubLifetimeContext, Exception, Task> next) => next(context, exception);
}

[davidfowl]

Spoke to @BrennanConroy about the design. Instead of building the pipeline dynamically per invocation we'll have a stable pipeline built once and creation of the specific HubFilters per invocation:

e.g.

var filters = new List<IHubFilter>()
{
   new AHubFilter(),
   new HubFilterFactory(typeof(BHubFilter)),
   new CHubFilter(),
   new FilterFactory(typeof(DHubFilter)),
};

Instead of storing a list of objects we can store a list of IHubFilter and have the HubFilterFactory create the HubFilter per call.

[halter73]

This seems overly defensive to me.

[davidfowl]

@BrennanConroy After re-thinking this can we just support a way to return an invocation error instead of inferring anything? We can treat it like HubException? We support a way that you can opt into sending a custom InvocationError but we don't do anything else.

[BrennanConroy]

can we just support a way to return an invocation error

That's what the HubResult is for

[davidfowl]

Undo

[davidfowl]

I think it's time to undraft this PR and finish up. It's looking pretty good.

[davidfowl]

One more thing we need to think about is non-conforming containers or @dotnetjunkie won't let me hear the end of it 😉

[dotnetjunkie]

Thanks for pinning me, @davidfowl.

As far as I can see, non-conforming containers would be able to plug in to this system, because:

• Filters are essentially a list of singletons managed outside the container, allowing the creation of a factory/proxy (like your internal HubFilterFactory).
• IHubFilter implementations are provided with a context object, which allows a non-conforming container to pull in its own scope, to resolve hub filters from.

Using Pure DI in this model will perhaps be more complicated, but certainly possible.

Here's a sample implementation for Simple Injector:

// Setup
services.AddSignalR(options =>
{
    options.AddFilter(new SimpleInjectorHubFilterProxy<CustomHubFilter>(container));
})

class SimpleInjectorHubFilterProxy<THubFilter> : IHubFilter
    where THubFilter : IHubFilter
{
    private readonly InstanceProducer<THubFilter> producer;

    public SimpleInjectorHubFilterProxy(Container container) =>
        producer = Lifestyle.Scoped.CreateProducer<IHubFilter, THubFilter>(container);
        
    private IHubFilter GetFilter() => this.producer.GetInstance();

    public async ValueTask<object> InvokeMethodAsync(
        HubInvocationContext context, Func<HubInvocationContext, ValueTask<object>> next) =>
        this.GetFilter().InvokeMethodAsync(context, next);

    public async Task OnConnectedAsync(
        HubLifetimeContext context, Func<HubLifetimeContext, Task> next) =>
        this.GetFilter().OnConnectedAsync(context, next);

    public async Task OnDisconnectedAsync(
        HubLifetimeContext c, Exception ex, Func<HubLifetimeContext, Exception, Task> next) =>
        this.GetFilter().OnDisconnectedAsync(c, ex, next);
}

Do note that this implementation doesn't use the provided HubLifetimeContext, because Simple Injector's scoping mechanism is ambient in nature. Other non-conforming implementations, however, might need to pull their own scope object from this HubLifetimeContext.

[davidfowl]

@dotnetjunkie Thanks for reviewing, yes that pattern works well!

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[pranavkm]

Suggested change

	public IReadOnlyList<object> HubMethodArguments { get => Arguments; }
	public IReadOnlyList<object> HubMethodArgument => Arguments;

[analogrelay]

What about just Arguments for the public property?

[BrennanConroy]

I'd rather not make another breaking change

[pranavkm]

Could we retain this property to avoid a breaking change?

[BrennanConroy]

It's not breaking because this was added in 5.0

[analogrelay]

Looks swanky!

[analogrelay]

What about just Arguments for the public property?

[analogrelay]

We should make sure to call out the ordering of Filters and Authorization in docs. I do remember @rynowak cautioning us against the complexity of moving things like AuthZ into filters, so I won't suggest that, but we should at least doc the order.

[davidfowl]

Yah we did that in the old signalr as well but I don’t see the need here

[analogrelay]

Agreed, but we need to doc the fixed "ordering" we have so people know when filters run.

[pranavkm]

Suggested change

	/// Methods to add <see cref="IHubFilter"/>'s to Hubs.
	/// Methods to add <see cref="IHubFilter"/> instances to Hubs.

[davidfowl]

Great work @BrennanConroy!

[halter73]

I thought we decided in API review that we were not going to obsolete HubMethodName.

[BrennanConroy]

Hmm, maybe. No one commented that so it was forgotten. Fortunately we can easily fix this :)