Skip to content

Should we disable asp0014 when using WebApplication.CreateEmptyBuilder() #50732

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

Open
1 task done
WeihanLi opened this issue Sep 15, 2023 · 6 comments
Open
1 task done
Labels
analyzer Indicates an issue which is related to analyzer experience area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc
Milestone

Comments

@WeihanLi
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

Since the empty WebAplicationBuilder would not register routing, maybe we could disable the ASP0014 using top-level route registrations diagnostic?

We may use the code like below, currently we would get ASP0014 warning
image

var builder = WebApplication.CreateEmptyBuilder(new WebApplicationOptions());
builder.Services.AddRoutingCore();
builder.WebHost.UseKestrelCore();
builder.Logging.AddConsole();

var app = builder.Build();

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

app.Run();

Describe the solution you'd like

Ignore it when using the empty WebApplicationBuilder since no default global routing

Additional context

No response

@ghost ghost added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Sep 15, 2023
@captainsafia
Copy link
Member

Thanks for the bug report, @WeihanLi! Yes, we should avoid emitting this diagnostic for web apps not configured with the routing middleware.

Let me see how easy this is to fix on the analyzer side...

@captainsafia captainsafia added the analyzer Indicates an issue which is related to analyzer experience label Sep 15, 2023
@captainsafia captainsafia added this to the 9.0-preview1 milestone Sep 18, 2023
@david-acker
Copy link
Member

david-acker commented Sep 23, 2023

I just took a quick look, but it seems like this would be relatively straight forward. Right now we're emitting the diagnostic for any method on WebApplication.

Maybe we could pass an array of excluded method symbols to the IsDisallowedMethod method in WebApplicationBuilderAnalyzer? That way we could check that the method we're analyzing isn't one we want to exclude, after we check the method's containing type here.

Edit: I took a closer look into this, and it's definitely not as straightforward as I originally thought, as discussed below.

@WeihanLi
Copy link
Contributor Author

Seemed we're emitting the ASP0014 diagnostic for UseEndpoints method only, we may need to distinguish whether the WebApplication is an empty web application, if it is, we may need to exclude it, while it seems there's no property or something indicates if it's empty for now

@captainsafia
Copy link
Member

Seemed we're emitting the ASP0014 diagnostic for UseEndpoints method only, we may need to distinguish whether the WebApplication is an empty web application, if it is, we may need to exclude it, while it seems there's no property or something indicates if it's empty for now

Yep! The challenge here is that we want to continue emitting the warning about UseEndpoints for non-empty WebApplicationBuilders. There's not a straightforward way for us to determine if a given builder is empty or not unless we do some flow analysis to determine if it was constructed via CreateEmptyBuilder.

@captainsafia
Copy link
Member

Backlogging this for now. Was hopping I'd be able to tackle this for 9.0-preview1 but that turned out to not be the case.

PRs welcome on this though!

@captainsafia captainsafia modified the milestones: 9.0-preview1, Backlog Jan 11, 2024
@ghost
Copy link

ghost commented Jan 11, 2024

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer Indicates an issue which is related to analyzer experience area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc
Projects
None yet
Development

No branches or pull requests

4 participants