Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
hisuwh opened this issue Jun 27, 2024 · 9 comments

Comments

@hisuwh
Copy link
Contributor

hisuwh commented Jun 27, 2024

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:

  • if it cannot find/close a handler it should ignore it (as per standard behaviour) - and not throw an exception!
  • there should be an option to opt-out
  • and/or this should be extended to handle multiple generic arguments
@hisuwh
Copy link
Contributor Author

hisuwh commented Jun 27, 2024

@jbogard any chance of a 12.2.1 to fix #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
Copy link
Owner

jbogard commented Jun 27, 2024

Gah. Care to submit a PR?

@zachpainter77
Copy link
Contributor

zachpainter77 commented Jul 3, 2024

@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
Copy link
Contributor Author

hisuwh commented Jul 3, 2024

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

@zachpainter77
Copy link
Contributor

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
Copy link
Owner

jbogard commented Jul 17, 2024

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

Can you verify that this fixes the issue?

@zachpainter77
Copy link
Contributor

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
Copy link
Owner

jbogard commented Jul 20, 2024

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

@jbogard
Copy link
Owner

jbogard commented Jul 22, 2024

Fixed by #1048

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants