[Refactoring] Add back middlesware that are registered by WebApplication

[captainsafia]

Background and Motivation

Proposed Analyzer

For middlesware that are registered automatically by the WebApplication, consider a refactoring to manually add them back to the application.

Analyzer Behavior and Message

Category

• [ ] Design
• [ ] Documentation
• [ ] Globalization
• [ ] Interoperability
• [ ] Maintainability
• [ ] Naming
• [ ] Performance
• [ ] Reliability
• [ ] Security
• [ ] Style
• [x] Usage

Severity Level

• [ ] Error
• [ ] Warning
• [x] Info
• [ ] Hidden

Usage Scenarios

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build(); // <-- underline 'app' with refactor suggestion
app.Run();

var app = WebApplication.Create(args); // <-- underline 'app' with refactor suggestion
app.Run();

Result:

var app = WebApplication.Create(args);
if (app.HostingEnvironment.IsDevelopment())
{
    app.UseDeveloperExceptionPage();
}

app.UseRouting();

app.UseAuthentication(); // detect any `UseAuthentication` call and omit this if there is one
app.UseAuthorization(); // detect any `UseAuthorization` call and omit this if there is one
// additionally, place after any existing `UseAuthentication` call

// detect user middleware/endpoints and put them here

app.UseEndpoints(e => {});
app.Run();

Risks

We will not be able to place the auto-injected middleware correctly in all situations, that should be fine since this refactor is to help users be explicit about their middleware order.

Another risk is if we add more implicit middleware we'll want to keep the refactoring up to date.

[Tratcher]

Why?

[captainsafia]

Why?

We were discussing the fact that the WebApplicationBuilder is opinionated about the order in which middlesware are inserted. This suggestion came up as a way to provide users with a refactoring that would explicitly register the middlesware that we auto-register so that users could re-adjust the ordering when needed.

[Tratcher]

Ah, hopefully thus preventing WebApplicationBuilder from adding duplicates?

[captainsafia]

Ah, hopefully thus preventing WebApplicationBuilder from adding duplicates?

Yes. The WebApplicationBuilder actually already avoids adding duplicates so when the user does the registration in their app code in whatever order they desire we won't register the same middlewares.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

API Review Notes:

• Should the "Severity Level" be "info"? Do we really want analyzer firing be default for something we don't necessarily recommend changing. Can it be "hidden" instead. Would this mean that you'd have to manually do something like dotnet_diagnostic.ASP####.severity = info to see it?
• Could this be an refactoring instead? That seems to be what "invert if" is.

@amcasey Do you have any preference based on what other analyzers/refactorings do.

[amcasey]

There's a fine line between sub-warning diagnostics and refactorings. The key functional differences are that code-fixes can be applied in multiple places at once (i.e. fix-all) and refactorings can take user-specified spans as inputs (e.g. for extract method).

There's probably also a (relatively minor) perf consideration - analyzers everywhere even though most hidden diagnostics will never be "fixed" and refactoring run every time the cursor is moved.

Either is probably fine in this case (unless this is the sort of thing you'd want to apply to multiple locations in the same project).

[ghost]

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.