Validation for a CQS system that throws an exception

Question

I've been reading that exceptions should only be for something "exceptional" and not used to control the flow of a program. However, with a CQS implementation, this seems impossible unless I start hacking up the implementation to deal with it. I wanted to show how I implemented this to see if this is something really bad or not. I'm using decorators so commands cannot return anything (other than Task for async), so a ValidationResult is out of the question. Let me know!

This example will use ASP.NET MVC

Controller: (api)

[Route(ApiConstants.ROOT_API_URL_VERSION_1 + "DigimonWorld2Admin/Digimon/Create")]
public class CreateCommandController : MetalKidApiControllerBase
{
    private readonly IMediator _mediator;

    public CreateCommandController(IMediator mediator) => _mediator = mediator;

    [HttpPost]
    public async Task Post([FromBody]CreateCommand command) => 
        await _mediator.ExecuteAsync(command);
}

CommandExceptionDecorator is first in the chain:

public class CommandHandlerExceptionDecorator<TCommand> : ICommandHandler<TCommand> where TCommand : ICommand
{
    private readonly ICommandHandler<TCommand> _commandHandler;
    private readonly ILogger _logger;
    private readonly IUserContext _userContext;

    public CommandHandlerExceptionDecorator(ICommandHandler<TCommand> commandHandler, ILogger logger,
        IUserContext userContext)
    {
        Guard.IsNotNull(commandHandler, nameof(commandHandler));
        Guard.IsNotNull(logger, nameof(logger));

        _commandHandler = commandHandler;
        _logger = logger;
        _userContext = userContext;
    }

    public async Task ExecuteAsync(TCommand command, CancellationToken token = default(CancellationToken))
    {
        try
        {
            await _commandHandler.ExecuteAsync(command, token).ConfigureAwait(false);
        }
        catch (BrokenRuleException)
        {
            throw; // Let caller catch this directly
        }
        catch (UserFriendlyException ex)
        {
            await _logger.LogAsync(new LogEntry(LogTypeEnum.Error, _userContext,
                "Friendly exception with command: " + typeof(TCommand).FullName, ex, command)).ConfigureAwait(false);
            throw; // Let caller catch this directly
        }
        catch (NoPermissionException ex)
        {
            await _logger.LogAsync(new LogEntry(LogTypeEnum.Error, _userContext,
                "No Permission exception with command: " + typeof(TCommand).FullName, ex, command)).ConfigureAwait(false);
            throw new UserFriendlyException(CommonResource.Error_NoPermission); // Rethrow with a specific message
        }
        catch (ConcurrencyException ex)
        {
            await _logger.LogAsync(new LogEntry(LogTypeEnum.Error, _userContext,
                "Concurrency error with command: " + typeof(TCommand).FullName, ex, command)).ConfigureAwait(false);
            throw new UserFriendlyException(CommonResource.Error_Concurrency); // Rethrow with a specific message
        }
        catch (Exception ex)
        {
            await _logger.LogAsync(new LogEntry(LogTypeEnum.Error, _userContext,
                "Error with command: " + typeof(TCommand).FullName, ex, command)).ConfigureAwait(false);
            throw new UserFriendlyException(CommonResource.Error_Generic); // Rethrow with a specific message
        }
    }
}

Validation Decorator:

public class CommandHandlerValidatorDecorator<TCommand> : ICommandHandler<TCommand> where TCommand : ICommand
{
    private readonly ICommandHandler<TCommand> _commandHandler;
    private readonly IEnumerable<ICommandValidator<TCommand>> _validators;

    public CommandHandlerValidatorDecorator(
        ICommandHandler<TCommand> commandHandler,
        ICollection<ICommandValidator<TCommand>> validators)
    {
        Guard.IsNotNull(commandHandler, nameof(commandHandler));
        Guard.IsNotNull(validators, nameof(validators));

        _commandHandler = commandHandler;
        _validators = validators;
    }

    public async Task ExecuteAsync(TCommand command, CancellationToken token = default(CancellationToken))
    {
        var brokenRules = (await Task.WhenAll(_validators.AsParallel()
                .Select(a => a.ValidateCommandAsync(command, token)))
            .ConfigureAwait(false)).SelectMany(a => a).ToList();

        if (brokenRules.Any())
        {
            throw new BrokenRuleException(brokenRules);
        }

        await _commandHandler.ExecuteAsync(command, token).ConfigureAwait(false);
    }
}

Other decorators exist but aren't important for this question.

Example of a Command Handler Validator: (Each rule is run on its own thread under the covers)

public class CreateCommandValidator : CommandValidatorBase<CreateCommand>
{
    private readonly IDigimonWorld2ContextFactory _contextFactory;

    public CreateCommandValidator(IDigimonWorld2ContextFactory contextFactory)
    {
        _contextFactory = contextFactory;
    }

    protected override void CreateRules(CancellationToken token = default(CancellationToken))
    {
        AddRule(() => Validate.If(string.IsNullOrEmpty(Command.Name))
            ?.CreateRequiredBrokenRule(DigimonResources.Digipedia_CreateCommnad_Name, nameof(Command.Name)));
        AddRule(() => Validate.If(Command.DigimonTypeId == 0)
            ?.CreateRequiredBrokenRule(DigimonResources.Digipedia_CreateCommnad_DigimonTypeId,
                nameof(Command.DigimonTypeId)));
        AddRule(() => Validate.If(Command.RankId == 0)
            ?.CreateRequiredBrokenRule(DigimonResources.Digipedia_CreateCommnad_RankId, nameof(Command.RankId)));

        AddRule(async () =>
        {
            using (var context = _contextFactory.Create(false))
            {
                return Validate.If(
                        !string.IsNullOrEmpty(Command.Name) &&
                        await context.Digimons
                            .AnyAsync(a => a.Name == Command.Name, token)
                            .ConfigureAwait(false))
                    ?.CreateAlreadyInUseBrokenRule(DigimonResources.Digipedia_CreateCommnad_Name, Command.Name,
                        nameof(Command.Name));
            }
        });
    }
}

Actual Command Handler:

public class CreateCommandValidatorHandler : ICommandHandler<CreateCommand>
{
    private const int ExpectedChangesCount = 1;

    private readonly IDigimonWorld2ContextFactory _contextFactory;
    private readonly IMapper<CreateCommand, DigimonEntity> _mapper;

    public CreateCommandValidatorHandler(
        IDigimonWorld2ContextFactory contextFactory, 
        IMapper<CreateCommand, DigimonEntity> mapper)
    {
        _contextFactory = contextFactory;
        _mapper = mapper;
    }

    public async Task ExecuteAsync(CreateCommand command, CancellationToken token = default(CancellationToken))
    {
        using (var context = _contextFactory.Create())
        {
            var entity = _mapper.Map(command);
            context.Digimons.Add(entity);
            await context.SaveChangesAsync(ExpectedChangesCount, token).ConfigureAwait(false);
        }
    }
}

When an exception is thrown for broken validation rules, the normal flow is broken. Each step assumes that the previous step succeeded. This makes the code very clean as we don't care about failures during the actual implementation. All commands end up going through this same logic so we only have to write it once. At the very top of MVC, I handle the BrokenRuleException like this: (I do AJAX calls, not full page posts)

internal static class ErrorConfiguration
{
    public static void Configure(
        IApplicationBuilder app, IHostingEnvironment env, ILoggerFactory loggerFactory, IConfigurationRoot configuration)
    {
        loggerFactory.AddConsole(configuration.GetSection("Logging"));
        loggerFactory.AddDebug();

        if (env.IsDevelopment())
        {
            app.UseDeveloperExceptionPage();
            app.UseBrowserLink();
        }
        else
        {
            app.UseExceptionHandler("/Home/Error");
        }

        app.UseExceptionHandler(errorApp =>
        {
            errorApp.Run(async context =>
            {
                var error = context.Features.Get<IExceptionHandlerFeature>()?.Error;

                context.Response.StatusCode = GetErrorStatus(error);
                context.Response.ContentType = "application/json";

                var message = GetErrorData(error);
                await context.Response.WriteAsync(message, Encoding.UTF8);
            });
        });
    }

    private static string GetErrorData(Exception ex)
    {
        if (ex is BrokenRuleException brokenRules)
        {
            return JsonConvert.SerializeObject(new
            {
                BrokenRules = brokenRules.BrokenRules
            });
        }

        if (ex is UserFriendlyException userFriendly)
        {
            return JsonConvert.SerializeObject(new
            {
                Message = userFriendly.Message
            });
        }

        return JsonConvert.SerializeObject(new
        {
            Message = MetalKid.Common.CommonResource.Error_Generic
        });
    }

    private static int GetErrorStatus(Exception ex)
    {
        if (ex is BrokenRuleException || ex is UserFriendlyException)
        {
            return (int)HttpStatusCode.BadRequest;
        }
        return (int)HttpStatusCode.InternalServerError;
    }
}

BrokenRule class has the message and a relation field. This relation allows the UI to tie a message to something on the page (i.e. a , or form label, etc.) to display the message in the correct location

public class BrokenRule
{      
    public string RuleMessage { get; set; }
    public string Relation { get; set; }

    public BrokenRule() { }

    public BrokenRule(string ruleMessage, string relation = "")
    {
        Guard.IsNotNullOrWhiteSpace(ruleMessage, nameof(ruleMessage));

        RuleMessage = ruleMessage;
        Relation = relation;
    }
}

If I don't do it like this, the controller would have to call a validation class first, look at the results, and then return it as a 400 with the correct response. Most likely, you would have to call a helper class to convert it correctly. However, then the controller would end up looking like this or something similar:

[Route(ApiConstants.ROOT_API_URL_VERSION_1 + "DigimonWorld2Admin/Digimon/Create")]
public class CreateCommandController : MetalKidApiControllerBase
{
    private readonly IMediator _mediator;
    private readonly ICreateCommandValidator _validator;

    public CreateCommandController(IMediator mediator, ICreateCommandValidator validator) 
    {
        _mediator = mediator;
        _validator = validator
    }

    [HttpPost]
    public async Task Post([FromBody]CreateCommand command)
    {
        var validationResult = _validator.Validate(command);
        if (validationResult.Errors.Count > 0) 
        {
           return ValidationHelper.Response(validationResult);
        }
        return await _mediator.ExecuteAsync(command);
    }
}

This validation check would need to be repeated on every single command. If it was forgotten, there would be big consequences. With the exception style, the code remains compact and developers don't have to worry about adding that redundant code everytime.

I would really love to get everyones feedback. Thanks!

* Edit * Another possible option would be to have another "mediator" for the response itself that could run validation directly first and then continue on:

[Route(ApiConstants.ROOT_API_URL_VERSION_1 + "DigimonWorld2Admin/Digimon/Create")]
public class CreateCommandController : MetalKidApiControllerBase
{
    private readonly IResultMediator _mediator;

    public CreateCommandController(IResultMediator mediator) => _mediator = mediator;

    [HttpPost]
    public async Task<IHttpAction> Post([FromBody]CreateCommand command) => 
        await _mediator.ExecuteAsync(command);
}

Inside this new ResultMediator class, it would look up the CommandValidator and if there were any validation errors it would simply return BadRequest(new { BrokenRules = brokenRules}) and call it good. Is this something that each UI will just have to create and handle? If there is an exception during this call, however, we'd have to handle that in this mediator directly. Thoughts?

Edit 2: Maybe I should explain decorators really quick. For example, I have this CreateCommand (with a specific namespace in this case). There is a CommandHandler that handles this command defined is ICommandHandler. This interface has one method defined as:

Task ExecuteAsync(TCommand, CancellationToken token);

Each decorator also implements this same interface. Simple Injector allows you to define these new classes, like CommandHandlerExceptionDecorator and CommandHandlerValidationDecorator using that same interface. When the code at the top wants to call the CreateCommandHandler with that CreateCommand, SimpleInjector will first call the last defined decorator (The ExceptionDecorator in this case). This decorator handles all exceptions and logs them for ALL commands since it is defined generically. I only have to write that code once. It then forwards the call to the next decorator. In this case, it could be the ValidationDecorator. This will validated the CreateCommand to make sure it is valid. If it is, it will forward it on to the actual command where it does the creation of the entity. If not, it throws an exception since I can't return anything back. CQS states that commands must be void. Task is okay, though, since it is just to implement the async/await style. It is effectively returning nothing. Since I have no way to return broken rules there, I throw an exception. I just wanted to know if this approach was okay since it makes all the code at all the different levels specific to the task (SRP) and I only have to write it once across all of the commands now and in the future. Any UI can simply catch any BrokenRuleException that comes out and knows what to do with that data to display it. This can be written generically so we can display any errors for any command as well (due to the Relation property on the rule). That way, we write this all once and are done. The issue, however, is that I keep seeing that User Validation isn't "exceptional", so we shouldn't throw an exception. The problem with that is it will make my code far more complex and less maintainable if I truly follow that path instead since every command caller has to write the same code to do that. If I only throw one BrokenRuleException for any validation errors, is that still okay?

Edit 3:

I decided to take a ResponseMediator approach at the top to make things more clear:

public interface IResponseMediator
{
    Task<IActionResult> ExecuteAsync(
        ICommand command, CancellationToken token = default(CancellationToken));

    Task<IActionResult> ExecuteAsync<TResponse>(
        IQuery<TResponse> query, CancellationToken token = default(CancellationToken));
}

public class ResponseMediator : IResponseMediator
{
    private readonly IMediator _mediator;

    public ResponseMediator(IMediator mediator)
    {
        _mediator = mediator;
    }

    public async Task<IActionResult> ExecuteAsync(
        ICommand command, CancellationToken token = default(CancellationToken))
    {
        try
        {
            await _mediator.ExecuteAsync(command, token).ConfigureAwait(false);
        }
        catch (BrokenRuleException ex)
        {
            return new BadRequestObjectResult(ex.BrokenRules);
        }
        catch (UserFriendlyException ex)
        {
            return new BadRequestObjectResult(new { Message = ex.Message });
        }
        catch (NoPermissionException)
        {
            return new UnauthorizedResult();
        }
        catch (DataNotFoundException)
        {
            return new NotFoundResult();
        }
        catch (Exception)
        {
            return new BadRequestObjectResult(new { Message = CommonResource.Error_Generic });
        }
        return new OkResult();
    }

    public async Task<IActionResult> ExecuteAsync<TResponse>(
        IQuery<TResponse> query, CancellationToken token = default(CancellationToken))
    {
        try
        {
            var result = await _mediator.ExecuteAsync(query, token).ConfigureAwait(false);
            return new OkObjectResult(result);
        }
        catch (BrokenRuleException ex)
        {
            return new BadRequestObjectResult(ex.BrokenRules);
        }
        catch (UserFriendlyException ex)
        {
            return new BadRequestObjectResult(new { Message = ex.Message });
        }
        catch (DataNotFoundException)
        {
            return new NotFoundResult();
        }
        catch (NoPermissionException)
        {
            return new UnauthorizedResult();
        }
        catch (Exception)
        {
            return new BadRequestObjectResult(new { Message = CommonResource.Error_Generic });
        }
    }
}

Answer 1

What you have written is virtually your own model validation framework as if the ASP.NET one didn't exist: Adding Validation to the Model. Decorating the CreateCommand's properties with appropriate attributes would be the right way to validate it. Other then this, there is not much more to say. Most of this code should be just thrown away and instead you should use APS.NET to do the job.

---

As far as the code is concerned I think this is the worst part of it:

context.Response.ContentType = "application/json";

where you decide that the response will always be served as json and do not allow any content negotiation allowing other formats that you might have defined.

This is then supported by an even worse implementation of manually serializing the message and the exception:

private static string GetErrorData(Exception ex)
{
  if (ex is BrokenRuleException brokenRules)
  {
      return JsonConvert.SerializeObject(new
      {
          BrokenRules = brokenRules.BrokenRules
      });
  }

  if (ex is UserFriendlyException userFriendly)
  {
      return JsonConvert.SerializeObject(new
      {
          Message = userFriendly.Message
      });
  }

  return JsonConvert.SerializeObject(new
  {
      Message = MetalKid.Common.CommonResource.Error_Generic
  });
}

ASP.NET should take care of it not you. Your job is to just give it some data that should be turned into a response. It is not the right place to decide about the format of the response or even serializing it by yourself.

---

When writing decorators you should not actually use the word decorator but give it a more meaningful name that clearly expresses its purpose.

public async Task ExecuteAsync(TCommand command, CancellationToken token = default(CancellationToken))
{
  try
  {
      await _commandHandler.ExecuteAsync(command, token).ConfigureAwait(false);
  }
  catch (BrokenRuleException)
  {
      throw; // Let caller catch this directly
  }
  catch (UserFriendlyException ex)
  {
      await _logger.LogAsync(new LogEntry(LogTypeEnum.Error, _userContext,
          "Friendly exception with command: " + typeof(TCommand).FullName, ex, command)).ConfigureAwait(false);
      throw; // Let caller catch this directly
  }
  catch (NoPermissionException ex)
  {
      await _logger.LogAsync(new LogEntry(LogTypeEnum.Error, _userContext,
          "No Permission exception with command: " + typeof(TCommand).FullName, ex, command)).ConfigureAwait(false);
      throw new UserFriendlyException(CommonResource.Error_NoPermission); // Rethrow with a specific message
  }
  catch (ConcurrencyException ex)
  {
      await _logger.LogAsync(new LogEntry(LogTypeEnum.Error, _userContext,
          "Concurrency error with command: " + typeof(TCommand).FullName, ex, command)).ConfigureAwait(false);
      throw new UserFriendlyException(CommonResource.Error_Concurrency); // Rethrow with a specific message
  }
  catch (Exception ex)
  {
      await _logger.LogAsync(new LogEntry(LogTypeEnum.Error, _userContext,
          "Error with command: " + typeof(TCommand).FullName, ex, command)).ConfigureAwait(false);
      throw new UserFriendlyException(CommonResource.Error_Generic); // Rethrow with a specific message
  }
}

This is another scary part because it does so many things that ASP.NET actully should be doing. For example the NoPermissionException should probably be handled by an authorization system like Implementing authorization in ASP.NET MVC.

It should also not be doing any logging. All this should be handled by the app.UseExceptionHandler.

Then there is also this:

throw new UserFriendlyException(CommonResource.Error_NoPermission); // Rethrow with a specific message

where you write Rethrow with a specific message but in fact this isn't rethrowing anything. It throws a new exception with a new stack-trace without an inner one that is completely ignored. It might be logged but at this point you loose the connection between the two.

What is by the way a UserFriendlyException? An exception should by its name already give a good explanation for what went wrong like: IndexOutOfRangeException or ArgumentException. But this one tells me that the user was too friendly and that was a reason for throwing it ;-)

---

And this is just for a single CreateCommand. If you now have a dozen of APIs there will be hundereds of lines of code that are completely unnecessary because ASP.NET can already handle most of it when used properly.

Answer 2

After months of going back and forth, I broke down and ended up returning IResult or IResult< T > from all commands/queries. The IResult looks like this:

public interface IResult
{
    bool IsSuccessful { get; }
    ICollection<BrokenRule> BrokenRules { get; }
    bool HasNoPermissionError { get; }
    bool HasNoDataFoundError { get; }
    bool HasConcurrencyError { get; }
    string ErrorMessage { get; }
}

public interface IResult<T> : IResult
{
    T Data { get; }
}

There are specific scenarios in my logic where I can easily not throw an exception and have the above layers just check those boolean flags to determine what to show the end user. If a real exception occurs, I can put that on the ErrorMessage property and pull it from there.

Looking at CQS, I realized that returning an IResult with this for a command is okay because it isn't returning any information about the actual process. Either it succeeded (IsSuccessful = true) or something bad occurred, meaning I need to show something bad happened to the end user and the command never ran, anyway.

I created some helper methods for creating results so coder's don't really care. The only thing that gets added to the main implementation is:

ResultHelper.Successful();

or

ResultHelper.Successful(data); (returns IResult<T>)

That way, the rest of those scenarios are being handled by the other decorators so returning an IResult doesn't become cumbersome.

At the UI level, I created a ResponseMediator instead that returned IActionResult items. This will handle the IResult and return the appropriate data/status code. i.e. (ICqsMediator is what IMediator used to be)

public class ResponseMediator : IResponseMediator
{
    private readonly ICqsMediator _mediator;

    public ResponseMediator(ICqsMediator mediator)
    {
        Guard.IsNotNull(mediator, nameof(mediator));

        _mediator = mediator;
    }

    public async Task<IActionResult> ExecuteAsync(
        ICommand command, CancellationToken token = default(CancellationToken)) =>
        HandleResult(await _mediator.ExecuteAsync(command, token).ConfigureAwait(false));

    public async Task<IActionResult> ExecuteAsync<TResponse>(
        ICommandQuery<TResponse> commandQuery, CancellationToken token = default(CancellationToken)) =>
        HandleResult(await _mediator.ExecuteAsync(commandQuery, token).ConfigureAwait(false));

    public async Task<IActionResult> ExecuteAsync<TResponse>(
        IQuery<TResponse> query, CancellationToken token = default(CancellationToken)) =>
        HandleResult(await _mediator.ExecuteAsync(query, token).ConfigureAwait(false));

    private IActionResult HandleResult<T>(IResult<T> result)
    {
        if (result.IsSuccessful)
        {
            return new OkObjectResult(result.Data);
        }
        return HandleResult((IResult)result);
    }

    private IActionResult HandleResult(IResult result)
    {
        if (result.IsSuccessful)
        {
            return new OkResult();
        }
        if (result.BrokenRules?.Any() == true)
        {
            return new BadRequestObjectResult(new {result.BrokenRules});
        }
        if (result.HasConcurrencyError)
        {
            return new BadRequestObjectResult(new {Message = CommonResource.Error_Concurrency});
        }
        if (result.HasNoPermissionError)
        {
            return new UnauthorizedResult();
        }
        if (result.HasNoDataFoundError)
        {
            return new NotFoundResult();
        }
        if (!string.IsNullOrEmpty(result.ErrorMessage))
        {
            return new BadRequestObjectResult(new {Message = result.ErrorMessage});
        }
        return new BadRequestObjectResult(new {Message = CommonResource.Error_Generic});
    }
}

This way, I don't have to handle any exceptions to change the flow of the code except when a truly exceptional thing happens. It is caught like this in the top level exception decorator handler:

 public async Task<IResult> ExecuteAsync(TCommand command,
        CancellationToken token = default(CancellationToken))
    {
        try
        {
            return await _commandHandler.ExecuteAsync(command, token).ConfigureAwait(false);
        }
        catch (UserFriendlyException ex)
        {
            await _logger.LogAsync(new LogEntry(LogTypeEnum.Error, _userContext,
                    "Friendly exception with command: " + typeof(TCommand).FullName, ex, command), token)
                .ConfigureAwait(false);
            return ResultHelper.Error(ex.Message);
        }
        catch (DataNotFoundException ex)
        {
            await _logger.LogAsync(new LogEntry(LogTypeEnum.Error, _userContext,
                    "Data Not Found exception with command: " + typeof(TCommand).FullName, ex, command), token)
                .ConfigureAwait(false);
            return ResultHelper.NoDataFoundError();
        }
        catch (ConcurrencyException ex)
        {
            await _logger.LogAsync(new LogEntry(LogTypeEnum.Error, _userContext,
                    "Concurrency error with command: " + typeof(TCommand).FullName, ex, command), token)
                .ConfigureAwait(false);
            return ResultHelper.ConcurrencyError();
        }
        catch (Exception ex)
        {
            await _logger.LogAsync(new LogEntry(LogTypeEnum.Error, _userContext,
                "Error with command: " + typeof(TCommand).FullName, ex, command), token).ConfigureAwait(false);
            return ResultHelper.Error(CommonResource.Error_Generic);
        }
    }

Edit

The last point here is that Exceptions actually are pretty slow. When I throw an exception, it ends up taking around 150 ms to get the result back. When I switched to not throwing an exception, it would get around 25ms response for the exact same check/data. That is 6 times slower! Thus, avoiding exceptions seems like a really good thing to do! :)