26

I've been looking into CQRS/MediatR lately. But the more I drill down the less I like it. Perhaps I've misunderstood something/everything.

So it starts out awesome by claiming to reducing your controller to this

public async Task<ActionResult> Edit(Edit.Query query)
{
    var model = await _mediator.SendAsync(query);

    return View(model);
}

Which fits perfectly with the thin controller guideline. However it leaves out some pretty important details - error handling.

Lets look at the default Login action from a new MVC project

public async Task<IActionResult> Login(LoginViewModel model, string returnUrl = null)
{
    ViewData["ReturnUrl"] = returnUrl;
    if (ModelState.IsValid)
    {
        // This doesn't count login failures towards account lockout
        // To enable password failures to trigger account lockout, set lockoutOnFailure: true
        var result = await _signInManager.PasswordSignInAsync(model.Email, model.Password, model.RememberMe, lockoutOnFailure: false);
        if (result.Succeeded)
        {
            _logger.LogInformation(1, "User logged in.");
            return RedirectToLocal(returnUrl);
        }
        if (result.RequiresTwoFactor)
        {
            return RedirectToAction(nameof(SendCode), new { ReturnUrl = returnUrl, RememberMe = model.RememberMe });
        }
        if (result.IsLockedOut)
        {
            _logger.LogWarning(2, "User account locked out.");
            return View("Lockout");
        }
        else
        {
            ModelState.AddModelError(string.Empty, "Invalid login attempt.");
            return View(model);
        }
    }

    // If we got this far, something failed, redisplay form
    return View(model);
}

Converting that presents us with a bunch of real world problems. Remember the goal is to reduce it to

public async Task<IActionResult> Login(Login.Command command, string returnUrl = null)
{
    var model = await _mediator.SendAsync(command);

    return View(model);
}

One possible solution to this is to return an CommandResult<T> instead of a model and then handle the CommandResult in a post action filter. As discussed here.

One implementation of the CommandResult could be like this

public interface ICommandResult  
{
    bool IsSuccess { get; }
    bool IsFailure { get; }
    object Result { get; set; }
}

source

However that doesn't really solve our problem in the Login action, because there are multiple failure states. We could add these extra failure states to ICommandResult but that is a great start for a very bloated class/interface. One might say it doesn't comply with Single Responsibility (SRP).

Another problem is the returnUrl. We have this return RedirectToLocal(returnUrl); piece of code. Somehow we need to handle conditional arguments based on the success state of the command. While I think that could be done (I'm not sure if the ModelBinder can map FromBody and FromQuery (returnUrl is FromQuery) arguments to a single model). One can only wonder what kind of crazy scenarios could come down the road.

Model validation have also become more complex along with returning error messages. Take this as an example

else
{
    ModelState.AddModelError(string.Empty, "Invalid login attempt.");
    return View(model);
}

We attach an error message along with the model. This sort of thing cannot be done using an Exception strategy (as suggested here) because we need the model. Perhaps you can get the model from the Request but it would be a very involved process.

So all in all I'm having a hard time converting this "simple" action.

I'm looking for inputs. Am I totally in the wrong here?

2
  • 10
    Sounds like you already understand the relevant concerns pretty well. There are plenty of "silver bullets" out there that have toy examples that prove their usefulness, but which inevitably fall over when they are squeezed by the reality of an actual, real-life application. Commented Jul 14, 2017 at 20:51
  • Check out MediatR Behaviors. It's basically a pipeline that allows you to tackle cross-cutting concerns.
    – fml
    Commented Jun 26, 2018 at 11:08

4 Answers 4

21

I think you're expecting too much of the pattern you're using. CQRS is specifically designed to address the difference in model between query and commands to the database, and MediatR is just in-process messaging library. CQRS doesn't claim to eliminate the need for business logic like you're expecting them to. CQRS is a pattern for data access, but your problems are with presentation layer--redirects, views, controllers.

I think you may be mis-applying the CQRS pattern to authentication. With login it cannot be a modelled as a command in CQRS because

Commands: Change the state of a system but do not return a value
- Martin Fowler CommandQuerySeparation

In my opinion authentication is a poor domain for CQRS. With authentication you need strongly consistent, synchronous request-response flow so you can 1. check user's credentials 2. create a session for the user 3. handle any of the variety of edge cases that you've identified 4. immediately grant or deny user in response.

Is CQRS/MediatR worth it when developing an ASP.NET application?

CQRS is a pattern that has very specific uses. It's purpose is to model queries and commands instead of having a model for records as used in CRUD. As systems become more complex, the demands of views are often more complex than just showing a single record or a handful of records, and a query can better model the needs of the application. Similarly commands can represent changes to many records instead of CRUD which you change single records. Martin Fowler warns

Like any pattern, CQRS is useful in some places, but not in others. Many systems do fit a CRUD mental model, and so should be done in that style. CQRS is a significant mental leap for all concerned, so shouldn't be tackled unless the benefit is worth the jump. While I have come across successful uses of CQRS, so far the majority of cases I've run into have not been so good, with CQRS seen as a significant force for getting a software system into serious difficulties.
- Martin Fowler CQRS

So to answer your question CQRS should not be the first resort when designing an application when CRUD is suitable. Nothing in your question gave me the indication that you have a reason to use CQRS.

As for MediatR, it's an in-process messaging library, it aims to decouple requests from request handling. You must again decide if it will improve your design to use this library. I'm personally not an advocate of in-process messaging. Loose-coupling can be achieved in simpler ways than messaging, and I would recommend you start there.

6
  • 2
    I 100% agree. CQRS is just a bit hyped, so I thought that "they" saw something I didn't. Because I'm having a hard time seeing the benefits of CQRS in CRUD web apps. So far the only scenario is CQRS + ES that makes sense to me.
    – Snæbjørn
    Commented Jul 18, 2017 at 6:52
  • Some guy on my new job decided to put MediatR on new ASP.Net system claiming it as an architecture. The implementation he made isn't DDD, nor SOLID, nor DRY, nor KISS. It is a small system full of YAGNI. And it has started long after some comments like yours, yours included. I'm trying to figure how I may refact the code to adapt its architecture gradually. I had the same opinion about CQRS outside a business layer and I'm glad there is several experieced devs thinking that way.
    – MFedatto
    Commented Sep 24, 2018 at 14:11
  • 1
    It's a bit ironic to affirm that the idea of incorporating CQRS/MediatR might be associated with a lot of YAGNI and a lack of KISS, when actually some of the popular alternatives, like the Repository pattern, promote YAGNI by bloating the repository class and forcing interfaces to specify a lot of CRUD operations on all of the root aggregates that want to implement such interfaces, often leaving those methods either unused or filled with "not implemented" exceptions. Because CQRS doesn't use these generalizations, it can implement only what is needed. Commented Aug 6, 2019 at 4:29
  • @LesairValmont Repository is only supposed to be CRUD. "specify a lot of CRUD operations" should only be 4 (or 5 with "list"). If you have more specific query access patterns they should not be in your repository interface. I've never run into a problem of unused repository methods. Can you give an example?
    – Samuel
    Commented Aug 6, 2019 at 9:53
  • 1
    @Samuel: I think the repository pattern is perfectly fine for certain scenarios, just as CQRS is. Actually, on a large application, there will be some parts whose best fit will be the repository pattern and others that would be more benefitted by CQRS. It depends on a lot of different factors, like the philosophy followed on that part of the application (e.g. task-based (CQRS) vs. CRUD (repo)), the ORM being used (if any), the modelling of the domain (e.g. DDD). For simple CRUD catalogs CQRS is definitively overkill, and some real-time collaborative features (like a chat) wouldn't use neither. Commented Sep 9, 2019 at 14:33
10

CQRS is more of a data management thing rather than and doesn't tend to bleed too heavily into an application layer (or Domain if you prefer, as it tends to be most often used in DDD systems). Your MVC application, on the other hand, is a presentation layer application and should be fairly well separated from the query/persistence core of the CQRS.

Another thing worth noting (given your comparison of the default Login method and desire for thin controllers): I wouldn't exactly follow default ASP.NET templates/boilerplate code as being anything we should worry about for best practices.

I like thin controllers as well, because they're very easy to read. Each controller I have usually has a "service" object that it pairs with that essentially handles the logic required by the controller:

public async Task<IActionResult> Login(LoginViewModel model, string returnUrl = null) {

    var result = _service.Login(model);
    switch (result) {
        case result.lockout: return View("Lockout");
        case result.ok: return RedirectToLocal(returnUrl);
        default: return View("GeneralError");
    }
}

Still thin enough, but we've not really changed how the code works, just delegate the handling to the service method, which really serves no purpose other than making the controller actions easy to digest.

Bear in mind, this service class is still responsible for delegating the logic to the model/application as required, it's really just a slight extension of the controller to keep the code neat. The service methods are generally pretty short as well.

I'm not sure the mediator would be doing anything conceptually different than that: moving some basic controller logic out of the controller and into somewhere else to be processed.

(I hadn't heard of this MediatR before, and a quick look at the github page doesn't seem to indicate that it's anything groundbreaking - certainly not something like CQRS - in fact, it looks to be something like just another abstraction layer you can put in to complicate the code by means of making it look simplier, but that's just my initial take)

7

I highly recommend you view Jimmy Bogard's NDC presentation on his approach to modelling http requests https://www.youtube.com/watch?v=SUiWfhAhgQw

You will then get a clear idea of what Mediatr is used for.

Jimmy does not have a blind adherence to patterns and abstractions. He's very pragmatic. Mediatr does clean up controller actions. As for the exception handling, I push that into a parent class called something like Execute. So you end up with a very clean controller action.

Something like:

public bool Execute<T>(Func<T> messageFunction)
{
    try
    {
        messageFunction();

        return true;
    }
    catch (ValidationException exception)
    {
        Errors = string.Join(Environment.NewLine, exception.Errors.Select(e => e.ErrorMessage));
        Logger.LogException(exception, "ValidationException caught in SiteController");
    }
    catch (SiteException exception)
    {
        Errors = exception.Message;
        Logger.LogException(exception);
    }
    catch (DbEntityValidationException dbEntityValidationException)
    {
        // Retrieve the error messages as a list of strings.
        var errorMessages = dbEntityValidationException.EntityValidationErrors
                .SelectMany(x => x.ValidationErrors)
                .Select(x => x.ErrorMessage);

        // Join the list to a single string.
        var fullErrorMessage = string.Join("; ", errorMessages);

        // Combine the original exception message with the new one.
        var exceptionMessage = string.Concat(dbEntityValidationException.Message, " The validation errors are: ", fullErrorMessage);

        Logger.LogError(exceptionMessage);

        // Throw a new DbEntityValidationException with the improved exception message.
        throw new DbEntityValidationException(exceptionMessage, dbEntityValidationException.EntityValidationErrors);                
    }
    catch (Exception exception)
    {
        Errors = "An error has occurred.";
        Logger.LogException(exception, "Exception caught in SiteController.");
    }

    // used to indicate that any transaction which may be in progress needs to be rolled back for this request.
    HttpContext.Items[UiConstants.Error] = true;

    Response.StatusCode = (int)HttpStatusCode.InternalServerError; // fail

    return false;
}

Usage looks a bit like this:

[Route("api/licence")]
public IHttpActionResult Post(LicenceEditModel licenceEditModel)
{
    var updateLicenceCommand = new UpdateLicenceCommand { LicenceEditModel = licenceEditModel };
    int licenceId = -1;

    if (Execute(() => _mediator.Send(updateLicenceCommand)))
    {
        return JsonSuccess(licenceEditModel);
    }

    return JsonError(Errors);
}

Hope that helps.

2
  • 1
    Isn't this what Behaviors in Mediatr are for? Commented Feb 27, 2020 at 7:17
  • @MathiasLykkegaardLorenzen yeah. I guess the code above would catch any exception thrown by a Behavior. In any case, I tend to use an Exception filter for that these days. Commented Aug 18, 2020 at 1:54
6

Many people (I did it too) confuse pattern with a library. CQRS is a pattern but MediatR is a library that you can use to implement that pattern

You can use CQRS without MediatR or any in-process messaging library and you can use MediatR without CQRS:

public interface IProductsWriteService
{
    void CreateProduct(CreateProductCommand createProductCommand);
}

public interface IProductsReadService
{
    ProductDto QueryProduct(Guid guid);
}

CQS would look like this:

public interface IProductsService
{
    void CreateProduct(CreateProductCommand createProductCommand);
    ProductDto QueryProduct(Guid guid);
}

In fact, you don't have to name your input models "Commands" like above CreateProductCommand. And input of your queries "Queries". Command and queries are methods, not models.

CQRS is about segregation of responsibility (read methods must be in a separate place from write methods - isolated). It's an extension to CQS but the difference is in CQS you can put these methods in 1 class. (no responsibility segregation, just command-query separation). See separation vs segregation

From https://martinfowler.com/bliki/CQRS.html:

At its heart is the notion that you can use a different model to update information than the model you use to read information.

There's confusion in what it says, it's not about having a separate model for input and output, it's about separation of responsibility.

CQRS and id generation limitation

There's one limitation you will face when using CQRS or CQS

Technically in the original description commands shouldn't return any value (void) which I find stupid because there's no easy way to get generated id from a newly created object: https://stackoverflow.com/questions/4361889/how-to-get-id-in-create-when-applying-cqrs.

so you have to generate id each time yourself instead of letting the database do it.


If you want to learn more: https://cqrs.files.wordpress.com/2010/11/cqrs_documents.pdf

3
  • 1
    I challenge your affirmation that a CQRS' command for persisting new data in a database being unable to return a newly database-generated Id is "stupid". I rather think this is a philosophical matter. Remember much of DDD and CQRS is about data immutability. When you think about it twice, you start to realize that the mere act of persisting data is a data mutation operation. And it's not only about new IDs, but it could be also fields filled with default data, triggers and stored procs that might alter your data as well. Commented Aug 6, 2019 at 4:17
  • Sure you can send some kind of event like "ItemCreated" with a new item as an argument. If you're dealing merely with request-response protocol and using "true" CQRS then id must be known upfront so you can pass it to a separate query function - absolutely nothing wrong with that. In many cases, CQRS is just overkill. You can live without it. It's nothing but a way of structuring your code and that depends mostly on what protocols you use too.
    – Konrad
    Commented Aug 6, 2019 at 8:11
  • And you can achieve data immutability without CQRS
    – Konrad
    Commented Aug 6, 2019 at 8:22

Not the answer you're looking for? Browse other questions tagged or ask your own question.