🐛 bug: Fix default value for MaxFunc in Limiter middleware#3871
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 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. WalkthroughThe pull request extracts a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5–10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
Summary of ChangesHello @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 enhances the robustness of the Highlights
Using Gemini Code AssistThe 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
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 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
|
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a panic that occurs when limiter.New() is called without arguments. It achieves this by providing a default MaxFunc in ConfigDefault. The introduction of the defaultLimiterMax constant is a good improvement for maintainability. The added regression test TestLimiterDefaultConfigNoPanic effectively prevents this issue from recurring.
While reviewing the code, I noticed a potential pre-existing bug in the sliding window implementation (middleware/limiter/limiter_sliding.go). It appears to use cfg.Max for rate limit calculations instead of the value returned by cfg.MaxFunc. This can lead to incorrect behavior if a user provides a MaxFunc that returns a value different from cfg.Max. Since this is outside the scope of the current changes, I'm mentioning it here for awareness. It might be worth creating a separate issue to track and fix this.
Overall, the changes in this PR are correct and improve the robustness of the limiter middleware.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3871 +/- ##
==========================================
- Coverage 91.69% 91.64% -0.06%
==========================================
Files 115 115
Lines 9819 9848 +29
==========================================
+ Hits 9004 9025 +21
- Misses 516 523 +7
- Partials 299 300 +1
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.
Pull Request Overview
This PR fixes a nil pointer panic that occurred when calling limiter.New() without any configuration arguments. The fix sets a default MaxFunc in ConfigDefault that returns the default max value of 5.
- Extracted the magic number 5 into a named constant
defaultLimiterMax - Added a default
MaxFunctoConfigDefaultthat returnsdefaultLimiterMax - Added a regression test to ensure the middleware works without panicking when called with no configuration
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/limiter/config.go | Introduces defaultLimiterMax constant and sets a default MaxFunc in ConfigDefault to prevent nil pointer panics |
| middleware/limiter/limiter_test.go | Adds regression test TestLimiterDefaultConfigNoPanic to verify the middleware can be instantiated and used without any configuration |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // ConfigDefault is the default config | ||
| var ConfigDefault = Config{ | ||
| Max: 5, | ||
| Max: defaultLimiterMax, | ||
| Expiration: 1 * time.Minute, | ||
| MaxFunc: func(_ fiber.Ctx) int { | ||
| return defaultLimiterMax | ||
| }, |
There was a problem hiding this comment.
Retaining MaxFunc defaults ignores overrides
Setting ConfigDefault.MaxFunc to a concrete closure now means copies of ConfigDefault carry a non‑nil MaxFunc that always returns 5. A common pattern is cfg := limiter.ConfigDefault; cfg.Max = 100; app.Use(limiter.New(cfg)). After this change, configDefault will not replace the copied MaxFunc, so the middleware still enforces a limit of 5 despite cfg.Max being updated. Prior to this commit MaxFunc was nil, so the helper generated a closure reflecting the overridden Max. This regression silently discards user overrides of Max unless they also manually set MaxFunc.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
If a user supplies a config, it's used in line 107: cfg := config[0]
|
@gaby Are the changes incomplete?. In |
|
@ilxqx I have another request coming, GitHub auto-closed the issue |
Summary
Fixes #3867