-
-
Notifications
You must be signed in to change notification settings - Fork 523
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
Basic CQRS with .NET 5 (endpoints, nullable reference types, records etc.) #41
Conversation
699316e
to
e9bbf5c
Compare
bce1e1b
to
b9341c4
Compare
{ | ||
internal static IEndpointRouteBuilder UseRegisterProductEndpoint(this IEndpointRouteBuilder endpoints) | ||
{ | ||
endpoints.MapPost("api/products/", async context => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I would really try to achieve here would be to have an endpoint configuration like
endpoints.Map<RegisterProduct>(HttpMethod.Post, "api/products/", HttpStatusCode.Created)
and behind the scenes, this method to deserialize the command, resolve the command handler or have a command processor that does it and send the appropriate response. By this, you could probably simplify the "registration" and maybe increase the adoption among Controller
fans who would prefer the simplicity of mapping a request to a query/command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrianiftode That's an intriguing idea. I added an example of how to do that in a separate branch: https://github.com/oskardudycz/EventSourcing.NetCore/pull/43/files.
The main endpoint mapping function looks as follows:
internal static class EndpointsExtensions
{
internal static IEndpointRouteBuilder MapCommand<TRequest>(
this IEndpointRouteBuilder endpoints,
HttpMethod httpMethod,
string url,
HttpStatusCode statusCode = HttpStatusCode.OK)
{
endpoints.MapMethods(url, new []{httpMethod.ToString()} , async context =>
{
var command = await context.FromBody<TRequest>();
var commandResult = await context.SendCommand(command);
if (commandResult == CommandResult.None)
{
context.Response.StatusCode = (int)statusCode;
return;
}
await context.ReturnJSON(commandResult.Result, statusCode);
});
return endpoints;
}
}
I had to add command result to command handler
public interface ICommandHandler<in T>
{
ValueTask<CommandResult> Handle(T command, CancellationToken token);
}
public record CommandResult
{
public object? Result { get; }
private CommandResult(object? result = null)
=> Result = result;
public static CommandResult None => new();
public static CommandResult Of(object result) => new(result);
}
then you could register is as:
endpoints.MapCommand<RegisterProduct>(HttpMethod.Post, "/api/products", HttpStatusCode.Created)
Still, I'm not yet convinced that's the better option. By that, you're losing:
- Strongly typed Command (as you need to consider that you may get nulls),
- you're losing the ability to customise the request,
- generalising it might be hard in the long term.
I think that this could work if you're just doing the basic mapping for this particular case, and if you're not, you can fall back to the custom one.
I need to sleep through that :D I might add it into GoldenEye.
@adrianiftode thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add to the cons list the fact the command will have to return a result. Some people are very religious about that, others are more practical and use it. Yes, you will lose some "compile" time support when registering the commands and I think this is fine, you would have some integration tests anyway :). As you said, there is always a more verbose way of doing it. I think the new version introduces a bug, as the ProductId is not generated anymore by the calling code.
There will probably be some new parameters for the MapCommand method, like ones to customize the response or others to instruct the serialization. Other way would be to use conventions: so for example when you send a Created response the location might always be "api/products/{id}" and the id is the "ProductId" property of the command. As people are used with MVC conventions, EF conventions, it would be easy to inspire from these and then to also ease the onboarding process of the new developers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrianiftode I also prefer Type-Driven Development, as in my opinion, it brings simplicity. When you're sure about the compile-time types, you need fewer IFs, unit tests, etc.
Regarding the bug, I simplified that. To get it not generated in the handler, I'd probably add some interface like, e.g. ICreateCommand
or some middleware/pipeline run before the code.
My main concern is that I'd prefer to keep people understanding the benefits of writing simpler code vs a more generic one rather than just migrating from the old practices into the new syntax. I think that to make that generic there is a danger of going round the circle and adding a lot of overhead that might not be needed normally.
As I said, it's an intriguing idea. I have to rethink it.
9cb8be6
to
54fdc6b
Compare
…, Records and other C# 8-9 goodies Added tests for Registering the Product Added tests for the Warehouse example's queries Updated project and test configuration to run migrations automatically
6b07daf
to
9c348dc
Compare
Reverted changes to meetings management tests - AppVeyor doesn't like them, I'll fight that on separate PR
This PR shows:
See also extended idea in #43.