jbogard / MediatR

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

Passing arguments to next in pipeline behaviors #523

Closed thomaslevesque closed 2 years ago

thomaslevesque commented 4 years ago

Currently, the next parameter in IPipelineBehavior<TRequest, TResponse>.Handle is a RequestHandlerDelegate<TResponse> which takes no argument.

This is a problem, because it means it's impossible for a behavior to pass anything other than the original request to the next behavior.

Here are two examples where it could be useful to be able to do that:

I realize this would be a breaking change, but maybe in a future major version?

lilasquared commented 4 years ago

It wouldn't necessarily be a breaking change if the library continued to support calling the delegate with no args. Its difficult to see how this would play out in terms of the type bindings. Would the arg type be part of the request? part of the pipeline? would it be a BehaviorArgs class with a dynamic payload? If you have thoughts on how it would work a PR would be nice.

For your first bullet as you say its easy to modify the existing request. I've have success with this before when hydrating some complex data model that would be needed by subsequent behaviors, but is not part of the request. Is there something blocking you from doing it this way, or is it more the principle of having immutable requests?

For your second, I remember a conversation about cancellation token in another issue...https://github.com/jbogard/MediatR/issues/496 the solution here was to wire up a new mediator that used the request aborted cancellation token if none was provided.

lilasquared commented 4 years ago

Actually, since the handlers and behaviors are bound by type to the requests themselves, the "transformation" you're talking about would have to transform it to the same request type anyway so maybe type bindings isn't as big of an issue.

thomaslevesque commented 4 years ago

Hi @lilasquared,

It wouldn't necessarily be a breaking change if the library continued to support calling the delegate with no args.

How would you do that? The interface has to change anyway. RequestHandlerDelegate<TResponse> would need an additional TRequest generic type parameter, so it would no longer be the same type, so existing implementations would be broken.

Of course, you could introduce a new interface for behaviors, but it would probably make things confusing...

or is it more the principle of having immutable requests?

Yes, that. My requests are often immutable.

For your second, I remember a conversation about cancellation token in another issue...#496 the solution here was to wire up a new mediator that used the request aborted cancellation token if none was provided.

Yes, I thought of doing this too. It's not a big deal, but I thought a behavior would be a really good fit for this, so I was a little disappointed when I realized it wasn't possible.

Actually, since the handlers and behaviors are bound by type to the requests themselves, the "transformation" you're talking about would have to transform it to the same request type anyway

Yes, I'm aware of this limitation.

lilasquared commented 4 years ago

Of course, you could introduce a new interface for behaviors, but it would probably make things confusing...

yeah this is what I was thinking but I can see how it would get cumbersome.

This has been discussed before in a few other issues as well I think, here is one with a response from the author https://github.com/jbogard/MediatR/issues/361

A mutable dependency on both the pipeline(s) and the handler seems to be the suggested approach.

matthiaslischka commented 3 years ago

+1 this request. Same reason: pass the HttpContext.CancellationToken to all handlers. For now I use a decorator registered with scrutor - but it feels wrong since mediatR already comes with behaviors.

public class HttpContextCancellationTokenRequestHandlerDecorator<TRequest, TResponse> : IRequestHandler<TRequest, TResponse>
    where TRequest : IRequest<TResponse>
{
    private readonly IRequestHandler<TRequest, TResponse> _handler;
    private readonly IHttpContextAccessor _httpContextAccessor;

    public HttpContextCancellationTokenRequestHandlerDecorator(IRequestHandler<TRequest, TResponse> handler,
        IHttpContextAccessor httpContextAccessor)
    {
        _handler = handler;
        _httpContextAccessor = httpContextAccessor ?? throw new ArgumentNullException(nameof(httpContextAccessor));
    }

    public Task<TResponse> Handle(TRequest request, CancellationToken cancellationToken)
    {
        cancellationToken = cancellationToken == CancellationToken.None
            ? _httpContextAccessor!.HttpContext?.RequestAborted ?? CancellationToken.None
            : cancellationToken;

        return _handler.Handle(request, cancellationToken);
    }
}
services.Decorate(typeof(IRequestHandler<,>), typeof(HttpContextCancellationTokenRequestHandlerDecorator<,>));

I know Jimmy does not like decorators, so maybe this adds additional motivation 😅

stefc commented 3 years ago

I voted also for such a change in the lib. In our case we want to do some kind of Data Cleansing on TRequest before the next() Handler was called. In the example e.g. I want to do a UpCase Transformation on the ping message. So the "Ping" became a "PING" in a PreProcessor IPipelineBehavior, the same kind of PostProcessor with the TRespond would not a problem ;)

amunim commented 2 years ago

+1 I want to get the default layout like, paragraphTop/bottom body content and then pass that. Such that MyReviewsPageDTO: MainLayout here Mainlayout has all the common props while MyReviewsPageDTO only has to get reviews and attach it to return the same object. Instead of doing this in every controller action it would be nice to have a pipeline behavior handling it for every query

thomaslevesque commented 2 years ago

@jbogard why was this issue closed as completed? IPipelineBehavior has changed, but we still can't pass arguments to next, so it doesn't resolve this issue.

jbogard commented 2 years ago

It wasn't closed as completed, just closed. There are other things we may want to pass in the next delegate beyond just the request and cancellation token (like other some other behavior pipelines). I wanted to think about this a bit more.

thomaslevesque commented 2 years ago

It wasn't closed as completed, just closed.

Github disagrees 😉 image

There are other things we may want to pass in the next delegate beyond just the request and cancellation token (like other some other behavior pipelines). I wanted to think about this a bit more.

Fair enough!

jbogard commented 2 years ago

I didn't know GitHub now differentiates between different "close" states, it doesn't show you that through the UI. Blarg.

ice19942335 commented 2 weeks ago

Hi. +1 for this idea. We have a case where we need to generate some data and pass it to the next handler using the data that was generated in the previous handler from the pipeline.