Fix handling of duplicate headers in MultiVerb client#1859
Open
gdeest wants to merge 2 commits intohaskell-servant:masterfrom
Open
Fix handling of duplicate headers in MultiVerb client#1859gdeest wants to merge 2 commits intohaskell-servant:masterfrom
gdeest wants to merge 2 commits intohaskell-servant:masterfrom
Conversation
Contributor
Author
|
Note: this probably needs a CHANGELOG entry. |
Contributor
|
@gdeest let me know when you want a review for this PR. :) |
Contributor
Author
|
@tchoutri I think it is reviewable as it is ! :-) |
tchoutri
reviewed
Dec 15, 2025
Comment on lines
+190
to
+192
| -- This implementation retrieves the *first* header with matching name. | ||
| -- It leaves other instances of the same header intact for subsequent extraction, which allows | ||
| -- multiple headers with the same name to be extracted (e.g. Set-Cookie). |
Contributor
There was a problem hiding this comment.
This behaviour should probably be put in the public documentation. :)
Contributor
Author
There was a problem hiding this comment.
Where exactly would you see it ? As module-level Haddock documentation ?
Note that this matches Verb behavior (this was actually motivated by / discovered via #1860 ).
Add test case that directly exercises MultiVerb's extractHeaders function with duplicate header names (Set-Cookie headers). Fails with current implementation, shows discrepancy with Verb (which has a similar test case). The test uses WithHeaders with two Set-Cookie headers and verifies both cookies are properly extracted on the client side.
The previous implementation used Seq.partition to remove ALL headers with the matching name at once. This commit changes its behavior to accept duplicate header names (makes dual `Set-Cookie` test pass).
gdeest
force-pushed
the
servant-headers-semantics
branch
from
e5610be to
8cbae7d
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Current handling of duplicate headers in
MultiVerbclient implementation is inconsistent with theVerbbehavior. The latter allows duplicate header names, which is illustrated by a test-case that sets two distinctSet-Cookieheaders. TheMultiVerbimplementation extracts the first matching header upon parsing, and removes the other instances.I believe the
Verbbehavior is the correct choice as it allows some valid use-cases that are otherwise forbidden (e.g. multipleSet-Cookie). It will also allow some invalid behaviors (such as multipleHostheaders) but this is a small price to pay.This PR:
extractHeadersfunction to exhibit the same behavior as theVerbimplementation.