Feature/global filter - #51
Conversation
|
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:
📝 WalkthroughWalkthroughPipeline now builds ordered filter chains per request (global + route filters, exact and wildcard matching). Pipeline lifecycle init/destroy removed. Connection factory stores Pipeline; ConnectionHandler passes the HttpRequest into createChain. Filter chain guards plugin when null. Tests and docs added for global/route filters. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConnHandler as ConnectionHandler
participant Pipeline
participant Chain as FilterChainImpl
participant GlobalF as GlobalFilter
participant RouteF as RouteFilter
participant Plugin
Client->>ConnHandler: send HTTP request
ConnHandler->>Pipeline: createChain(request)
Pipeline->>Pipeline: collect & sort global filters
Pipeline->>Pipeline: match & sort route filters (exact & wildcard)
Pipeline->>Chain: new FilterChainImpl(orderedFilters, plugin)
ConnHandler->>Chain: doFilter(request, response)
Chain->>GlobalF: doFilter(request, response)
GlobalF->>Chain: chain.doFilter(...)
Chain->>RouteF: doFilter(request, response)
RouteF->>Chain: chain.doFilter(...)
Chain->>Plugin: handle(request, response) (if plugin != null)
Plugin-->>Chain: return response
Chain-->>ConnHandler: completed response
ConnHandler-->>Client: send HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/juv25d/DefaultConnectionHandlerFactory.java (1)
11-22:⚠️ Potential issue | 🟡 MinorThe
pipelinefield is stored but never used — thecreatemethod's parameter shadows it.The constructor stores
pipelineinthis.pipeline(line 16), butcreate(Socket socket, Pipeline pipeline)on line 20 receives its ownpipelineparameter, which is what gets passed toConnectionHandleron line 21. The field is dead code.Either remove the field and constructor parameter, or remove the
Pipelineparameter fromcreateand use the field instead. The latter is likely the cleaner approach since the factory already has the pipeline at construction time. Note that theConnectionHandlerFactoryinterface signature would need to be updated as well.Proposed fix: use the field, remove the parameter from create
This requires updating the
ConnectionHandlerFactoryinterface:public interface ConnectionHandlerFactory { - Runnable create(Socket socket, Pipeline pipeline); + Runnable create(Socket socket); }Then update the implementation:
`@Override` - public Runnable create(Socket socket, Pipeline pipeline) { - return new ConnectionHandler(socket, httpParser, logger, pipeline); + public Runnable create(Socket socket) { + return new ConnectionHandler(socket, httpParser, logger, this.pipeline); }And update the caller in
Server.java:- Runnable handler = handlerFactory.create(socket, pipeline); + Runnable handler = handlerFactory.create(socket);
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/Pipeline.java`:
- Around line 29-41: The createChain method currently appends all globalFilters
first then routeFilters causing global filters to always run before route ones;
change it to collect both globalFilters and matching routeFilters into a single
list, sort that combined list by their natural ordering, then map to filter() to
build the filters passed into FilterChainImpl so order values across
globalFilters and routeFilters are honored; look for createChain, globalFilters,
routeFilters, matches(), and FilterChainImpl when making this change.
- Around line 44-50: The matches(String path, String pattern) method can throw
NPE when path is null; add a null guard at the top of matches (e.g., if (path ==
null) return false) so subsequent calls to path.startsWith(...) and
path.equals(...) are safe; update the matches method only (it’s the function to
modify) to return false when path is null.
- Around line 13-14: globalFilters and routeFilters are plain ArrayLists that
are written during setup via addGlobalFilter() and addRouteFilter() but read
concurrently in handler threads via createChain(), risking data races; fix by
replacing the backing lists with thread-safe collections (e.g., use
java.util.concurrent.CopyOnWriteArrayList for globalFilters and routeFilters) or
by documenting/enforcing immutability after startup (convert to
Collections.unmodifiableList once registration is finished) and update
addGlobalFilter()/addRouteFilter() usage accordingly so createChain() safely
iterates without synchronization.
🧹 Nitpick comments (3)
src/test/java/org/juv25d/filter/GlobalFilterTests.java (1)
39-62:RecordingFilterandNoOpPluginare duplicated acrossGlobalFilterTestsandRouteFilterTests.Consider extracting these into a shared test utility class to avoid duplication and keep test helpers in sync.
src/main/java/org/juv25d/App.java (1)
16-88: Excessive commented-out code clutters the main entry point.This 70+ line block of commented-out examples would be better placed in a README, wiki page, or a dedicated example/documentation file. Keeping this volume of dead code in the main application file hurts readability.
Also, the CI pipeline is failing due to Spotless formatting violations — run
mvn spotless:applyto fix.src/main/java/org/juv25d/Pipeline.java (1)
32-34: Sorting on every request is unnecessary overhead.
globalFilterslist is the same for every request. Consider pre-sorting at registration time or lazily caching the sorted result to avoid repeated sorting.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/App.java`:
- Around line 84-87: Update the stale comment block in App.java that says "Calls
init() on all registered filters" to reflect the current design: remove any
mention of Pipeline.init() (which was removed) and replace it with a short
accurate note such as "Filters are stateless/self-initializing; no explicit
init() call is required" or simply delete the block; locate the comment near the
filter initialization area in the App class and ensure no other references to
Pipeline.init() remain.
🧹 Nitpick comments (2)
src/main/java/org/juv25d/App.java (2)
15-78: Consider moving filter scaffolding to documentation instead of inline comments.~60 lines of commented-out code makes
main()harder to scan. A shortREADMEsection or Javadoc onPipelinelisting available filter types and registration examples would serve the same "menu" purpose without cluttering the bootstrap code. Team members could then add their one or two registration lines here without scrolling past a wall of comments.
93-97: Remove redundantpipelineparameter fromServer.The factory already stores
pipelineas a field after construction, making the redundant parameter inServer's constructor unnecessary. SinceServeronly uses the pipeline to pass tohandlerFactory.create(socket, pipeline)and never directly interacts with it for lifecycle management, either:
- Remove
pipelinefromServerand modifyDefaultConnectionHandlerFactory.create()to use its own stored pipeline instead of accepting it as a parameter, or- Remove
pipelinefrom the factory's field and rely only on the parameter passed throughcreate().The first approach clarifies that the factory owns the pipeline lifecycle.
addee1
left a comment
addee1
left a comment
There was a problem hiding this comment.
Nice improvement of the filter system 👍
The separation between global and route-specific filters makes the pipeline much more flexible and easier to extend.
I like the introduction of FilterRegistration with ordering and pattern matching, it makes the design cleaner and more scalable.
Good addition of tests covering both execution and matching behavior, and nice fix with the null-check for the plugin.
Looks good to me 🚀👌🏻😁
mattknatt
left a comment
mattknatt
left a comment
There was a problem hiding this comment.
Looks very nice!
Since Filter and Plugin are functional interfaces, could it be an idea to use lambdas in the tests instead of the helper classes RecordingFIlter and NoOpPlugin? Maybe not necessary but could be something futher down the line that improves readability of the tests.
Nice work :)
|
@addee1 @mattknatt I agree, I was thinking to create a separate issue to keep this PR small, mainly focused on the global and route filters. Since I only included Global and Route tests in this pr, I still need to add tests for mixed pipelines and specific behaviours such as ordering behavior, filters that stop the chain, for example: OrderingFilterTests, ShortCircuitFilterTests, ResponsePhaseFilterTests. (As I mentioned in the linked issue) I will then update this and upcoming implementation to use lambdas, this current test impl felt a bit stone-aged 🤣, and possibly introduce a small helper class to keep the upcoming filter tests clean and DRY? Since we expect more filters going forward, having a shared test helper could help keep things consistent and maintainable and DRY 💨 By merging this PR as-is, we can keep moving forward, It will also allow other PRs to start working on their filters independently, while I focus on expanding and refining the test coverage without affecting other filter-related changes 👽 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/Pipeline.java`:
- Line 19: The field sortedGlobalFilters in class Pipeline is used as a
cross-thread cache (addGlobalFilter nulls it on registration while createChain
reads/writes it on handler threads) and must be declared volatile to ensure
visibility across threads; update the declaration of sortedGlobalFilters to be
volatile (retain the existing nulling in addGlobalFilter and the read/write
logic in createChain) so that handler threads always see the latest null/write
from the registering thread.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/Pipeline.java`:
- Around line 39-44: sortedGlobalFilters is lazily initialized with a
check-then-act that can race (TOCTOU) and it is set to a mutable list; fix by
making the cache safe and immutable: declare sortedGlobalFilters as volatile,
wrap the initialization in double-checked locking inside Pipeline (check null,
synchronized(this) { if null compute sorted list from
globalFilters.stream().sorted().map(FilterRegistration::filter).collect(...) and
assign Collections.unmodifiableList(result) }), or alternatively compute and
assign an unmodifiable sorted list when filters are registered (update the
registration path instead of lazy init); ensure the cached value is always an
unmodifiable List so handler threads cannot mutate it.
jesperlarsson1910
left a comment
jesperlarsson1910
left a comment
There was a problem hiding this comment.
Looks cleaner moving the comments to a doc. Would it be worth to format the text ? It's very much readable now but markdown formatting could make it even easeier.
jesperlarsson1910
left a comment
jesperlarsson1910
left a comment
There was a problem hiding this comment.
The implementation looks like a solid foundation , and as your previous comments mentioned leaving expanding on the actual filters to seperate issues.
Do filterorder collisions ever occur/matter?
|
Collisions are currently allowed since there is no validation preventing duplicate order values, and sorting preserves registration order for equal values. I’ll leave it as-is for now, and we can revisit stricter ordering rules in a future PR if needed 😄 |
|
Looks good, approved |
jesperlarsson1910
left a comment
jesperlarsson1910
left a comment
There was a problem hiding this comment.
Great! Solves the stated issue and looks clean :)
6 participants
All team members can now easily plug in their filters 👯♀️ (hopefully) issue: #49
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation