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 div, 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<IHttpResult> Post([FromBody]CreateCommand command)
    {
        var validationResult = _validator.Validate(command);
        if (validationResult.Errors.Count > 0) 
        {
           return ValidationHelper.Response(validationResult);
        }
        await _mediator.ExecuteAsync(command);
        return Ok();
    }
}

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?

Answer 1

I use a very similar pattern for my commands, based on MediatR from Jimmy Bogard (using the pipelines feature to add multiple decorators around my handlers), and using Fluent Validation for the validators.

I have gone through a similar thought process to you - my validators throw exceptions (Which are caught in a similar way to yours, at the top of MVC), but there are any number of people who will tell you this should not be done - not least my favourite tech oracle Martin Fowler

A few thoughts:

• I've grown slightly wary of some of the 'Thou Shalt Not...' axioms that we encounter as developers, and believe that continuing your progress towards a clean, DRY pattern for commands and validators is more important than following that rule.
• In a similar manner, 'Thou Shalt Not Return Anything From a Command' could be subverted in my opinion, and seems to be the subject of some debate anyway, allowing you to utilise the notification pattern linked above.
• Finally, does your user facing app do any client side validation? Perhaps one could argue that if the client side app should prevent the command being in an invalid state, then exceptions on the server side would truly be exceptional anyway, and the issue goes away.

Hope that helps in some small way. I shall be interested in any other views on this.

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(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. This 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);
        }
    }