Skip to content

[release/8.0] Fix ambiguous route analyzer false positive with switch #52035

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 1 commit into from
Nov 17, 2023

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Nov 14, 2023

[release/8.0] Fix ambiguous route analyzer false positive with switch

The ambiguous route analyzer excludes comparing routes that are in different code paths because there is no way to know which code path is used at runtime.

The current logic doesn't correctly exclude routes in switch statements and expressions. The following will flag the two routes:

using System;
using Microsoft.AspNetCore.Builder;
var app = WebApplication.Create();
_ = Random.Shared.Next() switch
{
    0 => app.MapGet(""/"", () => Hello()),
    1 => app.MapGet(""/"", () => Hello()),
    _ => throw new Exception()
};
void Hello() { }

The fix separates routes in switch statements into different code paths for analysis.

Fixes #51285

Customer Impact

Without this fix, ASP.NET Core apps could get false positive warnings about duplicate routes.

Customers can workaround the bad warnings by disabling the analyzer.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

Fix is to an analyzer. No runtime impact.

Verification

  • Manual (required)
  • Automated

Before:

image

After:

image

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Nov 14, 2023
@ghost ghost added this to the 8.0.x milestone Nov 14, 2023
@ghost
Copy link

ghost commented Nov 14, 2023

Hi @JamesNK. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@ghost
Copy link

ghost commented Nov 14, 2023

Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at.

@JamesNK JamesNK marked this pull request as draft November 14, 2023 04:56
@JamesNK JamesNK added the Servicing-consider Shiproom approval is required for the issue label Nov 14, 2023
@ghost
Copy link

ghost commented Nov 14, 2023

Hi @JamesNK. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@JamesNK JamesNK marked this pull request as ready for review November 14, 2023 05:34
@JamesNK JamesNK modified the milestones: 8.0.x, 8.0.1 Nov 16, 2023
@JamesNK JamesNK added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Nov 17, 2023
@ghost
Copy link

ghost commented Nov 17, 2023

Hi @JamesNK. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@JamesNK JamesNK modified the milestones: 8.0.x, 8.0.1 Nov 17, 2023
@JamesNK
Copy link
Member Author

JamesNK commented Nov 17, 2023

Email approval

@wtgodbe wtgodbe merged commit 526417e into release/8.0 Nov 17, 2023
@wtgodbe wtgodbe deleted the jamesnk/abiguous-route-analyzer-switch-9 branch November 17, 2023 16:50
@ghost ghost modified the milestone: 8.0.1 Nov 17, 2023
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 Servicing-approved Shiproom has approved the issue
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants