dotnetjunkie / solidservices

Reference architecture application for .NET that demonstrates how to build highly maintainable web services.
MIT License
193 stars 26 forks source link

Mediator and DI #30

Open KrishnaKollu opened 5 years ago

KrishnaKollu commented 5 years ago

Hello @dotnetjunkie ,

I'm trying to better understand the DynamicQueryProcessor.cs implementation and the IQueryProcessor interface. I get how it's helpful in reducing dependencies.

However, I'm trying to make sense of it in light of DI principles mentioned in your book, and I'm hoping you can help me with this:

The execute method in DynamicQueryProcessor is going to a DI container. My understanding is that any time you go to a DI container outside of the Composition Root, the Service Locator Pattern is effectively used. It also seems to be that usage of IQueryProcessor has some of the drawbacks of a Service Locator i.e. hidden dependencies. This makes unit testing more challenging.

Is this mediator effectively a Service Locator, or am I missing something here? What are your thoughts?

dotnetjunkie commented 5 years ago

Hi @KrishnaKollu,

Whether or not the DynamicQueryProcessor (mediator) is effectively a Service Locator, depends on where it's used. You already mentioned this in your question. As you might recall from the book, the Composition Root is not a class, but rather an area or module. From that perspective, you can consider the DynamicQueryProcessor part of the Client's Composition Root. That Composition Root consists of multiple classes.

Do note, however, that this example application makes a few shortcuts from time to time. The goal is to demonstrate how command and query messages can be transported from client to server and back, in a maintainable way. To ensure you are following best practices, my book is probably the best resource for that.

KrishnaKollu commented 5 years ago

Thanks Steven!

In this article you mentioned:

By injecting an IQueryProcessor, it is no longer clear which queries a consumer is using. This makes unit testing more fragile, because the constructor no longer tells us what services the class depends on.

Do you have any recommendations for how to make it more clear what the consumer is using? i.e. would it make sense to do that via code comments?

You also mentioned in one of your comments, "Single file holding multiple queries is a model I use to get a team get accustomed to this model". So that I understand, would that be something like this:

class OrderRepository {

    /* Queries */
    class GetOrderByAddressQuery {
       ....
    }

    /* Query Results */
    class OrderResults {
        ..
    }

    /* Handlers */
    class GetOrderByAddressQuery implements IQueryHandler<GetOrderByAddressQuery, OrderResults>{
        ...
    }
}

Would this single file approach also make sense for commands?

dotnetjunkie commented 5 years ago

Do you have any recommendations for how to make it more clear what the consumer is using? i.e. would it make sense to do that via code comments?

Adding code comments would IMO make no sense. Code comments only tend to drift apart from the code that actually compiles. I have no further recommendations. If this loss of clarity is a problem, simply refrain from using a mediator.

Would this single file approach also make sense for commands?

I would not recommend nesting these types, because it makes the consuming code more cumbersome, as they will have to append the wrapping type (OrderRepository in your case, or at least—this would be the case in C#). Still, placing the command messages and command handler implementations in the same code file does make sense to start with. Later on, your project might benefit from keeping messages (i.e. contracts) and handler implementations separated, but starting off with keeping them close makes the model easier to start with.

KrishnaKollu commented 5 years ago

Thanks for explaining that.

In your article, you also mentioned:

I certainly wouldn’t advocate an ICommandProcessor for executing commands—consumers are less likely to take a dependency on many command handlers and if they do it would probably be a violation of SRP.

What do you think of the Command Bus pattern? Conceptually, I think it's the same thing - a mediator. Do you still advocate against an ICommandProcessor pattern? Here's an example of a Command Bus.

Given this above comment, is this something you would advocate against?

dotnetjunkie commented 5 years ago

I skimmed over the article and they indeed seem identical. My opinion hasn't changed about its use.

KrishnaKollu commented 5 years ago

Thx for the responses Steven.

I'm working in a programming language that doesn't support generics/parameterized interfaces, and am wrestling with how to correctly implement the command handler and query handler patterns there.

The book says:

If your programming language doesn't support generics, you might find the use of the non-generic ICommandService interface mixed with the Mediator design pattern an acceptable workaround

Can you help explain what this Mediator would look like here?

I wish the language supported parameterized interfaces. Do you recommend any alternatives to the Mediator design pattern that would still let me still use a single interface ICommandHandler interface?

dotnetjunkie commented 5 years ago

The book says:

Touché. The Mediator pattern I refer to in my book is the exact same pattern. IQueryProcessor, ICommandProcessor, and command bus are all implementations of the Mediator pattern.

The statement on my blog should be viewed in the context of .NET. Since you are not using a language that supports generics, the Mediator pattern is probably the best solution, both for handling queries and commands.

KrishnaKollu commented 5 years ago

Makes sense thank you.

I’m not entirely clear on how the mediator solves for the LSP. I can still bind a command with the wrong handler when setting up the mediator? The client can also be passed a mediator that is missing a binding all together as well?

Would it make sense to build a single mediator class that could handle both command mediation and query mediation, with two separate methods? I figure if I’m using a mediator I might as well use one mediator vs. two, as I can imagine there will be use cases where a client will need both the command dispatcher and query dispatcher functionality.

This question assumes using a DI connector isn’t an option. I will be doing pure DI.

dotnetjunkie commented 5 years ago

I’m not entirely clear on how the mediator solves for the LSP.

A Mediator solves LSP from the perspective of the handler's client, as the Mediator's contract promises that it can dispatch any given message.

I can still bind a command with the wrong handler when setting up the mediator? The client can also be passed a mediator that is missing a binding all together as well?

Yes, you can obviously break stuff inside the Mediator implementation, but that's a bug; not something the consumer concerns. Besides, in languages that have a type system and allow reflection, this problem can be easily prevented. A Mediator can be created in a resilient way, so that chances of making errors when need handlers are added are slim.

Would it make sense to build a single mediator class that could handle both command mediation and query mediation, with two separate methods?

This is not something I can answer. This is something you have to verify by applying the Interface Segregation Principle to your application and asking whether there will be clients that only use one of the two methods, but not both. If that's the case, you are violating ISP and it would typically be better to split them. If all clients use both methods, that warrents keeping the methods together.

KrishnaKollu commented 4 years ago

Steven,

Thanks for your responses. I've tried to take a stab at building a mediator implementation. I ran into a few challenges though, because the programming language that I am using, Salesforce Apex, has very limited reflection capabilities. It isn't for instance able to dynamically identify the type or name of the class of a given object (without hacky workarounds). It also isn't able to dynamically call methods on a class, like your Query Dispatcher implementation does. There are also no DI containers available for this programming language that support constructor injection, so I am using pure DI.

What are your thoughts on the below?


    public class EntryPoint {
        public void execute()
        {
            IMediator mediator = new Mediator(new Map<Type, ICommandHandler> {
                UpdateAccountAddressCommand.class => (
                    new LoggingCommandHandlerDecorator(
                        new UpdateAccountAddressCommandHandler()
                    )
                )
            });

            SomeClass sample = new SomeClass(mediator);
            sample.someMethod();
        }
    }

    public interface ICommand {
        //this is needed because the programming language doesn't support detecting what type an object is dynamically.
        Type getType();
    }

    public interface ICommandHandler {
        void handle(ICommand command);

        //this is used to identify what type of command this handler can accept
        Type getCommandType();
    }

    public interface IMediator {
        void send(ICommand command);

        //this is used to ensure the mediator has a binding for a command type.
        void validate(Type commandType);
    }

    public class MediatorException extends Exception {}

    public class Mediator implements IMediator {
        private Map<Type, ICommandHandler> handlerByCommandType;

        public Mediator(Map<Type, ICommandHandler> handlerByCommandType)
        {
            this.handlerByCommandType = handlerByCommandType;

            // Validate command to handler bindings
            for(Type commandType : handlerByCommandType.keySet())
            {
                ICommandHandler handler = handlerByCommandType.get(commandType);
                if(handler == null || !handler.getCommandType().equals(commandType))
                    throw new MediatorException('There is an error binding the ' + commandType + ' with the appropriate handler when constructing the mediator.');
            }
        }

        public void send(ICommand command)
        {
            ICommandHandler handler = handlerByCommandType.get(command.getType());
            handler.handle(command);
        }

        public void validate(Type commandType)
        {
            if(!handlerByCommandType.containsKey(commandType))
                throw new MediatorException('The mediator object does not have a handler binding for ' + commandType);
        }
    }

    public class UpdateAccountAddressCommand implements ICommand {
        public String street {get;set;}
        public String city {get;set;}
        public String state {get;set;}
        public String country {get;set;}
        public String zipcode {get;set;}

        public Type getType()
        {
            return UpdateAccountAddressCommand.class;
        }
    }

    public class UpdateAccountAddressCommandHandler implements ICommandHandler {
        public void handle(ICommand command) {
            UpdateAccountAddressCommand myCommand = (UpdateAccountAddressCommand) command;

            //logic
            //....
        }

        public Type getCommandType()
        {
            return UpdateAccountAddressCommand.class;
        }
    }

    public class LoggingCommandHandlerDecorator implements ICommandHandler {
        private ICommandHandler decoratee;

        public LoggingCommandHandlerDecorator(ICommandHandler decoratee)
        {
            this.decoratee = decoratee;
        }

        public void handle(ICommand command)
        {
            //do logging
            //....
            //then, invoke decorated method
            this.decoratee.handle(command);
        }

        public Type getCommandType()
        {
            return this.decoratee.getCommandType();
        }
    }

    public class SomeClass {
        private final IMediator mediator;

        public SomeClass(IMediator mediator) {
            mediator.validate(UpdateAccountAddressCommand.class);
            this.mediator = mediator;   
        }

        public void someMethod() {
            UpdateAccountAddressCommand cmd = new UpdateAccountAddressCommand();
            cmd.State = 'NC';
            mediator.send(cmd);
        }
    }
dotnetjunkie commented 4 years ago

I'm unfamiliar with Salesforce Apex and its constraints. This means I'm unable to give you any feedback on your code.

pholly commented 3 years ago

Thank you @dotnetjunkie and @KrishnaKollu for this great conversation. I have read and love the DI Patterns, Principles, and Practices book. I feel that if it had the word "Architecture" in the title it would be even more of a hit.

I also want to apply CQS / AOP and have noticed most apply it using the Mediator pattern, either using MediatR project or using a QueryProcessor (this sample project), QueryExecutor, etc. Is there a reason to not consume query handlers directly? Why does this project use an IQueryProcessor but not an ICommandProcessor? Basically, I was thinking to inject IQueryHandler<TQuery, TResult> directly in my Asp.net Core controllers. I prefer explicitly defining my dependencies.

From what I understand, MediatR has pipelines and such for cross-cutting concerns, so someone can register their decorators (pipelines) with MediatR and let it do its magic. I was planning on using SimpleInjector to decorate my query and command handlers for cross-cutting concerns, so I don't see the need of a Mediator.

Thanks!

pholly commented 3 years ago

Just realized @dotnetjunkie you talk about drawbacks of injecting IQueryHandler directly here.

I like how you mentioned start without a Mediator. If the only advantage of using a Mediator is avoiding constructor over-injection and code verbosity, I'll leave it out for now.

Thanks again for getting us on the SOLID path, practically.