Analyzer: ASP0022 and ASP0023 Report False Positives for Versioned Routes

[commonsensesoftware]

Background and Motivation

Minimal APIs and MVC controller can define versioned routes, which typically use the same route template.

ASP0022 will fire for the following Minimal APIs:

var forecast = app.NewVersionedApi();
var v1 = forecast.MapGroup("/weatherforecast");
var v2 = forecast.MapGroup("/weatherforecast");

v1.MapGet("/", () => [new WeatherForecastV1()]).HasApiVersion(1.0);
v2.MapGet("/", () => [new WeatherForecastV2()]).HasApiVersion(2.0);

ASP0023 will fire for the following controllers:

namespace My.Api.Controllers.V1
{
    [ApiVersion(1.0)]
    [Route("[controller]")]
    public sealed class WeatherForecast : ControllerBase
    {
        [HttpGet]
        public WeatherForecastV1[] Get() => Ok([new WeatherForecastV1()]);
    }
}

namespace My.Api.Controllers.V2
{
    [ApiVersion(2.0)]
    [Route("[controller]")]
    public sealed class WeatherForecast : ControllerBase
    {
        [HttpGet]
        public WeatherForecastV2[] Get() => Ok([new WeatherForecastV2()]);
    }
}

ASP0022 and ASP0023 are:

• Enabled by default
• Indicate that these rules should not be suppressed

These routes are not ambiguous and will not result in a runtime error. The lack of extensibility or knowledge of this capability, reports false positives and instructs users not to suppress these warnings. That is confusing to users. While it is possible for library authors to suppress this warning, that is unintuitive to users and there is still a scenario where the route pattern with its metadata is ambiguous and will produce a runtime error.

Proposed Analyzer

The analyzers for ASP0022 and ASP0023 should provide one or more of the following:

1. Be API version-aware
2. Allow/surface route-disambiguation metadata
3. Allow analyzer extensibility (to be defined)

Analyzer Behavior and Message

In the examples above, ASP0022 and ASP0023 should not produce any warnings. The rules should, however, still be produced in the following scenarios:

ASP0022 should fire for the following Minimal APIs:

var forecast = app.NewVersionedApi();
var v1 = forecast.MapGroup("/weatherforecast");
var v2 = forecast.MapGroup("/weatherforecast");

v1.MapGet("/", () => [new WeatherForecastV1()]).HasApiVersion(1.0);
v2.MapGet("/", () => [new WeatherForecastV2()]).HasApiVersion(1.0); // ← duplicate route + version

ASP0023 should fire for the following controllers:

namespace My.Api.Controllers.V1
{
    [ApiVersion(1.0)]
    [Route("[controller]")]
    public sealed class WeatherForecast : ControllerBase
    {
        [HttpGet]
        public WeatherForecastV1[] Get() => Ok([new WeatherForecastV1()]);
    }
}

namespace My.Api.Controllers.V2
{
    [ApiVersion(1.0)] // ← duplicate route + version
    [Route("[controller]")]
    public sealed class WeatherForecast : ControllerBase
    {
        [HttpGet]
        public WeatherForecastV2[] Get() => Ok([new WeatherForecastV2()]);
    }
}

Category

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

Severity Level

• Error
• Warning
• Info
• Hidden

Usage Scenarios

Any developer using ASP.NET Core with API Versioning is likely to encounter false positive warnings that they must either ignore or suppress. Users should be able to version their APIs, while still having the benefit of detecting duplicate routes.

Risks

• Suppressing these rules for versioned routes can still result in ambiguous routes, which is what the analysis rules were meant to solve

[commonsensesoftware]

I'm unsure if it should be filed as a separate issue or updated in the description, but I also noticed that ASP0018 fires as a false positive when versioning by URL segment:

var forecast = app.NewVersionedApi();
var v1 = forecast.MapGroup("/v{version:apiVersion}/weatherforecast");
var v2 = forecast.MapGroup("/v{version:apiVersion}/weatherforecast");

v1.MapGet("/", () => [new WeatherForecastV1()]).HasApiVersion(1.0);
v2.MapGet("/", () => [new WeatherForecastV2()]).HasApiVersion(2.0);

The route parameter version is used, but it is just unrecognized and unsupported by the analyzer.

[captainsafia]

I think we can consider revising the suppression info to allow users to repress if they know that API versioning is at play.

Be API version-aware

This approach is feasible but it'll involve some work to update the route handler analyzer to detect IEndpointRouteBuilders that are versioned.

Allow/surface route-disambiguation metadata

Assuming I understand the proposal here, I don't think this will add much value over just allowing developers to suppress the warning. We can also avoid introducing new API surface this way.

Allow analyzer extensibility (to be defined)

This seems the most ideal, but might not be feasible from a Roslyn perspective.

Thoughts on this, @JamesNK?

[commonsensesoftware]

As it is kind of related, SO 77621179 highlights a Minimal API issue with API Version and discovery in the new Endpoints Explorer because the developer used the IVersionedEndpointRouteBuilder instead of IEndpointRouteBuilder. As previously covered in a walkthough, IVersionedEndpointRouteBuilder is required by API Versioning because there is no other way to unify IEndpointRouteBuilder and IEndpointConventionBuilder.

If there is some type of metadata - say via attributes or MSBuild - that can provide the analyzers with additional information they can leverage to make better decisions, I'm happy to put in the effort on my side. Totally shooting from the hip, but perhaps some type of metadata than can be added via AdditionalFiles and recognized by the analyzer such as symbol names or something. I'm sure @JamesNK will cook up an 💡 we can make work. 😁

[JamesNK]

var forecast = app.NewVersionedApi();
var v1 = forecast.MapGroup("/weatherforecast");
var v2 = forecast.MapGroup("/weatherforecast");

v1.MapGet("/", () => [new WeatherForecastV1()]).HasApiVersion(1.0);
v2.MapGet("/", () => [new WeatherForecastV2()]).HasApiVersion(2.0);

I would have thought the minimal API above wouldn't generate ambiguous route warnings because HasApiVersion isn't a known method and the map calls are on different groups.

Can you create a minimal reproduction of the minimal API issue that I can test?

namespace My.Api.Controllers.V1
{
    [ApiVersion(1.0)]
    [Route("[controller]")]
    public sealed class WeatherForecast : ControllerBase
    {
        [HttpGet]
        public WeatherForecastV1[] Get() => Ok([new WeatherForecastV1()]);
    }
}

namespace My.Api.Controllers.V2
{
    [ApiVersion(2.0)]
    [Route("[controller]")]
    public sealed class WeatherForecast : ControllerBase
    {
        [HttpGet]
        public WeatherForecastV2[] Get() => Ok([new WeatherForecastV2()]);
    }
}

I think the problem with controllers is the allow list of attributes (aka metadata) is only being run against action attributes. It should also run against controller attributes. Then it would see ApiVersion, an unknown attribute, and not run.

In the examples above, ASP0022 and ASP0023 should not produce any warnings. The rules should, however, still be produced in the following scenarios:

The analyzer will never be this smart. It's only for basic scenarios and there isn't a way to extend it with new logic. When in doubt it won't generate warnings. The goal is to not have false positives.

[JamesNK]

I think the problem with controllers is the allow list of attributes (aka metadata) is only being run against action attributes. It should also run against controller attributes. Then it would see ApiVersion, an unknown attribute, and not run.

I think I'm wrong. Ambiguous routes are only detected against other actions in the same controller. I'm really surprised your sample is generating ambiguous warnings when each class only has one action.

You'll need to provide a repo for minimal API and controllers. I don't understand how these false positive warnings are occurring.

Hi @commonsensesoftware. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. If it is closed, feel free to comment when you are able to provide the additional information and we will re-investigate.

See our Issue Management Policies for more information.

[commonsensesoftware]

I honestly can't remember how I started on this issue. I normally already have (or had) a repro before I bother submitting an issue. I'm a bit embarrassed to say that my attempt to create a repro resulted in just a handful of false positives. 😳 I guess I got ahead of myself. 😞

For reference, here was my stab at a very contrived repro that mixes and matches a lot of stuff:

Issue-52556-Repo.zip

The actual false positives are:

1. HTTP method without a path is treated separately when they are equivalent (ex: [HttpGet] vs [HttpGet("")])
   a. ASP0023 doesn't fire in this scenario
   b. You might already know about this one (I didn't check for another issue)
   c. This requires 2+ actions, which was not provided as an example (@JamesNK is correct that it requires this)
2. Applying API version metadata by convention to controllers results in a warning
   a. This is fairly obvious why; the analyzer doesn't understand how it is applied and doesn't have attributes as a disambiguator
   b. Removing the suppressions in the API Versioning codebase itself, I noticed that almost all of the false positives fell into this category
   c. I suspect I just didn't pay close enough attention to the warning/error causes. Apologies; my bad.
3. ASP0018 fires on an unused route parameter

The majority of the takeaways in this issue are really use cases that are completely missed. I can repurpose this issue, but I understand you prefer a whole new issue. Here are a few of the scenarios I can think of, some of which you may have already thought about:

• Duplicate routes across files (controllers or min apis)
• Duplicate routes between controllers and min apis (rare, but possible)
• Duplicate routes with metadata (ex: duplicate API versions, etc)

I know this is the first cut of the analyzer so I'm sure there are/were many scenarios considered that were differed to a later release. If there were a way to provide metadata to the analyzer, perhaps via an extension analyzer, that can be used for further route qualification, that would be beneficial for a consistent error reporting and potentially less work for extenders. Forking the current analyzer to extend that behavior seems suboptimal. You may have other ideas on how that might be achieved. Happy to collaborate on a solution.