feat(core): QUERY is a read, not a write (Refs #462) - #675
Open
rejifald wants to merge 1 commit into
Open
Conversation
Part 1 of #462 only — the keepalive/Beacon half (part 2) is untouched, which is why this is `Refs`, not `Closes`. `method: 'QUERY'` already sent its body and already cached correctly under `cache: { methods: 'QUERY' }`. What it did not have was the engine's agreement that it is a read: "safe method" was spelled `=== 'GET' || === 'HEAD'` inline, so everything else was a write by default. One `isSafeMethod` predicate (RFC 9110 §9.2.1's safe set plus QUERY) now answers that in the two places that ask it — `applyIdempotency`, so a QUERY is no longer stamped with an `Idempotency-Key`, and the construction nudge that mirrors that drop, so it is never silent. OPTIONS/TRACE stop being stamped too; they were only getting a key because they were not GET or HEAD. A 301/302 no longer downgrades a QUERY to a bodyless GET on the default fetch transport. That downgrade is a historical exception granted to POST, and draft-ietf-httpbis-safe-method-w-body rules it out by name for QUERY; applying it dropped the body, silently turning a filtered read into an unfiltered one. The two look-alike method tests are deliberately NOT routed through the predicate, and the comments say why: `encodeRequestBody` still drops a body on GET/HEAD only (a transport constraint — widening it to "safe" would delete the payload of every QUERY), and the redirect exemption is spec text about QUERY, not a safety rule. `cache.methods` keeps its `['GET','HEAD']` default; QUERY is documented as a valid opt-in entry, not added to it. `StitchConfig.method` widens to `KnownMethod | (string & {})` — an autocomplete list, never an allowlist; a tsd test pins that any string still typechecks. The bundle budget rises 24.10→24.20 / 21.50→21.65 KB (measured 24.12 / 21.56). `main` had one byte of headroom on `import { stitch }`, so the whole +58 B lands over; the advertised figure moves ~21 → ~22 kB across the eight tethered sites. Refs #462 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Implements part 1 only of #462 — first-class
QUERY. Part 2 (keepalive/Beacon) is untouched, which is why this isRefs, notCloses; the issue itself says the two are independently shippable.method: 'QUERY'already sent its body and already cached correctly undercache: { methods: 'QUERY' }, because the key folds the request body in. What it did not have was the engine's agreement that it is a read: "safe method" was spelled=== 'GET' || === 'HEAD'inline, so everything else was a write by default.The four sites, and what each one actually asks
The issue's framing is that GET/HEAD is special-cased in three places and one
isSafehelper fixes all three. That is the one thing here I'd push back on. The sites look alike and ask three different questions, and conflating them is precisely how aQUERYloses its body. So: one predicate, used at the two sites that mean it, and a sharpened comment at the two that don't.engine.tsapplyIdempotencyisSafeMethodstitch.tswarnConstructionisSafeMethodhttp-adapter.tsencodeRequestBodyGET/HEAD, commentedhttp-adapter.tsfollowRedirectsQUERYexempted — spec text, not safetycache.ts['GET','HEAD']defaultisSafeMethod— RFC 9110 §9.2.1's safe set, plus QUERYGET,HEAD,OPTIONS,TRACE,QUERY. Internal (util.ts, not on the barrel), case-insensitive, fail-closed on an unknown verb.Idempotency-Keyon a safe method. Withidempotencyconfigured, aQUERYis no longer stamped. There is no side effect to collapse, and the header would have varied the cache key on every send — so a stitch that opted intomethods: 'QUERY'was quietly getting a 0% hit rate.idempotencyon aQUERYnow logs the same "the key is sent on writes only" hint aGETgets.A second, incidental behaviour change, stated plainly:
OPTIONSandTRACEstop being stamped too, and now get the nudge. They were only ever getting a key because they were notGETorHEAD. I chose the RFC-complete set over{GET, HEAD, QUERY}because a predicate calledisSafeMethodhas to mean what RFC 9110 says it means — a future call site routed through a set that quietly omitsOPTIONSwould inherit a bug. It is pinned by test, not left to be discovered. (It is not free: see the budget section — it is 21 of the 58 bytes.)stitch.ts:427— the fourth site, which the issue does not listIt is the same "is this a read?" notion, not a different question wearing the same shape. It is the construction-time mirror of
applyIdempotency: it fires exactly when the key would be dropped, to say so. Fixing the engine without it would have created the failure the nudge exists to prevent — aQUERY+idempotencyconfig that is now silently inert with nothing printed. Sharing one predicate means the two cannot drift: a method the engine skips is a method this warns about.encodeRequestBody— deliberately NOT routed through the helperThis branch enforces a transport constraint, not a safety rule:
fetchthrows aTypeErrorwhen aGET/HEADinit carries a body.QUERYis equally safe and must keep its body — the body is the query. Widening this to "safe" would have deleted the payload of everyQUERYrequest, i.e. the one thing that already worked. Left atGET/HEADwith a comment saying why, andOPTIONS(safe, body kept) is asserted in the suite as the proof this is a different predicate.Redirects — changed, because the draft says so by name
The issue does not mention this; I read the spec while checking whether the site should be routed through the helper, and found a real bug.
The downgrade-to-
GETon a 301/302 is a historical exception granted toPOST(RFC 9110 §15.4.2/§15.4.3). draft-ietf-httpbis-safe-method-w-body rules it out forQUERYin as many words:— the server is asking for "a similar QUERY request to the new target URI". Applying it dropped the body, so a redirect silently turned a filtered read into an unfiltered one. Now exempt.
Scoped to exactly what the spec mandates:
isSafeMethod.OPTIONS/TRACEare safe and no spec text exempts them, so they keep today's behaviour. This is the one place where the RFC-complete set would have been the wrong predicate.303stays unconditional — the draft agrees it means the result "can be accomplished via a normal retrieval request" at theLocation.307/308already preserved method and body; unchanged.POST/PUT/PATCH/DELETEredirect exactly as before, asserted.fetchtransport only.axiosAdapterdelegates rewriting tofollow-redirectsandxhrAdapterto the browser; neither is reachable from here.One pre-existing deviation I noticed and did not touch: this loop rewrites
HEAD→GETon a 303, where the fetch spec preservesHEAD. Unrelated toQUERY, so it stays out.Cache default — not changed, on purpose
Still
['GET','HEAD']. The issue says explicit opt-in "is defensible" and asks only that it be documented, so that is all this does.QUERYis now documented as a validcache.methodsentry that keys on the body, alongside the GraphQLPOSTopt-in it is exactly analogous to.The optional type widening — included
StitchConfig.methodis nowKnownMethod | (string & {}). Included because it is what closes the issue's gap 4 (nothing told an authorQUERYexists), it is purely additive, and it passes every gate:check:contractclean (KnownMethodtrips no R1 suffix),check:typesacross 38 projects,tsd,check:exports. Zero runtime bytes — the type is erased.known-method.test-d.tspins the additive claim rather than asserting it, and I verified it is load-bearing: narrowingmethodto a closedKnownMethodmakes it fail on both the custom-verb and thedeclare const runtimeVerb: stringcases.One cosmetic regression, so it is not discovered later: the playground autocomplete's
detailline formethodnow readsKnownMethod | (string & {})where it readstring. A reader who cannot resolve the name learns less than before. The IDE hover — the thing the widening is for — expands it properly. Worth flagging as a revert candidate if you dislike it; it is one line oftypes.tsand the generated file.What I left out of the issue's proposal
isIdempotentMethodis not here. It has zero call sites:retryis gated on status (retry.on/ the verdict), never on method idempotency, so the helper would be a function with no behaviour attached, shipping bytes on an entry with one byte of headroom (below). The set it would return is derivable the moment something needs it (safe ∪PUT,DELETE). Adding it now is how a codebase acquires a second, silently-diverging source of truth — the exact thing this PR is fixing.Tests
New
packages/core/test/query-method.spec.ts— 18 tests, driven through the published kit (mockAdapterfromstitchapi/testing) and the internal node mock server, pluspackages/core/test-d/known-method.test-d.ts.They are real guards, verified by reverting. With the three call-site changes stashed and
util.tskept (so the file still compiles), 6 of 18 fail:The 12 that pass either way are the regression guards on unchanged behaviour —
GET/HEADstill drop a body,OPTIONSdoes not,POST/PUT/PATCH/DELETEstill get a key, a caller-supplied key still wins, a 303 still downgrades, a 307 still doesn't, the default cache still refuses aQUERY, andmethods: 'QUERY'caches with two different bodies landing in two entries.Bundle budget — raised, and the advertised figure moves
mainhad one byte of headroom onimport { stitch }(22015 B of a 22016 B ceiling) and 0.02 KB on the entry. The next core-path change of any size was going to pay for the raise; this is that change.mainimport { stitch }Attributed by measuring each piece: +33 B the helper and its two rerouted call sites, +21 B carrying
OPTIONS/TRACEin the safe set, +4 B the redirect exemption. None of it can move behind a subpath —applyIdempotencyis inbuildRequest, the nudge is inmakeStitch,fetchAdapteris the default transport. I also measured a regex form of the predicate (identical gzip, worse brotli) and a{GET, HEAD, QUERY}-only set (22052 B — still over), so there is no version of this that fits under a zero-byte ceiling. A minimum step, not the ~0.2 KB this gate usually restores, matching #477/#524/#485: the change is 58 bytes and the entry is full, and the tight ceiling is the signal.The advertised figure moves. 22015 B is 21.499 KB and rounds to 21; 22073 B is 21.556 and rounds to 22. So
import { stitch }goes ~21 → ~22 kB across the eight sites under thebundle-advertised-sizetether — both READMEs, installation, principles, the home-page metrics component, and the docs' source blurb. The whole entry stays ~24. This is a maintainer-visible number, so it is called out here and written into the budget comment rather than left in the diff. Propagated by hand (yakir will not auto-resolve a two-value conflict) and then verified with the tether, not assumed:playground-completionsalso drifted (themethodJSDoc feeds it) and was regenerated. While regenerating I found the generator renders{@link A | b}as a bare| b, so the JSDoc avoids the piped form.Docs
methodandcache.methodsJSDoc (which is whatAutoTypeTablerenders), theIdempotencyOptionsdoc comment — "a read" is now defined rather than spelled(GET/HEAD)— a new The QUERY method section on thestitch()guide, acache.methodsparagraph on the config-types reference, and a Reads never get a key section on the idempotency guide. The guide section carries the caveat the rest of this cannot:QUERYis young, and the proxies, CDNs and WAFs between you and the server may not route it.CHANGELOG under
Unreleased → Added.Verification
Every gate, run to completion, exit codes:
check:contractcheck:lintcheck:typescheck:types-d(tsd)testcheck:formatcheck:changelogcheck:docs-links/docs/...link resolvescheck:unknown-keyscheck:exports(attw)check:releasecheck:size--tier executable/--tier tokenNothing failed.
yakir.lockis not touched: its baseline was already behind on four tethers before this branch (bundle-advertised-sizerecords"20, 23"against amainmeasuring"21, 24"), andcheckpasses on site agreement, so refreshing it here would have swept up unrelated tethers.🤖 Generated with Claude Code