Skip to content
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

Warning on use of regex constraints in route parameters. #46546

Open
2 of 15 tasks
mitchdenny opened this issue Feb 9, 2023 · 3 comments
Open
2 of 15 tasks

Warning on use of regex constraints in route parameters. #46546

mitchdenny opened this issue Feb 9, 2023 · 3 comments
Labels
analyzer Indicates an issue which is related to analyzer experience api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-routing
Milestone

Comments

@mitchdenny
Copy link
Member

Background and Motivation

As part of #46227 we are removing the default regex constraint from the set of constraints available when CreateSlimBuilder(...) is used. The reason for this is that including regular expression support on constraints pulls in about 800K of extra file size. We still include the regex constraint resolver when CreateBuilder(...) is used.

However ... there is an opportunity for people to continue to use regular expressions for router parameters if they implement their own route constraints and provided they know what the regular expression is in advance they can take advantage of source generated regular expressions. We want an analyzer which detects the use of regex constraints and adds a warning which links to documentation on how to create a custom route constraint that uses source generated regex.

Proposed Analyzer

Analyzer Behavior and Message

When a route pattern is used that includes a regex constraint, we would output the following info message:

INFO: Recommend use of custom reoute constraint with source generated regular expressions. See https://aka.ms/aspet/tba for more details.

Note, if we detect that we are using CreateSlimBuilder (won't always be possible) then we can increase this to a warning?

Category

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

Severity Level

  • Error
  • Warning
  • Info
  • Hidden

Usage Scenarios

app.MapGet("/products/{productId:regex(...)}", (string productId) => {});

Risks

To discuss.

@mitchdenny mitchdenny added api-suggestion Early API idea and discussion, it is NOT ready for implementation analyzer Indicates an issue which is related to analyzer experience labels Feb 9, 2023
@mitchdenny mitchdenny self-assigned this Feb 9, 2023
@mitchdenny mitchdenny added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Feb 9, 2023
@ghost
Copy link

ghost commented Feb 9, 2023

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.

@mkArtakMSFT mkArtakMSFT added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-routing labels Feb 10, 2023
@captainsafia captainsafia removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 14, 2023
@captainsafia
Copy link
Member

Triage: This analyzer would be most helpful in scenarios where the user has updated their code to use the SlimBuilder and is still using regular expressions in their route. In this case, it would fallback to a runtime warning that describes the issue. Users who are upgrading from .NET 7 to .NET 8 won't run into any issues since they don't use the SlimBuilder.

For a full-fidelity solution, we can consider this analyzer and a fixer that generates code the Regex constraint referenced in the route.

@captainsafia captainsafia added this to the Backlog milestone Mar 14, 2023
@ghost
Copy link

ghost commented Mar 14, 2023

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.

@mitchdenny mitchdenny added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels May 24, 2023
@mitchdenny mitchdenny removed their assignment Jul 3, 2023
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer Indicates an issue which is related to analyzer experience api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-routing
Projects
None yet
Development

No branches or pull requests

5 participants
@mitchdenny @captainsafia @wtgodbe @mkArtakMSFT and others