Skip to content

[Analyzer] Unused route parameter detection and fixer #44319

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

Closed
JamesNK opened this issue Oct 3, 2022 · 4 comments · Fixed by #44393
Closed

[Analyzer] Unused route parameter detection and fixer #44319

JamesNK opened this issue Oct 3, 2022 · 4 comments · Fixed by #44393
Labels
api-approved API was approved in API review, it can be implemented

Comments

@JamesNK
Copy link
Member

JamesNK commented Oct 3, 2022

Background and Motivation

When adding a route parameter to a minimal API or action:

  1. The route parameter needs to be added to the route
  2. .NET parameter needs to be added to the lambda/method.

An analyzer and fixer can automate step 2.

Proposed API

An analyzer inspects the route for route parameters and the lambda/action parameters. If the route has a parameter that isn't present in the lambda/action then the analyzer will suggest a fix.

  • Suggestion level (it's valid to have a route parameter that's not used so this isn't a warning)
  • Optional route parameter = nullable argument
  • Route parameter constraint is used to infer the .NET type. e.g. int constraint = int method parameter. Same with guid, int, long, double, float, datetime, decimal, etc. No route parameter means the new parameter type is string.
  • Put the new parameter in the method based on its position in the route.

Usage Examples

MicrosoftTeams-image

Alternative Designs

Risks

@JamesNK JamesNK added api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Oct 3, 2022
@ghost
Copy link

ghost commented Oct 3, 2022

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 halter73 added api-approved API was approved in API review, it can be implemented api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews api-approved API was approved in API review, it can be implemented labels Oct 5, 2022
@ghost
Copy link

ghost commented Oct 5, 2022

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.

@JamesNK
Copy link
Member Author

JamesNK commented Oct 5, 2022

Double check it works with RequestDelegate overload.

@halter73
Copy link
Member

halter73 commented Oct 5, 2022

API Review Notes:

  • It does seem unusual to define a route value and not take it as a parameter to your delegate.
  • This will file a lot on older code using the RequestDelegate overload, but maybe that's okay.
  • This will recommend changing to an API that is less trim safe in some cases.

Category: Usage
Severity: Info

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Oct 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants