Add an Analyzer to recommend against IHeaderDictionary.Add

[Tratcher]

Is there an existing issue for this?

• [X] I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

IHeaderDictionary.Add throws if duplicate keys are added (Matches IDictionary). Most people want to set or append. They can set using the indexer (IHeaderDictioanry[key] = value) or append by calling Append. This is especially confusing when moving from System.Web where Headers.Add appended.

Describe the solution you'd like

We should add an analyzer that warns people not to use Add, but to use the indexer or Append instead.

Additional context

No response

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[halter73]

Would we ever consider making IHeaderDictioanry.Add(key, value); equivalent to IHeaderDictionary[key] = value;? I know the current Add behavior is more consistent with other dictionaries, but the indexer behavior appending instead of replacing is already weird. Does anyone rely on the InvalidOperationException?

[Tratcher]

but the indexer behavior appending instead of replacing is already weird.

Huh? The indexer replaces, not appends.

If anything, I think we'd make Add and Append do the same thing. That would be consistent with System.Web. The unfortunate part is that IHeaderDictionary doesn't even define Add, it inherits it directly from IDictionary, and many of the basic implementaitons are based directly on a Dictionary. I think that's why we've avoided changing the behavior. That said, we've intentionally deviated to avoid throwing KeyNotFoundException on Gets, so 🤷.

[ghost]

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.

[david-acker]

I'd be interested in helping out with this. Where should this new analyzer live in the project? Also, would it make sense to register code fixes for changing from Add to the indexer or Append?

[Tratcher]

The analyzers live here: https://github.com/dotnet/aspnetcore/tree/main/src/Analyzers/Analyzers/src

A code fixer could be helpful.

[qJake]

FWIW I just ran into this issue, and found this issue thread via Google.

I think an analyzer is OK, but Add and Append are somewhat ambiguous. I know we have to conform to IDictionary but it almost makes sense for Add and Append to have the same behavior (and maybe deprecate Append in the future in favor of Add?).

I just don't see a realistic scenario where someone would want to call Add with the explicit intention of only wanting a single header - it makes far more sense to write that code out clearly by checking for the existence of a key first using ContainsKey.

Just my two cents. 🙂