🔥 feat: Add HTTP QUERY method support (RFC 10008)#4436
Conversation
Register QUERY as a first-class HTTP method alongside the existing nine. QUERY is safe and idempotent, allowing request bodies for complex queries without the URL-length constraints of GET. - Add MethodQuery constant and methodQuery iota - Add Query() to Router, Register interfaces and all implementations (App, Group, Registering, domainRouter, domainRegistering) - Add Query() client shorthand on Request - Mark QUERY as safe in IsMethodSafe (idempotent follows from safe) - Include QUERY in CSRF safe-method branch (no token required) - Include QUERY in CORS default AllowMethods - Add QUERY color to logger middleware - Update constants documentation
…itch-branches lint
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds the ChangesQUERY HTTP Method Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
I intentionally excluded cache middleware since its signature cannot handle body, maybe some kind of change needed for this? |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
middleware/cors/cors_test.go (1)
645-645: ⚡ Quick winAdd a preflight case where
Access-Control-Request-MethodisQUERY.These assertions now expect
QUERY, but the loopedmethodstable still omitsfiber.MethodQuery, so the request-type path for QUERY itself isn’t exercised. Add it to the table to close the test gap.Also applies to: 679-679
🤖 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 `@middleware/cors/cors_test.go` at line 645, The test assertions at lines 645 and 679 now expect QUERY to be included in the Access-Control-Allow-Methods header, but the methods table being looped in the preflight test case does not include fiber.MethodQuery. Add fiber.MethodQuery to the methods table in the test to ensure the preflight request path for QUERY method is actually exercised and tested, closing the gap between the assertion expectations and the actual test coverage.
🤖 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.
Nitpick comments:
In `@middleware/cors/cors_test.go`:
- Line 645: The test assertions at lines 645 and 679 now expect QUERY to be
included in the Access-Control-Allow-Methods header, but the methods table being
looped in the preflight test case does not include fiber.MethodQuery. Add
fiber.MethodQuery to the methods table in the test to ensure the preflight
request path for QUERY method is actually exercised and tested, closing the gap
between the assertion expectations and the actual test coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 364fe260-35d6-4f4f-849e-9255b8bde8c9
📒 Files selected for processing (16)
app.goapp_test.goclient/request.goconstants.godocs/api/constants.mddocs/client/request.mddocs/partials/routing/handler.mddomain.gogroup.gohelpers.gomiddleware/cors/config.gomiddleware/cors/cors_test.gomiddleware/csrf/csrf.gomiddleware/logger/utils.goregister.gorouter.go
Yes, we should add that, since the QUERY explicitly states that it can be cached. |
|
Small changes needed but need to decide how can we generate cache key in defaultKeyGenerator for QUERY request.
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4436 +/- ##
==========================================
+ Coverage 91.58% 91.60% +0.02%
==========================================
Files 134 134
Lines 13548 13513 -35
==========================================
- Hits 12408 12379 -29
+ Misses 725 723 -2
+ Partials 415 411 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
@gaby What documentation and units should be added? |
@nekoworks-magic ^ Coverage report |
That would be a bad idea, a large body would take a while to hash and can be used to cause DoS. |
|
Then we can do:
|
|
@nekoworks-magic Shouldnt it do the same as POST? What does the RFC say? |
|
RFC says: So no, it is different from POST. When caching QUERY responses, the cache key MUST include the request content, as required by the RFC. |
RFC 10008 requires the cache key to incorporate request content for QUERY responses. Hash the body with SHA-256 in defaultKeyGenerator when the method is QUERY. QUERY is not in the default Methods list, so this is opt-in only.
|
I added draft implementation. |
@nekoworks-magic I think we got to use a pool, similar to how |
Cover cache behavior when the HTTP QUERY method (RFC 10008) is in the Methods allow-list: same body → hit, different body → miss, empty body, and default Methods exclusion. Add a benchmark for the QUERY cache path.
|
Did some benchmark and profiling, then I found current implementation does not allocate anything, and adding pool actually makes things slower. |
Review — HTTP QUERY method (RFC 10008)I went through the full diff and the discussion thread. Overall this is a clean, well-tested addition: the new method is wired consistently through A few things worth confirming before merge: 1. Cache body hashing — DoS surface (the open thread)The cache key now does However, @gaby's original point about hashing large bodies isn't fully resolved: once a user adds
Not a blocker given the opt-in gating, but I'd lean toward documenting it explicitly. 2. Cache key — no method component (verify, not a bug here)
3. CORS default change (intentional behavior change)Adding Minor
Nice work overall — the test coverage across app/group/domain/route-chain/client and the cache scenarios (same body hit, different body miss, empty body, not-in-Methods) is thorough. Generated by Claude Code |
|
gaby
left a comment
There was a problem hiding this comment.
One performance suggestion on the QUERY cache-key body hashing — details inline. The current code is correct; this just keeps SHA-256 off the hot path for typical (small) request bodies by reusing the existing boundKeySegment helper.
Generated by Claude Code
gaby
left a comment
There was a problem hiding this comment.
LGTM, only thing left is the CORS change. Pending @sixcolors review.
|
This is what the RFC says about CORS:
|
gaby
left a comment
There was a problem hiding this comment.
One optional test suggestion on the CORS side: a small dedicated regression test documenting RFC 10008's preflight requirement for QUERY (it's not CORS-safelisted). Details inline — no production change needed, the middleware already behaves correctly.
Generated by Claude Code
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
feat: complete HTTP QUERY method support (follow-up to #4436)
Description
Add first-class support for the HTTP QUERY method (RFC 10008). QUERY is safe and idempotent like GET, but allows request bodies for complex queries.
Changes introduced
MethodQueryconstant,methodQueryiota,DefaultMethodsentryQuery()onRouter,Registerinterfaces and all implementations (App, Group, Registering, domainRouter, domainRegistering)Request.Query()client shorthandIsMethodSafereturns true for QUERYAllowMethodsincludes QUERYType of change
Checklist
/docs/directory for Fiber's documentation.