feat: add defineQueryHandler with cacheable GET equivalence#1448
Open
pi0x wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds ChangesQUERY handler support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 4.8 <[email protected]>
pi0x
force-pushed
the
feat/define-query-handler
branch
from
July 10, 2026 08:06
9795969 to
3d8ad75
Compare
pi0
marked this pull request as ready for review
July 10, 2026 08:07
pi0x
changed the title
defineQueryHandlerdefineQueryHandler with cacheable GET equivalence
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/utils/query.ts (1)
100-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting
defineQueryHandlerinto its own file.The file is ~243 lines, exceeding the 200-line guideline. Moving
defineQueryHandlerand its types (QueryHandlerGetOptions,QueryHandlerBase) to a dedicated file would bringquery.tsunder the target while improving cohesion.As per coding guidelines: "Keep files short — aim for less than 200 lines of code per file, split when larger."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/query.ts` around lines 100 - 242, Split defineQueryHandler and its related types QueryHandlerGetOptions and QueryHandlerBase into a dedicated module, then update query.ts imports/exports and any callers so behavior and public API remain unchanged. Keep the supporting query parsing utilities in query.ts and ensure the extracted module references them through appropriate imports.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/2.utils/1.request.md`:
- Around line 142-151: Move the HTTP-cacheable GET behavior prose out of the
`@example` block in the source JSDoc for the query handler utility, placing it in
the main description so automd renders it as regular documentation rather than
code. Update the JSDoc in the relevant query utility source, not the generated
markdown, and preserve the example’s actual code content.
In `@docs/4.examples/handle-query.md`:
- Line 15: Update the defineQueryHandler link in the documentation paragraph to
use the generated anchor `/utils/request#definequeryhandler`, replacing the
incorrect `#definequeryhandlerdef` target.
In `@examples/query.mjs`:
- Around line 90-93: Update the cache-control condition in the request handler
to apply when event.req.method is either "GET" or "HEAD", ensuring HEAD
responses receive the same cacheable GET headers.
In `@src/utils/internal/query-get.ts`:
- Around line 78-87: Update setQueryContentLocation() to construct
Content-Location from the client-facing external pathname rather than
event.url.pathname, preserving the mounted base path introduced by
mount()/withBase(). Keep the existing query parameter updates and length guard,
but use the request URL or framework-provided external URL source that retains
the original pathname.
---
Nitpick comments:
In `@src/utils/query.ts`:
- Around line 100-242: Split defineQueryHandler and its related types
QueryHandlerGetOptions and QueryHandlerBase into a dedicated module, then update
query.ts imports/exports and any callers so behavior and public API remain
unchanged. Keep the supporting query parsing utilities in query.ts and ensure
the extracted module references them through appropriate imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0105dbe8-1373-420b-a3d3-5eb5cd0dac18
📒 Files selected for processing (9)
docs/2.utils/1.request.mddocs/4.examples/handle-query.mdexamples/query.mjssrc/index.tssrc/utils/internal/media-type.tssrc/utils/internal/query-get.tssrc/utils/query.tstest/query.test.tstest/unit/package.test.ts
Comment on lines
+78
to
+87
| const params = new URLSearchParams(event.url.search); | ||
| params.set(get.param, query); | ||
| if (!defaultFormat) { | ||
| params.set(get.formatParam, format); | ||
| } | ||
| const location = `${event.url.pathname}?${params}`; | ||
| if (location.length <= MAX_CONTENT_LOCATION_LENGTH) { | ||
| event.res.headers.set("content-location", location); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how event.url is constructed and whether pathname includes mount/base prefixes.
fd -e ts . src | xargs rg -nP '\burl\b.*=|get url\(|this\.url' -g '!**/*.test.ts' | rg -n 'url' | head -50
ast-grep outline src/event.ts --items allRepository: h3js/h3
Length of output: 3700
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in src/event.ts src/utils/request.ts src/utils/base.ts src/h3.ts src/utils/internal/query-get.ts; do
echo "===== $f ====="
wc -l "$f"
done
echo "===== src/event.ts (60-95) ====="
sed -n '60,95p' src/event.ts
echo "===== src/utils/request.ts (1-60) ====="
sed -n '1,60p' src/utils/request.ts
echo "===== src/utils/base.ts (1-60) ====="
sed -n '1,60p' src/utils/base.ts
echo "===== src/h3.ts (130-155) ====="
sed -n '130,155p' src/h3.ts
echo "===== src/utils/internal/query-get.ts (70-95) ====="
sed -n '70,95p' src/utils/internal/query-get.tsRepository: h3js/h3
Length of output: 6971
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact code paths that manipulate event.url.pathname and the Content-Location builder.
sed -n '60,95p' src/event.ts
sed -n '1,60p' src/utils/request.ts
sed -n '1,60p' src/utils/base.ts
sed -n '130,155p' src/h3.ts
sed -n '70,95p' src/utils/internal/query-get.tsRepository: h3js/h3
Length of output: 6511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== src/event.ts (60-95) ====="
sed -n '60,95p' src/event.ts
echo "===== src/utils/request.ts (1-60) ====="
sed -n '1,60p' src/utils/request.ts
echo "===== src/utils/base.ts (1-60) ====="
sed -n '1,60p' src/utils/base.ts
echo "===== src/h3.ts (130-155) ====="
sed -n '130,155p' src/h3.ts
echo "===== src/utils/internal/query-get.ts (70-95) ====="
sed -n '70,95p' src/utils/internal/query-get.tsRepository: h3js/h3
Length of output: 6705
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== usages of setQueryContentLocation ====="
rg -n "setQueryContentLocation" src
echo "===== src/utils/internal/query-get.ts (1-120) ====="
sed -n '1,120p' src/utils/internal/query-get.ts
echo "===== related query handler code ====="
rg -n "QueryGetOptions|defaultFormat|content-location|query-get|get\.formatParam|get\.param" srcRepository: h3js/h3
Length of output: 4903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '180,255p' src/utils/query.tsRepository: h3js/h3
Length of output: 2541
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== tests mentioning defineQueryHandler / Content-Location / mount / withBase ====="
rg -n "defineQueryHandler|content-location|Content-Location|withBase|mount\\(" test src --glob '!**/*.snap'
echo "===== query-related tests ====="
rg -n "defineQueryHandler|QUERY|accept-query|content-location" test src --glob '!**/*.snap'Repository: h3js/h3
Length of output: 16727
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "defineQueryHandler|content-location|Content-Location|withBase|mount\\(" . --glob '!**/*.snap' --glob '!**/dist/**' --glob '!**/coverage/**'Repository: h3js/h3
Length of output: 11320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the docs / comments around QUERY equivalence for clues about the intended URL form.
rg -n "RFC 10008|equivalent GET|Content-Location|mounted|base path|base url" src README.md docs . --glob '!**/*.snap' --glob '!**/dist/**' --glob '!**/coverage/**'Repository: h3js/h3
Length of output: 15398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any tests or examples showing QUERY under a base path.
rg -n "withBase\\(|mount\\(|QUERY" . --glob '!**/*.snap' --glob '!**/dist/**' --glob '!**/coverage/**'Repository: h3js/h3
Length of output: 17476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the main handler wrapper to see whether it rewrites the URL around handler execution.
sed -n '1,140p' src/handler.tsRepository: h3js/h3
Length of output: 4341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== matches ====="
rg -n "defineQueryHandler|content-location|Content-Location|withBase\\(|mount\\(|RFC 10008|equivalent GET|QUERY" . --glob '!**/*.snap' --glob '!**/dist/**' --glob '!**/coverage/**'
echo "===== src/handler.ts (1-140) ====="
sed -n '1,140p' src/handler.ts
echo "===== src/utils/query.ts (180-255) ====="
sed -n '180,255p' src/utils/query.tsRepository: h3js/h3
Length of output: 28739
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== src/event.ts (140-170) ====="
sed -n '140,170p' src/event.ts
echo "===== docs nested-app note ====="
sed -n '35,55p' docs/1.guide/1.basics/7.nested-apps.mdRepository: h3js/h3
Length of output: 1493
Content-Location should use the external pathname under mounts
setQueryContentLocation() reads event.url.pathname, but mount()/withBase() rewrite that to the internal path before the handler runs. Under a mounted base, the header will point to /books?... instead of the client-facing /api/books?....
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/internal/query-get.ts` around lines 78 - 87, Update
setQueryContentLocation() to construct Content-Location from the client-facing
external pathname rather than event.url.pathname, preserving the mounted base
path introduced by mount()/withBase(). Keep the existing query parameter updates
and length guard, but use the request URL or framework-provided external URL
source that retains the original pathname.
Unify `defineQueryHandler` so the body is read as text by default and
provided as `context.query` on both the QUERY and GET paths (single
`{ format, query }` context). Add `body: false` to opt out and read the
body manually (`{ format }` context).
Also support the `get: true` shortcut and switch the GET-equivalence
defaults to `?q=` (query) and `?f=` (format) param names.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
4 tasks
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.
This PR adds
defineQueryHandler, a handler factory for the HTTPQUERYmethod (RFC 10008) that captures the ceremony aQUERYhandler repeats. building on top ofrequireContentTypeandappendAcceptQueryutils (#1446) and first-classQUERYrouting (#1445). This PR also supports an optionalgetoption implementing RFC 10008 §2.3's cacheability guidance.The request body is read as text and passed to the handler as
query. Passbody: falseto read it yourself (e.g. as a stream or with a custom parser) — the handler then receives only{ format }.With
get, the same handler also serves an equivalent, HTTP-cacheableGET, and successfulQUERYresponses advertise it viaContent-Location— no server-side result store needed:Behavior
Core
formatslists the accepted query media types (wildcardsapplication/*/*/*supported). Serialized once at definition time — invalid media types throw aTypeErrorat startup, not per-request.query, so the common case is a single{ format, query }context on every path.body: falseopts out: the handler receives only{ format }and reads the request body itself (typed via overloads).Accept-Queryis advertised on every response, including error responses (viaevent.res.errHeaders), so a rejected client learns what it should have sent.405+Allowfor other methods, then400(missingContent-Type) /422(malformed) /415(unsupported) viarequireContentType.format(lower-cased, without parameters);middleware/metapass throughdefineHandlerlike otherdefine*factories.getequivalence (#1449)getset, the handler context always hasquery(body text onQUERY, URL param onGET/HEAD).get: trueuses the default?q=(query) /?f=(format) param names; pass a string to set the query param (get: "q") or an object to set both (get: { param, formatParam }).GETformat comes from?f=; it may be omitted only when exactly one concrete (non-wildcard) format is accepted — silent defaulting across formats would change query semantics. AllGET-path rejections are400(per RFC 9110,415/422apply to request content, which aGEThas none of).Content-Locationpreserves the request's existing search params (aQUERYto/books?lang=enadvertises/books?lang=en&q=…), is built viaURLSearchParams, is skipped when the URL would exceed 2048 chars (long queries being the reasonQUERYexists), and is never set on theGETpath.app.all("/books", searchBooks)— no per-method wiring. It servesQUERY(andGET/HEADwhengetis set) and returns405+Allowfor anything else. The method guard is405+Allow: GET, HEAD, QUERYwhengetis set (Allow: QUERYotherwise).HEADhere is the bodiless form of the cacheableGET(RFC 9110 §9.3.2: "identical to GET except that the server MUST NOT send content"), not a variant ofQUERY— there is no HEAD-of-QUERY. It's handled on theGETpath (query read from?q=) and only whengetis set; withoutgetthere's no URL-addressable representation, soHEADgets405. Theallroute matches it directly; even aGET-only registration would match it automatically (feat: automatically match GET routes for HEAD requests #1452), letting clients revalidate/probe the advertised URL cheaply.Design notes
body: false: reading the body as text by default unifies the context to{ format, query }across the QUERY and GET paths (QUERYbodies are text queries by definition), and removes the earlier asymmetry where only thegetpath pre-read the body.body: falsekeeps the escape hatch for streaming/binary/custom parsing; body-size limiting still works viabodyLimitmiddleware (runs before the handler).getstays opt-in: default-on would be inert under the idiomaticapp.query()registration (the router never routesGETto aQUERY-only route) and would push query contents into URLs/logs/history for everyone — the opposite of whatQUERY's body-carried queries are for. Explicitgetsignals intent to also mount broadly (app.all).cache: {...}option (wiringhandleCacheHeaders) was dropped: it duplicates a documented one-liner, andhandleCacheHeaderscurrently force-prependspublictoCache-Controland ORsIf-None-Match/If-Modified-Since(RFC 9110 §13.2.2 wants ETag to take precedence) — worth fixing upstream before building sugar on it. Can open a separate issue.Changes
src/utils/query.ts:defineQueryHandlerwithgetoption,body: falseopt-out + overloadssrc/utils/internal/media-type.ts: Structured-Fields/media-type helpers moved out ofutils/query.ts;src/utils/internal/query-get.ts: GET resolution +Content-Locationhelpers (keepsquery.tsunder the size guideline)test/query.test.ts: matrix tests (dispatch, parameters, wildcards,400/415/422/405,Accept-Queryon errors,body: false,get: trueshortcut, middleware, definition-time throws, round-trip identity, param preservation, encoding, length cutoff,HEADvia automatic GET matching)examples/query.mjs: rewritten around the new API — drops the manual resultMap,queryIdhash, and/books/:idroute; registers a singleapp.all("/books", …)route (the handler gates the verbs); curl comments include aHEADprobeTest plan
query.test.tsmatrix green (web + node), no type errors, lint clean.Disclaimer
🤖 This PR was written by Claude Code (design and implementation reviewed and directed by a human).
🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit
New Features
defineQueryHandlerfor first-class HTTPQUERYrouting with media-type/format negotiation, wildcard support, and consistentAccept-Queryon both success and error responses.GET/HEADequivalence viaContent-Location, including preservation of existing search params and safe URL-length handling.format(andquerywhen enabled), includingbody: false.Documentation
QUERYandGETequivalence model.Tests