RFC: Delegate Verb/NoContentVerb implementations to MultiVerb. #1860
Conversation
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
Add test case that directly exercises MultiVerb's extractHeaders function with duplicate header names (Set-Cookie headers). This validates the fix in commit c2cc84f which changed extractHeaders to use findIndexL + deleteAt instead of Seq.partition, allowing multiple headers with the same name to be correctly parsed. 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 (particularly important for some special headers such as `Set-Cookie`).
Contributor
Author
|
cc @alpmestan (let's discuss #841 again ;-) ) |
gdeest
force-pushed
the
humble-unification
branch
from
d09fce0 to
5d848b5
Compare
This is the first step toward unifying endpoint types around MultiVerb. Changes: - Verb's HasServer instances now delegate to MultiVerb - NoContentVerb delegates to MultiVerb with RespondAs '() 204 - Verb and NoContentVerb method parameter changed from polymorphic (k1) to strict (StdMethod) for consistency with MultiVerb. This is a breaking change, but arguably not a major one: I doubt that this extra polymorphism was ever used. - Added KnownStatus constraint to Verb instances. We previously only required `KnownNat`, but we need `KnownStatus` to be able to express `Verb` in terms of `MultiVerb. This is another potential breaking change: users using non-standard, custom statuses will have to implement `KnownStatus` instances. - Added ResponseRender instance for Respond with Headers to support the delegation Removed dead code: - methodRouter (was only used by Verb) - noContentRouter (was only used by NoContentVerb) - responseLBS import (no longer needed) Test changes: - Added KnownStatus instances for non-standard test status codes (210, 214, 280)
HasClient instances become thin wrappers around the MultiVerb one. Introduced a NPToHList class as a compatiblity layer between Verb / MultiVerb for header handling. Breaking change: Response header handling is now stricter in clients, matching MultiVerb behavior. Before: Missing/malformed headers returned MissingHeader/UndecodableHeader constructors - callers could inspect and handle gracefully. After: Missing/malformed headers fail the request immediately with "Failed to parse headers". In a way, this enforces the API contract: if a header is declared in the type, it must be present and valid. Use Optional headers if truly optional.
gdeest
force-pushed
the
humble-unification
branch
from
5d848b5 to
37b543a
Compare
Currently failing due to Verb now fixing method to `StdMethod`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
NOTE: This PR is based on #1859 . You may skip the first two commits when reviewing (but the second one is necessary for the tests to pass).
The goal of this PR is to start exploring the standardization of the core of servant around One True Endpoint Combinator (hereinfafter referred to as OTEC) that supports all of our use cases.
Current situation is more or less the following:
Verb/NoContentVerbis the historical endpoint combinator inservant.The
Streamcombinator is distinct fromVerband supports advanced streaming usages with custom framing and arbitrary item type.UVerbimproved uponVerbby allowing multiple instances, but leaks details of the HTTP protocol into handler types (namely HTTP status codes) and is about to be deprecated (see Deprecate UVerb #1820 ).MultiVerbis the closest thing we have to OTEC but:RespondStreamingis limited toSourceIO ByteStringand is not as expressive as the currentStreamcombinator.Verbthat need to be clarified.This PR tries to re-express the
Verbsemantics forHasServerandHasClientin terms ofMultiVerbinstances, in order to explore the feasibility of the OTEC vision, verify that MultiVerb provides everything we need, and work out some backward compatibility solution. It turns out to be easier than I feared, but does require some breaking changes. Notably:KnownStatusinstances, not justKnownNat.Verbmethod parameter was (probably) needlessly polykinded ; it must now be restricted toStdMethod, as this is whatMultiVerbdoes, in order to support delegation.MultiVerbandVerb:a)
MultiVerbincorrectly removed duplicate headers when parsing responses (see Fix handling of duplicate headers in MultiVerb client #1859 ), which madeVerbtest case failed when expressed in terms of MultiVerb. We fix this.b)
Verbclient implementation did not really check that headers were provided even when marked as mandatory. We could break user code that relies on some server not honoring the Servant API from which the client was derived.Hopefully we can do something with the work on this branch.
Doing the same thing for
Streamis essentially feasible but requires extendingRespondStreamingto support the features required byStream(or add a new response type toMultiVerb). I have some non-primetime-ready code around that I'll consolidate in a PR later.The end goal would be:
MultiVerbasVerb, and possibly move currentVerbto a dedicated module, or rename it toSimpleVerb.