Prevent Override Max value by default value when passing 0 to Max
#3288
Conversation
ad3n
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe pull request modifies the configuration validation logic in the rate limiter middleware. Specifically, the change adjusts the condition for setting a default maximum request limit from checking if the value is less than or equal to zero to only checking if it is less than zero. This subtle modification means that zero will now be treated as a valid configuration value, potentially allowing more flexibility in rate limiting configuration. Changes
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3288 +/- ##
==========================================
- Coverage 84.04% 84.01% -0.03%
==========================================
Files 116 116
Lines 11557 11557
==========================================
- Hits 9713 9710 -3
- Misses 1411 1413 +2
- Partials 433 434 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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 (1)
middleware/limiter/config.go (1)
96-97: Consider documenting this behavior change.This change modifies how
Max=0is handled, which could affect existing users. Consider:
- Updating the field documentation to explicitly mention that
Max=0blocks all requests- Adding a note in the changelog about this behavior change
- Providing migration guidance for users who might be relying on the old behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/limiter/config.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (1)
middleware/limiter/config.go (1)
96-97: LGTM! The change aligns with the PR objective.The modification allows
Max=0to be a valid configuration value instead of being overridden by the default value. This is a logical change as it enables the rate limiter to completely block requests when desired.Let's verify that the limiter middleware properly handles
Max=0:✅ Verification successful
Verified: Max=0 is properly handled in the limiter middleware
The code safely handles Max=0 by skipping rate limiting and allowing requests to pass through. This is:
- Explicitly implemented in both fixed and sliding window limiters
- Well-tested with dedicated test cases
- Consistent with the PR objective
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how Max=0 is handled in the limiter middleware implementation # Search for usage of Max or MaxFunc in the limiter implementation ast-grep --pattern 'MaxFunc($_)' || ast-grep --pattern 'Max($_)' # Look for any existing tests that might need updating rg -l "Max.*0" test_*Length of output: 107
Script:
#!/bin/bash # Find and analyze limiter-related files echo "=== Limiter Files ===" fd -t f limiter echo -e "\n=== Max Usage in Implementation ===" rg -n "Max" --type go -A 3 -B 3 echo -e "\n=== Test Cases ===" fd -t f test -e go | xargs rg -l "Max.*[0-9]"Length of output: 62890
|
@ad3n What's the point of setting this to 0? |
|
You can use https://github.com/gofiber/fiber/blob/main/middleware/limiter/config.go#L30 |
The behavior when we set |
Why use the limiter then? It adds quite a bit of overhead. |
The limiter is mandatory on my case, so i just make it simple to pass all to the limiter and then the limiter control the flow. As simple as that |
If you set it to 0, the limiter is skipped and https://github.com/gofiber/fiber/blob/main/middleware/limiter/limiter_fixed.go#L33 |
Yup, i think the limiter will skipped, but on config, when i supply So i create this PR to make sure, the default config not override it when i supply Ref: https://github.com/gofiber/fiber/blob/main/middleware/limiter/config.go#L96 |
ad3n
changed the title
Max value by default value when passing 0 to Max
ad3n
changed the title
Max value by default value when passing 0 to MaxMax value by default value when passing 0 to Max
|
the default values of an empty structs always correspond to the default value of the datenTypes in your pull request you set the condition for the max value to below 0, which makes these default value useless when it comes to skipping the middleware functionality, please use other solutions like using the |
Description
This PR makes this code https://github.com/gofiber/fiber/blob/main/middleware/limiter/limiter_fixed.go#L33 reachable, because when we supply the Max to
0, it always override by default value