jbogard / MediatR

Simple, unambitious mediator implementation in .NET
Apache License 2.0
10.84k stars 1.15k forks source link

Add Support for Generic Handlers #1013

Closed zachpainter77 closed 1 month ago

zachpainter77 commented 3 months ago

This PR essentially adds the required logic to register open generic request handlers.

For example:

//class to use as generic request type parameter
public class Pong
{
    string Message? { get; set; }
}

//generic request definition
public class GenericPing<T> : IRequest<T>
    where T : Pong 
{
    public T? Pong { get; set; }
}

//generic request handler
public class GenericPingHandler<T> : IRequestHandler<GenericPing<T>, T>
    where T : Pong
{
    public Task<T> Handle(GenericPing<T> request, CancellationToken cancellationToken) => Task.FromResult(request.Pong!);
}

//usage
var pong = _mediator.Send(new GenericPing<Pong>{ Pong = new() { Message = "Ping Pong" } });
Console.WriteLine(pong.Message); //would output "Ping Pong"

Huge fan of this library.. With that said.. I find it kind of tedious to have to create separate handlers for each thing, when the code is pretty much copy paste. Just the entity or type I'm working with has changed. I think it would be much easier to work with if we could essentially cut some corners by leveraging c# generics.

The only way you can do this without needing a third party library is use assembly scanning and register every single concrete implementation of the generic handlers. So that is what the code is doing.

I also added some tests to ensure the handlers were being registered.

**Note: this feature doesn't introduce any breaking changes. The feature is simply opt in by creating handlers like above.. if you don't want to use them then you can keep on using the library the same way without any hiccups.

zachpainter77 commented 2 months ago

Yeah no problem. I can help convert the tests too as part of the pr if you'd like. Unfortunately I have a ton of down time at the moment. 😉

On Tue, Apr 16, 2024, 6:46 AM Jimmy Bogard @.***> wrote:

@.**** requested changes on this pull request.

In test/MediatR.Tests/SendTests.cs https://github.com/jbogard/MediatR/pull/1013#discussion_r1567393482:

@@ -213,4 +251,59 @@ public async Task Should_raise_execption_on_null_request()

     await Should.ThrowAsync<ArgumentNullException>(async () => await mediator.Send(default!));
 }

+

  • [Fact]

Nitpick, but these tests don't exercise those service scanning tests. You'd want to use services.AddMediatR instead. Which I am fine with, I need to convert all the tests to stop using Lamar anyway.

— Reply to this email directly, view it on GitHub https://github.com/jbogard/MediatR/pull/1013#pullrequestreview-2003703853, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACL2QUWEXFEUWRUH75UCXLDY5UTRPAVCNFSM6AAAAABFEKKOYCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMBTG4YDGOBVGM . You are receiving this because you authored the thread.Message ID: @.***>

kevingates commented 1 month ago

Looks like a neat, new feature. I'd like to see additional documentation with more practical examples, but I think this is neat.

zachpainter77 commented 1 month ago

@jbogard , are there any more changes necessary for this I think I converted the tests to use the default .net container and removed the dependency on Lamar like you suggested. Just wondering if anything else was needed.

Thanks.

jbogard commented 1 month ago

@zachpainter77 if you have time, can you add an example in the wiki?

zachpainter77 commented 1 month ago

Yes no problem will do ASAP. 😊

On Thu, Jun 6, 2024, 1:03 PM Jimmy Bogard @.***> wrote:

@zachpainter77 https://github.com/zachpainter77 if you have time, can you add an example in the wiki?

— Reply to this email directly, view it on GitHub https://github.com/jbogard/MediatR/pull/1013#issuecomment-2153314493, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACL2QUSGQI7IB6I7SEWYMY3ZGC6A5AVCNFSM6AAAAABFEKKOYCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJTGMYTINBZGM . You are receiving this because you were mentioned.Message ID: @.***>

hisuwh commented 2 weeks ago

@zachpainter77

**Note: this feature doesn't introduce any breaking changes. The feature is simply opt in by creating handlers like above.. if you don't want to use them then you can keep on using the library the same way without any hiccups.

Unfortunately, this assumption is incorrect. I'm getting an error on startup with the latest version: image

It would seem this is incompatible with some code I already have for handling Open Generic Handlers.

The only way you can do this without needing a third party library is use assembly scanning and register every single concrete implementation of the generic handlers. So that is what the code is doing.

This is a slight misnomer. You DI tool likely has a way to handle this - which is what I'm doing, using a fallback policy to catch missing open handlers and close them.


I think the main problem is you are only handling a single generic argument here: https://github.com/jbogard/MediatR/pull/1013/files#diff-c4d1d5960b86a86bee0fd5974803c90f8656cde09914a4c43ae14dc193c3cf35R195

But my handlers have multiple.

zachpainter77 commented 1 week ago

@hisuwh

Yes, you are absolutely correct.. I found out about this a while back (#1038 ) and have been working on a fix but haven't had time until today. So I have pushed a pull request out that fixes it and adds some extensive testing.. But there could be more to discuss. Can you please review it if you have time?

PR -> #1048

hisuwh commented 1 week ago

I'm away from my computer today but I'll try and review it in the next few days.