# Validation for a CQS system that throws an exception

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
{

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
{

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
{

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))
{
.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>
{

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

protected override void CreateRules(CancellationToken token = default(CancellationToken))
{
nameof(Command.DigimonTypeId)));

{
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))
nameof(Command.Name));
}
});
}
}


Actual Command Handler:

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

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);
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)
{

if (env.IsDevelopment())
{
app.UseDeveloperExceptionPage();
}
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.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
{

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

[HttpPost]
{
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
{

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
{
ICommand command, CancellationToken token = default(CancellationToken));

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

public class ResponseMediator : IResponseMediator
{

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

ICommand command, CancellationToken token = default(CancellationToken))
{
try
{
await _mediator.ExecuteAsync(command, token).ConfigureAwait(false);
}
catch (BrokenRuleException ex)
{
}
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();
}

IQuery<TResponse> query, CancellationToken token = default(CancellationToken))
{
try
{
var result = await _mediator.ExecuteAsync(query, token).ConfigureAwait(false);
return new OkObjectResult(result);
}
catch (BrokenRuleException ex)
{
}
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 });
}
}
}

• Why not validate in the handler? What happens if you don't validate in the controller and call ExecuteAsync, will it fail? Sep 12, 2017 at 9:19
• The way I have it currently, the validation happens in a handler decorator. It throws an exception before it hits the actual handler that does the actual work. I have just been reading that throwing an exception for validation is considered bad and wanted an opinion on it. Sep 12, 2017 at 11:15
• When you say "I'm using decorators so commands cannot return anything (other than Task for async)", I'm not convinced. Why could not a decorator be generic or at the very least return an easily composable type such as Task<bool>. The example also does not demonstrate a read request, only a write request. Sep 16, 2017 at 4:30
• Decorators have to have the same signature as everything below. CQS states commands cannot return anything. They need to be void. I return Task for async/await only. Returning a bool would do nothing as I need the errors. I could return a Result object but that violates CQS. Queries don't really need validation. Sep 16, 2017 at 13:02
• Decorators all share the same interface as the actual command. It is how you can chain them together so they can call the next one. The bottom command cant return anything so their interface is simply void (or Task) so the decorators must match if I want to use them as true decorators with SimpleInjector. Sep 16, 2017 at 13:13

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.

• Yes, I realize I need to rewrite that part. That top part isn't actually reviewed yet (just got it working before). I hate the attribute based Model validation built into ASP.NET. It is impossible to do ALL of the validation that way, so now you have to spread the validation all over the system. I'd rather they all be in the same place. All of my commands are going to be AJAX calls made by the system. I will NEVER do a full page post. How is Asp.NET going to convert the BrokenRule exception into a JSON message that the client is going to parse and show without me telling it. Sep 17, 2017 at 18:34
• NoPermission exception is there in case something is missed or you need to do some sort of more complex query to verify the user can do the query/command. Also, while my example uses Asp.NET, this code doesn't have to be utilized in ASP.NET. A WPF app can use the same thing. I don't know/care what the UI is, thus it needs to ensure the caller can do all of this stuff the same way regardless of the caller. Yes, I WANT to throw a new exception. I don't want the UI to accidently show sensitive information. That is why I handle it all there and make sure all the data is safe. Sep 17, 2017 at 18:37
• UserFriendly Exception is an exception thrown in the lower layers where it's message is safe to display to the user. There is no sensitive data in it. The actual error is already logged along with the Command that had an error and the actual data of the command itself with the user context. Everything is logged that you could ever need/want. You could rerun it without going through the UI. The way this is written, you could have 1 command or 200, I don't need to add any more code here and I have full control. Sep 17, 2017 at 18:41
• Ah, //rethrow comment is wrong. Should have said Throw safe error message. My guess is that you'd rather I return BadRequest(errors) on every single command even though I could instead handle all of that in one place at the top. I have to add an IsAjaxRequest check in that error routine yet, too. I haven't gone over all of that yet. There are a few reasons I hate the data annotation validation. 1. You have to remember to call Model.IsValid(). 2. It can't hit the database. 3. You can't show all errors at once if complex ones exist. 4. Translation/Actual message more difficult. Sep 17, 2017 at 18:54
• Model.IsValid also requires you post the page not do an Ajax vs (I thought). If you use angular as a front end then you can't even use this. It only works with a 1:1 model on a page which often can get very complex or there are multiple sections on the page that only need part of a model. I think for attributes you need custom classes that would hold a single rule or multiple when we can just do a quick check in the decorator and not spread that logic all over. Plus I want to support more than asp.net so I don't have to write that code again in a new UI. Sep 17, 2017 at 19:42

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
{

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

_mediator = mediator;
}

ICommand command, CancellationToken token = default(CancellationToken)) =>
HandleResult(await _mediator.ExecuteAsync(command, token).ConfigureAwait(false));

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

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)
{
}
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! :)