7
\$\begingroup\$

I've created a "framework" or more of a library for a CQS implementation with a few decorators. Using ASP.NET Core for the front end, I wanted an opinion about how this looks/feels. I changed the default mvc approach and created a Features folder that stores both the Views and Controllers in the same folder. Then each command/query ends up being in its own folder in the implementation. I have the following decorators: Exception/Logging, Retry, Cache, Cache Invalidation, Validation, and Permission. Any input at all is welcomed!

This will be along post, but it'll be an interesting read, in my opinion. :)

First, CQS = Command Query Separation. Essentially, queries return data and do not modify state and commands modify state but return no data. There is a gray area here where you need to run a command but then return the data (i.e. Queues, Stacks, primary keys from identity fields). Thus, I created a "CommandQuery" for those scenarios where just a command won't be good enough. CQS normally has ICommandHandler<> and IQueryHandler<> interfaces to support decorators. Decorators are classes that can add functionality to these classes without changing them. i.e. The calls are chained. They implement the same interface, but I use SimpleInjector to make those classes run before/after the actual implementation. All code becomes narrow and easy to maintain. Also, you can add more decorators to all commands/queries with one line of code.

I tried to follow SOLID through all the layers here with the intention that every piece of code will be unit testable at a high level. The problems being solved inside may end up more difficult than that, but I digress. :)

(My mock implementation deals with Digimon World 2... Don't ask) :)

Controller

[Route(ApiConstants.RootApiUrlVersion1 + "DigimonWorld2Admin/Digimon/Create")]
public class CreateCommandQueryController : MetalKidApiControllerBase
{
    private readonly IResponseMediator _responseMediator;

    public CreateCommandQueryController(IResponseMediator responseMediator) =>
        _responseMediator = responseMediator;

    [HttpPost]
    public async Task<IActionResult> Post([FromBody] CreateCommandQuery commandQuery) =>
        await _responseMediator.ExecuteAsync(commandQuery).ConfigureAwait(false);
}

Since we can route the different verbs to different classes, we can focus on only 1 method at a time per class. I'm using Mediators everywhere so the code looks the same across all classes. Mediators tie a request 1:1 with an implementation via a specific generic interface. I thought this was really the Service Locator anti-pattern, but it isn't because you still know the mediator dependency. I also use async through the entire call stack because Task.CompletedTask, if needed, has very little overhead and not blocking threads is the way to go.

ResponseMediator

public class ResponseMediator : IResponseMediator
{
    private readonly ICqsMediator _mediator;
    private readonly ICommonResource _resource;

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

        _mediator = mediator;
        _resource = resource;
    }

    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> query, CancellationToken token = default(CancellationToken)) =>
        HandleResult(await _mediator.ExecuteAsync(query, 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 = _resource.ErrorConcurrency});
        }
        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 = _resource.ErrorGeneric});
    }
}

All my queries/commands/command queries end up returning IResult because I found throwing exceptions is really expensive. Thus, this lets me deal with any issues. IResult is returned by Command and doesn't return any info about the actual command itself, so it doesn't violate CQS. Query returns IResult<T> that stores the data.

I'll show an example here of the Create Command through the layers.

CommandQuery

public class CreateCommandQuery : ICommandQuery<int>, ICommandQueryRetry
{
    public string Name { get; set; }
    public int DigimonTypeId { get; set; }
    public int RankId { get; set; }
    public int? LearnedSkillId { get; set; }
    public int? SpecialtyId { get; set; }
    public string Found { get; set; }
    public bool? HasImage { get; set; }
    public int? HP { get; set; }
    public int? MP { get; set; }
    public int? Att { get; set; }
    public int? Def { get; set; }
    public int? Speed { get; set; }

    int ICommandQueryRetry.MaxRetries => 3;
}

ICommandQueryRetry will cause this to retry general exceptions up to 3 times before failing.

For caching, you just add a different interfaces (a few different ones for different timeframes) and it takes care of it for you.

public class ListQuery : IQuery<ICollection<ListDto>>, IQueryCacheableAbsoluteTimespan
{
    public string Name { get; set; }

    TimeSpan IQueryCacheableAbsoluteTimespan.ExpireTimeout => TimeSpan.FromDays(1);
}

CommandQueryHandler

public class CreateCommandQueryHandler : ICommandQueryHandler<CreateCommandQuery, int>
{
    private readonly IRepositoryMediator _repositoryMediator;

    public CreateCommandQueryHandler(IRepositoryMediator repositoryMediator) =>
        _repositoryMediator = repositoryMediator;

    public async Task<IResult<int>> ExecuteAsync(
        CreateCommandQuery commandQuery, CancellationToken token = default(CancellationToken))
    {
        var digimonId = await _repositoryMediator.ExecuteAsync<int>(commandQuery, token).ConfigureAwait(false);
        return ResultHelper.Successful(digimonId);
    }
}

Validation

The validation decorator will run validation on multiple threads (if you want) and async will not block. I didn't use expressions since a lot of times validation involves multiple properties and has a lot of overhead. The BrokenRule() parameter is the "Relation" and it can be used to tie the rule somewhere on the UI.

public class CreateCommandQueryValidator : CommandQueryValidatorBase<CreateCommandQuery, int>
{
    private readonly IValidationRepositoryMediator _validationRepositoryMediator;
    private readonly IDigimonWorld2AdminResources _resources;

    public CreateCommandQueryValidator(IValidationRepositoryMediator validationRepositoryMediator,
        IDigimonWorld2AdminResources resources)
    {
        _validationRepositoryMediator = validationRepositoryMediator;
        _resources = resources;
    }

    protected override void CreateRules(CancellationToken token = default(CancellationToken))
    {
        AddRules(
            () => Validate.If(string.IsNullOrEmpty(CommandQuery.Name))?.BrokenRule(nameof(CommandQuery.Name))
                .Required(_resources.DigimonCreateCommandName),
            () => Validate.If(CommandQuery.DigimonTypeId == 0)?.BrokenRule(nameof(CommandQuery.DigimonTypeId))
                .Required(_resources.DigimonCreateCommandDigimonTypeId),
            () => Validate.If(CommandQuery.RankId == 0)?.BrokenRule(nameof(CommandQuery.RankId))
                .Required(_resources.DigimonCreateCommandRankId)
        );

        AddRules(_validationRepositoryMediator.GetValidationRules(CommandQuery, token));
    }
}

public class CreateCommandQueryValidationRepository : IValidationRepository<CreateCommandQuery>
{
    private readonly IValidator _validate;
    private readonly IDigimonWorld2ContextFactory _contextFactory;
    private readonly IDigimonWorld2AdminResources _resources;

    public CreateCommandQueryValidationRepository(IValidator validate, IDigimonWorld2ContextFactory contextFactory,
        IDigimonWorld2AdminResources resources)
    {
        _validate = validate;
        _contextFactory = contextFactory;
        _resources = resources;
    }

    public ICollection<Func<Task<BrokenRule>>> GetValidationRules(
        CreateCommandQuery request, CancellationToken token = default(CancellationToken)) =>
        new List<Func<Task<BrokenRule>>>(1)
        {
            async () =>
            {
                using (var context = _contextFactory.Create(false))
                {
                    return _validate.If(
                            !string.IsNullOrEmpty(request.Name) &&
                            await context.Digimons
                                .AnyAsync(a => a.Name.Equals(request.Name, StringComparison.OrdinalIgnoreCase),
                                    token)
                                .ConfigureAwait(false))?.BrokenRule(nameof(request.Name))
                        .AlreadyInUse(_resources.DigimonCreateCommandName, request.Name);
                }
            }
        };
}

Validation uses ?. here so that the second part isn't run if there is no rule. .NET Core changed the way translations work, so I had to IoC inject a reference to them now, which is why you see that.

CreateCommandRepository

public class CreateCommandQueryRepository : IRepository<CreateCommandQuery, int>
{
    private const int ExpectedRowsAffected = 1;

    private readonly IDigimonWorld2ContextFactory _contextFactory;
    private readonly IMapperMediator _mapperMediator;

    public CreateCommandQueryRepository(
        IDigimonWorld2ContextFactory contextFactory,
        IMapperMediator mapperMediator)
    {
        _contextFactory = contextFactory;
        _mapperMediator = mapperMediator;
    }

    public async Task<int> ExecuteAsync(CreateCommandQuery request, CancellationToken token = default(CancellationToken))
    {
        using (var context = _contextFactory.Create())
        {
            var entity = _mapperMediator.Map<DigimonEntity>(request);
            context.Digimons.Add(entity);

            await context.SaveChangesAsync(ExpectedRowsAffected, token).ConfigureAwait(false);

            return entity.DigimonId;
        }
    }
}

My "repository" is a little different. The repository here is above the database technology being used. That way, if you want to change from EF to a webservice or use Dapper/EF interchangeably or something, you can change that implementation easily without affecting the handler above.

Mapper

public class CreateCommandQueryMapper : IMapper<CreateCommandQuery, DigimonEntity>
{
    public DigimonEntity Map(CreateCommandQuery command, DigimonEntity to = default(DigimonEntity))
    {
        to = to ?? new DigimonEntity();

        to.Name = command.Name;
        to.DigimonTypeId = command.DigimonTypeId;
        to.RankId = command.RankId;
        to.LearnedSkillId = command.LearnedSkillId;
        to.Att = command.Att;
        to.Def = command.Def;
        to.Found = command.Found;
        to.HasImage = command.HasImage;
        to.HP = command.HP;
        to.MP = command.MP;
        to.SpecialtyId = command.SpecialtyId;
        to.Speed = command.Speed;

        return to;
    }
}

I find if you do this in chunks together (all these classes, minus the controller) are all in the same folder as they are all related, that coding the mapping by hand isn't that big of a deal. Though, I made this generic so you could setup your own that just used AutoMapper or whatever instead.

Invalidate Cache

public class CreateCommandQueryInvalidateCache : CacheManagerCommandQueryInvalidateCache<CreateCommandQuery, int>
{
    public CreateCommandQueryInvalidateCache(
        IQueryCacheRegion queryCacheRegion, 
        ICacheManager<object> cache) : base(queryCacheRegion, cache) { }

    protected override Task<ICollection<Type>> GetTypeOfQueriesToInvalidateAsync(
        CancellationToken token = default(CancellationToken)) =>
        Task.FromResult<ICollection<Type>>(new[] {typeof(ListQuery)});
}

If you know the queries you affect, you just have to give it the query type and it'll take care of the rest. I made it async in case it took some IO to figure out all the types.

Decorators

The most important decorator I wanted to show was the exception handling one that is at the top of the chain:

public class CommandQueryHandlerExceptionDecorator<TCommandQuery, TResult> : ICommandQueryHandler<TCommandQuery, TResult>
    where TCommandQuery : ICommandQuery<TResult>
{
    private readonly ICommandQueryHandler<TCommandQuery, TResult> _commandQueryHandler;
    private readonly ILogger _logger;
    private readonly IUserContext _userContext;
    private readonly ICommonResource _resource;

    public CommandQueryHandlerExceptionDecorator(
        ICommandQueryHandler<TCommandQuery, TResult> commandQueryHandler, ILogger logger, IUserContext userContext,
        ICommonResource resource)
    {
        Guard.IsNotNull(commandQueryHandler, nameof(commandQueryHandler));
        Guard.IsNotNull(logger, nameof(logger));

        _commandQueryHandler = commandQueryHandler;
        _logger = logger;
        _userContext = userContext;
        _resource = resource;
    }

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

If you are still reading and your eyes haven't glazed over yet, what are your thoughts? Is this too much? Thanks!

---Edit---

Retry Decorator

    public class CommandHandlerRetryDecorator<TCommand> : ICommandHandler<TCommand> where TCommand : ICommand
    {
        private readonly ICommandHandler<TCommand> _commandHandler;
        private int _maxRetries;
        private int _retryDelayMilliseconds;

        public CommandHandlerRetryDecorator(ICommandHandler<TCommand> commandHandler)
        {
            Guard.IsNotNull(commandHandler, nameof(commandHandler));

            _commandHandler = commandHandler;
        }

        public async Task<IResult> ExecuteAsync(TCommand command, CancellationToken token = default(CancellationToken))
        {
            if (!(command is ICqsRetry retry))
            {
                return await _commandHandler.ExecuteAsync(command, token).ConfigureAwait(false);
            }

            _maxRetries = retry.MaxRetries;
            _retryDelayMilliseconds = retry.RetryDelayMilliseconds;
            return await ExecuteWithRetryAsync(command, 0, token).ConfigureAwait(false);
        }

        private async Task<IResult> ExecuteWithRetryAsync(TCommand command, int tries,
            CancellationToken token = default(CancellationToken))
        {
            try
            {
                return await _commandHandler.ExecuteAsync(command, token).ConfigureAwait(false);
            }
            catch (BrokenRuleException)
            {
                throw; // Cannot retry this
            }
            catch (NoPermissionException)
            {
                throw; // Cannot retry this
            }
            catch (ConcurrencyException)
            {
                throw; // Cannot retry this
            }
            catch (DataNotFoundException)
            {
                throw; // Cannot retry this
            }
            catch (Exception ex)
            {
                if (tries >= _maxRetries) throw;

                if (command is ICqsRetrySpecific specific)
                {
                    if (!RetryHelper.HasAnyExceptionMatch(
                        specific.RetryCheckBaseTypes,
                        specific.RetryCheckInnerExceptions,
                        ex,
                        specific.OnlyRetryForExceptionsOfTheseTypes)) throw;
                }
                if (_retryDelayMilliseconds > 0)
                {
                    await Task.Delay(_retryDelayMilliseconds, token).ConfigureAwait(false);
                }
                return await ExecuteWithRetryAsync(command, ++tries, token).ConfigureAwait(false);
            }
        }
    }

Delete Command

public class DeleteCommand : ICommand, ICqsRetry
{
    public int DigimonId { get; set; }
    public byte[] Timestamp { get; set; }

    int ICqsRetry.MaxRetries => 3;
    int ICqsRetry.RetryDelayMilliseconds => 100;
}

public class DeleteCommandHandler : ICommandHandler<DeleteCommand>
{
    private readonly IRepositoryMediator _repositoryMediator;

    public DeleteCommandHandler(IRepositoryMediator repositoryMediator) =>
        _repositoryMediator = repositoryMediator;

    public async Task<IResult> ExecuteAsync(DeleteCommand command,
        CancellationToken token = default(CancellationToken))
    {
        await _repositoryMediator.ExecuteAsync(command, token).ConfigureAwait(false);
        return ResultHelper.Successful();
    }
}

public class DeleteCommandRepository : IRepository<DeleteCommand>
{
    private const int ExpectedRowsAffected = 1;

    private readonly IDigimonWorld2ContextFactory _contextFactory;
    private readonly IMapperMediator _mapperMediator;

    public DeleteCommandRepository(
        IDigimonWorld2ContextFactory contextFactory,
        IMapperMediator mapperMediator)
    {
        _contextFactory = contextFactory;
        _mapperMediator = mapperMediator;
    }

    public async Task ExecuteAsync(DeleteCommand request, CancellationToken token = default(CancellationToken))
    {
        using (var context = _contextFactory.Create())
        {
            var entity = _mapperMediator.Map<DigimonEntity>(request);
            context.Digimons.Remove(entity);
            await context.SaveChangesAsync(ExpectedRowsAffected, token).ConfigureAwait(false);
        }
    }
}

public class DeleteCommandMapper : IMapper<DeleteCommand, DigimonEntity>
{
    public DigimonEntity Map(DeleteCommand command, DigimonEntity to = default(DigimonEntity))
    {
        to = to ?? new DigimonEntity();

        to.DigimonId = command.DigimonId;
        to.Timestamp = command.Timestamp;

        return to;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Just looking for any opinions that this looks good or this looks like crap (and reasons why). \$\endgroup\$ – Daniel Lorenz Jan 22 '18 at 14:56
  • \$\begingroup\$ Also, I now added a Timing decorator that logs information about how long a command/query takes to run with an added ability to log a warning if it took longer than a specified threshold. \$\endgroup\$ – Daniel Lorenz Apr 9 '18 at 17:23
0
\$\begingroup\$

I have to play with the code in an IDE, but I really like that you’ve addressed caching at the outaet as it seems one of the more obvious benefits of havung well defined queries & results (rather than a generic repository which would then need a second layer to cache responses.

\$\endgroup\$
  • 1
    \$\begingroup\$ Moderator note: This answer does mention a positive thing about the code and thus I consider it to be an answer. \$\endgroup\$ – Simon Forsberg Apr 1 '18 at 14:33
  • \$\begingroup\$ I agree. Hopefully he'll have time to play with it, but then again, I left a lot of other code out so might be tough. :) \$\endgroup\$ – Daniel Lorenz Apr 2 '18 at 18:39
0
\$\begingroup\$

This looks great! The only things I can think of are the following:

Validation:

Your validation interface is called IValidator, but the instance is called _validate. Both make sense in their own context, but seeing IValidator _validate just doesn't seem right. Maybe rename the instance to _validator, or the interface to IValidationAction or IValidationRule?

Access modifiers:

It is hard to say without having a view on your project/solution structure, but do all methods need to be public?

\$\endgroup\$
  • \$\begingroup\$ Heh, yeah the naming itself needs to be updated to match correctly, I agree. Oversight. As for access modifiers, handler works with internal but repository does not for some reason. You could update the mediators to call these methods dynamically if you really didn't want them to be public, but you probably also want to unit test these things. The internal dependencies to these classes can be whatever you want, though. As for the ExceuteAsync methods, yes, they have to be public since they are interface driven. \$\endgroup\$ – Daniel Lorenz Aug 30 '18 at 15:51
0
\$\begingroup\$

Async Synchronization

CreateCommandQueryController provides method Post as seen below. I am personally not a fan of using expression body definition (arrow natotation) on multiline code. We have parentheses for that. More importantly, there is no need whatsoever to call ConfigureAwait(false) at a controller entrypoint method. Either ASP.NET provides a synchronization context (.NET Framework) and it can handle synchronization just fine, or it will not have a context (.NET Core) in which case configuring the await is redundant anyway.

[HttpPost]
public async Task<IActionResult> Post([FromBody] CreateCommandQuery commandQuery) =>
    await _responseMediator.ExecuteAsync(commandQuery).ConfigureAwait(false);
[HttpPost]
public async Task<IActionResult> Post([FromBody] CreateCommandQuery commandQuery) 
{
    return await _responseMediator.ExecuteAsync(commandQuery);
}

Layer Complexity

CommandQueryHandler uses both the mediator and some helper class. This introduces additional class complexity. Why can't the mediator handle that helper part?

public async Task<IResult<int>> ExecuteAsync(
    CreateCommandQuery commandQuery, CancellationToken token = default(CancellationToken))
{
    var digimonId = await _repositoryMediator.ExecuteAsync<int>(commandQuery, token).ConfigureAwait(false);
    return ResultHelper.Successful(digimonId);
}
 return await _repositoryMediator.ExecuteAsyncAsSucceeded<int>(commandQuery, token).ConfigureAwait(false);

I am not convinced about using a custom mapping mediator IMapperMediator. Use third-party frameworks for aspects such as logging and mapping. Do not introduce yet another layer for wrapping existing frameworks. And don't roll out your own framework for such aspects. The existing frameworks are mature and extensible.


\$\endgroup\$
  • \$\begingroup\$ => for 1 line of code is now the standard, at least with resharper and sonarqube. ConfigureAwait false is redundant on core, yes, but not .net framework. if any async call gets called sync, you are now deadlocked without it. As for helper, this is a simple example. many times you will have multiple repos to call or external services and the success result isnt for sure. also, that would require more interfaces all over the place. All mediators are optional, they are just shorthand instead of IoC injecting lots of dependencies, keeping unit tests easier. you can still use automapper underneath \$\endgroup\$ – Daniel Lorenz Aug 4 at 12:37
  • \$\begingroup\$ I don't agree with that deadlock. The controller is hosted in ASP.NET, where a custom synchronization context is available in .NET Framework. Hence, the ConfigureAwait should not be called. Let ASP.NET handle synchronization. \$\endgroup\$ – dfhwze Aug 4 at 14:50
  • \$\begingroup\$ you've never called async code synchronously before have you? There is a thing called ConfigureAwait all the way down and you are screwes if you miss even one. ConfigureAwait just let's it resume on another thread, but it loses sync context. But if an async is called sync, the top is waiting for the current process to stop while the bottom is waiting for the top to stop. I've gotten burned enough times with this to know to always use ConfigureAwait false in .NET framework unless I have to have translations. but then you have to ensure no async -> sync, even in 3rd party dlls \$\endgroup\$ – Daniel Lorenz Aug 4 at 23:13
  • \$\begingroup\$ @DanielLorenz Have a look at the accepted answer here: stackoverflow.com/questions/40198816/…. A controller is an ASP.NET entry point, running in the synchronization context of ASP.NET. So ConfigureAwait should not be called here. \$\endgroup\$ – dfhwze Aug 5 at 4:24
  • \$\begingroup\$ The answer assumes no Async calls being called Synchronously, but if thats true, then yes, you don't need ConfigureAwait(false). BTW, the point of this was not to get hung up on the details of the implementation but the general implementation itself. \$\endgroup\$ – Daniel Lorenz Aug 5 at 13:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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