jbogard / MediatR.Extensions.Microsoft.DependencyInjection

MediatR extensions for Microsoft.Extensions.DependencyInjection
MIT License
327 stars 89 forks source link

Handler of base type is not being executed when handler of derived type is not executed #49

Closed jonhwong closed 3 years ago

jonhwong commented 5 years ago

I traced this issue to src\MediatR\Internal\NotificationHandlerWrapper.cs: line 20 where the serviceFactory is not returning the base handler type in the case of publishing DerivedNotification2

Example

public static class Program
{
    public static async Task Main(string[] args)
    {
        ServiceCollection services = new ServiceCollection();

        services.AddMediatR(Assembly.GetEntryAssembly());

        ServiceProvider provider = services.BuildServiceProvider();

        IMediator m = provider.GetRequiredService<IMediator>();

        await m.Publish(new DerivedNotification1());
        await m.Publish(new DerivedNotification2());
    }
}

public class DerivedNotification1Handler : INotificationHandler<DerivedNotification1>
{
    public Task Handle(DerivedNotification1 notification, CancellationToken cancellationToken)
    {
        Console.WriteLine($"DerivedNotification1 Handler");
        return Task.CompletedTask;
    }
}

public class BaseNotificationHandler : INotificationHandler<BaseNotification>
{
    public Task Handle(BaseNotification notification, CancellationToken cancellationToken)
    {
        Console.WriteLine("BaseNotification Handler");
        return Task.CompletedTask;
    }
}

public class BaseNotification : INotification { }
public class DerivedNotification1 : BaseNotification { }
public class DerivedNotification2 : BaseNotification { }

Expected

DerivedNotification1 Handler
BaseNotification Handler
BaseNotification Handler

Actual

DerivedNotification1 Handler
BaseNotification Handler
jbogard commented 5 years ago

Does MS DI support this?

jbogard commented 5 years ago

Or is this a bug in the base MediatR library?

jonhwong commented 5 years ago

MS DI does support it. It seems to be the way in which you look for interfaces and concrete classes to register that causes the registration for INotificationHandler to BaseNotificationHandler not be discovered. I'm not sure if this is intended or not. I had written an extension, which I had already tossed =(, to look for classes that implement INotifications and classes that implement INotificationHandler<> and pair them up. With this I was able to get the results I was looking for.

julienroubieu commented 5 years ago

I would also be interested in a solution for this. Currently I'm creating a lot of DerivedNotificationXHandlers just for BaseNotificationHandler to be called for every derived notification.

kstrouse commented 5 years ago

It appears this doesn't work since the Microsoft container doesn't support Covariant resolution. I have written an enhanced ServiceFactory that will perform the covariance resolution via reflection. I have just started playing around with using MediatR so this isn't fully tested but maybe someone else can find it useful.

Just register it with the container after AddMediatR

services.AddTransient<ServiceFactory>(sp => MicrosoftDIContainerServiceFactory.Using(sp).Resolve);

public class MicrosoftDIContainerServiceFactory
{
    public static MicrosoftDIContainerServiceFactory Using(IServiceProvider services)
        => new MicrosoftDIContainerServiceFactory(services);

    private readonly IServiceProvider services;

    public MicrosoftDIContainerServiceFactory(IServiceProvider services)
    {
        this.services = services;
    }

    public object Resolve(Type serviceType)
    {
        if (IsNotificationHandlerEnumerable(serviceType))
        {
            return new NotificationHandlerResolver(services)
                .ResolveHandlersFor(GetNotificationType(serviceType));
        }
        return services.GetService(serviceType);
    }
    private bool IsNotificationHandlerEnumerable(Type serviceType)
        => serviceType.IsGenericType
            && serviceType.GetGenericTypeDefinition() == typeof(IEnumerable<>)
            && serviceType.GetGenericArguments()[0].IsGenericType
            && serviceType.GetGenericArguments()[0].GetGenericTypeDefinition() == typeof(INotificationHandler<>);
    private Type GetNotificationType(Type serviceType) 
        => serviceType.GetGenericArguments()[0].GetGenericArguments()[0];

    private class NotificationHandlerResolver
    {
        private readonly IServiceProvider services;
        private object handlers;

        public NotificationHandlerResolver(IServiceProvider services)
        {
            this.services = services;
        }

        public object ResolveHandlersFor(Type notificationType)
        {
            handlers = MakeHandlersList(notificationType);

            AddHandlersForNotificationType(notificationType);
            AddHandlersForBaseTypes(notificationType.BaseType);
            AddHandlersForInterfaces(GetInterfaceNotificationTypes(notificationType));

            return handlers;
        }

        private object MakeHandlersList(Type notificationType)
        {
            var handlerListType = typeof(List<>).MakeGenericType(typeof(INotificationHandler<>).MakeGenericType(notificationType));
            return Activator.CreateInstance(handlerListType);
        }

        private void AddHandlers(IEnumerable<object> handlers)
        {
            var addRangeMethod = this.handlers.GetType().GetMethod("AddRange");
            addRangeMethod.Invoke(this.handlers, new[] { handlers });
        }

        private void AddHandlersForNotificationType(Type notificationType) 
            => AddHandlers(GetServicesThatHandle(notificationType));

        private void AddHandlersForBaseTypes(Type baseType)
        {
            if (!IsNotificationType(baseType))
                return;

            AddHandlers(GetServicesThatHandle(baseType));
            AddHandlersForBaseTypes(baseType.BaseType);
        }
        private void AddHandlersForInterfaces(IEnumerable<Type> interfaces)
        {
            if (!interfaces.Any())
                return;

            foreach (var interfaceType in interfaces)
            {
                AddHandlers(GetServicesThatHandle(interfaceType));
                AddHandlersForInterfaces(GetInterfaceNotificationTypes(interfaceType));
            }
        }
        private IEnumerable<Type> GetInterfaceNotificationTypes(Type type)
            => type.GetInterfaces().Where(IsNotificationType);

        private bool IsNotificationType(Type type)
            => type.GetInterfaces().Any(t => t == typeof(INotification));

        private IEnumerable<object> GetServicesThatHandle(Type notificationType)
            => services.GetServices(MakeNotificationHandlerType(notificationType));

        private Type MakeNotificationHandlerType(Type notificationType)
            => typeof(INotificationHandler<>).MakeGenericType(notificationType);
    }
}
pregoli commented 3 years ago

Same issue here: System.InvalidOperationException: Handler was not found for request of type MediatR.IRequestHandler`2[ExampleNamespace.Command1,MediatR.Unit]. Register your handlers with the container. See the samples in GitHub for examples.

My Handler: ExampleHandler<T> : IRequestHandler<T> where T : BaseCommand

Following the commands:

  1. public abstract class BaseCommand: IRequest
  2. public class Command1: BaseCommand
  3. public class Command2: BaseCommand

Following the registration Through Microsoft DI: services.AddSingleton(typeof(IRequestHandler<>), typeof(ExampleHandler<>)); services.AddMediatR(typeof(ExampleInstance).Assembly, typeof(ExampleHandler<>).Assembly);

Any Thoughts?

jbogard commented 3 years ago

MS DI now supports covariant resolution in 5.0, you can try that

pregoli commented 3 years ago

Any example based on my snippets?

jbogard commented 3 years ago

Probably not, there's not enough code there. You seem to be using constraints for request handlers, that's not the typical use case. The typical one is pipeline behaviors and notification handlers, where you're resolving a collection of open generic types. You're trying to use it for a single one. I'm not sure what you're trying to do here.

pregoli commented 3 years ago

@jbogard Thanks for your reply.

What I'm trying to achieve is quite simple. I have some commands, one base and others inheriting from it. Because of their nature of "commands", The send endpoint should be used. I don't want to create an handler for every single child command because it's handle body implementation is the same.

I noticed the following:

With the below lines: services.AddScoped(typeof(IRequestHandler<>), typeof(ExampleHandler<>)); services.AddMediatR(typeof(Startup).Assembly);

It's working fine when implementing the INotificationHandler, which is good to publish events, NOT commands: ExampleHandler<T> : INotificationHandler<T> where T : ExampleBaseCommand

usage: await _mediator.Publish(derivedCommand); =====> OK

but NOT with IRequestHandler, which is recommended to send commands: ExampleHandler<T> : IRequestHandler<T> where T : ExampleBaseCommand

or

ExampleHandler<T> : AsyncRequestHandler<T> where T : ExampleBaseCommand

usage: await _mediator.Send(derivedCommand); =====> KO

I understand this issue has been marked as closed, I could open a new issue if it makes sense for you.

jbogard commented 3 years ago

In that case, you almost certainly can't use the default container. You should create a minimal test use case WITHOUT MediatR. MediatR piggybacks on top of the container itself, relying on its features. Make a minimal repro that uses IServiceCollection and IServiceProvider by itself, then you'll be able to try other containers to see if they support the behavior you're looking for.

You might not find one that does, however. This is highly atypical usage - you have to actually try to close the type to see if the handler fits, then when it doesn't, try to resolve some....other handler? The MOST I've seen is something like:

public class TypicalHandler<TRequest, TResponse> : IRequestHandler<TRequest, TResponse> {
}

public class SpecificHandler : IRequestHandler<SpecificRequest, SpecificResponse> { 
}

Most containers handle this case because they'll check the closed interface registration first, then open generic registrations. Adding constraints to the mix...you'll have to check different container implementations.