perf(state): skip response body on HEAD requests#8348
Conversation
soyuka
left a comment
There was a problem hiding this comment.
I think this is quite safe, we need a note in the changelog for that cache key change though.
| 'headers' => ['Accept' => 'application/ld+json'], | ||
| ]); | ||
|
|
||
| $this->assertResponseStatusCodeSame(500); |
There was a problem hiding this comment.
Throw a proper HttpException in the test so that you can use a fixed status code.
There was a problem hiding this comment.
Done, SpyPaginator now raises a very specific HttpException(Response::HTTP_I_AM_A_TEAPOT) that this test catches.
| // @see ApiPlatform\State\Processor\RespondProcessor | ||
| $context['original_data'] = $data; | ||
|
|
||
| if ($request->isMethod('HEAD')) { |
There was a problem hiding this comment.
This will make skip the cache keys (below _resources) but if we want them we need to serialize...
There was a problem hiding this comment.
This should now be handled by the second commit (the opt-out flag) — what do you think?
|
I'm concerned about the Cache-Tags issue, we need to: add this at the top of changelog: Introduce a flag in the configuration: This would solve my concerns about this, have a clean upgrade path + optimize but totally (don't leave out cache tags) |
67527f7 to
c88d8ac
Compare
c88d8ac to
11e2d91
Compare
|
Hi @soyuka, thanks for the review! I understand that you already know omitting these headers on I added a second commit for this: One note: the CHANGELOG seems to be generated from commit messages at release time, so I kept the entry you suggested, but I can move it to an UPGRADE guide instead if you prefer. Let me know if this matches what you had in mind? And if my implementation is what you expected! |
HEAD responses now omit some observable headers (Content-Length, per-item cache tags); per semver that is a backward- incompatible behavior change, so this commit ships an opt-out flag — set `enable_head_request_optimization: false` to restore the prior GET-equivalent behavior.
e8c12ff to
7726f2e
Compare
|
While working on this HEAD topic I found a separate, smaller bug — fixed in the latest commit! When a resource declares no GET operation (for example a POST-only resource), the Unlike the body-skip change, this one needs no BC handling. As it stands API Platform already returns |
This is the follow-up to the
HEADremark I left in #7856 (limitation #1): today aHEADis processed exactly like aGET— the collection is fully read, hydrated and serialized, and Symfony only strips the body right before sending. So the database does all the work for nothing.This PR makes a
HEADrequest skip body construction entirely: the (lazy) Doctrine collection is never iterated → no rowSELECT. The response still carries its headers, just no body — which is whatHEADis for.One single commit, decoupled from the Content-Range topic on purpose: it stands on its own even if #7856 isn't merged.
RFC 9110 section followed
§9.3.2 HEAD — "The HEAD method is identical to GET except that the server MUST NOT send content in the response." The spec explicitly allows a server to omit header fields whose value is only determined while generating the content, and states this is "preferable to generating and discarding the content for a HEAD request, since HEAD is usually requested for the sake of efficiency." That's exactly what we do.
Summed up design considerations
$request— aHEADis matched to theGEToperation, so$operation->getMethod()returnsGET)Main logic implemented
A
HEADshort-circuit at each of the 3 points where a response body is born — right afteroriginal_datais captured (so headers still compute) and before any iteration:SerializeProcessor— standard serialization (all formats) → forwards an empty body.Hydra\State\JsonStreamerProcessor&Serializer\State\JsonStreamerProcessor— thejsonStream: truepaths, which build the body independently ofSerializeProcessor(and, in listener mode, via their ownkernel.viewlistener — hence their own guard).Detection is
$request->isMethod('HEAD'): the operation can't tell us, by construction.Tests
HeadRequestTest(functional) — backed by a spy paginator whosegetIterator()/count()throw, so aHEADreturning200proves the collection was never iterated (a naive "200 + empty body" test would pass even if theSELECTran). Covers: collectionHEAD(no iteration),GET(does iterate → sanity check, keeps the test non-vacuous),OPTIONS(unaffected), and ajsonStream: trueresource.SerializeProcessorTest(unit) — asserts the serializer is never called onHEAD.Limitations & concerns
Content-Lengthis no longer set onHEAD(we don't build the body to measure it). Allowed by RFC 9110 §9.3.2, but a small observable change for strict clients._resources, surrogate keys) aren't populated onHEADsince serialization is skipped → cache-tag headers may differ betweenGETandHEAD. Fine IMO (aHEADhas no rows), but worth flagging.securityexpression on a collection that iteratesobjectstill triggers theSELECT— it runs in the provider, before the processors, so this optimization can't prevent it. The common case (is_granted('ROLE_X')) is unaffected.HEADon an item saves nothing on the database — and this is by design: there is no DB win to be had there at all. An itemHEADmust still return the correct200/404/403, which requires the read. Existence is only knowable by querying, and an object-levelsecurityexpression (e.g.is_granted('VIEW', object)) needs the hydrated entity — it is evaluated in the provider, before any processor, and onHEADexactly likeGET. So even degrading the fetch to a bareEXISTSwould not help as soon as security (or validation, or serialization) touches the object. And unlike a collection — whose paginator is lazy, so the rowSELECTis deferred to body-building and thus skippable — an item is fetched eagerly, upstream of the processors. The body is still correctly empty; there is simply nothing to gain. This PR is about collections.