🐛 bug: fix client iterators when using break statement #3357
Conversation
efectn
requested review from
gaby,
sixcolors and
ReneWerner87
and removed request for
a team
WalkthroughThe changes modify the iteration methods in the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3357 +/- ##
==========================================
+ Coverage 83.61% 83.73% +0.12%
==========================================
Files 118 118
Lines 11727 11728 +1
==========================================
+ Hits 9806 9821 +15
+ Misses 1491 1481 -10
+ Partials 430 426 -4
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/request.go (3)
300-307: Fix for iterator panic when using break.The previous implementation using
VisitAllwould continue iterating even after theyieldfunction returnedfalse, causing a panic. This directforloop approach properly respects the return value ofyieldand stops iteration whenfalseis returned.While the fix works correctly, consider adding a comment explaining why this change was made to prevent future regressions.
func (r *Request) Cookies() iter.Seq2[string, string] { return func(yield func(string, string) bool) { + // Direct iteration over map to respect yield's return value + // and properly handle break statements within consumer loops var res bool for k, v := range *r.cookies { res = yield(k, v) if !res { return } } } }
348-352: Fix for iterator panic when using break.Similar to the Cookies method, this implementation properly respects the
yieldfunction's return value and stops iteration whenfalseis returned, preventing panics when usingbreakwithin consumer loops.The same recommendation applies here - consider adding a comment to document the reason for this implementation.
func (r *Request) PathParams() iter.Seq2[string, string] { return func(yield func(string, string) bool) { + // Direct iteration over map to respect yield's return value + // and properly handle break statements within consumer loops for k, v := range *r.path { if !yield(k, v) { return } } } }
301-306: Consider using a consistent approach between methods.There's a slight inconsistency between the
Cookies()andPathParams()implementations:
Cookies()stores the result in a variable before checkingPathParams()directly checks the return value ofyield()While both approaches work correctly, using the same pattern in both methods would improve code consistency.
func (r *Request) Cookies() iter.Seq2[string, string] { return func(yield func(string, string) bool) { - var res bool for k, v := range *r.cookies { - res = yield(k, v) - if !res { + if !yield(k, v) { return } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/request.go(2 hunks)client/request_test.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
client/request_test.go
[failure] 456-456:
string bar has 6 occurrences, make it a constant (goconst)
🪛 GitHub Actions: golangci-lint
client/request_test.go
[warning] 456-456: string bar has 6 occurrences, make it a constant (goconst)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Analyse
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (2)
client/request_test.go (2)
454-460: Good addition of test case to prevent regression.This test case ensures that using
breakwithin the iteration loop forCookies()doesn't cause a panic, which addresses the core issue mentioned in the PR objectives.🧰 Tools
🪛 GitHub Check: lint
[failure] 456-456:
stringbarhas 6 occurrences, make it a constant (goconst)🪛 GitHub Actions: golangci-lint
[warning] 456-456: string
barhas 6 occurrences, make it a constant (goconst)
576-582: Good addition of test case to prevent regression.Similar to the Cookies test, this test ensures that using
breakwithin the iteration loop forPathParams()doesn't cause a panic, addressing the core issue mentioned in the PR objectives.
|
@efectn pls fix the review hint and lint errors |
Done |
Description
When iterators don't finishes iterations in case yield(k, v) returns false, it causes panic like
range function continued iteration after loop body. Some of client iterators are also affected from this issue. This PR fixes it and add testcases to ensure it doesn't panic.Fixes # (issue)
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md