test: add HTTP/2 coverage for the HEAD method - #1238
Open
pjfanning wants to merge 1 commit into
Open
Conversation
Motivation: The HTTP/2 engine had no test coverage for HEAD requests at all, and no HEAD handling either: `grep HEAD` over the http2 engine and its tests returns only HEADERS frame matches. That left three divergences from HTTP/1.1 invisible. Modification: Add a "support HEAD requests" section to Http2ServerSpec covering request delivery, content-length rendering for strict and known-length entities, the response body, `transparent-head-requests`, and a 304 response. Three of the tests pin behaviour that is currently wrong and carry a FIXME naming the cause and the relevant RFC, so that a fix has to flip the assertion deliberately rather than silently: - the response entity is emitted as DATA frames, although RFC 9110 section 9.3.2 says a server MUST NOT send content in a response to HEAD. ResponseRendering only ever sees the HttpResponse plus its stream id, so the engine cannot know the request was a HEAD. - `transparent-head-requests` is applied only by HttpServerBluePrint, so it has no effect over HTTP/2 and the handler always sees a HEAD request. - HttpMessageRendering.addContentHeaders renders content-length straight from the entity and never consults HttpMethod.contentLengthAllowed, so a 304 gets `content-length: 0` where HTTP/1.1 omits the header. The content-length tests confirm HTTP/2 already does the right thing for the object-store use case from apache#1236. Result: HEAD over HTTP/2 is covered, and the three gaps are recorded as assertions instead of being absent. Tests: - sbt "http2-tests / Test / testOnly org.apache.pekko.http.impl.engine.http2.Http2ServerSpec" - 118 passed, 16 pending - scalafmt --mode diff-ref=upstream/main - clean - git diff --check - clean References: Refs apache#1236, Refs apache#1237
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
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.
Motivation
While looking at #1236 I noticed the HTTP/2 engine has no test coverage for
HEADat all — and noHEADhandling either.grep HEADoverhttp-core/.../impl/engine/http2andhttp2-testsreturns only HEADERS-frame matches. That absence was hiding three divergences from HTTP/1.1.Modification
Adds a
"support HEAD requests"section toHttp2ServerSpeccovering request delivery,content-lengthrendering for strict and known-length entities, the response body,transparent-head-requests, and a304response.Two tests assert behaviour that is correct today:
HEADwith an empty entitycontent-lengthon a strict200content-lengthonDefault(ct, 100, Source.empty)Three pin behaviour that is currently wrong, each with a
FIXMEnaming the cause and the relevant RFC, so a fix has to flip the assertion deliberately rather than silently:HEADanswered withHttpEntity(ct, ByteString("abcde"))producesDATA endStream=true data=abcde.ResponseRenderingonly ever sees theHttpResponseplus its stream-id attribute, so the engine has no way to know the request was a HEAD. HTTP/1.1 strips the body inHttpResponseRendererFactoryvianoEntity.transparent-head-requestsis ignored. It is applied only atHttpServerBluePrint.scala:159, which is the HTTP/1.1 blueprint, so over HTTP/2 the handler always seesHEADeven with the setting on.304getscontent-length: 0.HttpMessageRendering.addContentHeadersrenderscontent-lengthstraight fromentity.contentLengthOptionand never consultsHttpMethod.contentLengthAllowed, so HTTP/2 emitscontent-length: 0where HTTP/1.1 deliberately omits the header (RFC 9110 §15.4.5: a304should carry theContent-Lengtha200would have had, and0actively misstates that).This PR is coverage only — no engine changes. Happy to open issues for the three gaps and follow up with fixes; item 1 is a spec violation and looks worth fixing on its own.
Result
HEADover HTTP/2 is covered, and the three gaps are recorded as assertions instead of being absent.Tests
sbt "http2-tests / Test / testOnly org.apache.pekko.http.impl.engine.http2.Http2ServerSpec"— 118 passed, 16 pending (pre-existing)scalafmt --mode diff-ref=upstream/main— clean;git diff --check— cleanTest-only change; no production code touched, so MiMa is unaffected.
References
Refs #1236, Refs #1237