AsyncRequestHandler hides the Handle method

[OldrichDlouhy]

The AsyncRequestHandler class hides the Handle method, requiring tests to explicitly type the tested handler as IRequestHandler<TRequest>.

Consider following code:

public class MyRequest : IRequest {
}

public class MyHandler: AsyncRequestHandler<MyRequest> {
  protected override Task Handle(MyRequest request, CancellationToken cancellationToken) {
    // Do some work
  }
}

To test the code, one would expect following:

[Fact]
public async Task MyTest {
  var handler = new MyHandler();
  await handler.Handle(new MyRequest(), default);
  // Verify results
}

This results in compilation error CS0122: 'MyHandler.Handle(MyRequest, CancellationToken)' is inaccessible due to its protection level'

The workaround is to explicitly type the handler, making the code unnecessary bloated, especially with longer request and handler class names:

[Fact]
public async Task MyTest {
  IRequestHandler<MyRequest> handler = new MyHandler();
  await handler.Handle(new MyRequest(), default);
  // Verify results
}

[jbogard]

I probably should just remove those helper classes, they seem to confuse more than anything.

I only test handlers through the mediator, which is why I never hit this.

[phlashdev]

@jbogard as they don't seem to add any other functionality by default, I would also think that removing them might be a good idea. I also stumbled across this, and find it confusing as well. Because from my understanding the separation of the handler should make it so that it is easily possible to unit test the handler without the mediator.

But I could see that maybe some folks are used to this and won't be happy. Probably it would be a good thing to mark them as deprecated for some time, before ruling them out eventually.

[eskensberg]

Hey Jimmy,

The new requirement to have protected override handler stops me from being able to Unit Test the handler.

I found a way around it by adding a public handle() method that calls Protected handler.

Is there a cleaner way to deal with it? 😅

[lilasquared]

The AsyncRequestHandler is a helper class that simply hides returning the Unit.Value as the TResponse. You can simply use the interfaces for your handlers IRequestHandler<TRequest, TResponse>

[cheesi]

Hello!

For me personally, the confusing thing about this is not the AsyncRequestHandler, it abstracts away the Unit.Value, which is a good thing imho, as for me the Unit.Value is the actual problem. I can understand, that it was most likely added to simplify things, but if you have never worked with MediatR before, you will find this thing very confusing.

Why is it named Unit.Value? Especially if it hasn't any value? Wouldn't NoValue be a better name? Also I would not remove AsyncRequestHandler. Adding a return Unit.Value to each command doesn't provide any business value, it is an necessary implementation detail, that should be hidden way.

From my point of view there would be two good ways to go forward:

• Keep AsyncRequestHandler and make the Handle public, to make the unit testing easier without the workaround of setting the variable type to IRequestHandler<MyRequest>. @jbogard Do you have any problems with the execution time of your unit tests? We did a quick test setup on our side and saw the setup alone took about 160ms, which is too slow in our opinion (our "limit" is 100ms, which we try do achieve when it is possible)
• Another way I could see, would be using Default interface methods. Then the AsyncRequestHandler could be deprecated and the abstraction of it could be moved into the interface. I guess this would avoid any confusion between the interface and the abstract class.

What are your thoughts?

[jbogard]

Why is it named Unit.Value? Especially if it hasn't any value?

I don't remember to be honest. The Rx one is called "Unit.Default", which is equally weird to me. There's been discussions of adding a unit type to C# but it hasn't gone anywhere. But it's the value of the Unit type, that's as good as I could make it.

Why would test setup be any different? Are you calling Handle in your test setup? I don't know about timings, I don't unit test handlers, it's always an integration test. This is a decent example of what my handler tests look like:

https://github.com/jbogard/ContosoUniversityDotNetCore-Pages/blob/master/ContosoUniversity.IntegrationTests/Pages/Courses/EditTests.cs

What would the default interface look like? I've never used them before so I'm not quite sure I follow what you're suggesting here.

[jbogard]

Ah I remember why they can't be public. The overload only differs in the return type.

The way to "fix" this would be to make the method public and rename the protected method to "HandleCore" or similar. They can't have the same name.

[cheesi]

Hey @jbogard !

Thanks for the explanation with the Unit, I was simply not aware of this whole "concept" of a single non binary value (and that it even exists in other languages) that has no value at all.

Regarding the described the solutions, I just gave both variants a quick tests (only executed all the unit tests), and both worked for me.

Ah I remember why they can't be public. The overload only differs in the return type.

I cannot confirm that, it seems to work just fine (as the implemented Handle in AsyncRequestHandler is defined as IRequestHandler<TRequest, Unit>.Handle). I changed the access modifier from protected to public and had to adapt the two unit test implementations:

Then I was able to call the Handle method directly

Regarding the approach with the default interface methods, it should look something like this:

public interface IRequestHandler<in TRequest> : IRequestHandler<TRequest, Unit>
    where TRequest : IRequest<Unit>
{
    async Task<Unit> IRequestHandler<TRequest, Unit>.Handle(TRequest request, CancellationToken cancellationToken)
    {
        await Handle(request, cancellationToken).ConfigureAwait(false);
        return Unit.Value;
    }

    public new Task Handle(TRequest request, CancellationToken cancellationToken);
}

In this case, I also had to adapt the RequestHandler and some unit tests again. You can check the full code here: https://github.com/cheesi/MediatR (there are two branches, with one (new) commit each)

Regarding unit testing. Yeah we currently directly tested our handlers. Looks something like this:

var _handler = new OurQueryHandler(mock1, mock2);
_handler.Handle(request, cancellationToken);
// etc.

We also tried a similar approach as seen in your link:

//Arrange
var services = new ServiceCollection();
services.AddTransient((_) => mock1);
services.AddTransient((_) => mock2);
services.AddMediatR(typeof(OurCommand));
var provider = services.BuildServiceProvider();
var mediator = provider.GetRequiredService<IMediator>();

var command = new OurCommand();

//Act
var result = await mediator.Send(command);

//Assert
result.Should().Be(Unit.Value);
// etc.

The problem with this is, that the setup (IaC container) alone already takes about 160ms. The full unit test then even more.

[jbogard]

I cannot confirm that, it seems to work just fine

Changing the access modifier to public does not work. Nor does removing the explicit interface implementation:

public abstract class AsyncRequestHandler<TRequest> : IRequestHandler<TRequest>
    where TRequest : IRequest
{
    // These two overloads only differ in return types so this can't be public/protected/private
    public async Task<Unit> Handle(TRequest request, CancellationToken cancellationToken)
    {
        await Handle(request, cancellationToken).ConfigureAwait(false);
        return Unit.Value;
    }

    protected abstract Task Handle(TRequest request, CancellationToken cancellationToken);

Regarding the approach with the default interface methods, it should look something like this:

That example uses shadowing, I'm really not a fan of that. You'll get different behaviors depending on the variable's type.

The problem with this is, that the setup (IaC container) alone already takes about 160ms.

Don't use a container then. Just instantiate your handler directly, passing in whatever mocks/fakes you need. Containers are meant to be used in integration tests.

[cheesi]

Changing the access modifier to public does not work. Nor does removing the explicit interface implementation:

I changed the protected (the second) Handle to public, not the first one. This works because of the explicit interface implementation. Add a public to the first Handle doesn't work.

That example uses shadowing, I'm really not a fan of that

Isn't that exactly what the current implementation with the explicit interface definition is doing? If you have the type IRequestHandler, you can call the Handle, and if the type is AsyncRequestHandler, you can't.

Don't use a container then. Just instantiate your handler directly, passing in whatever mocks/fakes you need.

That's exactly what we are doing, and why it would be good, if the Handle of AsyncRequestHandler would be public, so we can call it without the explicit cast to IRequestHandler (e. g. the workaround of @OldrichDlouhy)

[jbogard]

Honestly, I'd rather just remove these classes than anything else. I don't use them anyway and they just seem to cause more problems than solve.

[bteamsoftware]

@jbogard Regardless of your decision, I'm a big fan of being able to unit test with a minimalistic code approach. Requiring a mediator instance to unit test external code seems to be backasswards to me. I should be able to new up a handler instance, passing in mock dependencies and invoke the Handle() method to execute my unit tests. Nice. Clean. Simple. If it makes sense to have a mediator to perform internal unit tests, fine. I'd really like for external unit tests to not be forced to do so. $0.02.

[jbogard]

You're not forced to use a mediator instance, that's not what this issue is. The issue is that you have to cast to get the explicitly implemented interface handler.