jbogard / MediatR.Extensions.Microsoft.DependencyInjection

MediatR extensions for Microsoft.Extensions.DependencyInjection
MIT License
326 stars 90 forks source link

Added suggestion for open bound generic handlers WIP #115

Closed amunim closed 1 year ago

amunim commented 2 years ago

If I had known this, it would've saved 24 hours. Now at least someone will not waste their time about it.

amunim commented 2 years ago

I am currently working on a project where this issue occured, for some time I kept adding transient services for every class like this.

services.AddTransient<GenericBase<ConstrainedClass>.Query,
GenericBase<ConstrainedClass>.Handler>();

After I finally I gave up, I found that adding autofac fixes my problem. That's why I don't want anyone else to waste time on this nonsense. Not sure though if it is fixed in .Net 6, regardless adding this won't hurt anyone, if a person is searching for this then it means it doesn't work for him.

On Mon, 31 Jan 2022, 10:16 pm Jimmy Bogard, @.***> wrote:

@.**** commented on this pull request.

In README.md https://github.com/jbogard/MediatR.Extensions.Microsoft.DependencyInjection/pull/115#discussion_r795890386 :

@@ -55,5 +55,14 @@ services.AddTransient(typeof(IRequestHandler<,>), typeof(GenericHandlerBase<,>))

This won't work with generic constraints, so you're better off creating an abstract base class and concrete closed generic classes that fill in the right types.

I don't think this statement is true anymore? MS.Ext.DI supports generic constraints now.

— Reply to this email directly, view it on GitHub https://github.com/jbogard/MediatR.Extensions.Microsoft.DependencyInjection/pull/115#pullrequestreview-868168520, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFZQTKENGLNJVXFMB5AS7RDUY27XXANCNFSM5NFOL7YA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: <jbogard/MediatR.Extensions.Microsoft. @.***>

jbogard commented 2 years ago

Oh wait I was confused about the comment here. The "Or..." has nothing to do with the previous statement. But I am confused here what's not working and how Autofac fixes it.

amunim commented 2 years ago

Open bound generic handlers are not automatically registered in Mediator. I Have manually add them which becomes a tedious process. Like if I have a generic constraint of interface IInputPage, which contains some form properties to be used by the front-end. I now have to register every class which inherits this interface(of course the type will be used in GenericBase<MyInputPage>.Handler. Adding autofac eases my work now I can register Generic base by builder.RegisterGeneric(typeof(GenericBase<>.Handler)).AsImplementedInterfaces().

If I try above with MS DI, services.AddMediatR(typeof(GenericBase<>.Handler))

It throws handler was not found error.

On Mon, 31 Jan 2022, 10:32 pm Jimmy Bogard, @.***> wrote:

Oh wait I was confused about the comment here. The "Or..." has nothing to do with the previous statement. But I am confused here what's not working and how Autofac fixes it.

— Reply to this email directly, view it on GitHub https://github.com/jbogard/MediatR.Extensions.Microsoft.DependencyInjection/pull/115#issuecomment-1026030977, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFZQTKB2DRILZDMJQLTLRPTUY3BSXANCNFSM5NFOL7YA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: <jbogard/MediatR.Extensions.Microsoft. @.***>

jbogard commented 2 years ago

So this:

services.AddMediatR(typeof(GenericBase<>.Handler))

Says "go to this assembly for this type and add the handlers". If you have additional open generics, you have to register them manually. Are you saying that this does not work:

services.AddMediatR(typeof(GenericBase<>.Handler));
services.AddTransient(typeof(GenericBase<>.Handler));

?

amunim commented 2 years ago

This is how I have to register them:


            services.AddTransient<IRequestHandler<Application.Core.Main<T_Classifieds, T_ClassifiedDomains>.Query, List<Application.Core.Main<T_Classifieds, T_ClassifiedDomains>.Data>>, Application.Core.Main<T_Classifieds, T_ClassifiedDomains>.Handler>();

See how this looks ugly, and I have to o it for every similar classes(T_Classifieds). With autofac, I don't have to write all of this code

Is it clear now?

jbogard commented 2 years ago

So you can't register the open generic with the stock DI? That doesn't work properly?

amunim commented 2 years ago

sigh yes

On Mon, 31 Jan 2022, 11:54 pm Jimmy Bogard, @.***> wrote:

So you can't register the open generic with the stock DI? That doesn't work properly?

— Reply to this email directly, view it on GitHub https://github.com/jbogard/MediatR.Extensions.Microsoft.DependencyInjection/pull/115#issuecomment-1026100732, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFZQTKBJE2I6NTAE2DCQKWDUY3LGFANCNFSM5NFOL7YA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: <jbogard/MediatR.Extensions.Microsoft. @.***>

amunim commented 2 years ago

@jbogard I hope this clarifies what I'm trying to achieve.

jbogard commented 2 years ago

Ah yes it does. The old "arity does not match" issue. I like calling this out but I would like the language of the readme to be specific to the problem, but also generic for the solution - Autofac is one container that can handle this, there might be others too. I don't necessarily just want to recommend a specific container if it's not one I use on a daily basis.

amunim commented 2 years ago

Yeah, you're right. I haven't tried any other but I will starting tomorrow because MyTested.Aspnet.Mvc does not seem to provide a way to register service factories. This breaks all of my tests and I have already wasted this day learning the code of the library, which now is too much a job. I'll try another container with my tests project and update the read me to reflect that.

Any other suggestions?

On Tue, 1 Feb 2022, 11:37 pm Jimmy Bogard, @.***> wrote:

Ah yes it does. The old "arity does not match" issue. I like calling this out but I would like the language of the readme to be specific to the problem, but also generic for the solution - Autofac is one container that can handle this, there might be others too. I don't necessarily just want to recommend a specific container if it's not one I use on a daily basis.

— Reply to this email directly, view it on GitHub https://github.com/jbogard/MediatR.Extensions.Microsoft.DependencyInjection/pull/115#issuecomment-1027163365, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFZQTKEA45NDXGPOYYLDT43UZAR6DANCNFSM5NFOL7YA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: <jbogard/MediatR.Extensions.Microsoft. @.***>

jbogard commented 2 years ago

A 3rd-party container is the way to go when the stock container doesn't have the features.

amunim commented 2 years ago

Hey, just wanted to let you know I gave up on 3rd party containers because I was using MyTest.AspNetCore.Mvc to test my API but with .net5 we need to use service provider factory on Host builder to successfully use it and the library does not have support for it, with last commit being older than 2 years ago, that was a dead end. I also asked the question on stack here https://stackoverflow.com/questions/70960528/register-autofac-service-provider-factory-from-services-instead-of-createhostbui, but to no avail. Finally after deciding I have spent enough time, I used Reflection to register my handlers.

Long story short, I'll wire up some overloaded extension methods over the weekend(or the next) after studying and submit a PR for folks in the same boat as me, who need to register generic handlers by simply .registergeneric() instead of adding transient service for each class that is in contract with the constrained interface(or inherits the constrained class.in my case).

On Tue, 1 Feb 2022, 11:47 pm Jimmy Bogard, @.***> wrote:

A 3rd-party container is the way to go when the stock container doesn't have the features.

— Reply to this email directly, view it on GitHub https://github.com/jbogard/MediatR.Extensions.Microsoft.DependencyInjection/pull/115#issuecomment-1027172154, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFZQTKCI2F2OUAGCBNY5KJ3UZATFHANCNFSM5NFOL7YA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: <jbogard/MediatR.Extensions.Microsoft. @.***>

jbogard commented 2 years ago

Yes please do! I'm happy to take a look if it can be done by the stock container.

This library does assembly scanning/reflection to register handlers but I took that code from a 3rd-party container's scanning code. It can certainly be enhanced.

amunim commented 2 years ago

I've completed writing the extension method to register and it is working on my project. MediatR.Extensions.Microsoft.DependencyInjection is currently on .Net 6, which I don't have on my local machine, i.e. I can't write tests on the project, anyway I'm committing changes on my branch, you can check out if you like and I'll submit a PR from the office this week.

@jbogard can you take a look and give feedback :)

Cubody commented 1 year ago

This extension can not make generic for such handler:

public record SaveEntity<T>(T Entity) : IRequest<T> where T : IEntity;

public class SaveEntityHandler<T> : IRequestHandler<SaveEntity<T>, T> where T : IEntity
{
    //...
}

It's trying to make generic type with these args: 'SaveEntity' and 'T', when second should be ConcreteEntity. As result it do nothing because of error that says:

System.ArgumentException: GenericArguments[0], 'SaveEntity`1[ConcreteEntity]', on 'MediatR.IRequestHandler`2[TRequest,TResponse]' violates the constraint of type 'TRequest'.

Possible solution: We need to change checking of 'IsGenericType' to 'IsGenericTypeParameter' and also get type from concreteType dictionary.

static Type IRequestHandlerReturnType(Type requestHandlerInterface, Dictionary<Type, Type> concreteType)
    {
        var genericTypes = requestHandlerInterface.GetTypeInfo().GetGenericArguments();
        switch (genericTypes.Length)
        {
            //it can either be 1 or 2, IRequestHandler<,>
            case 1:
                return typeof(Unit);
            //if someday somebody decides to make IRequestHandler without any generic arguments
            case > 1:
            {
                var returnType = genericTypes[1];
                return returnType.IsGenericTypeParameter ? concreteType[returnType] : returnType;
            }
            default:
                return typeof(Unit);
        }
    }

But it'll not work with something like:

 public record GetEntitiesQuery<T>() : IRequest<IQueryable<T>> where T : IEntity;

public class GetEntitiesQueryHandler<T> : IRequestHandler<GetEntitiesQuery<T>, IQueryable<T>> where T : class, IEntity
{
...
}

@amunim look at this

sjd2021 commented 1 year ago

Why hasn't this been merged? This is a desperate need of mine, and it feels wrong to have to manually write out the workarounds right now if the suggested solution works for most cases.

jbogard commented 1 year ago

I've moved all of this code in to the MediatR repository, can you apply this PR there?

Tommo56700 commented 1 year ago

@jbogard Sorry if I'm missing something but it's not clear to me where this has been moved to. Are you able to link it?

jbogard commented 1 year ago

To the main MediatR repository. I'm folding this whole thing in over there. I should archive this repo...

Tommo56700 commented 1 year ago

Is there an issue in the main MediatR repo that tracks this specifically?