Broad changes to handle case properly in headers and parameters #5221
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oracle-contributor-agreement
bot
added
the
OCA Verified
tjquinno
requested review from
tomas-langer,
romain-grecourt,
Verdent,
danielkec and
spericas
…f Headers-based factory method
romain-grecourt
previously approved these changes
Oct 24, 2022
…eaders instead of deprecating the methods and adding new pre-deprecated methods
tomas-langer
approved these changes
Oct 31, 2022
| * @return new instance with specified initial content | ||
| * @deprecated Use {@link #create(Headers)} instead, passing {@code Headers} instead of {@code Parameters}. | ||
| */ | ||
| @Deprecated(since = "3.0.3", forRemoval = true) |
There was a problem hiding this comment.
No need to deprecate, 4.0.0 will not use this class at all
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #5149
Header names should be case-insensitive. What Helidon calls "parameters" (query parameter, path params) should be case-sensitive. (Note that our use of "parameters" in this context does not correspond to other usages such as https://httpwg.org/specs/rfc9110.html#parameter, in which a parameter is an optional addition to the value of a "field"--basically, an addition to a header value.)
This PR fixes problems in both headers and parameters.
I tried to be fairly minimally-invasive as possible while still addressing the problems. Still, there are quite a few changes needed to fix this situation.
Some highlights:
Key changes are in
helidon-common-httpbut other components are affected as well.HashHeadersextendsHashParameters. Each now creates the correct type ofMap(case-sensitive or -insensitive) in separate protected factory methods. Various classes related to writeable headers now extendHashHeaders(instead of extendingHashParametersas they did before) and so now inherit the correct case-insensitive behavior.ReadOnlyHeadersextendsReadOnlyParameters. Each now creates the correct type ofMapin protected factory methods for various purposes. Various classes related to read-only headers now extendReadOnlyHeadersto inherit the correct case-insensitive behavior.Headersor its various implementations rather thanParametersor its counterparts so the data is handled in the (correct) case-insensitive manner.helidon-config-testingnow contains a newCaseSensitivityTesterutility class and is a new test-scoped dependency for several components.Some lower-level details:
Parametersnow extendsIterable<Map.Entry<String, List<String>>>and provides a default implementation ofiterator. Adding thisextendsreduces the need to create temporary data structures in other code. For our classes which implementParameters, this PR includes overrides ofiteratorwith more efficient code than the default.I kept some methods that the IDE shows as no longer used because, although unlikely, developers might have written extensions of our classes and overridden those methods. Removing them would be a backwards-incompatible change.
There is abundant opportunity to--but I did not--refactor further to reduce even more code duplication. For example,
HeaderandParameterscould extend a common parameterized interfaceHashParametersandHashHeaderscould extend a common abstract superclassReadOnlyParametersandReadOnlyHeadersBut doing that refactoring would be quite a bit more far-reaching than the proposed changes here. Plus, header handling is much different in 4.x so further major refactoring work in 3.x would not carry forward and is likely not worth the investment.