🧹 chore: Handle Transfer-Encoding bodies in HasBody#3748
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughExpanded request body detection: HasBody now recognizes Content-Length > 0, Content-Length == -1, Transfer-Encoding with non-identity tokens, or already-buffered payload data; tests and interface comment updated. Several generated msgp files and tests had header/formatting or minor decoding refactors; some map-clear and return-expression tweaks applied. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Ctx as Ctx
participant H as Headers
Caller->>Ctx: HasBody()
alt ContentLength > 0
Ctx-->>Caller: true
else ContentLength == -1
Ctx-->>Caller: true
else ContentLength == 0
Ctx->>H: read Transfer-Encoding header(s)
alt TE contains non-"identity" token (e.g., chunked)
Ctx-->>Caller: true
else
Ctx->>Ctx: check buffered raw body length
alt raw body length > 0
Ctx-->>Caller: true
else
Ctx-->>Caller: false
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Summary of Changes
Hello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refines the HasBody method within the application's context to accurately determine if an incoming request contains a body. Previously, requests using Transfer-Encoding (like chunked encoding) might have been misidentified as having no body if their Content-Length was zero. The updated logic now correctly parses the Transfer-Encoding header and accounts for fasthttp's internal representation of chunked bodies, ensuring robust and accurate request body detection.
Highlights
- Improved HasBody Logic: The
HasBodyfunction inctx.gohas been updated to correctly identify requests that contain a body, even when theContent-Lengthheader is zero, by inspecting theTransfer-Encodingheader. - Handling Transfer-Encoding: chunked: The
HasBodymethod now explicitly checks forContent-Lengthbeing -1, whichfasthttpuses to indicateTransfer-Encoding: chunkedbodies, ensuring these requests are correctly recognized as having a body. - New Test Cases: New test scenarios have been added to
ctx_test.goto validate the updatedHasBodylogic, specifically covering cases withTransfer-Encoding: chunkedandContent-Length: 0.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR improves the HasBody method to properly handle Transfer-Encoding requests, particularly chunked encoding, even when Content-Length is zero.
- Replace simple Content-Length check with comprehensive logic handling chunked transfers
- Add Transfer-Encoding header parsing to detect non-identity encodings
- Add comprehensive test coverage for chunked and Transfer-Encoding scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ctx.go | Enhanced HasBody method with Transfer-Encoding support and chunked transfer detection |
| ctx_test.go | Added test cases for chunked transfers and Transfer-Encoding header scenarios |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3748 +/- ##
==========================================
- Coverage 92.03% 91.98% -0.06%
==========================================
Files 115 114 -1
Lines 11632 11753 +121
==========================================
+ Hits 10706 10811 +105
- Misses 671 682 +11
- Partials 255 260 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request correctly extends HasBody to handle Transfer-Encoding headers, which is an important improvement for correctly identifying requests with bodies, especially with chunked encoding. The changes are logical and the new test cases cover the main scenario. I've added a couple of suggestions: one to improve the correctness of whitespace handling when parsing the header, and another to expand test coverage with more edge cases for the Transfer-Encoding header to ensure robustness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ctx.go (1)
340-340: Docstring now outdated — reflect new semantics.HasBody no longer only checks “Content-Length > 0”; it also returns true for chunked/any non-identity Transfer-Encoding and still falls back to raw body bytes. Update the comment for accuracy.
Apply this diff:
-// HasBody returns true if the request has a body or a Content-Length header greater than zero. +// HasBody returns true if the request has a body, which is indicated by any of: +// - Content-Length > 0 +// - ContentLength() == -1 (e.g. Transfer-Encoding: chunked) +// - Transfer-Encoding contains any non-identity coding (even if Content-Length is 0) +// - or non-empty raw body bytes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx.go(1 hunks)ctx_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
ctx_test.goctx.go
🧬 Code graph analysis (2)
ctx_test.go (2)
client/request.go (2)
Request(46-71)Header(675-677)constants.go (2)
HeaderTransferEncoding(258-258)HeaderContentLength(214-214)
ctx.go (1)
constants.go (1)
HeaderTransferEncoding(258-258)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, macos-13)
🔇 Additional comments (3)
ctx.go (1)
342-362: LGTM: Transfer-Encoding and chunked handling is correct and robust.
- Correctly treats ContentLength() == -1 (chunked) as having a body.
- When ContentLength() == 0, scanning Transfer-Encoding tokens and returning true for any non-identity coding covers conflicted CL/TE headers.
ctx_test.go (2)
5128-5148: Nice coverage for chunked and TE+CL conflict paths.Both added scenarios exercise the new HasBody behavior without requiring an actual payload. Good use of Disable/EnableSpecialHeader to simulate conflicted headers.
5128-5148: Add negative and case-insensitivity tests for Transfer-Encoding.To lock in edge cases, add:
- identity only → HasBody() false
- mixed-case/whitespace “Chunked, identity” → HasBody() true
[ suggest_nitpick_refactor ]
Apply this diff within Test_Ctx_HasBody (after the existing TE test):+ // Transfer-Encoding: identity with CL=0 -> no body + ctxWithTEIdentity := app.AcquireCtx(&fasthttp.RequestCtx{}) + require.NotNil(t, ctxWithTEIdentity) + t.Cleanup(func() { app.ReleaseCtx(ctxWithTEIdentity) }) + ctxWithTEIdentity.Request().Header.DisableSpecialHeader() + ctxWithTEIdentity.Request().Header.Set(HeaderTransferEncoding, "identity") + ctxWithTEIdentity.Request().Header.Set(HeaderContentLength, "0") + ctxWithTEIdentity.Request().Header.EnableSpecialHeader() + require.Zero(t, ctxWithTEIdentity.Request().Header.ContentLength()) + require.False(t, ctxWithTEIdentity.HasBody()) + + // Mixed case + whitespace, includes a non-identity coding -> body present + ctxWithTEChunkedCase := app.AcquireCtx(&fasthttp.RequestCtx{}) + require.NotNil(t, ctxWithTEChunkedCase) + t.Cleanup(func() { app.ReleaseCtx(ctxWithTEChunkedCase) }) + ctxWithTEChunkedCase.Request().Header.DisableSpecialHeader() + ctxWithTEChunkedCase.Request().Header.Set(HeaderTransferEncoding, " Chunked , identity ") + ctxWithTEChunkedCase.Request().Header.Set(HeaderContentLength, "0") + ctxWithTEChunkedCase.Request().Header.EnableSpecialHeader() + require.Zero(t, ctxWithTEChunkedCase.Request().Header.ContentLength()) + require.True(t, ctxWithTEChunkedCase.HasBody())
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ctx_test.go (1)
5134-5193: Great coverage for Transfer-Encoding/body-detection edge cases.The new subtests (chunked sentinel, case/whitespace, params, multiple encodings, identity variants, and no-body) exercise the TE parsing well and align with the PR goal. Looks good.
🧹 Nitpick comments (1)
ctx_test.go (1)
5116-5121: Add t.Helper() to test helper to satisfy thelper lint.golangci-lint flags this helper; add t.Helper() as the first statement.
- acquire := func(t *testing.T) CustomCtx { - ctx := app.AcquireCtx(&fasthttp.RequestCtx{}) + acquire := func(t *testing.T) CustomCtx { + t.Helper() + ctx := app.AcquireCtx(&fasthttp.RequestCtx{})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
ctx.go(1 hunks)ctx_interface_gen.go(1 hunks)ctx_test.go(1 hunks)middleware/cache/manager_msgp.go(3 hunks)middleware/cache/manager_msgp_test.go(1 hunks)middleware/csrf/storage_manager_msgp.go(1 hunks)middleware/csrf/storage_manager_msgp_test.go(1 hunks)middleware/idempotency/response_msgp.go(3 hunks)middleware/idempotency/response_msgp_test.go(1 hunks)middleware/limiter/manager_msgp.go(1 hunks)middleware/session/data_msgp.go(1 hunks)middleware/session/data_msgp_test.go(1 hunks)redirect_msgp.go(10 hunks)
✅ Files skipped from review due to trivial changes (6)
- middleware/cache/manager_msgp_test.go
- ctx_interface_gen.go
- middleware/csrf/storage_manager_msgp.go
- middleware/limiter/manager_msgp.go
- middleware/csrf/storage_manager_msgp_test.go
- middleware/session/data_msgp.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
ctx.goredirect_msgp.gomiddleware/cache/manager_msgp.gomiddleware/idempotency/response_msgp.gomiddleware/idempotency/response_msgp_test.gomiddleware/session/data_msgp_test.goctx_test.go
🧠 Learnings (2)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
middleware/session/data_msgp_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/session.go:46-61
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In this codebase, the `sessionPool` only contains `Session` instances, so type assertions without additional checks are acceptable.
Applied to files:
middleware/session/data_msgp_test.go
🧬 Code graph analysis (2)
ctx.go (1)
constants.go (1)
HeaderTransferEncoding(258-258)
ctx_test.go (4)
ctx_interface.go (1)
CustomCtx(13-31)ctx_interface_gen.go (1)
Ctx(18-430)client/request.go (2)
Request(46-71)Header(675-677)constants.go (2)
HeaderTransferEncoding(258-258)HeaderContentLength(214-214)
🪛 GitHub Check: lint
ctx.go
[failure] 366-366:
SA1019: hdr.VisitAll is deprecated: Use All instead. (staticcheck)
ctx_test.go
[failure] 5116-5116:
test helper function should start from t.Helper() (thelper)
🪛 GitHub Actions: golangci-lint
ctx.go
[error] 366-366: golangci-lint: Step 'golangci-lint run' failed. SA1019: hdr.VisitAll is deprecated: Use All instead. (staticcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
🔇 Additional comments (9)
middleware/session/data_msgp_test.go (1)
3-4: Header/package order LGTM.Matches our generated-code convention; no functional impact.
middleware/idempotency/response_msgp_test.go (1)
3-4: Header/package order LGTM.Consistent with other msgp-generated tests; no behavior change.
redirect_msgp.go (1)
9-106: Explicit error returns improve clarity; confirm generator alignment.The switch from bare
returntoreturn err/return o, erris fine and clearer. Since this file is marked DO NOT EDIT, please ensure the msgp generator (or post‑processor) is updated accordingly so these changes don’t get overwritten on the next regenerate.Also applies to: 108-185, 187-273
middleware/idempotency/response_msgp.go (2)
3-89: Use of clear(map) is correct and minimizes churn.Replacing per‑key deletes with
clear(z.Headers)is idiomatic and faster; capacity is retained for reuse. Error context with keys also looks good.
169-267: Unmarshal mirror of the map‑clear pattern LGTM.Header key/value read order and error wrapping are sound. No API changes.
middleware/cache/manager_msgp.go (2)
3-55: Map clear + reordered header reads LGTM.
clear(z.headers)and reading the key before the value reduce allocations and improve readability.
260-378: Unmarshal side mirrors the Decode changes correctly.Capacity reuse + clear is good; error wrapping includes the offending key. No concerns.
ctx.go (2)
340-357: HasBody logic improvement LGTM.Ordering (Content-Length, then TE, then buffered payload) is sensible;
cl == -1handling for chunked bodies matches fasthttp semantics.
359-399: Replace deprecated hdr.VisitAll with PeekAll (apply suggested diff)rg found a single VisitAll at ctx.go:366. Tests in ctx_test.go (≈5155–5189) cover Transfer-Encoding tokenization, so the PeekAll-based change preserves behavior. Apply the diff, confirm fasthttp.RequestHeader.PeekAll exists for your fasthttp version, then run golangci-lint and go test.
|
linting again^^ |
Summary
Transfer-Encodingrequests as having bodies whenContent-Lengthis zero by scanning the header tokens insideHasBodyTest_Ctx_HasBodywith aDisableSpecialHeaderscenario so chunked transfer-encoded requests are exercised alongside the fixed-length checks