feat: complete HTTP QUERY method support (follow-up to #4436)#4456
Merged
Conversation
Contributor
|
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:
WalkthroughThe PR adds QUERY client helpers and route documentation, expands redirect handling for QUERY requests, updates cache-key construction for QUERY bodies, and revises middleware method classifications, docs, and tests to include QUERY behavior. ChangesQUERY method support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4456 +/- ##
==========================================
+ Coverage 91.70% 91.80% +0.10%
==========================================
Files 134 134
Lines 13498 13504 +6
==========================================
+ Hits 12378 12398 +20
+ Misses 713 699 -14
Partials 407 407
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/middleware/cors.md`:
- Line 117: The Default Config example in the CORS docs is missing
fiber.MethodQuery in AllowMethods, causing it to diverge from the documented
table and the actual middleware defaults. Update the default configuration
snippet in the CORS docs to include fiber.MethodQuery in the AllowMethods list,
matching the behavior defined by the cors config/defaults and keeping the
example consistent with the AllowMethods entry.
In `@helpers_test.go`:
- Around line 1831-1832: The local slice named unsafe in helpers_test.go is
shadowing the unsafe import name and triggering the revive import-shadowing lint
failure. Rename the safe and unsafe slices in the relevant test setup to
non-conflicting names such as safeMethods and unsafeMethods, and update the
nearby assertions/usages accordingly so the test logic stays the same while
avoiding the import shadow.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 83824e8f-4f1d-47de-8502-d02ed1837c92
📒 Files selected for processing (13)
client/client.goclient/request_test.godocs/api/app.mddocs/client/rest.mddocs/middleware/cache.mddocs/middleware/cors.mddocs/middleware/csrf.mddocs/middleware/earlydata.mddocs/middleware/idempotency.mdhelpers_test.gomiddleware/cache/cache.gomiddleware/cache/keygen_test.gomiddleware/csrf/csrf_test.go
ReneWerner87
force-pushed
the
feat/http-query-method-followup
branch
from
June 25, 2026 08:32
c339022 to
b7a9181
Compare
Follow-up filling gaps found in review and aligning the cache key with the keygen refactor already on main (#4450): - client: add Client.Query and package-level Query to match every other verb shorthand (previously only Request.Query existed) - cache: escape the QUERY body and append it via appendBoundKeySegment, consistent with the per-dimension escaping on main (was raw, old-style); pin the exact format with QUERY golden cases in keygen_test.go - tests: assert IsMethodSafe/IsMethodIdempotent for QUERY; cover QUERY in the CSRF safe-method tests; add Test_Client_Query - docs: add QUERY to CORS AllowMethods default, the Register interface, the safe-method lists (idempotency, earlydata, csrf), the cache body-key note, and the client REST reference Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ReneWerner87
force-pushed
the
feat/http-query-method-followup
branch
from
June 25, 2026 08:37
b7a9181 to
e5fd66f
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/client_test.go (1)
1251-1251: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove fixed sleep from test setup.
time.Sleep(1 * time.Second)makes the test slower and can still be flaky as a readiness mechanism; prefer relying onstartTestServerWithPortreadiness (or polling the endpoint with timeout if needed).🤖 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 `@client/client_test.go` at line 1251, The test setup in client_test.go uses a fixed time.Sleep as a readiness wait, which slows the suite and can still be flaky. Remove the sleep from the test flow around startTestServerWithPort and rely on the server’s readiness signal instead; if readiness is still uncertain, replace it with a bounded polling check against the endpoint using a timeout. Keep the change localized to the test helper or setup path that currently waits before proceeding.
🤖 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 `@client/client_test.go`:
- Line 1251: The test setup in client_test.go uses a fixed time.Sleep as a
readiness wait, which slows the suite and can still be flaky. Remove the sleep
from the test flow around startTestServerWithPort and rely on the server’s
readiness signal instead; if readiness is still uncertain, replace it with a
bounded polling check against the endpoint using a timeout. Keep the change
localized to the test helper or setup path that currently waits before
proceeding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e40bdee4-accd-4fb7-bdfb-04a70858f7a1
📒 Files selected for processing (13)
client/client.goclient/client_test.godocs/api/app.mddocs/client/rest.mddocs/middleware/cache.mddocs/middleware/cors.mddocs/middleware/csrf.mddocs/middleware/earlydata.mddocs/middleware/idempotency.mdhelpers_test.gomiddleware/cache/cache.gomiddleware/cache/keygen_test.gomiddleware/csrf/csrf_test.go
✅ Files skipped from review due to trivial changes (7)
- docs/middleware/earlydata.md
- docs/api/app.md
- middleware/csrf/csrf_test.go
- docs/middleware/cache.md
- docs/middleware/idempotency.md
- docs/middleware/csrf.md
- docs/middleware/cors.md
🚧 Files skipped from review as they are similar to previous changes (5)
- middleware/cache/cache.go
- helpers_test.go
- client/client.go
- docs/client/rest.md
- middleware/cache/keygen_test.go
…ase notes A second review pass over the merged QUERY method surfaced a few remaining gaps: - client: allow QUERY to follow redirects. client/core.go gated DoRedirects to GET/HEAD only, so QUERY silently ignored SetMaxRedirects. QUERY is safe+idempotent, so redirects are now followed with method and body preserved (verified with a 308 round-trip test). - tests: assert QUERY appears in the OPTIONS and 405 Allow header; add QUERY cases to the earlydata and idempotency behavioral tests (both treat QUERY as safe via IsMethodSafe). - docs: document the QUERY method (RFC 10008) in whats_new.md; it was absent from the v3 release notes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/core.go (1)
116-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEligibility logic is correct; consider deduplicating the repeated method check.
The QUERY addition is sound: per RFC 10008 QUERY is safe/idempotent, and
transport.goonly downshiftsPOST→GETon 301/302/303, so QUERY (like GET/HEAD) follows redirects preserving method and body. The identical three-method condition is now duplicated across the retry and non-retry branches; extracting a small helper would keep them from drifting.♻️ Extract redirect-eligibility helper
func shouldFollowRedirects(method string, maxRedirects int) bool { return maxRedirects > 0 && (method == fiber.MethodGet || method == fiber.MethodHead || method == fiber.MethodQuery) }- if c.req.maxRedirects > 0 && (string(reqv.Header.Method()) == fiber.MethodGet || string(reqv.Header.Method()) == fiber.MethodHead || string(reqv.Header.Method()) == fiber.MethodQuery) { + if shouldFollowRedirects(string(reqv.Header.Method()), c.req.maxRedirects) { return c.client.DoRedirects(reqv, respv, c.req.maxRedirects) } return c.client.Do(reqv, respv) }) } else { - if c.req.maxRedirects > 0 && (string(reqv.Header.Method()) == fiber.MethodGet || string(reqv.Header.Method()) == fiber.MethodHead || string(reqv.Header.Method()) == fiber.MethodQuery) { + if shouldFollowRedirects(string(reqv.Header.Method()), c.req.maxRedirects) { err = c.client.DoRedirects(reqv, respv, c.req.maxRedirects) } else { err = c.client.Do(reqv, respv) } }🤖 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 `@client/core.go` around lines 116 - 127, The redirect eligibility check in core.go is duplicated in both the retry and non-retry branches, so extract it into a small helper to prevent the method list from drifting. Add a helper such as shouldFollowRedirects and use it from the logic around c.client.DoRedirects and c.client.Do so the GET/HEAD/QUERY condition is defined once and reused consistently.
🤖 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 `@client/core.go`:
- Around line 116-127: The redirect eligibility check in core.go is duplicated
in both the retry and non-retry branches, so extract it into a small helper to
prevent the method list from drifting. Add a helper such as
shouldFollowRedirects and use it from the logic around c.client.DoRedirects and
c.client.Do so the GET/HEAD/QUERY condition is defined once and reused
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b298edcb-c7a9-40ca-a22e-d170a5136008
📒 Files selected for processing (6)
app_test.goclient/core.goclient/request_test.godocs/whats_new.mdmiddleware/earlydata/earlydata_test.gomiddleware/idempotency/idempotency_test.go
ReneWerner87
force-pushed
the
feat/http-query-method-followup
branch
from
June 25, 2026 10:03
a819e6e to
948553f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@middleware/cache/cache.go`:
- Around line 247-252: The QUERY body handling in the cache key path currently
materializes the fully escaped body string before `boundKeySegment` can decide
to hash it, causing unnecessary allocations for large bodies. Update the
`requestMethod == fiber.MethodQuery` branch to route body key generation through
a helper like `queryBodyKeySegment` that short-circuits long inputs and hashes
the escaped bytes incrementally instead of building the escaped string first;
keep the same escaping/bounding behavior for small bodies and preserve the
existing `escapeKeyDelimiters`, `boundKeySegment`, and `utils.UnsafeString`
semantics.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 675f2d12-a573-4fc9-807d-5974f25cddb5
📒 Files selected for processing (6)
docs/middleware/cache.mddocs/middleware/csrf.mdmiddleware/cache/cache.gomiddleware/cache/cache_test.gomiddleware/csrf/csrf.gomiddleware/csrf/csrf_test.go
✅ Files skipped from review due to trivial changes (2)
- docs/middleware/csrf.md
- docs/middleware/cache.md
ReneWerner87
force-pushed
the
feat/http-query-method-followup
branch
from
June 25, 2026 10:21
948553f to
faee8dd
Compare
3 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.
Description
Follow-up to #4436 (HTTP QUERY method, RFC 10008), filling gaps found in review and aligning the QUERY cache key with the keygen refactor from #4450 (already on main).
Changes
Client.Query()and package-levelQuery()so the QUERY verb has the same shorthands as every other method. 🔥 feat: Add HTTP QUERY method support (RFC 10008) #4436 only addedRequest.Query(), leaving theClientAPI inconsistent.appendBoundKeySegment, consistent with the per-dimension escaping the keygen already applies to path/query/headers/cookies. Before this the body was appended raw and in the old return-string style, which left a stylistic inconsistency and a latent key-injection asymmetry (not exploitable today, since the body is the terminal key dimension). Pinned with new QUERY golden cases inkeygen_test.go, including a delimiter case (a|b:c->b=a\pb\cc).IsMethodSafe/IsMethodIdempotentassertions for QUERY (none existed), cover QUERY in the CSRF safe-method tests, and addTest_Client_Query.AllowMethodsdefault, theRegisterinterface, the safe-method lists (idempotency, earlydata, csrf), the cache body-key note, and the client REST reference.Type of change
Checklist
/docs/.go test ./..., all packages green).escapeKeyDelimitershas a no-alloc fast path for bodies without delimiters).🤖 Generated with Claude Code