[Analyzer] : Warn when registering custom IProblemDetailsWriter after calling AddRazorPages, etc

[Rick-Anderson]

Background and Motivation

Registering a custom IProblemDetailsWriter after calling AddRazorPages, etc results in the DefaultProblemDetailsWriter, not the custom IProblemDetailsWriter being called.

Proposed Analyzer

Analyzer Behavior and Message

Issue a warning when Registering a custom IProblemDetailsWriter after calling AddRazorPages, etc.

ASP0001: Authorization middleware is incorrectly configured is similar.

See dotnet/AspNetCore.Docs#29152

Note: When using a custom IProblemDetailsWriter, the custom IProblemDetailsWriter must be registered before calling AddRazorPages, AddControllers, or AddControllersWithViews.

Category

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

Severity Level

• Error
• Warning
• Info
• Hidden

Usage Scenarios

Issue a warning when Registering a custom IProblemDetailsWriter after calling AddRazorPages, etc.

Risks

None.

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[david-acker]

We could add this new analyzer to the Microsoft.AspNetCore.Analyzers project in src/Analyzers since a similar middleware analyzer (UseAuthorizationAnalyzer) for ASP0001 lives there. Then we can leverage the existing middleware information that's available on the StartupAnalysis context and then register the new analyzer alongside the others StartupAnalyzer.

[david-acker]

Here's a simple suggestion for the title and message of the new diagnostic:

Title: Custom IProblemDetailsWriter is incorrectly configured
Message: The custom IProblemDetailsWriter must be registered before calling AddControllers, AddControllersWithViews, AddMvc, or AddRazorPages.

[david-acker]

I'd be happy to put up a PR for this once the API for the new diagnostic is approved.

[david-acker]

We'll want to report this diagnostic for AddMvc as well, since it calls AddControllersWithViews and AddRazorPages internally.

[david-acker]

API for proposed analyzer

Diagnostic:

Title: Custom IProblemDetailsWriter is incorrectly configured
Message: The custom IProblemDetailsWriter must be registered before calling AddControllers, AddControllersWithViews, AddMvc, or AddRazorPages.
Category: Usage
Severity: Warning

Context:

If a custom IProblemDetailsWriter is registered after a MvcServiceCollectionExtensions method, the DefaultProblemDetailsWriter will be used rather of the custom IProblemDetailsWriter.

This analyzer would report a warning level diagnostic when an IProblemDetailsWriter is registered (using AddTransient, AddScoped, or AddSingleton) and appears after a call to a MvcServiceCollectionExtensions method (AddControllers, AddControllersWithViews, AddMvc, or AddRazorPages).

Example Usage:

Diagnostic Reported:

services.AddControllers();

// Diagnostic reported on method invocation below
services.AddTransient<IProblemDetailsWriter, SampleProblemDetailsWriter>();

No Diagnostic Reported:

services.AddTransient<IProblemDetailsWriter, SampleProblemDetailsWriter>();

services.AddControllers();

[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:

• We discussed whether there's something clever we can do at runtime to avoid these errors like only adding default writers if there is no IProblemDetailsWriter configured and/or looping over the writers backwards, but we think adding complication is worth it.
• We also discussed using a different service type to mark the "default" writer so it would never unintendedly be used when a custom writer is provided.
• A runtime warning if there's a writer registered after the default may be more reliable since it could catch things a static analyzer cannot.
• Would there be a code fix to move the services.AddTransient<IProblemDetailsWriter,... up?
  • That seems useful.
• Note: We only want this to warn if someone calls AddControllers, AddControllersWithViews, AddMvc, or AddRazorPages, not if they add multiple IProblemDetailsWriter's manually even if that includes the default.

While all the runtime ideas seem good, it doesn't negate the fact that the analyzer would also be helpful.

API Approved!

Diagnostic:
Title: Custom IProblemDetailsWriter is incorrectly configured
Message: The custom IProblemDetailsWriter must be registered before calling AddControllers, AddControllersWithViews, AddMvc, or AddRazorPages.
Category: Usage
Severity: Warning

[david-acker]

@halter73 I can add a code fix for moving the IProblemDetailsWriter registration up. Should we add the code fixes to the existing analyzers project, like we do with Mvc.Api.Analyzers, or create a separate project for them? Microsoft.AspNetCore.Analyzers doesn't currently have any code fixes associated with it.

[halter73]

Thanks for offering to do the code fix @david-acker! New analyzers should go into src/Framework/AspNetCoreAnalyzers which includes both Microsoft.AspNetCore.App.Analyzers and Microsoft.AspNetCore.App.CodeFixes projects.

We prefer that over the legacy Microsoft.AspNetCore.Analyzers project in /src/Analyzers because the "App" analyzers are included in the Microsoft.AspNetCore.App.Ref pack which stays aligned with specific versions of ASP.NET Core while the legacy project is shipped as part of the Microsoft.Net.Sdk.Web SDK and run against arbitrary versions.

[david-acker]

Sounds good! I'll move the new analyzer over to AspNetCoreAnalyzers in the PR that I have up. Originally, I added it to the legacy analyzers project in order to leverage the StartupAnalyzer that's included there.

Do we want to clone some of the StartupAnalyzer infrastructure that it's using (minus the analyzers themselves) into AspNetCoreAnalyzers while we're at it, or should I just update the new analyzer to use something ad hoc?