Skip to content

Add analyzer to suggest top level route registration #42937

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

Merged
merged 6 commits into from
Aug 9, 2022

Conversation

pedro-camargo11
Copy link
Contributor

ASP0014 analyzer: Use top level route registration.

Add analyzer ASP0014 to WAB analyzers. Suggests top level route registration instead of using UseEndpoints.

Fires in the following scenario:

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

app.UseRouting();
app.UseEndpoints(endpoints =>
{
    endpoints.MapGet("/", () => "Hello World!");
});

Fixes #35759

@pedro-camargo11 pedro-camargo11 requested a review from Pilchie as a code owner July 26, 2022 20:39
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jul 26, 2022
@pedro-camargo11 pedro-camargo11 merged commit 426a204 into dotnet:main Aug 9, 2022
@ghost ghost added this to the 7.0-rc1 milestone Aug 9, 2022
@erichiller
Copy link

Could you add some docs for this, I didn't get what it was wanting me to do, finally I figured it out. Maybe docs like this:


ASP0014

x Description
Code ASP0014
Title Suggest using top level route registrations
Message Suggest using top level route registrations instead of UseEndpoints (UseEndpoints will be filled in with whatever method is being used and should be replaced)
Name UseTopLevelRouteRegistrationsInsteadOfUseEndpoints
Introduced PR #42937
Source WebApplicationBuilderAnalyzer.cs
Tests Tests

Fix by replacing UseEndpoints with the same method calls directly on WebApplication which also implements IEndpointRouteBuilder

Example, old:

webApp.UseEndpoints(endpointRouteBuilder => {
    endpointRouteBuilder.MapGrpcService<MyService>();
});

Example, new:

webApp.MapGrpcService<MyService>();

Suppress via attribute:

[SuppressMessage("Usage", "ASP0014:Suggest using top level route registrations")]

@captainsafia
Copy link
Member

@erichiller Thanks for the feedback, Eric! We can definitely add some docs for this.

I'll admit I have to dust off some of the cobwebs around how we handle docs for new analyzers. I had thought that some of this was automated but turns out we'll actually have to populate some content for the new analyzers that we added in .NET 7 over at https://docs.microsoft.com/en-us/aspnet/core/diagnostics/code-analysis?view=aspnetcore-6.0.

I've added #44009 to track adding these docs.

@HarelM
Copy link

HarelM commented Jan 15, 2023

I've seen this warning in my code and moved from endpoints to top level route.
This completely broke my app.
Can the docs/analyzers be improved so that this won't happen to other people or better explain how to change it so that it won't break the app API?
The following is the commit I did in order to revert back to a working state, I'd love to know what I did wrong in the first place and how to properly resolve the warning I get...

@ghost
Copy link

ghost commented Jan 15, 2023

Hi @HarelM. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@captainsafia
Copy link
Member

I've seen this warning in my code and moved from endpoints to top level route. This completely broke my app. Can the docs/analyzers be improved so that this won't happen to other people or better explain how to change it so that it won't break the app API? The following is the commit I did in order to revert back to a working state, I'd love to know what I did wrong in the first place and how to properly resolve the warning I get...

I took a look at the app linked. I believe it might be related to the fact that you invoke UseRouting and UseEndpoints via your own ordering in the target application (see this).

@HarelM
Copy link

HarelM commented Jan 16, 2023

Thanks for the tip @captainsafia!

@ghost
Copy link

ghost commented Jan 16, 2023

Hi @HarelM. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fixer/Analyzer: Remove UseEndpoints() and hoist route registrations top level to WebApplication
5 participants