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

[Analyzer] Header dictionary indexer analyzer and fixer #44318

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

[Analyzer] Header dictionary indexer analyzer and fixer #44318

JamesNK opened this issue Oct 3, 2022 · 2 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

IHeaderDictionary has DIM properties for many commonly used headers: https://github.com/dotnet/aspnetcore/blob/ff2148be7592be83f1f88e2f372ef04f02fdfdb8/src/Http/Http.Features/src/IHeaderDictionary.Keyed.cs

These properties are better because:

  • Strongly typed so no chance of typos.
  • Faster because they directly fetch the known header value from the underlying collection.

Proposed API

An analyzer that looks for indexer use with a name that matches a property on IHeaderDictionary. This will only apply to header names that have a property, e.g.

var accepts = context.Request.Headers["accepts"]; // <- valid to fix
var id = context.Request.Headers["contoso-id"]; // <- ignored

Usage Examples

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
Copy link
Member

halter73 commented Oct 5, 2022

API Review Notes:

  • The fix looks nicer and is faster! Approved.
  • Should it be Performance? Usage? Style? Usage wins the vote. The perf impact isn't big, but it also doesn't really avoid bugs because it only flags string literals that are known correct header names.

Category: Usage
Severity: Info

@halter73 halter73 added api-approved API was approved in API review, it can be implemented 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 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