jbogard / MediatR

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

Generic Handler implementation only handles single generic argument and breaks existing implementations #1047

Closed hisuwh closed 4 months ago

hisuwh commented 4 months ago

Support for Generic Handlers has been added in v12.3.0 in this pr: #1013

However, this is not opt-in and only supports a single generic argument.

So a setup like this is incompatible:

public class MyRequest<T1, T2> : IRequest<T2>

public class MyRequestHandler<T1, T2> : IRequestHandler<MyRequest<T1, T2>, T2>

Also rather than simply not matching these requests this throws an exception on startup meaning this can't even fallback to existing code I have to handle these more complex generic handlers.

I suggest:

hisuwh commented 4 months ago

@jbogard any chance of a 12.2.1 to fix https://github.com/jbogard/MediatR/pull/989

If and when I get some time I can look at the above as I have implemented generic handlers in a few projects.

jbogard commented 4 months ago

Gah. Care to submit a PR?

zachpainter77 commented 4 months ago

@jbogard I have a fix for the above issue.. My only question is what limitations should be present? What I mean is, should we let the user define handlers that have 10 type parameters, each with no constraint? You can see how this could be problematic for the user, right? My solution is to allow more than one generic type parameter but throw errors if the max number of generic type params is exceeded OR the max number of concrete class combinations is exceed OR if a timeout occurs. My question is.. what should those limitations be?

I'm also trying to figure out a way to test these scenarios since even defining a class that should throw an exception does so for all tests that call AddMediatR.

So, any discussion around this is welcome. I think I'm just going to create a PR and let discussion take place there.

hisuwh commented 4 months ago

Is there a way to define a sensible limit internally, but then allow for this to be configurable?

zachpainter77 commented 4 months ago

yes, I think there is a way to do this.. I will set up some configuration options with default values that can be overridden when calling AddMediatR.. I will have a PR coming today. We can probably discuss what is sensible in terms of default limitations in that thread.

jbogard commented 4 months ago

This fix is on the MyGet feed: https://myget.org/gallery/mediatr-ci

Can you verify that this fixes the issue?

zachpainter77 commented 4 months ago

image Seems to work with my test project. However I had to use this myget feed url in order to install the package via nuget package manager in visual studio..

image

https://www.myget.org/F/mediatr-ci/api/v3/index.json

jbogard commented 4 months ago

Yes the pre release packages are all on MyGet. I'll get a release out then.

jbogard commented 4 months ago

Fixed by #1048