Fix AutoRegisterRequestProcessors to include all implementations

[hisuwh]

This is how this previously worked. When this option was restored the addIfAlreadyExists property was mistakenly set to false.

I have updated the tests and changed this property.

[jbogard]

Will this double-register processors?

[hisuwh]

I don't believe so. I didn't want to make the tests too brittle by asserting based upon the Pre/Post-processors currently defined.
I.e. I could have done

preProcessors.Count.ShouldBe(4);

But then if another PreProcessor is added to cover another test case this test will break.

Without the fix the second assertion here fails - because it only registers the first type:

preProcessors.ShouldContain(p => p != null && p.GetType() == typeof(FirstConcretePreProcessor));
preProcessors.ShouldContain(p => p != null && p.GetType() == typeof(NextConcretePreProcessor)); // fail

I'm happy to change the tests here to make them more explicit - just be aware this will make them potentially more brittle.

Looking at the code: https://github.com/jbogard/MediatR/blob/cccd3782a14c956db1920e153295aa4271511025/src/MediatR/Registration/ServiceRegistrar.cs#L62

The only way I can see a sort of duplicate registration would be if you had something like this:

public class Processor1 : IRequestPreProcessor<Ping>
{
}

public class Processor2 : Processor1, IRequestPreProcessor<Ping>
{
}

But I can't think of a legitimate reason to do that anyway.

[asimmon]

Hi @jbogard, I agree with @hisuwh's claim that there is a bug in the way the new AutoRegisterRequestProcessors boolean flag works. Here's the simplest repro case I've got. The pre/post processors are simply not executed. I believe this is the most trivial use of processors.

// <PackageReference Include="MediatR" Version="12.2.0" />
// <PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="8.0.0" />

using MediatR;
using MediatR.Pipeline;
using Microsoft.Extensions.DependencyInjection;

var services = new ServiceCollection().AddMediatR(x =>
{
    x.AutoRegisterRequestProcessors = true;
    x.RegisterServicesFromAssemblyContaining<Program>();
});

await using var serviceProvider = services.BuildServiceProvider();
await serviceProvider.GetRequiredService<IMediator>().Send(new MyRequest());

public class MyRequest : IRequest;

public class MyRequestHandler : IRequestHandler<MyRequest>
{
    public Task Handle(MyRequest request, CancellationToken cancellationToken) => Task.CompletedTask;
}

public class MyRequestProcessor : IRequestPreProcessor<MyRequest>, IRequestPostProcessor<MyRequest, Unit>
{
    public Task Process(MyRequest request, CancellationToken cancellationToken) => Task.CompletedTask;
    public Task Process(MyRequest request, Unit response, CancellationToken cancellationToken) => Task.CompletedTask;
}

It's very unfortunate as we were about to leverage pre/post processors in one of my team's features. Our only option right now is to downgrade MediatR to 12.0.1, which involves not benefiting from the recent improvements and other bug fixes.

[bryanboettcher]

+1 for "this is impacting me"

[hisuwh]

@jbogard do you want me to change anything on this or add any more tests? I'm happy to do so.
Otherwise, can we get this in?

[jbogard]

That scanning stuff is code I copied from some other DI projects, so you can see why I don't really want to copy more behaviors from DI containers because that code is...complicated

[hisuwh]

@jbogard when are you planning new release with this in please 🙏🥺

[asimmon]

I thought this change would fix this very simple usecase but it still doesn't work with 12.3.0:

#989 (comment)