Feature/routing separate from plugin - #68
Conversation
…SimpleRouter` implementation. Updated pipeline and tests to support new routing system.
…SimpleRouter` implementation. Updated pipeline and tests to support new routing system.
📝 WalkthroughWalkthroughReplaces direct Plugin wiring with a Router abstraction. Adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Pipeline as Pipeline
participant Chain as FilterChainImpl
participant Router as SimpleRouter
participant Plugin as Plugin
Client->>Pipeline: send(HttpRequest)
Pipeline->>Chain: createFilterChain(request, response)
Chain->>Chain: applyFilters(request,response)
alt filters continue
Chain->>Router: resolve(request)
Router-->>Chain: Plugin
Chain->>Plugin: handle(request,response)
else short-circuit by filter
Chain--xClient: response returned
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 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: 4
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/Pipeline.java (1)
43-65:⚠️ Potential issue | 🟡 Minor
routermay be null whencreateChainis called.If
setRouterhas not been called beforecreateChain,routerisnulland will be passed toFilterChainImpl, likely causing an NPE when a request is handled. Consider adding a defensive check or initializing the field with a default router.Option A: Fail fast with a clear message
+ if (router == null) { + throw new IllegalStateException("Router has not been configured"); + } return new FilterChainImpl(filters, router);
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/App.java`:
- Around line 3-6: The file contains duplicate import statements for the
IpFilter and LoggingFilter classes; in App.java remove the redundant lines so
each of the two imports appears only once (leave a single import for
org.juv25d.filter.IpFilter and a single import for
org.juv25d.filter.LoggingFilter), ensuring the App class still compiles and no
duplicate import warnings remain.
In `@src/main/java/org/juv25d/Pipeline.java`:
- Line 21: The router field in Pipeline must be made volatile to ensure
visibility across threads; update the declaration of the Router field (router)
to be volatile and keep existing accesses in setRouter and createChain unchanged
so writes in setRouter are visible to reads in createChain; this aligns router
with other volatile/shared state (e.g., sortedGlobalFilters) for thread-safety
in class Pipeline.
In `@src/main/java/org/juv25d/router/Router.java`:
- Around line 15-18: The Javadoc for Router.resolve incorrectly allows null
while FilterChainImpl.doFilter assumes a non-null Plugin, risking NPE; tighten
the contract by updating Router.resolve's Javadoc to state it never returns
null, add a non-null annotation (e.g., `@Nonnull` or `@NotNull`) to the method
signature, and ensure all implementations (e.g., SimpleRouter) never return null
(return a default/no-op Plugin or throw a well-documented exception instead);
alternatively, if you prefer guarding at call site, add a null-check in
FilterChainImpl.doFilter before calling handle and handle the missing plugin
case explicitly.
In `@src/main/java/org/juv25d/router/SimpleRouter.java`:
- Around line 53-58: The wildcard-match loop in SimpleRouter (iterating over
routes and returning the first entry whose key endsWith("/*") and matches the
request path) is nondeterministic because routes is a HashMap; update the
implementation to deterministically pick the most specific wildcard: either
change the routes field to a LinkedHashMap to preserve registration order (so
insertion order wins) or, better, collect matching wildcard keys in the method
that does lookup and choose the longest matching prefix (registeredPath with the
greatest length) before returning its Plugin; adjust the code in the
SimpleRouter method that performs the loop and the routes declaration
accordingly (use LinkedHashMap<> or add a comparison by
registeredPath.length()).
🧹 Nitpick comments (6)
src/test/java/org/juv25d/filter/RouteFilterTests.java (2)
23-27: Router setup registers plugin only at"/", but test requests hit other paths.The
NoOpPluginis registered at"/", butexecute(pipeline, "/api/test")andexecute(pipeline, "/home")won't match that route — they'll fall through toNotFoundPlugin. This works today because the tests only assert on filter execution, not on the response. However, registering a catch-all"/*"route would better express the intent and avoid confusion if these tests are later extended to validate response content.Also applies to: 39-43
80-83:NoOpPlugininner class is duplicated across test files.This same
NoOpPlugininner class also appears inGlobalFilterTests. Consider extracting it to a shared test utility to reduce duplication.src/test/java/org/juv25d/filter/GlobalFilterTests.java (1)
23-27: Same observation: plugin registered at"/"but request path is"/anything".The global filter test will route to
NotFoundPluginsince"/anything"doesn't match"/". This is fine for verifying filter execution, but consider using"/*"for the catch-all registration to better match the test intent.src/main/java/org/juv25d/App.java (1)
34-37: Consider reusing a singleStaticFilesPlugininstance.Two separate
StaticFilesPlugininstances are created for"/"and"/*". SinceStaticFilesPluginappears stateless, a single shared instance would suffice.Suggested change
+ StaticFilesPlugin staticFiles = new StaticFilesPlugin(); SimpleRouter router = new SimpleRouter(); - router.registerPlugin("/", new StaticFilesPlugin()); - router.registerPlugin("/*", new StaticFilesPlugin()); + router.registerPlugin("/", staticFiles); + router.registerPlugin("/*", staticFiles); router.registerPlugin("/notfound", new NotFoundPlugin());src/test/java/org/juv25d/router/SimpleRouterTest.java (1)
19-26:notFoundPluginfield is initialized but never referenced in assertions.The field on line 19 is set up in
@BeforeEachbut all not-found assertions useinstanceof NotFoundPluginchecks instead. Consider removing it to avoid confusion.src/main/java/org/juv25d/router/SimpleRouter.java (1)
46-49: Minor: doubleHashMaplookup can be simplified.
containsKeyfollowed bygetperforms two lookups. Usegetwith a null check instead.Suggested change
- if (routes.containsKey(path)) { - return routes.get(path); + Plugin exactMatch = routes.get(path); + if (exactMatch != null) { + return exactMatch; }
…logic; add test coverage for specific and wildcard match scenarios.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/juv25d/router/SimpleRouter.java (1)
56-67: Minor: wildcard stream is rebuilt on every request.For a hot path, materializing and sorting the wildcard entries on each
resolve()call is wasteful. If route registration is done at startup, consider pre-sorting the wildcard routes once (e.g., in a separateListmaintained at registration time). Not urgent at current scale.src/test/java/org/juv25d/router/SimpleRouterTest.java (1)
19-26: Unused field:notFoundPluginis never referenced in assertions.The field is created on line 19/26 but all not-found assertions use
instanceof NotFoundPlugin(which is correct, since the router creates its own instance). Remove the dead field to reduce noise.♻️ Proposed fix
private Plugin mockPluginA; private Plugin mockPluginB; - private Plugin notFoundPlugin; `@BeforeEach` void setUp() { router = new SimpleRouter(); mockPluginA = mock(Plugin.class); mockPluginB = mock(Plugin.class); - notFoundPlugin = new NotFoundPlugin(); // Assuming NotFoundPlugin is a concrete class }
|
Nice refactor! 👍 |
mattknatt
left a comment
mattknatt
left a comment
There was a problem hiding this comment.
Looks good, felt easier to understand the routing logic! Nice work.
One thing to maybe be considered in the test-file is to make a helper method that returns a request for the requested path, something like "
private HttpRequest requestFor(String path) {
return new HttpRequest("GET", path, null, "HTTP/1.1", Map.of(), new byte[0], "UNKNOWN");
}
"
Just to not have to write the full request in each test. But not a necessary change, so looks good to merge!
3 participants
Closes #61
For the current scope, routing inside Plugin.handle() works, but it couples routing and endpoint logic.
Introducing a lightweight Router would improve separation of concerns, testability, and extensibility without adding much complexity.
I intend to solve this by introducing a simple Router/RouteHandler abstraction, not a full framework.
Routerabstraction;SimpleRouterimplementation.Summary by CodeRabbit
Release Notes
New Features
Chores
Tests