jbogard / MediatR

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

IRequestPreProcessor not called before IPipeLineBehavior #868

Closed AleRoe closed 1 year ago

AleRoe commented 1 year ago

Hi there,

I'm having some difficulties migrating my code to version 12.0.1.

I have a RequestPreProcessor defined as

public class RequestContextPreProcessor<TRequest> : IRequestPreProcessor<TRequest>  
where TRequest : notnull, IHomeControllerRequest

and a number of Behaviors which I register using the follwoing code:

services.AddMediatR(config => {
                        config.RegisterServicesFromAssemblies(PluginProvider.GetAssemblies());
                        config.AddOpenBehavior(typeof(TelemetryBehavior<,>));
                        config.AddOpenBehavior(typeof(LoggingBehavior<,>));
                        config.AddOpenBehavior(typeof(ErrorHandlingBehavior<,>));
                    })

I can't get the RequestPreProcessor to execute. For some reason, it's not being registered and injected as part of the pipeline. My code worked fine with version 11. I tried explicitly registering the RequestPreProcessor, but that did not work either.

Any suggestions?

Thanks Alexander

AleRoe commented 1 year ago

Update: I did some more tests. It turns out that the RequestPreProcesser is actually registered and being loaded (CTOR is called), but the Process() method is not being executed prior to any other behaviors (verified using breakpoints)

My workaround for now is to declare my PreProcessor as a regular IPipelineBehavior and adding it as the first behavior, but this kinda defies the purpose of the RequestPreProcessor.

Thanks, Alexander

StevenTCramer commented 1 year ago

@AleRoe do you have small reproducible repo?

StevenTCramer commented 1 year ago

I have an example where this is happening, and the issue is that the RequestPostProcessorBehavior isn't being registered. I am trying my hardest to debug this but right now it only happens in WASM for me and VS and Rider frankly suck at debugging in WASM.

AleRoe commented 1 year ago

I'll try to create a sample from my code asap.

StevenTCramer commented 1 year ago

@jbogard @AleRoe I found it!!! AddMediatR will NOT register the RequestPostProcessorBehavior if it thinks there are no implementations.

Previously in (version 11) if you called AddMediatR and then registered your PostProcessor it would have worked. Now you have to Register your PostProcessor before calling AddMediatR or have it in the assembly being scanned.

I couldn't get VS or Rider to debug and resulted to old school Console.Writelines.

    private static void RegisterBehaviorIfImplementationsExist(IServiceCollection services, Type behaviorType, Type subBehaviorType)
    {
        if (typeof(RequestPostProcessorBehavior<,>) == behaviorType)
        {
            Console.WriteLine("YOYO ---- This is where it is trying to register the RequestPostProcessorBehavior");
        }
        var hasAnyRegistrationsOfSubBehaviorType = services
            .Select(service => service.ImplementationType)
            .OfType<Type>()
            .SelectMany(type => type.GetInterfaces())
            .Where(type => type.IsGenericType)
            .Select(type => type.GetGenericTypeDefinition())
            .Any(type => type == subBehaviorType);

        if (typeof(RequestPostProcessorBehavior<,>) == behaviorType && !hasAnyRegistrationsOfSubBehaviorType)
        {
            Console.WriteLine("It doesn't think there are any subtypes therefore doesn't attempt to register the behavior");
        }

        if (hasAnyRegistrationsOfSubBehaviorType)
        {
            if (typeof(RequestPostProcessorBehavior<,>) == behaviorType)
            {
                Console.WriteLine("Register RequestPostProcessorBehavior if not already.");
            }
            services.TryAddEnumerable(new ServiceDescriptor(typeof(IPipelineBehavior<,>), behaviorType, ServiceLifetime.Transient));
        }
    }
AleRoe commented 1 year ago

Hi @StevenTCramer, thanks for investigating. But your findings don't quite apply to my situation, as my problem relates to IRequestPreProcessor. In my case, the IRequestPreProcessor is registered, but the Process() method is not being called prior to any other IPipelineBevahiors. Registering my IRequestPreProcessor prior to calling AddMediatR() does not help either.

After looking at the source code, I did some more testing, and as it turns out, explicitely adding the RequestPreProcessorBehavior() prior to any other custom behaviors solves my problem:

services.AddMediatR(config => {
                        config.RegisterServicesFromAssemblies(PluginProvider.GetAssemblies());
                        config.AddOpenBehavior(typeof(RequestPreProcessorBehavior<,>));
                        config.AddOpenBehavior(typeof(TelemetryBehavior<,>));
                        config.AddOpenBehavior(typeof(LoggingBehavior<,>));
                        config.AddOpenBehavior(typeof(ErrorHandlingBehavior<,>));
                    });

Now this is not quite what one would expect, as previously it was not required to register that behavior explicitely in order to process IRequestPreProcessor implementations. AddMediatR() should register the bult-in RequestPreProcessorBehavior() by default and prior to any other custom behaviors added using AddOpenBehavior(). I would assume this is a bug or should be noted as a breaking change for v12.x. @jbogard , I would appreciate if you could advise on this.

Kind regards Alexander

jbogard commented 1 year ago

It's called out elsewhere, but not in the upgrade guide. I'll add a section there that the "built-in" extra behaviors are only registered if AddMediatR finds any implementations.

Those extra behaviors were removed from the default pipeline because there's a performance hit to having them, so I only add them if I can determine they're needed. This behavior is now by design. If there's a missing scenario in your case, we can add it to the scanning or add other similar methods to register them.

AleRoe commented 1 year ago

Hi @jbogard! Thanks for getting back and the explanations. Funny thing though is that I do have an IRequestPreProcessor implementation in the main assembly, but its Process() method does not get called (the class is being loaded though, because the constructor is called). The only way to get it to work is by manually adding the RequestPreProcessorBehavior<,> prior to any other custom behaviors.

So this works:

services.AddMediatR(config => {
                        config.RegisterServicesFromAssemblies(PluginProvider.GetAssemblies());
                        config.AddOpenBehavior(typeof(RequestPreProcessorBehavior<,>));
                        config.AddOpenBehavior(typeof(TelemetryBehavior<,>));
                        config.AddOpenBehavior(typeof(LoggingBehavior<,>));
                        config.AddOpenBehavior(typeof(ErrorHandlingBehavior<,>));
                    });

Also, my code worked fine with v11.

I can live with manually adding the RequestPreProcessorBehavior, but it does seem strange and not how it's supposed to work. So I did some more testing:

And as it turns out, my IRequestPreProcessor is being called, but in the wrong order. It gets called after my custom IPipeLineBehaviors, instead of before. I verified this by first removing all my custom behaviors and then by adding only a single custom beviour that does not depend on the IRequestPreProcessor, and then checking the call-order using breakpoints.

So it seems that the built in pipeline discovery is adding the behaviors in the wrong order, at least when configuring AddMediatR() the way I do it (see my first post please).

Hope this helps. Kind regards Alexander

kipkoei commented 1 year ago

I'm having the exact same issue. Since upgrading to v12, My IRequestPreProcessors are being called after my IPipelineBehaviors. This breaks a lot in my application since I rely on PreProcessors to fetch some information in the database which is required for both validation and regular handling of the request. The validator is called before my preprocessor now, which means the data required to validate is not yet available.

See the following example:

Let's say we have a CreateTransactionCommand which depends on an Order. The Order is needed in the validation fase to check whether creating a transaction is even allowed and it is needed when handling the request to get the appropriate metadata. Since we don't want to do the same database call twice, we fetch the Order in a IRequestPreProcessor (which is generic so we can reuse it for similar requests):

abstract record WithOrder(int OrderId)
{
    public Order? Order { get; set; }
}

internal class WithOrderPreProcessor<T> : IRequestPreProcessor<T> where T : WithOrder
{
    private readonly IOrderRepository _orderRepository;

    public WithOrderPreProcessor(IOrderRepository orderRepository)
    {
        _orderRepository = orderRepository;
    }

    public async Task Process(T request, CancellationToken cancellationToken)
    {
        request.Order = await _orderRepository.Get(request.OrderId);
    }
}

Furthermore, we have a ValidationBehavior which looks as such:

public class ValidationBehaviour<TRequest, TResponse> : IPipelineBehavior<TRequest, TResponse> where TRequest : notnull
{
    private readonly IEnumerable<IValidator<TRequest>> _validators;

    public ValidationBehaviour(IEnumerable<IValidator<TRequest>> validators)
    {
        _validators = validators;
    }

    // Is somehow called before WithOrderPreProcessor.Process
    public async Task<TResponse> Handle(TRequest request, RequestHandlerDelegate<TResponse> next, CancellationToken cancellationToken)
    {
       // Handle the registered Validators for this request...

        return await next();
    }
}

The request is then handled like this:

public record CreateTransactionCommand(int OrderId) : WithOrder(OrderId), IRequest<Transaction>;

internal class CreateTransactionCommandHandler : IRequestHandler<CreateTransactionCommand, Transaction>
{
    public async Task<Transaction> Handle(CreateTransactionCommand request, CancellationToken cancellationToken)
    {
        // This is filled by the preprocessor
        var order = request.Order;

        // Do stuff...
    }
}

public class CreateTransactionCommandValidator : AbstractValidator<CreateTransactionCommand>
{
    public CreateTransactionCommandValidator()
    {
        // This fails since v12 because the ValidationBehavior is called before the WithOrderPreProcessor
        RuleFor(c => c.Order)
            .Cascade(CascadeMode.Stop)
            .NotNull();

        RuleFor(c => c.Order!.OrderStatus)
            .Equal(OrderStatus.Approved);
    }
}

The DI registration looks like this:

        services.AddValidatorsFromAssemblies(assemblies.Where(p => !p.IsDynamic));
        services.AddMediatR(o =>
        {
            o.RegisterServicesFromAssembly(applicationAssembly)
                .AddOpenBehavior(typeof(ValidationBehaviour<,>));
        });

Hope this helps to pinpoint the origin of the bug.

Kind regards, Lou

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] commented 1 year ago

This issue was closed because it has been stalled for 14 days with no activity.

jbogard commented 1 year ago

I'm going to address this in the next release, to put request pre/post processors before behaviors.

AleRoe commented 1 year ago

Thanks for revisiting this topic!