applied order filter annotations to existing filters - #107
Merged
Conversation
📝 WalkthroughWalkthroughApplied Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/java/org/juv25d/App.java (1)
25-26: Reformat the conditionalRateLimitingFilterregistration.The current inline style is hard to read and inconsistent with the surrounding code.
♻️ Proposed refactor
- if (config.isRateLimitingEnabled()) {pipeline.addGlobalFilter(new RateLimitingFilter( - config.getRequestsPerMinute(), config.getBurstCapacity()), 1);} + if (config.isRateLimitingEnabled()) { + pipeline.addGlobalFilter( + new RateLimitingFilter(config.getRequestsPerMinute(), config.getBurstCapacity()), 1); + }🤖 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 25 - 26, The inline registration of RateLimitingFilter is hard to read; refactor the conditional that calls pipeline.addGlobalFilter(new RateLimitingFilter(config.getRequestsPerMinute(), config.getBurstCapacity()), 1) into a clear multi-line if block: call config.isRateLimitingEnabled() in the if condition, instantiate RateLimitingFilter on its own line using config.getRequestsPerMinute() and config.getBurstCapacity(), then pass that instance to pipeline.addGlobalFilter(...) on a separate line (preserving the priority arg 1); this improves readability and matches surrounding code style.src/main/java/org/juv25d/filter/LoggingFilter.java (1)
14-14: ReplaceSystem.out.printlnwith a structured logger.
LoggingFilteris the dedicated request-logging component yet it writes tostdoutinstead of using aLoggerinstance. This bypasses log-level control, log formatting, and centralised output routing.♻️ Proposed refactor
+import java.util.logging.Logger; + `@Global`(order = 3) public class LoggingFilter implements Filter { + private static final Logger logger = Logger.getLogger(LoggingFilter.class.getName()); `@Override` public void doFilter(HttpRequest req, HttpResponse res, FilterChain chain) throws IOException { - System.out.println(req.method() + " " + req.path()); + logger.info(req.method() + " " + req.path()); chain.doFilter(req, res); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/LoggingFilter.java` at line 14, Replace the System.out.println call in LoggingFilter with a structured logger: add a private static final Logger (e.g., LoggerFactory.getLogger(LoggingFilter.class)) to the LoggingFilter class and replace the println(req.method() + " " + req.path()) with a logger call (e.g., logger.info or logger.debug) that uses parameterized logging like logger.info("{} {}", req.method(), req.path()); also add the necessary import for the logger implementation (SLF4J/your project's logging facade) and remove the stdout call so request logging goes through the centralized logger.
🤖 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 23: The IpFilter is currently constructed with empty sets at
pipeline.addGlobalFilter(...), making IpFilter.doFilter() always allow requests;
either make this explicit or implement real filtering: if this is a placeholder,
add a clear comment next to pipeline.addGlobalFilter(new IpFilter(Set.of(),
Set.of()), 0) stating it’s intentionally permissive and will be replaced,
otherwise change the call to pass actual values (e.g., load whitelist/blacklist
from configuration or environment and pass those Sets into new IpFilter) so
IpFilter.doFilter() enforces the intended IP rules. Ensure references to the
IpFilter constructor and the doFilter method remain consistent when you wire in
config-loading logic.
- Around line 42-44: Remove the redundant router.registerPlugin("/", ...) call
and stop using the all-matching "/*" for StaticFilesPlugin because that wildcard
prevents SimpleRouter's notFoundPlugin from ever running; instead register
StaticFilesPlugin on a specific static prefix (e.g., "/static/*") or other
explicit paths and register NotFoundPlugin as the catch-all ("/*") so
SimpleRouter's fallback behavior is preserved—update the calls to
router.registerPlugin and leave NotFoundPlugin as the final wildcard handler.
In `@src/main/java/org/juv25d/filter/SecurityHeadersFilter.java`:
- Line 13: SecurityHeadersFilter is currently annotated as `@Global`(order = 4) so
it runs last and can be skipped by upstream short-circuiting filters, causing
security headers to be missing on error/redirect responses; change the `@Global`
annotation on the SecurityHeadersFilter class from order = 4 to order = 0 so it
runs outermost (wrapping all downstream filters) and guarantees the try/finally
always applies its headers to every response.
---
Duplicate comments:
In `@src/main/java/org/juv25d/App.java`:
- Line 38: SecurityHeadersFilter is currently added at position 4 so it never
runs for responses short-circuited earlier (IpFilter, RateLimitingFilter,
RedirectFilter); move it back to the outermost position so its try/finally can
always add headers. Update the pipeline wiring to register SecurityHeadersFilter
with order 0 (or before the other global filters) in App.java (the
pipeline.addGlobalFilter call) so SecurityHeadersFilter.wrap/try-finally logic
executes around all downstream filters and covers error/redirect responses.
Ensure no other filter is registered with order 0 to avoid ordering conflicts.
---
Nitpick comments:
In `@src/main/java/org/juv25d/App.java`:
- Around line 25-26: The inline registration of RateLimitingFilter is hard to
read; refactor the conditional that calls pipeline.addGlobalFilter(new
RateLimitingFilter(config.getRequestsPerMinute(), config.getBurstCapacity()), 1)
into a clear multi-line if block: call config.isRateLimitingEnabled() in the if
condition, instantiate RateLimitingFilter on its own line using
config.getRequestsPerMinute() and config.getBurstCapacity(), then pass that
instance to pipeline.addGlobalFilter(...) on a separate line (preserving the
priority arg 1); this improves readability and matches surrounding code style.
In `@src/main/java/org/juv25d/filter/LoggingFilter.java`:
- Line 14: Replace the System.out.println call in LoggingFilter with a
structured logger: add a private static final Logger (e.g.,
LoggerFactory.getLogger(LoggingFilter.class)) to the LoggingFilter class and
replace the println(req.method() + " " + req.path()) with a logger call (e.g.,
logger.info or logger.debug) that uses parameterized logging like
logger.info("{} {}", req.method(), req.path()); also add the necessary import
for the logger implementation (SLF4J/your project's logging facade) and remove
the stdout call so request logging goes through the centralized logger.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/juv25d/filter/RedirectFilter.java (1)
31-32: Nit: stray blank line between Javadoc and annotation.The blank line at line 31 between the closing
*/and@Global(order = 3)is inconsistent with how other filter classes (RateLimitingFilter, etc.) position their class-level annotation immediately after the Javadoc.✏️ Proposed fix
* </pre> */ - `@Global`(order = 3) public class RedirectFilter implements Filter {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/RedirectFilter.java` around lines 31 - 32, Remove the stray blank line between the class Javadoc and the class-level annotation in RedirectFilter so the annotation appears immediately after the closing */; specifically, move the `@Global`(order = 3) annotation up to directly follow the Javadoc for the RedirectFilter class to match other filters like RateLimitingFilter.src/main/java/org/juv25d/App.java (1)
27-28: Expand the inlineif-block for readability.The opening brace, body, and closing brace are all collapsed onto two lines, which makes the conditional registration hard to scan at a glance.
✏️ Proposed fix
- if (config.isRateLimitingEnabled()) {pipeline.addGlobalFilter(new RateLimitingFilter( - config.getRequestsPerMinute(), config.getBurstCapacity()), 2);} + if (config.isRateLimitingEnabled()) { + pipeline.addGlobalFilter(new RateLimitingFilter( + config.getRequestsPerMinute(), config.getBurstCapacity()), 2); + }🤖 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 27 - 28, The inline if-statement registering the RateLimitingFilter is compressed onto two lines and hurts readability; expand the conditional into a normal multi-line block: use if (config.isRateLimitingEnabled()) { on its own line, then call pipeline.addGlobalFilter(new RateLimitingFilter(config.getRequestsPerMinute(), config.getBurstCapacity()), 2); on the next line, then close the block with } on its own line. Locate the conditional that references config.isRateLimitingEnabled(), RateLimitingFilter, and pipeline.addGlobalFilter and reformat it to a standard three-line if-block for clarity.
🤖 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`:
- Around line 36-38: Logging is registered last (pipeline.addGlobalFilter(new
LoggingFilter(), 4)) so LoggingFilter misses requests that are short-circuited
by IpFilter, RateLimitingFilter, and RedirectFilter; move LoggingFilter to
highest precedence so it wraps the chain (or change its implementation to log in
a try/finally around chain.doFilter), and update the `@Global` order annotations
on LoggingFilter, IpFilter, RateLimitingFilter, and RedirectFilter to reflect
the new ordering (e.g., LoggingFilter order=1, IpFilter order=2,
RateLimitingFilter order=3, RedirectFilter order=4) so the logger runs before
downstream filters and always observes blocked/redirected requests.
---
Duplicate comments:
In `@src/main/java/org/juv25d/App.java`:
- Line 25: IpFilter is being instantiated with empty Set arguments which makes
IpFilter.doFilter a no-op; update the pipeline.addGlobalFilter call to pass real
whitelist/blacklist sets (e.g., values loaded from configuration or environment)
or change the construction to use the intended source (e.g.,
IpFilter.of(config.getWhitelist(), config.getBlacklist()) or similar) so the
filter enforces rules; locate the IpFilter constructor usage in the
addGlobalFilter call and replace the empty Set.of() arguments with the proper
collections or a factory method that supplies configured IP sets.
- Around line 42-44: Remove the redundant router.registerPlugin("/", ...) call
and stop using a broad "/*" pattern that swallows the NotFoundPlugin; instead
register StaticFilesPlugin on a specific path (e.g. "/static/*") using
router.registerPlugin("...static.../*", new StaticFilesPlugin()) and ensure
router.registerPlugin("/notfound", new NotFoundPlugin()) remains or is
registered last so NotFoundPlugin can act as the fallback; adjust the pattern
and registration order around router.registerPlugin and the
StaticFilesPlugin/NotFoundPlugin to restore proper fallback behavior.
---
Nitpick comments:
In `@src/main/java/org/juv25d/App.java`:
- Around line 27-28: The inline if-statement registering the RateLimitingFilter
is compressed onto two lines and hurts readability; expand the conditional into
a normal multi-line block: use if (config.isRateLimitingEnabled()) { on its own
line, then call pipeline.addGlobalFilter(new
RateLimitingFilter(config.getRequestsPerMinute(), config.getBurstCapacity()),
2); on the next line, then close the block with } on its own line. Locate the
conditional that references config.isRateLimitingEnabled(), RateLimitingFilter,
and pipeline.addGlobalFilter and reformat it to a standard three-line if-block
for clarity.
In `@src/main/java/org/juv25d/filter/RedirectFilter.java`:
- Around line 31-32: Remove the stray blank line between the class Javadoc and
the class-level annotation in RedirectFilter so the annotation appears
immediately after the closing */; specifically, move the `@Global`(order = 3)
annotation up to directly follow the Javadoc for the RedirectFilter class to
match other filters like RateLimitingFilter.
fmazmz
reviewed
Feb 21, 2026
fmazmz
left a comment
fmazmz
left a comment
Member
There was a problem hiding this comment.
Nice, this will make things easier and more professional.
Is it not a perfect time for us to remove the order from the Plugin method parameters, so that we dont have to apply the order twice?
Right now each filter class has the annotation with the order inside. But then in App.java we are writing the order again during instantiation.
There should be a way for us to read the order directly from the class annotation inside the respective methods in Plugin.
That way App.java doesnt have to care about specifying the order as it is handled automatically inside Plugin.
Just a thought.
Author
|
Yes exactly, that was my thinking as well! :) For this PR I just wanted to add the orderering and make sure the filters are in correct order. Then in the next PR, as I mentioned in the issue, we can decide how we want to handle it properly, whether we read them inside pipeline, add some type of automation or refactor so the responsibility is handled in one place..? Since we havent decided this yet, I would take this small step for now. (: @fmazmz |
fmazmz
approved these changes
Feb 21, 2026
addee1
approved these changes
Feb 22, 2026
addee1
left a comment
addee1
left a comment
There was a problem hiding this comment.
Nice update 👍
This makes the filter order much clearer and easier to follow. Adding @global(order = X) directly on the filters feels cleaner. The order also makes sense with blocking filters before redirect. Overall, this makes the pipeline easier to understand.
Good job :)
Author
…r-juv25d into feature/add-annotations-to-filters
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/juv25d/App.java (2)
32-36: Externalise redirect rules to configuration instead of hardcoding them.Paths like
/old-page,/temp, and/docs/*are effectively runtime policy. Hardcoding them inApp.javameans every redirect change requires a rebuild and redeploy.ConfigLoaderis already wired in; the redirect rules should be loaded from it (or from a dedicated config file) and passed intoRedirectFilter.♻️ Sketch of config-driven approach
- List<RedirectRule> redirectRules = List.of( - new RedirectRule("/old-page", "/new-page", 301), - new RedirectRule("/temp", "https://example.com/temporary", 302), - new RedirectRule("/docs/*", "/documentation/", 301) - ); + // Load redirect rules from configuration (e.g., redirect.properties or ConfigLoader) + List<RedirectRule> redirectRules = config.getRedirectRules();🤖 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 32 - 36, The redirect rules are hardcoded in App.java; change this to load them from the existing configuration system by using ConfigLoader (or a dedicated config source) to deserialize a list of RedirectRule entries and pass that list into RedirectFilter at startup; replace the List.of(...) instance (and any use of new RedirectRule(...) in App) with code that reads redirect definitions (pattern, target, status) from the config, validates them, constructs RedirectRule objects, and injects the resulting collection into RedirectFilter so rule changes are driven by config rather than code.
29-30: Expand the single-lineifblock for readability.The compact style is inconsistent with every other
addGlobalFiltercall in this block.♻️ Proposed formatting fix
- if (config.isRateLimitingEnabled()) {pipeline.addGlobalFilter(new RateLimitingFilter( - config.getRequestsPerMinute(), config.getBurstCapacity()), 3);} + if (config.isRateLimitingEnabled()) { + pipeline.addGlobalFilter(new RateLimitingFilter( + config.getRequestsPerMinute(), config.getBurstCapacity()), 3); + }🤖 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 29 - 30, The single-line if in App.java that adds the RateLimitingFilter should be expanded to a multi-line block for readability and consistency: replace the compact if (config.isRateLimitingEnabled()) { pipeline.addGlobalFilter(new RateLimitingFilter(config.getRequestsPerMinute(), config.getBurstCapacity()), 3); } with a standard block that checks config.isRateLimitingEnabled(), constructs the RateLimitingFilter using config.getRequestsPerMinute() and config.getBurstCapacity(), and calls pipeline.addGlobalFilter(...) on its own indented line(s), matching the formatting used for the other addGlobalFilter calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/juv25d/App.java`:
- Around line 32-36: The redirect rules are hardcoded in App.java; change this
to load them from the existing configuration system by using ConfigLoader (or a
dedicated config source) to deserialize a list of RedirectRule entries and pass
that list into RedirectFilter at startup; replace the List.of(...) instance (and
any use of new RedirectRule(...) in App) with code that reads redirect
definitions (pattern, target, status) from the config, validates them,
constructs RedirectRule objects, and injects the resulting collection into
RedirectFilter so rule changes are driven by config rather than code.
- Around line 29-30: The single-line if in App.java that adds the
RateLimitingFilter should be expanded to a multi-line block for readability and
consistency: replace the compact if (config.isRateLimitingEnabled()) {
pipeline.addGlobalFilter(new RateLimitingFilter(config.getRequestsPerMinute(),
config.getBurstCapacity()), 3); } with a standard block that checks
config.isRateLimitingEnabled(), constructs the RateLimitingFilter using
config.getRequestsPerMinute() and config.getBurstCapacity(), and calls
pipeline.addGlobalFilter(...) on its own indented line(s), matching the
formatting used for the other addGlobalFilter calls.
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
3 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.
Closes #106
0 SecurityHeadersFilter (wrapper)
1 LoggingFilter (wrapper)
2 IpFilter (can block)
3 RateLimitingFilter (can block)
4 RedirectFilter (can short-circuit)
Summary by CodeRabbit
New Features
Improvements