AspnetCore DI and contravariant fun

[NinoFloris]

This line is the bane of my existence ;)
https://github.com/jbogard/MediatR/blob/master/src/MediatR/Mediator.cs#L35

Could we not do another major version bump and update IMediator.Send to be

        Task<TResponse> Send<TRequest, TResponse>(
            TRequest request,
            CancellationToken cancellationToken = default (CancellationToken))
            where TRequest : IRequest<TResponse>;

In that case we at least have some way of forcing MediatR to fetch the correct type from the container by casting it beforehand.

[MarkusEischeid]

With this change you would have to define the type arguments explicitly when calling Send():

mediator.Send<IRequest<Message>, bool>(...)

otherwise you would get a compiler error:

The type arguments for method
'CustomMediator.Send<TRequest, TResponse>(TRequest, CancellationToken)'
cannot be inferred from the usage.

Here is my solution for this:

ITypedRequest.cs (new Interface)

    public interface ITypedRequest<TRequest, TResponse> : IRequest<TResponse>
    {

    }

Mediator.cs (and according signatures for Send() in IMediator.cs)

        ...
        public Task<TResponse> Send<TResponse>(IRequest<TResponse> request, CancellationToken cancellationToken = default(CancellationToken))
        {
            if (request == null)
            {
                throw new ArgumentNullException(nameof(request));
            }

            return this.Send(request, request.GetType(), cancellationToken);
        }

        public Task<TResponse> Send<TResponse>(IRequest<TResponse> request, Type requestType, CancellationToken cancellationToken = default(CancellationToken))
        {
            if (request == null)
            {
                throw new ArgumentNullException(nameof(request));
            }

            if (requestType == null)
            {
                throw new ArgumentNullException(nameof(requestType));
            }

            var handler = (RequestHandlerWrapper<TResponse>)_requestHandlers.GetOrAdd(requestType,
            t => Activator.CreateInstance(typeof(RequestHandlerWrapperImpl<,>).MakeGenericType(requestType, typeof(TResponse))));
            return handler.Handle(request, cancellationToken, _serviceFactory);
        }

        public Task<TResponse> Send<TRequest, TResponse>(Lan.Cloud.Accounts.Queries.ITypedRequest<TRequest, TResponse> request, CancellationToken cancellationToken = default(CancellationToken))
        {
            if (request == null)
            {
                throw new ArgumentNullException(nameof(request));
            }

            return this.Send(request, typeof(TRequest), cancellationToken);
        }
        ...

Now you can create a IRequestHandler for an interface like this:

    public interface IMessage : ITypedRequest<IMessage, bool>
    {
    }

    public class Message1 : IMessage
    {
    }

    public class Message2 : IMessage
    {
    }

    public class MessageRequestHandler : IRequestHandler<IMessage, bool>
    {
        public async Task<bool> Handle(IMessage request, CancellationToken cancellationToken)
        {
            // logic here
            return true;
        }
    }

[NinoFloris]

I keep forgetting C# generic argument inference is so much worse than that of F# (no offence meant with it).

Yeah it might be a good overload candidate then?
I don't mind specifying the generic arguments when it's the only way to fix a problem ;)

[jbogard]

Can you provide an example that shows the problem you're seeing?

[jbogard]

And years and years ago I think either me or @khellang opened an issue with the C# team to have better generic inference for exactly this scenario, but I can't find it.

[khellang]

There are tons of them. You can start off this PR and dig your way through all the linked issues 😄 There seemed to be some hope recently, but the effort died off (again).

[NinoFloris]

So right now I've just created my own version of IMediator with these Send methods

   public interface IMediator
    {
        Task<TResponse> SendDynamic<TResponse>(IRequest<TResponse> request, CancellationToken cancellationToken = default);
        Task<TResponse> Send<TRequest, TResponse>(TRequest request, CancellationToken cancellationToken = default)
            where TRequest: IRequest<TResponse>;

        ...
    }

This works well for both C# and F#.
F# has simpler overload selection rules so having both methods be called Send instead means having to call Send<_> or Send<_,_> everywhere to disambiguate, even with inference it's no fun.

For C# having both be called Send works and currently picks the IRequest<TResponse> overload however in that case we're basically creating the scenario that LDM is afraid of — why they call it a breaking change to have constraints influence generic method inference.
With those inference improvements it'll start picking the TRequest overload, subtly changing semantics.

Having these two methods would work for all cases and keeps the option open of deprecating SendDynamic once/if C# gets inference improvements, who knows, there may actually be some good use for it :)

Obviously naming is open for suggestions.
As more C# users use this library I can imagine you might want Send and SendStatic instead, letting the most used method have the shortest name.

I'm happy to make a PR!

[jbogard]

What issue are you running into that makes that line the bane of your existence?

[NinoFloris]

Aspnet DI won't resolve a contravariant service. It'll treat it as invariant. I have a request type that's extended (say class B : A) in our webhost project with extra attributes for query param converters. So I pass in B that should be resolved to a handler that takes A. I cannot achieve that as I cannot go around value.GetType

[jbogard]

Why not open a PR to fix that? They're suuuuuuuper receptive about adding valuable features to core DI.

Or use a container that does support variance?

[khellang]

They're suuuuuuuper receptive about adding valuable features to core DI.

😂

[NinoFloris]

So I'm being laughed out of the room for proposing to do the work?

Aspnet DI won't change, there are enough issues around variance on the basis that they want to be a lowest common denominator container.

MediatR regardless of container choice should allow you to statically pass in the request type you want to retrieve a handler for, not offer just this runtime resolution.

[jbogard]

My hesitance comes from adding more to the public API. I know it won't break anyone, but it's a bit ugly to use.

[jbogard]

I'll take a PR for it, can you include a test that shows the issue?

[NinoFloris]

@jbogard would this be something to add with it right away?
#385 (comment)

Proposed interface is a bit bloated if we can't remove anything, but it works...

public interface IMediator
{
    Task<TResponse> Send<TResponse>(IRequest<TResponse> request, CancellationToken cancellationToken = default);

    // Start of new methods
    Task<object> Send(object request, CancellationToken cancellationToken = default);
    
    // Naming open for suggestions
    Task<TResponse> SendExact<TRequest, TResponse>(TRequest request, CancellationToken cancellationToken = default) where TRequest: IRequest<TResponse>;

   // ... Publish methods, do they need an extra method for 'Exact' dispatch too?
}

My ideal interface would have been:
But that ship has sailed I guess. (unless we would define a new IMediator in another namespace?)

public interface IMediator
{
    Task<TResponse> Send<TRequest, TResponse>(TRequest request, CancellationToken cancellationToken = default) where TRequest: IRequest<TResponse>;
}

@jbogard @khellang thoughts?

On top of that ideal interface you could easily define some extension methods for the dynamic things.

    public static class MediatorExtensions
    {
        private static readonly ConcurrentDictionary<Type, InvocationHelper> InvocationHelpers = new ConcurrentDictionary<Type, InvocationHelper>();

        private abstract class InvocationHelper
        {
            public abstract Task<object> InvokeDynamic(IMediator mediator, object request, CancellationToken cancellationToken);

            public class Impl<TRequest, TResponse> : InvocationHelper where TRequest : IRequest<TResponse>
            {
                public override async Task<object> InvokeDynamic(IMediator mediator, object request, CancellationToken cancellationToken)
                {
                    return await mediator.Send<TRequest, TResponse>((TRequest)request, cancellationToken);
                }
            }

        }

        // Public but not defined as an extension member
        public static Task<object> SendDynamic(IMediator mediator, Type requestType, object request, CancellationToken cancellationToken = default)
        {
            var handler = InvocationHelpers.GetOrAdd(
                requestType,
                rType =>
                {
                    Type responseType = null;
                    if (typeof(IRequest).IsAssignableFrom(rType))
                    {
                        responseType = typeof(Unit);
                    }
                    else
                    {
                        foreach (var i in rType.GetInterfaces())
                        {
                            if (i.IsConstructedGenericType && i.GetGenericTypeDefinition() == typeof(IRequest<>))
                            {
                                responseType = i.GenericTypeArguments[0];
                            }
                        }
                    }

                    if (responseType == null)
                    {
                        throw new ArgumentException("Invalid argument, request should implement IRequest<TResponse> or IRequest.", nameof(request));
                    }

                    return (InvocationHelper) Activator.CreateInstance(
                        typeof(InvocationHelper.Impl<,>).MakeGenericType(rType, responseType), true);
                }
            );

            // This may blow up once InvokeDynamic.Impl tries to cast the request value to requestType, caller beware.
            return handler.InvokeDynamic(mediator, request, cancellationToken);
        }

        public static Task<object> SendDynamic(this IMediator mediator, object request, CancellationToken cancellationToken = default)
            => SendDynamic(mediator, request.GetType(), request, cancellationToken);

        public static async Task<TResponse> SendDynamic<TResponse>(this IMediator mediator, IRequest<TResponse> request, CancellationToken cancellationToken = default)
            => (TResponse)await SendDynamic(mediator, request.GetType(), request, cancellationToken);
    }

[NinoFloris]

Any preference?

Both will be a breaking change anyway. I'd rather do the second option and possibly let the PR wait a bit until you feel happy pushing a new major version into the world.

But it's your call in the end.

[jbogard]

Why is it a breaking change?

…

On Tue, Apr 16, 2019 at 10:36 AM Nino Floris ***@***.***> wrote: Any preference? Both will be a breaking change anyway. I'd rather do the second option and possibly let the PR wait a bit until you feel happy pushing a new major version into the world. But it's your call in the end. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#393 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGYMhecwq1Gjo_9K7p4e6llMdAZ-u6zks5vhe3pgaJpZM4cq1YQ> .

[NinoFloris]

Changing interfaces is a breaking change until we have default interface implementations

[jbogard]

Derp. Of course

…

On Tue, Apr 16, 2019 at 3:01 PM Nino Floris ***@***.***> wrote: Changing interfaces is a breaking change until we have default interface implementations — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#393 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGYMsDOptiwmqyH6McoXWTlKMtnZX3aks5vhiw3gaJpZM4cq1YQ> .

[jbogard]

I would prefer an overload instead of a new method - that shouldn't break anyone (existing callsites would use the existing method, no?)

[NinoFloris]

Adding it as an overload could change method overload choice in future C# and it breaks inference in current F#. It really should be to be a new method.

Regarding adding that method to IMediator.
If people implemented their own IMediator we have binary and source breaks. If people only called Send on IMediator we should have neither as the original interface slot is still there. Would need to actually test this to be absolutely sure though.

In any case you're not going to make those implementers of custom IMediator instances happy if you don't do a semver major bump

[jbogard]

Yeah of course, if we break the interface then we'll need to bump major versions. A new method though is...weird.

…

On Wed, Apr 17, 2019 at 11:08 AM Nino Floris ***@***.***> wrote: Adding it as an overload could change method overload choice in future C# and it breaks inference in current F#. It really should be to be a new method. Regarding adding that method to IMediator. If people implemented their own IMediator we have binary and source breaks. If people only called Send on IMediator we should have neither as the original interface slot is still there. Would need to actually test this to be absolutely sure though. In any case you're not going to make those implementers of custom IMediator instances happy if you don't do a semver major bump — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#393 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGYMh1oB_euAQNVf0_LIwvP7p3FWqFRks5vh0b8gaJpZM4cq1YQ> .

[NinoFloris]

If we do major bump we don't have to care about binary compatibility. We should then optimize for largest group of users.

Change IMediator by removing the old Send<TResponse> and introduce new SendRequest<TRequest, TResponse> or some name like it.

Then to keep source compatibility have an extension method to keep the old Send method signature intact. It'll make the interface less pretty but keeps mostly everything going.

[NinoFloris]

Could also add a new focussed interface for Send ...

IRequestMediator 
{
    TResponse Send<Request, Response>(...)
}

Just this method and then again adding the dynamic parts via extension methods.

Would be very non breaking and not too confusing.

[jbogard]

Extension method possible?

…

On Wed, Apr 17, 2019 at 11:38 AM Nino Floris ***@***.***> wrote: Could also add a new focussed interface for Send ... IRequestMediator { TResponse Send<Request, Response>(...) } Just this method and then again adding the dynamic parts via extension methods. Would be very non breaking and not too confusing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#393 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGYMvoS_5BPm7cw2rLVccrn2IASkmcoks5vh04fgaJpZM4cq1YQ> .

[NinoFloris]

Not for the generic one, it needs a type argument so we can lock in the exact type of the request

[jbogard]

No I mean can you accomplish it with an extension method instead of changing the interface?

…

On Wed, Apr 17, 2019 at 11:59 AM Nino Floris ***@***.***> wrote: Not for the generic one, it needs a type argument so we can lock in the exact type of the request — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#393 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGYMlSQt9u-ocg--mL_oXwjLO3O_3n9ks5vh1MCgaJpZM4cq1YQ> .

[NinoFloris]

Yeah not really, like I said we need an exact type and we need to pass that exact type to the servicefactory which is private instance state :/

[NinoFloris]

I get if you don't want to go through with it, any option is really a bit suboptimal from this starting point. No hard feelings either, much of this work was also super useful for our own internal version!

[jbogard]

Yeah, I think I'll put this in the back burner for now. Maybe in C# 42?