Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate Mediator class that allow for the internal access level on Requests and Handlers / better support for large solutions #79

Closed
ppasieka opened this issue Jan 15, 2023 · 8 comments

Comments

@ppasieka
Copy link

I have a project where I would like to keep many concepts marked as internal. It also applies to the mediator 'requests' and 'handlers'. At the moment, I can't use the internal keyword because I'm getting an error:

Inconsistent accessibility: parameter type 'Request' is less accessible than method' Mediator.Send(Request, CancellationToken)'

Is there w way to generate the mediator class as internal? Then, I could completely hide that I'm using the Mediator (it is just an implementation detail) and not expose it from the assembly.

@martinothamar
Copy link
Owner

It's not possible right now, but I do have an idea that would fit this scenario nicely I think - if we could configure the sourcegenerator to include all the abstractions, then both packages could be as source code and would add no runtime dependencies at all.

The closest we can get right now is to have both the source generator and abstractions package added to the project. You could then mark the handlers and messages as internal, but the mediator implementation itself would still be public. We could add an option to make the DI extension method and mediator implementation internal as well I guess, that should not be much work. So options as I see them:

  • No changes in this library - use Mediator.Abstractions and Mediator.SourceGenerator on the project. Would still have runtime dependency on Mediator.Abstractions, and the DI extension and Mediator implementation would still be public. Would not play nice in solutions where multiple projects follow this pattern I think
  • Add option to make all generated code internal. Still runtime dependency, and doesnt't change much overall
  • Both the option to make all generated code internal, but also generate all the abstractions as part of the source generator as well. IMediator etc would have to be configured for some user-defined namespace. Then there could exist multiple projects following this "everything internal" pattern in the same DI container, so I suppose this is the only proper solution.

Thoughts?

@ppasieka
Copy link
Author

I need to familiarize myself with source generators and their limitations; therefore, giving helpful feedback is difficult. It would be great if somebody more knowledgeable were here and could chime in.

I initially thought to use your second suggestion:

Add option to make all generated code internal. Still runtime dependency, and doesnt't change much overall

But with a tiny modification. It would be great if there were an option to configure whether I want to have it all generated as internal or not (with the default being as is - public). It could be done by creating the partial interface with an attribute with a boolean flag.

[Generate(internal: true)]
partial interface IMediator { }

So, this falls under your third suggestion, where abstractions and implementations are in the same assembly 🙂

@petar-maletic
Copy link

Deal breaker for me also. I'm building a modular monolith-ish infrastructure and most of my module classes are internal, except for controllers (per module), other communication is done trough message broker. Everything is composed in one API (bootstrap) project where I should be placing source generator package. I get bunch of errors about protection levels.

@Heehaaw
Copy link

Heehaaw commented Mar 29, 2023

Maybe it would be a good idea to enable source generation in project hierarchy. For example, my Application layer has handlers for the business logic, so adding the SourceGenerator dependency to this particular project would generate a partial Mediator implementation with these handlers only. That could be added to the service container using something like AddMediatorForApplication. Then, adding SourceGenerator dependency to the edge project (API or whatever) would generate another partial Mediator implementation, that would include only the parts sourced from the particular assembly.
Using this approach one could have all the implementation details marked as internal, as long as the source generation is turned on for the specific assembly.
What do you think?

@geraldmaale
Copy link

@ppasieka I also mark my handlers internal but use either the ICommandHanlder or the IQueryHandler just to have clearer abstractions. I only leave the ICommand and the IQuery as public for testing purposes.

Take a peek or I'm getting it wrong.

internal sealed class CreateEventCommandHandler : ICommandHandler<CreateEventCommand, ErrorOr<ApiSuccessResponse>>
{
    private readonly ILogger<CreateEventCommandHandler> _logger;
    private readonly IEventRepo _eventRepo;
    private readonly IMapper _mapper = new Mapper();
    private readonly Generate _generate;
    private static readonly ActivitySource ActivitySource = new(nameof(CreateEventCommandHandler));

    public CreateEventCommandHandler(ILogger<CreateEventCommandHandler> logger,
        IEventRepo eventRepo,
        IDateTimeProvider dateTimeProvider)
    {
        _logger = logger ?? throw new ArgumentNullException(nameof(logger));
        _eventRepo = eventRepo ?? throw new ArgumentNullException(nameof(eventRepo));
        _generate = new Generate(dateTimeProvider);
    }

    public async ValueTask<ErrorOr<ApiSuccessResponse>> Handle(CreateEventCommand command, CancellationToken cancellationToken)
    {
        // Start activity
        ...
    }
}

@martinothamar
Copy link
Owner

Hi sorry for being a bit slow.

Maybe it would be a good idea to enable source generation in project hierarchy

I'm a bit sceptical of partial generation and stuff like this because (I think) it will lead to more virtual calls and indirection. Perf was one of the main reasons I created this library in that I wanted the overhead from using the pattern to be minimal. Of course, there is already some virtual dispatch in the current impl. anyway, so the impact may not be anything to worry about, so would need some testing/benchmarking to verify. But this is atleast an important consideration to me.

Another issue is that I'm not sure this feature (in whatever form it would take) is really worth it, since right now this isn't really a limitation of any specific way this library works, more a fact of how C# works with its access modifiers. The generated Mediator implementation is just a normal C# class located in a project you specificy, which then needs access to the request messages (IQuery, ICommand etc) and the downstream handlers (IQueryHandler, ICommandHandler etc) because the Mediator class will have code that refers to it. Sure, the approach described could be a workaround for this, but at some cost, so I'm not sure if it's worth it. Needing to send the requests across project boundaries while having them be internal at the same time is a bit counterintuitive(?), although I get what the intention is (defaulting to as little accessibility as possible). Feels a bit like trying to hide a contract/public API behind IMediator/Mediator which I'm not sure is of any major benefit?

@Heehaaw
Copy link

Heehaaw commented Apr 11, 2023

Greetings,

I believe that virtual calls are unnecessary in this scenario. The mediator class generated in the leaf project can directly invoke the .AddMediator implementations (or equivalent) in its dependencies, if any exist. Additionally, the request objects themselves must be public as they expose the APIs. However, the handlers should be inherently internal and not reveal any implementation details to external consumers. For this reason, the ability to source-generate an intermediary implementation in the dependency projects would be advantageous.

I would like to emphasize that this is not a deal-breaker for my use-cases, as I have recently employed your exceptional library in production code without issue. Nevertheless, the ability to generate intermediary implementations would be a welcome addition.

Please let me know if you require any further assistance.

@martinothamar martinothamar changed the title Generate Mediator class that allow for the internal access level on Requests and Handlers Generate Mediator class that allow for the internal access level on Requests and Handlers / better support for large solutions Jun 21, 2023
@martinothamar
Copy link
Owner

Tracking some improvements around this here: #97

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants