CleanArchitecture icon indicating copy to clipboard operation
CleanArchitecture copied to clipboard

✨ Switch to Minimal API

Open jasontaylordev opened this issue 2 years ago • 26 comments

I've switched from controllers to endpoints with Minimal API. Love to hear any comments, suggestions, or improvements.

jasontaylordev avatar Oct 25 '22 11:10 jasontaylordev

I'm really liking the direction Minimal API is going, but it's still lacking features in comparison to controllers. For new projects I have been using Fast Endpoints. It's as fast as Minimal API and more feature rich.

scottkuhl avatar Oct 25 '22 15:10 scottkuhl

Yes please to Minimal Api + endpoints. 👏🏻

PureKrome avatar Dec 07 '22 13:12 PureKrome

How is it testability wise in integration tests? Is it possible to create a webserver against it and run a test?

misha130 avatar Dec 10 '22 18:12 misha130

How is it testability wise in integration tests? Is it possible to create a webserver against it and run a test?

Yes, ASP.NET Core integration tests are working well

jasontaylordev avatar Dec 11 '22 21:12 jasontaylordev

What are the benefits of using Minimal API over controllers specifically in this solution?

ramax495 avatar Dec 12 '22 08:12 ramax495

What are the benefits of using Minimal API over controllers specifically in this solution?

Recommend watching this talk if you haven't already; https://youtu.be/pYl_jnqlXu8.

I don't that there are any benefits specific to this solution. However, for me, I like separating each endpoint into a separate file and certainly appreciate the performance gains of minimal API over API controllers.

This is worth a read too; https://ardalis.com/mvc-controllers-are-dinosaurs-embrace-api-endpoints/#:~:text=MVC%20Controllers%20are%20essentially%20an,They're%20not%20cohesive..

jasontaylordev avatar Jun 27 '23 09:06 jasontaylordev

Are you going to use FastEndpoints? :) Thanks.

tonven avatar Jun 27 '23 09:06 tonven

Are you going to use FastEndpoints? :) Thanks.

No, at this stage I'm sticking with ASP.NET Core Minimal API. Happy to discuss options though.

jasontaylordev avatar Jun 27 '23 09:06 jasontaylordev

What are the benefits of using Minimal API over controllers specifically in this solution?

Recommend watching this talk if you haven't already; https://youtu.be/pYl_jnqlXu8.

I don't that there are any benefits specific to this solution. However, for me, I like separating each endpoint into a separate file and certainly appreciate the performance gains of minimal API over API controllers.

This is worth a read too; https://ardalis.com/mvc-controllers-are-dinosaurs-embrace-api-endpoints/#:~:text=MVC%20Controllers%20are%20essentially%20an,They're%20not%20cohesive..

Thank you, Jason! I'm already familiar with this video and article. But those examples are not using Mediator. In each endpoint-file there is located handler with logic. And in your PR these endpoint-files contains only mediator calls. As a result, a lot of monotonous small files are obtained.

IMO, the transition to Minimal Apis makes sense only with the transfer of handlers to endpoint-files.

ramax495 avatar Jun 27 '23 09:06 ramax495

What are the benefits of using Minimal API over controllers specifically in this solution?

Recommend watching this talk if you haven't already; https://youtu.be/pYl_jnqlXu8. I don't that there are any benefits specific to this solution. However, for me, I like separating each endpoint into a separate file and certainly appreciate the performance gains of minimal API over API controllers. This is worth a read too; https://ardalis.com/mvc-controllers-are-dinosaurs-embrace-api-endpoints/#:~:text=MVC%20Controllers%20are%20essentially%20an,They're%20not%20cohesive..

Thank you, Jason! I'm already familiar with this video and article. But those examples are not using Mediator. In each endpoint-file there is located handler with logic. And in your PR these endpoint-files contains only mediator calls. As a result, a lot of monotonous small files are obtained.

IMO, the transition to Minimal Apis makes sense only with the transfer of handlers to endpoint-files.

I see what you mean. In that case, endpoints in separate files is adding no value. Do you think it would be better just to have an endpoint group per file, for example, TodoListEndpoints.cs, TodoItemEndpoints.cs. This should help to cut down on the small monotonous files while still gaining benefits by moving away from controllers.

jasontaylordev avatar Jun 27 '23 10:06 jasontaylordev

I see what you mean. In that case, endpoints in separate files is adding no value. Do you think it would be better just to have an endpoint group per file, for example, TodoListEndpoints.cs, TodoItemEndpoints.cs. This should help to cut down on the small monotonous files while still gaining benefits by moving away from controllers.

Of course, this will be more compact, but it also does not seem like an ideal solution... The Fastpoints approach seems more logical - handler with logic in endpoint-file.

ramax495 avatar Jun 27 '23 11:06 ramax495

@jasontaylordev Sorry for question not related to this repo. Are you going to implement corresponding changes to RapidBlazor repository?

tonven avatar Jun 27 '23 12:06 tonven

Of course, this will be more compact, but it also does not seem like an ideal solution... The Fastpoints approach seems more logical - handler with logic in endpoint-file.

Do you mean to replace the current Mediator requests with FastEndpoint endpoints?

jasontaylordev avatar Jun 27 '23 22:06 jasontaylordev

@jasontaylordev Sorry for question not related to this repo. Are you going to implement corresponding changes to RapidBlazor repository?

For sure. Do you mean endpoints specifically or are there some other features you would like to see? Go ahead and raise an issue on that repo if you liike.

jasontaylordev avatar Jun 27 '23 22:06 jasontaylordev

I see what you mean. In that case, endpoints in separate files is adding no value. Do you think it would be better just to have an endpoint group per file, for example, TodoListEndpoints.cs, TodoItemEndpoints.cs. This should help to cut down on the small monotonous files while still gaining benefits by moving away from controllers.

Sorry, could you explain what do you mean here. You suggested just to put all endpoint classes in one file or to create one class with mapping multiple endpoints (maybe using RouteGroupBuilder) just like it was in controllers? If you mean second option I think it's suitable way.

ramax495 avatar Jun 28 '23 07:06 ramax495

Of course, this will be more compact, but it also does not seem like an ideal solution... The Fastpoints approach seems more logical - handler with logic in endpoint-file.

Do you mean to replace the current Mediator requests with FastEndpoint endpoints?

No, I just want to say that FastEndpoint uses separate endpoint-files in proper way beacuse every such file doesn't contain boilerplate code but business logic.

IMO, using FastEndpoint is suitable for vertical sliced architecture but not for layered architecture. By using FastEndpoint we mix WebUI and Application layer. This way has its own pros and cons but goes against Clean Architecture approach and your template.

ramax495 avatar Jun 28 '23 08:06 ramax495

Regarding FastEndpoints. I see you were already refering ardalis CleanArchitecture as an example. Or at least as project that can be compared to.

https://github.com/ardalis/CleanArchitecture/blob/main/src/Clean.Architecture.Web/Endpoints/ContributorEndpoints/Create.cs

He is using FastEndpoints in his CleanArchitecture template. Not saying that it should be used here as well. As @ramax495 said, there are pros and cons.

tonven avatar Jun 28 '23 08:06 tonven

Sorry, could you explain what do you mean here. You suggested just to put all endpoint classes in one file or to create one class with mapping multiple endpoints (maybe using RouteGroupBuilder) just like it was in controllers? If you mean second option I think it's suitable way.

@ramax495 Yes, I mean the second way. This way we have the least amount of files possible, with one class representing each group. Just a little plumbing (no logic) to wire-up the endpoint to the correct MediatR request.

jasontaylordev avatar Jun 28 '23 10:06 jasontaylordev

Regarding FastEndpoints. I see you were already refering ardalis CleanArchitecture as an example. Or at least as project that can be compared to.

https://github.com/ardalis/CleanArchitecture/blob/main/src/Clean.Architecture.Web/Endpoints/ContributorEndpoints/Create.cs

He is using FastEndpoints in his CleanArchitecture template. Not saying that it should be used here as well. As @ramax495 said, there are pros and cons.

@Tonvengo thanks for the heads up. I've reviewed and I think it works well enough for that template since the endpoints contain some basic logic. In our case it will just be plumbing, making sure the endpoint hits the relevant MediatR request.

jasontaylordev avatar Jun 28 '23 10:06 jasontaylordev

@ramax495 I just pushed; https://github.com/jasontaylordev/CleanArchitecture/pull/720/commits/d88bff0ab2c95a1eb0d4de2cdc68ce6ecf607f73.

You can see that TodoListEndpoints is responsible for mapping all related endpoints. I don't think it's a big improvement. Admittedly in the other approach, EndpointBase does a lot of heavy lifting so it is very clean. Additional effort might be required for this approach.

Just not sure it's worth the effort. What do you think?

jasontaylordev avatar Jun 28 '23 11:06 jasontaylordev

EndpointBase does a lot of heavy lifting so it is very clean

I think in the current state this code in not clean enough. But if we can move group initialisation to EndpointGroupBase and also create there wrappers for MapGet, MapPost, etc. which will use one param for route and endpoint name then we get much cleaner solution. Similar how it is now made with EndpointBase. I see advantage in keeping endpoints in the group when we have for example 10 queries and 10 commands for feature. If you open directories with queries and command and also endpoints (if they are not grouped) your solution explorer will take up full screen. Also there will be more opened files in IDE.

ramax495 avatar Jun 28 '23 13:06 ramax495

I've updated the implementation of Minimal API to demonstrate two approaches! 💥

The first approach is a single file per endpoint group. With this approach, all endpoints are contained within a single folder Endpoints, with each endpoint group represented by a single file, e.g. TodoListEndpointGroup.cs. Routes are constructed using configuration and magic strings. This approach results in fewer folders, files, and code. However, the endpoint group files may become large and difficult to maintain.

The second approach is a single file per endpoint. With this approach, project-level folders are used to represent groups. In group folders, each group endpoint is represented in a single file, e.g. TodoItems/CreateTodoItem.cs. Routes are constructed by convention through analyse of namespaces and type names. This approach ensures each endpoint will be easy to maintain since it is clearly represented in a single file. Additionally, the chosen folder structure mirrors that within the Application layer, ensuring relevant features are easily located. However, this approach results in more folders, files, and code. Rout

This second approach could alternatively include an additional folder per endpoint. This would allow for the grouping of endpoint-related files, e.g. CreateTodoItem.cs, CreateTodoItemRequest.cs, CreateTodoItemResponse.cs.

While the second approach results in more folders, files, and code I think that it reduces complexity and increases maintainability. What do you think?

jasontaylordev avatar Jun 29 '23 06:06 jasontaylordev

Codespaces FTW:

image

I think I might be a bit of option 1 and 2 😬

Something like this:

Endpoints
  \_ ToDoItems
    \_ ToDoItemEndpoints.cs  <-- MapGroup + Map<Verb> (MapGet, MapPost)
    |_ MapGetToDoLists.cs <-- delegate for the  MapGet("GetTodoLists", ..) registration
    |_ MapPostodoList.cs ...
    |- ... etc

because each delegate is (for me) usually a few lines. Sometimes the request poco is not exactly the same as the query/command poco, so there's a transform stage. Sometimes the mediatR handler/static class is a discriminated union, so I need to return multiple HTTP codes/results for that single endpoint -> think: 404 not found, 200 ok.

If I had to pick out of option 1 and 2, i'd go 1.

PureKrome avatar Jun 29 '23 07:06 PureKrome

I see that both ways have its own pros and cons. As for me more preferable is approach with a single file per endpoint group. If file becomes large we can use more readable format:

        MapGroup("TodoLists", app);

        MapGet("GetTodoLists", GetTodoLists);
        MapGet("ExportTodos", "Export/{id}", ExportTodos);
        MapPost("CreateTodoList", CreateTodoList);
        MapPut("UpdateTodoList", "{id}", UpdateTodoList);
        MapDelete("DeleteTodoList", "{id}", DeleteTodoList);

IMO, one of the goals of creating the Minimal API was reducing boilerplate code. And this approach seems closer to philosophy of Minimal API.

ramax495 avatar Jun 29 '23 14:06 ramax495

I see that both ways have its own pros and cons. As for me more preferable is approach with a single file per endpoint group. If file becomes large we can use more readable format:

        MapGroup("TodoLists", app);

        MapGet("GetTodoLists", GetTodoLists);
        MapGet("ExportTodos", "Export/{id}", ExportTodos);
        MapPost("CreateTodoList", CreateTodoList);
        MapPut("UpdateTodoList", "{id}", UpdateTodoList);
        MapDelete("DeleteTodoList", "{id}", DeleteTodoList);

IMO, one of the goals of creating the Minimal API was reducing boilerplate code. And this approach seems closer to philosophy of Minimal API.

The latest update includes this approach:

    public override void Map(WebApplication app)
    {
        MapGroup(app, "TodoLists");
        MapGet(GetTodoLists);
        MapGet(ExportTodos, "Export/{id}");
        MapPost(CreateTodoList);
        MapPut(UpdateTodoList, "{id}");
        MapDelete(DeleteTodoList, "{id}");
    }

So if you use a method-based delegate, you don't need to specify the name. It's starting to look like a controller now, so not sure if we're heading in the right direction. Perhaps we're building features that belong in ASP.NET Core? Essentially using conventions that simplify the creation of endpoints and groups.

I've returned the relevant builders, e.g. RouteGroupBuilder and RouteHandlerBuilder, so that additional configuration can be added.

Thoughts?

jasontaylordev avatar Jul 01 '23 01:07 jasontaylordev

It's starting to look like a controller now, so not sure if we're heading in the right direction.

It seems so. The reason for this is that we are dividing the app into web-UI and application layer. I think it's ok. Otherwise, we need to combine both of these levels into one. But for controllers with this approach, we get code with a bunch of extra dependencies and logic mixed in one class. And Mimmal Api made it possible to do this in more clean way without unnecessary dependencies. Fastapi demonstrates this approach. But this is not consistent with the principles of clean architecture that this template implements.

ramax495 avatar Jul 02 '23 10:07 ramax495

I got a request, you should add some sort of global exception handler in order to handle uncatched exceptions like Entity Framework mess up. Other than that, maybe add native API versioning support?

spa5k avatar Jul 03 '23 06:07 spa5k

This PR was broken, see #896

jasontaylordev avatar Jul 05 '23 06:07 jasontaylordev

Just a thought here.

Since generally the endpoint is nothing more than taking the HTTP request, mapping to the command (which aspnet does for you) and sending the command or query to mediator, then taking the result and returning it as a response... do we really need an endpoint for every command and query?

I wonder if a single middleware can do the same work as every endpoint? I haven't tried this, but my thought is, based on the URL it could determine what command or query object to create, mapping the payload into the command or query, and sending it to Mediatr which will then call the handler/pipeline. The main issue here, no Open API because there are no routes to explore. So maybe...

Another possible thought, perhaps the endpoints could be code generated based on the handlers? Although the handlers are in the different layer (core) vs presentation, I'm not sure what the entry point for the codegen would be. Perhaps it would just be a single ENDPOINT file. If the code is generated, there's no reason every endpoint can't be in the same file.

PilotBob avatar Jul 05 '23 15:07 PilotBob

Thank you for sharing your thoughts, @PilotBob. While I agree that using a single middleware or code generation for all commands and queries is possible as an alternative approach, it would introduce significant complexity and deviate from the simplicity of Minimal API. The complexity involved in fine-tuning endpoint generation and accommodating additional filters (as an example) would become more challenging, leading to increased maintenance and a steeper learning curve for new developers.

Therefore, I believe it is better to maintain the focus of the template on a basic Minimal API approach. However, I think the alternative approach could be explored in a separate Minimal API extensions project. This would allow for further customisation and flexibility without compromising the simplicity of the template, while maintaining a clear separation between core functionality and advanced features.

jasontaylordev avatar Jul 05 '23 22:07 jasontaylordev