Consider making ICarterModule.AddRoutes a static abstract method
mariusz96 opened this issue · 8 comments
Hi,
Being accustomed to controllers I installed Carter and ran into an issue similar to these when injecting a scoped service into a module's constructor threw an exception:
And I also think that this unknowingly affects many people who inject transient services (ie MediatR library) into a module's constructor.
It appears to me that with an interface like this:
public interface ICarterModuleV2 // Naming things is hard.
{
static abstract void AddRoutes(IEndpointRouteBuilder app);
}this could be completely avoided and AddRoutes could still be called through reflection (easy, less efficient) or a source generator (hard, as efficient as hard-coded by a dev).
What do you think? Would this be a worthwhile addition?
EDIT: Turns out there's even a tutorial for this exact source generator: https://dev.to/joaofbantunes/mapping-aspnet-core-minimal-api-endpoints-with-c-source-generators-3faj.
As #279 says, you shouldn't be injecting services into your classes constructors, minimal API does not work like that. They should be in the RequestDelegate
public class MyModule : ICarterModule
{
public void AddRoutes(IEndpointRouteBuilder app)
{
app.MapGet("/home", IService service => {
var result = service.Foo();
return Results.Ok(result);
});
}
}As #279 says, you shuldn't be injecting services into your cla*** constructors, minimal API does not work like that. They sh*uld be in the RequestDelegate
public cla** MyModule : ICarterModule { public void AddRoutes(IEndpointRouteBuilder app) { app.MapGet("/h*me", IService service => { var result = service.Foo(); return Results.Ok(result); }); } }
Yes, I understand that. But with a static abstract method: static abstract void AddRoutes(IEndpointRouteBuilder app), using a dependency injected into a constructor would not even compile. So it would prevent me from making an error and enforce correct usage.
Which is good for a library interface, no?
I've had a play and making the interface have the abstract static method would be an aid to help this.
As it's a breaking change I will do this for .NET9. For now I will add some XML docs as a pointer not to inject dependencies into classes.
What it will look like:
/// <summary>
/// An interface to define HTTP routes
/// </summary>
/// <remarks>Implementations of <see cref="ICarterModule"/> should not inject constructor dependencies. All dependencies should be supplied in the route <see cref="RequestDelegate"/></remarks>
public interface ICarterModule
{
/// <summary>
/// Invoked at startup to add routes to the HTTP pipeline
/// </summary>
/// <remarks>Implementations of <see cref="ICarterModule"/> should not inject constructor dependencies. All dependencies should be supplied in the route <see cref="RequestDelegate"/></remarks>
/// <param name="app">An instance of <see cref="IEndpointRouteBuilder"/></param>
static abstract void AddRoutes(IEndpointRouteBuilder app);
}@mariusz96 can you test the latest Carter version, it has an analyzer that warns if you have constructor dependencies 😄
@mariusz96 can you test the latest Carter version, it has an analyzer that warns if you have constructor dependencies 😄
If you mean #349 then I think it's a great solution and this issue can be closed.
But I was not able to test the analyzer because though it gets installed under Dependencies\Analyzers, it throws an exception during build. Could be something on my end - maybe I just need to update something on my machine?
Is it a Windows machine? There’s an open issue for this unfortunately
Yes, it is and the exception is the same as in the other issue.
For what it's worth, xunit analyzers that get installed when creating new unit test project in Visual Studio work fine over here and are probably "battle-tested". Maybe their setup could be stolen/leveraged somehow?