Skip to content

Add ambiguous route analyzer #45582

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.

Sign up for GitHub

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

Jump to bottom
Merged
JamesNK merged 19 commits into from
Jan 7, 2023
Merged

Add ambiguous route analyzer #45582

JamesNK merged 19 commits into from
Jan 7, 2023

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented Dec 14, 2022 •
edited
Loading

Fixes #45223

Not quite finished. The remaining work:

  • Check that the map call's IEndpointConventionBuilder isn't used to add metadata that could affect routing.
  • Allow list of metadata methods that don't affect matching and still allow endpoints to be compared, e.g. RequireAuthorization.

It only works with Minimal API. Most of the infrastructure is in place to expand it to analyze controller actions in the future.

@JamesNK JamesNK added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Dec 14, 2022
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Dec 14, 2022
@JamesNK JamesNK marked this pull request as ready for review December 14, 2022 09:14
@build-analysis build-analysis bot mentioned this pull request Dec 14, 2022
Open
5 tasks
mitchdenny
mitchdenny approved these changes Dec 14, 2022
View reviewed changes
Copy link
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly a few edge cases I wouldn't have considered testing there!

@JamesNK JamesNK added the blocked The work on this issue is blocked due to some dependency label Dec 15, 2022
@JamesNK
Copy link
Member Author

JamesNK commented Dec 15, 2022

Merging blocked until API review is approved.

@JamesNK JamesNK force-pushed the jamesnk/ambiguous-route-analyzer branch from abd483a to 10be76e Compare December 20, 2022 05:41
@JamesNK JamesNK removed the blocked The work on this issue is blocked due to some dependency label Jan 6, 2023
@JamesNK
Copy link
Member Author

JamesNK commented Jan 6, 2023

API review approved, and all current feedback is applied.

Any other changes before merging?

@JamesNK JamesNK mentioned this pull request Jan 6, 2023
Merged
@JamesNK JamesNK merged commit f19d4a1 into main Jan 7, 2023
@JamesNK JamesNK deleted the jamesnk/ambiguous-route-analyzer branch January 7, 2023 10:05
@ghost ghost added this to the 8.0-preview1 milestone Jan 7, 2023
@commonsensesoftware commonsensesoftware mentioned this pull request Apr 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danmoseley danmoseley danmoseley left review comments

@Youssef1313 Youssef1313 Youssef1313 left review comments

@mitchdenny mitchdenny mitchdenny approved these changes

@captainsafia captainsafia Awaiting requested review from captainsafia

@halter73 halter73 Awaiting requested review from halter73

@brunolins16 brunolins16 Awaiting requested review from brunolins16

+1 more reviewer

@mavasani mavasani mavasani left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels

Projects

None yet

Milestone

8.0-preview1

Development

Successfully merging this pull request may close these issues.

[Analyzer] Warn on ambiguous route patterns

6 participants

@JamesNK @mitchdenny @danmoseley @mavasani @Youssef1313