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

✨ Switch to Minimal API #720

Closed
wants to merge 9 commits into from
Closed

✨ Switch to Minimal API #720

wants to merge 9 commits into from

Conversation

jasontaylordev
Copy link
Owner

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

@scottkuhl
Copy link
Contributor

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.

@PureKrome
Copy link

Yes please to Minimal Api + endpoints. 👏🏻

@misha130
Copy link
Contributor

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

@jasontaylordev
Copy link
Owner Author

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

@ramax495
Copy link
Contributor

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

Lanz86 added a commit to Lanz86/CleanArchitecture that referenced this pull request Mar 11, 2023
@jasontaylordev
Copy link
Owner Author

jasontaylordev commented Jun 27, 2023

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..

@tonven
Copy link

tonven commented Jun 27, 2023

Are you going to use FastEndpoints? :) Thanks.

@jasontaylordev
Copy link
Owner Author

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.

@ramax495
Copy link
Contributor

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.

@jasontaylordev
Copy link
Owner Author

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.

@ramax495
Copy link
Contributor

ramax495 commented Jun 27, 2023

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.

@tonven
Copy link

tonven commented Jun 27, 2023

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

@jasontaylordev
Copy link
Owner Author

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
Copy link
Owner Author

@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.

@ramax495
Copy link
Contributor

ramax495 commented Jun 28, 2023

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
Copy link
Contributor

ramax495 commented Jun 28, 2023

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.

@tonven
Copy link

tonven commented Jun 28, 2023

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.

@jasontaylordev
Copy link
Owner Author

jasontaylordev commented Jun 28, 2023

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
Copy link
Owner Author

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
Copy link
Owner Author

@ramax495 I just pushed; d88bff0.

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?

@ramax495
Copy link
Contributor

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.

@jasontaylordev
Copy link
Owner Author

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?

@PureKrome
Copy link

PureKrome commented Jun 29, 2023

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.

@ramax495
Copy link
Contributor

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.

@jasontaylordev
Copy link
Owner Author

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?

@ramax495
Copy link
Contributor

ramax495 commented Jul 2, 2023

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.

@spa5k
Copy link

spa5k commented Jul 3, 2023

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?

@jasontaylordev
Copy link
Owner Author

This PR was broken, see #896

@PilotBob
Copy link

PilotBob commented Jul 5, 2023

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.

@jasontaylordev
Copy link
Owner Author

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.

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

Successfully merging this pull request may close these issues.

8 participants