Feature: Rate Limiting Filter - #83
Merged
Merged
Conversation
Co-authored-by: Mattias Hagström <mattiashagstrommusic@gmail.com>
Co-authored-by: Mattias Hagström <mattiashagstrommusic@gmail.com>
…gFilter using Bucket4j and add response handling for rate limit exceeded
…dation, and server cleanup. Add to App pipeline and configure properties. Co-authored-by: Mattias Hagström <mattiashagstrommusic@gmail.com>
…ng, rate limit enforcement, and cleanup behavior Co-authored-by: Mattias Hagström <mattiashagstrommusic@gmail.com>
…mprove test method naming, and add validation test Co-authored-by: Mattias Hagström <mattiashagstrommusic@gmail.com>
…and documentation Co-authored-by: Mattias Hagström <mattiashagstrommusic@gmail.com>
Closed
|
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:
📝 WalkthroughWalkthroughAdds a Bucket4J-based per-IP rate limiting filter, registers it in the global pipeline, introduces configuration and tests, extends ConfigLoader with rate-limiting fields/accessors, and adds the bucket4j Maven dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Filter as RateLimitingFilter
participant BucketStore as BucketStore
participant Chain as FilterChain
participant Server as Server
Client->>Filter: HTTP request
Filter->>Filter: extract client IP
Filter->>BucketStore: get/create per-IP bucket
Filter->>BucketStore: tryConsume(1)
alt token available
BucketStore-->>Filter: success
Filter->>Chain: doFilter(request,response)
Chain->>Server: handle request
Server-->>Chain: response
Chain-->>Filter: return
Filter-->>Client: response
else token not available
BucketStore-->>Filter: failure
Filter->>Filter: log warning
Filter-->>Client: 429 Too Many Requests (Retry-After)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
simonforsberg
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/org/juv25d/App.java (2)
36-37: Three independentStaticFilesPlugininstances are created for what appears to be the same plugin.Lines 32, 36, and 37 each call
new StaticFilesPlugin(). If the plugin is stateless (as it typically would be), a single shared instance avoids the overhead of multiple objects and makes the intent clearer.♻️ Suggested refactor
+StaticFilesPlugin staticFilesPlugin = new StaticFilesPlugin(); -pipeline.setPlugin(new StaticFilesPlugin()); // fix the method name first +pipeline.setPlugin(staticFilesPlugin); ... -router.registerPlugin("/", new StaticFilesPlugin()); -router.registerPlugin("/*", new StaticFilesPlugin()); +router.registerPlugin("/", staticFilesPlugin); +router.registerPlugin("/*", staticFilesPlugin);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/App.java` around lines 36 - 37, Multiple new StaticFilesPlugin() instances are created for the same plugin; instead instantiate a single StaticFilesPlugin and reuse it when calling router.registerPlugin for all routes. Locate the three registerPlugin calls that pass new StaticFilesPlugin() (the calls to router.registerPlugin for the root and wildcard paths), create one shared variable (e.g., staticFilesPlugin) assigned to new StaticFilesPlugin(), and replace each new StaticFilesPlugin() argument with that variable so the same instance is registered everywhere.
31-31: UnboundedConcurrentHashMapinRateLimitingFilterwill leak memory over time.As seen in
RateLimitingFilter.java(lines 24–25), a newBucketis created and retained for every unique client IP, and is never evicted — onlydestroy()clears the map. On a long-running public-facing server this accumulates one entry per unique IP indefinitely and can exhaust heap memory.Consider switching to a bounded cache (e.g., Caffeine or Guava
CacheBuilder) with a TTL/expiry, or periodically pruning stale entries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/App.java` at line 31, The RateLimitingFilter currently stores per-IP Bucket instances in an unbounded ConcurrentHashMap causing memory leaks; change its storage to a bounded expiring cache (e.g., Caffeine or Guava Cache) with a maximumSize and expireAfterAccess/expireAfterWrite policy, update constructor/newBucket lookup to use Cache.get(key, ...) or Cache.getIfPresent/computeIfAbsent equivalent, and modify destroy() to call cache.invalidateAll()/cache.cleanUp() instead of clearing the map; ensure all references to the old ConcurrentHashMap (the map that holds Bucket) are replaced and tests still create/remove buckets via the cache API so stale IPs are evicted automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/juv25d/App.java`:
- Line 32: Remove the erroneous pipeline.setPlugin(new StaticFilesPlugin())
call: Pipeline has no setPlugin method so delete that line and instead register
the StaticFilesPlugin with the SimpleRouter instance used in this file (where
other plugins are being configured), then keep setting the configured router on
the pipeline (pipeline.setRouter(router) / similar) so the pipeline uses the
router with the plugin registered.
- Line 31: Replace the hardcoded rate-limiter magic numbers in the App startup
with values from the configuration: read the limit and burst (or equivalent
names) via ConfigLoader (or its accessor method) and pass those variables into
the RateLimitingFilter constructor instead of (60, 10); update the call site
pipeline.addGlobalFilter(new RateLimitingFilter(...), 0) to use
ConfigLoader.getXxx() (or the existing config object) so operators can tune
limits via application-properties.yml.
---
Nitpick comments:
In `@src/main/java/org/juv25d/App.java`:
- Around line 36-37: Multiple new StaticFilesPlugin() instances are created for
the same plugin; instead instantiate a single StaticFilesPlugin and reuse it
when calling router.registerPlugin for all routes. Locate the three
registerPlugin calls that pass new StaticFilesPlugin() (the calls to
router.registerPlugin for the root and wildcard paths), create one shared
variable (e.g., staticFilesPlugin) assigned to new StaticFilesPlugin(), and
replace each new StaticFilesPlugin() argument with that variable so the same
instance is registered everywhere.
- Line 31: The RateLimitingFilter currently stores per-IP Bucket instances in an
unbounded ConcurrentHashMap causing memory leaks; change its storage to a
bounded expiring cache (e.g., Caffeine or Guava Cache) with a maximumSize and
expireAfterAccess/expireAfterWrite policy, update constructor/newBucket lookup
to use Cache.get(key, ...) or Cache.getIfPresent/computeIfAbsent equivalent, and
modify destroy() to call cache.invalidateAll()/cache.cleanUp() instead of
clearing the map; ensure all references to the old ConcurrentHashMap (the map
that holds Bucket) are replaced and tests still create/remove buckets via the
cache API so stale IPs are evicted automatically.
Co-authored-by: Mattias Hagström <mattiashagstrommusic@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/juv25d/App.java (1)
26-31: Execution order for equal-priority filters is deterministic, not undefined.
FilterRegistration.compareTo()compares only theorderfield usingInteger.compare(). When filters share priority0, the method returns0, and Java's stable sort (used byStream.sorted()) preserves their insertion order. The three filters will reliably execute in the order they were added:IpFilter,LoggingFilter, thenRateLimitingFilter.That said, if you prefer rate limiting to run before logging to avoid unnecessary token-bucket consumption and incorrect
429responses for already-blocked IPs, assigning distinct priorities (e.g.,IpFilter: 1,RateLimitingFilter: 2,LoggingFilter: 3) is a reasonable architectural choice—but it's not required for ordering reliability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/App.java` around lines 26 - 31, The comment points out that equal orders are deterministic due to FilterRegistration.compareTo() using Integer.compare(), so the current insertion order (pipeline.addGlobalFilter with IpFilter, LoggingFilter, RateLimitingFilter) will run in that sequence; if you want RateLimitingFilter to run before LoggingFilter (to avoid logging or token use for blocked IPs), assign distinct priority values when registering the filters (e.g., give IpFilter=1, RateLimitingFilter=2, LoggingFilter=3) or otherwise change the integer order passed to pipeline.addGlobalFilter; locate FilterRegistration.compareTo, and the three registrations (IpFilter, LoggingFilter, RateLimitingFilter) in App.java to update the order integers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/juv25d/App.java`:
- Line 31: The hardcoded RateLimitingFilter parameters in App.java
(pipeline.addGlobalFilter(new RateLimitingFilter(60, 10), 0)) should be replaced
with values read from your configuration loader: call the ConfigLoader (e.g.,
ConfigLoader.getRateLimitingRequestsPerMinute() and
ConfigLoader.getRateLimitingBurstCapacity() or equivalent methods) to obtain
requests-per-minute and burst-capacity, validate/convert them to the expected
types, and pass those values into new RateLimitingFilter; keep sensible defaults
if config values are missing and update any unit tests or usages that assumed
the hardcoded values.
---
Nitpick comments:
In `@src/main/java/org/juv25d/App.java`:
- Around line 26-31: The comment points out that equal orders are deterministic
due to FilterRegistration.compareTo() using Integer.compare(), so the current
insertion order (pipeline.addGlobalFilter with IpFilter, LoggingFilter,
RateLimitingFilter) will run in that sequence; if you want RateLimitingFilter to
run before LoggingFilter (to avoid logging or token use for blocked IPs), assign
distinct priority values when registering the filters (e.g., give IpFilter=1,
RateLimitingFilter=2, LoggingFilter=3) or otherwise change the integer order
passed to pipeline.addGlobalFilter; locate FilterRegistration.compareTo, and the
three registrations (IpFilter, LoggingFilter, RateLimitingFilter) in App.java to
update the order integers.
…ndling in tests Co-authored-by: Mattias Hagström <mattiashagstrommusic@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/java/org/juv25d/filter/RateLimitingFilterTest.java (2)
120-129: Consider also asserting that negative constructor arguments are rejected.Only
0is tested for each parameter; negative values (-1, etc.) are equally invalid for a capacity/rate but are currently untested. If the validation guard is> 0, negatives are already covered — but the tests don't confirm this.✅ Suggested additional assertions
assertThatThrownBy(() -> new RateLimitingFilter(60, 0)) .isInstanceOf(IllegalArgumentException.class); + + assertThatThrownBy(() -> new RateLimitingFilter(-1, 5)) + .isInstanceOf(IllegalArgumentException.class); + + assertThatThrownBy(() -> new RateLimitingFilter(60, -1)) + .isInstanceOf(IllegalArgumentException.class);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/filter/RateLimitingFilterTest.java` around lines 120 - 129, Add assertions to RateLimitingFilterTest that verify negative constructor arguments are rejected: call new RateLimitingFilter(-1, 5) and new RateLimitingFilter(60, -1) (and optionally both negative) inside assertThatThrownBy and assert IllegalArgumentException. This ensures the constructor validation in RateLimitingFilter for both capacity/rate parameters (the RateLimitingFilter(...) constructor) rejects values less than zero as well as zero.
85-85: Nit: misleading comment — "Empty first bucket" should be "Exhaust first bucket"."Empty" implies clearing the IP map; the intent is to consume all tokens in the bucket for IP1.
📝 Suggested fix
- for (int i = 0; i < 6; i++) { // Empty first bucket + for (int i = 0; i < 6; i++) { // Exhaust first bucket🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/filter/RateLimitingFilterTest.java` at line 85, Update the misleading inline comment in the test loop inside RateLimitingFilterTest (the for-loop at the top of the test method that iterates 6 times to consume tokens for IP1) from "Empty first bucket" to "Exhaust first bucket" so it accurately reflects that the loop is consuming all tokens rather than clearing the IP map; locate the loop in the RateLimitingFilterTest class and replace the comment text only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 65-67: The getLong(String) method is incorrect and should be
removed; instead extend ConfigLoader.loadConfiguration() to parse the
"rate-limiting" section and populate new instance fields (e.g., long
requestsPerMinute, long burstCapacity), then add typed accessors
getRequestsPerMinute() and getBurstCapacity() used by App.java (which currently
calls getLong with keys); ensure loadConfiguration reads
"rate-limiting.requests-per-minute" and "rate-limiting.burst-capacity" from the
source config, converts to long safely, assigns to the new fields, and replace
any calls to getLong(...) with the new getters.
In `@src/test/java/org/juv25d/filter/RateLimitingFilterTest.java`:
- Around line 64-69: The Retry-After header is currently hardcoded to "60" in
RateLimitingFilter (around the token-exhaustion logic) but should use the bucket
probe to compute the actual refill wait; replace the hardcoded value with the
bucket4j computation probe.estimateAbilityToConsume(1).getRoundedSecondsToWait()
when setting the "Retry-After" response header in RateLimitingFilter, and update
the test in RateLimitingFilterTest to assert the header equals that computed
value (or compute the same probe value in the test before verifying) instead of
expecting "60".
---
Nitpick comments:
In `@src/test/java/org/juv25d/filter/RateLimitingFilterTest.java`:
- Around line 120-129: Add assertions to RateLimitingFilterTest that verify
negative constructor arguments are rejected: call new RateLimitingFilter(-1, 5)
and new RateLimitingFilter(60, -1) (and optionally both negative) inside
assertThatThrownBy and assert IllegalArgumentException. This ensures the
constructor validation in RateLimitingFilter for both capacity/rate parameters
(the RateLimitingFilter(...) constructor) rejects values less than zero as well
as zero.
- Line 85: Update the misleading inline comment in the test loop inside
RateLimitingFilterTest (the for-loop at the top of the test method that iterates
6 times to consume tokens for IP1) from "Empty first bucket" to "Exhaust first
bucket" so it accurately reflects that the loop is consuming all tokens rather
than clearing the IP map; locate the loop in the RateLimitingFilterTest class
and replace the comment text only.
…ne to use dynamic values Co-authored-by: Mattias Hagström <mattiashagstrommusic@gmail.com>
Co-authored-by: Mattias Hagström <mattiashagstrommusic@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pom.xml (1)
47-47: Redundant<scope>compile</scope>
compileis Maven's default scope; the explicit declaration adds noise without effect.♻️ Proposed cleanup
<dependency> <groupId>com.bucket4j</groupId> <artifactId>bucket4j_jdk17-core</artifactId> <version>8.16.1</version> - <scope>compile</scope> </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` at line 47, Remove the redundant <scope>compile</scope> elements from dependency declarations in the POM (they're the default and add noise); locate occurrences of the <scope>compile</scope> tag in the pom.xml (within <dependency> blocks) and delete those lines so dependencies rely on the implicit compile scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 13-14: The fields requestsPerMinute and burstCapacity in class
ConfigLoader are left at Java default 0L when the rate-limiting YAML section is
absent, causing RateLimitingFilter to throw; initialize these fields to the same
default constants/values used in the getOrDefault calls (the defaults currently
expected for rate limiting, e.g. 300 and 75) so that requestsPerMinute and
burstCapacity are non-zero even if the rateLimitingConfig block is skipped;
update the field declarations for requestsPerMinute and burstCapacity in
ConfigLoader (and any related default handling around where getOrDefault is
used) to use those default values.
- Around line 51-55: Add support for the rate-limiting "enabled" flag: update
ConfigLoader to read the "enabled" boolean from the rate-limiting map (e.g., set
a new private boolean field like rateLimitingEnabled when parsing the
"rate-limiting" block where requestsPerMinute and burstCapacity are read) and
expose it via a public isRateLimitingEnabled() getter; then change App (where
the filter is registered) to guard the RateLimitingFilter registration with
config.isRateLimitingEnabled() so the filter is only added when the flag is
true.
---
Nitpick comments:
In `@pom.xml`:
- Line 47: Remove the redundant <scope>compile</scope> elements from dependency
declarations in the POM (they're the default and add noise); locate occurrences
of the <scope>compile</scope> tag in the pom.xml (within <dependency> blocks)
and delete those lines so dependencies rely on the implicit compile scope.
johanbriger
previously approved these changes
Feb 18, 2026
johanbriger
left a comment
johanbriger
left a comment
There was a problem hiding this comment.
Nice work!
The Token Bucket implementation via Bucket4j is clean and robust solution.
… and ConfigLoader Co-authored-by: Mattias Hagström <mattiashagstrommusic@gmail.com>
johanbriger
approved these changes
Feb 18, 2026
TatjanaTrajkovic
self-requested a review
February 18, 2026 09:49
TatjanaTrajkovic
approved these changes
Feb 18, 2026
This was referenced Feb 18, 2026
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
4 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.
Summary
Introduced
RateLimitingFilterto protect the web server from request bursts and potential DoS attacks by limiting the number of requests per client IP address.Changes
RateLimitingFilter: A new filter implementation using the Token Bucket algorithm (viaBucket4J) to enforce rate limits.ConcurrentHashMapto maintain independent buckets for each unique client IP.429 Too Many Requestsresponse with aRetry-Afterheader when limits are exceeded.RateLimitingFilterTest: Comprehensive unit tests covering:Technical Details
io.github.bucket4j:bucket4j-corefor robust and thread-safe rate limiting logic.Retry-Afterheader to inform clients when they can attempt to reconnect.ServerLoggingto provide visibility into rate-limited events for security monitoring.Verification Results
RateLimitingFilterTestpassed.Summary by CodeRabbit
New Features
Configuration
Tests
Chores