Feature/filter lifecycle and builder refactor - #94
Closed
kristinaxm wants to merge 32 commits into
Closed
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:
📝 WalkthroughWalkthroughThis refactor restructures the request-processing architecture by introducing a fluent Changes
Sequence DiagramsequenceDiagram
participant App as App.main()
participant Builder as ServerBuilder
participant Config as FilterConfiguration
participant Pipeline as Pipeline
participant Scanner as FilterScanner
participant Filters as Filters<br/>[`@Global/`@Route]
participant Server as Server
App->>Builder: new ServerBuilder()
App->>Builder: port(8080)
App->>Builder: logger(logger)
App->>Builder: router(router)
App->>Config: configure(builder, config)
Config->>Builder: addFilter(RedirectFilter)
Config->>Builder: addFilter(IpFilter)
Config->>Builder: addFilter(LoggingFilter)
Config->>Builder: addFilterIf(enabled, RateLimitingFilter)
App->>Builder: build()
Builder->>Pipeline: new Pipeline()
Builder->>Pipeline: setRouter(router)
Builder->>Scanner: register(filters, pipeline)
Scanner->>Filters: inspect `@Global/`@Route
Filters-->>Scanner: annotations found
Scanner->>Pipeline: addGlobalFilter() / addRouteFilter()
Builder->>Pipeline: initFilters()
Pipeline->>Filters: init() on each
Filters-->>Pipeline: initialized
Builder->>Server: new Server(port, logger, handlerFactory, pipeline)
Builder-->>App: Server instance
App->>Server: start()
Note over Server: On shutdown
Server->>Pipeline: destroyFilters()
Pipeline->>Filters: destroy() on each
Filters-->>Pipeline: destroyed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 8
🧹 Nitpick comments (6)
src/main/java/org/juv25d/Server/ServerBuilder.java (2)
22-25: No port validation — invalid ports fail silently untilServerSocketcreation.
port(int port)accepts anyint. Values ≤ 0 (except 0, which selects an ephemeral port) or > 65535 are invalid and will only fail at runtime insidestart(). Validating eagerly inport()orbuild()gives a cleaner developer experience.🔧 Suggested guard in `port()`
public ServerBuilder port(int port) { + if (port < 0 || port > 65535) { + throw new IllegalArgumentException("Port must be between 0 and 65535, got: " + port); + } this.port = port; return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Server/ServerBuilder.java` around lines 22 - 25, The ServerBuilder.port(int port) setter currently accepts any int and defers invalid-port failures to runtime; add eager validation in ServerBuilder.port (or in build() if you prefer centralized checks) to ensure port is either 0 or in range 1..65535 and throw an IllegalArgumentException with a clear message for out-of-range values, otherwise assign this.port and return this; also update any Javadoc/comments on ServerBuilder.port and adjust callers/tests if they relied on silent acceptance.
37-40:addFilter(null)silently no-ops — consider explicit null guard.
FilterScanner.register()short-circuits when the instance is not aFilter(null satisfies that check), so passingnulltoaddFilter()is swallowed without any error. An explicit precondition check would surface misconfigurations earlier.🔧 Suggested null guard
public ServerBuilder addFilter(Filter filter) { + Objects.requireNonNull(filter, "Filter must not be null"); FilterScanner.register(filter, pipeline); return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Server/ServerBuilder.java` around lines 37 - 40, The addFilter method currently forwards null to FilterScanner.register, which silently no-ops; update ServerBuilder.addFilter(Filter filter) to explicitly guard against null by validating the parameter (e.g., if filter == null) and throw a clear runtime exception (IllegalArgumentException or NullPointerException) with a descriptive message before calling FilterScanner.register, so misconfigurations are surfaced immediately; reference ServerBuilder.addFilter, FilterScanner.register, and the Filter parameter/pipeline variable when making the change.src/test/java/org/juv25d/Server/ServerBuilderTest.java (2)
63-67:TestGlobalFilter.doFilter()is a no-op and does not forward tochain.doFilter().While this is harmless for the current test (which only verifies
init()is called), a filter that silently swallows the chain call would block all downstream filters and the terminal route handler if accidentally reused in an integration context. Adding the chain forward makes the intent clearer.🔧 Suggested fix
`@Override` public void doFilter(HttpRequest request, HttpResponse response, FilterChain chain) { + chain.doFilter(request, response); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/Server/ServerBuilderTest.java` around lines 63 - 67, The TestGlobalFilter.doFilter(HttpRequest request, HttpResponse response, FilterChain chain) currently swallows the call; modify this method to forward the request and response to the next filter by invoking chain.doFilter(request, response) so downstream filters and the terminal handler are executed—update the doFilter implementation to call chain.doFilter(...) using the provided FilterChain parameter.
19-47: Missing coverage forFilterScannerconflict detection and theaddFilterIfpath.The three existing tests cover required-field validation and the happy-path initialization. The following scenarios described in the PR objectives and
FilterScannercontract are not exercised:
- A filter annotated with both
@Globaland@Route—FilterScannershould throwIllegalStateException.- A filter with no annotation — should be silently ignored (no-op registration).
addFilterIf(false, supplier)— supplier must not be invoked and filter must not be registered.addFilterIf(true, supplier)— filter must be registered and initialized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/Server/ServerBuilderTest.java` around lines 19 - 47, Add tests exercising FilterScanner conflict detection and the addFilterIf paths: create a filter class annotated with both `@Global` and `@Route` and assert ServerBuilder.build() throws IllegalStateException (verify FilterScanner detects the conflict), create a filter class with no annotation and assert build() does not register or initialize it (silently ignored), add tests calling ServerBuilder.addFilterIf(false, () -> supplier) and assert the supplier is never invoked and no filter is registered, and add tests calling ServerBuilder.addFilterIf(true, () -> supplier) and assert the supplier is invoked and the filter is registered and initialized (use the existing TestGlobalFilter/initialized AtomicBoolean pattern and reference ServerBuilder, FilterScanner, addFilterIf, and TestGlobalFilter to locate code).src/main/java/org/juv25d/Server/Server.java (1)
26-29:destroyFilters()is called without draining in-flight requests.Virtual threads spawned in the accept loop are daemon threads; when the JVM starts shutdown, the hook calls
destroyFilters()while those threads may still be mid-request and actively using filters. Consider waiting for in-flight handlers to complete (e.g., tracking them with aCountDownLatchor aSemaphore) before callingpipeline.destroyFilters().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Server/Server.java` around lines 26 - 29, The shutdown hook calls pipeline.destroyFilters() while virtual threads in the accept loop may still be processing requests; change shutdown to first wait for in-flight handlers to finish by adding a concurrency tracker (e.g., an AtomicInteger or a Semaphore/CountDownLatch) that is incremented at the start of each request handler in the accept loop and decremented when the handler completes, then in the Runtime.getRuntime().addShutdownHook block call await with a reasonable timeout on that tracker before invoking pipeline.destroyFilters(); update the accept-loop handler logic (where virtual threads are spawned) to use the tracker so destroyFilters() runs only after in-flight requests drain (or timeout).src/main/java/org/juv25d/App.java (1)
38-52: Filter execution order is driven by@Global(order=…)annotations, not registration order.The builder registers filters via
FilterScanner, which reads theorderfield from@Global/@Routeannotations and passes it toPipeline.addGlobalFilter(), which re-sorts on every insertion. The order of.addFilter(...)calls inApp.javahas no effect on execution order. Developers who expect execution to follow registration sequence (e.g., assumingRedirectFilterruns beforeIpFilter) will be surprised. A comment documenting the intended annotation-order values of each filter would prevent future mis-ordering.🤖 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 38 - 52, The current .addFilter(...) calls in App.java suggest registration order matters but FilterScanner and Pipeline.addGlobalFilter() actually drive execution via `@Global`(order=...), so update the codebase to make ordering explicit: annotate each global filter class (RedirectFilter, IpFilter, LoggingFilter, RateLimitingFilter) with a clear `@Global`(order = X) value reflecting the intended execution sequence and/or add a concise in-file comment above the builder block listing those intended order values (e.g., RedirectFilter -> order N, IpFilter -> order N+1, LoggingFilter -> order N+2, RateLimitingFilter -> order N+3) so future maintainers know which numeric orders to use; verify FilterScanner and Pipeline.addGlobalFilter() will pick up those annotation values.
🤖 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/filter/FilterScanner.java`:
- Around line 26-38: In FilterScanner, when scanning an object (instance) that
implements Filter but has neither the `@Global` (global) nor `@Route` (route)
annotation, do not silently ignore it: add a check after the existing if blocks
that detects global == null && route == null and then either throw a clear
IllegalStateException (include the class name of instance) or call the
pipeline/processLogger to emit a warning identifying the unannotated Filter;
update the logic around pipeline.addGlobalFilter and pipeline.addRouteFilter so
the new check runs for the same code path that currently falls through,
referencing the same symbols (FilterScanner, instance, global, route,
pipeline.addGlobalFilter, pipeline.addRouteFilter).
In `@src/main/java/org/juv25d/Pipeline.java`:
- Around line 58-74: createChain currently adds filters from
routeFilters.get(path) and then again from wildcard entries (routeFilters entry
keys ending with "*"), which causes duplicate FilterRegistration.filter()
invocations when both exact (e.g., "/users") and wildcard (e.g., "/users*")
registrations exist; fix by deduplicating before adding to the filters
list—e.g., maintain a LinkedHashSet of Filter instances or their identity keys
while building filters in createChain (use FilterRegistration::filter as the
unique symbol) so you only add each filter once and preserve order, or
alternatively skip wildcard entry processing when an exact match for the same
base path was already applied.
- Around line 94-96: destroyFilters currently calls
getAllFilters().forEach(Filter::destroy) which allows an exception in one
Filter.destroy() to abort cleanup; change destroyFilters to iterate over
getAllFilters() and call filter.destroy() inside a try-catch for each filter,
logging the exception (using your logger) and continuing so all filters get
their destroy lifecycle callback; leave initFilters
(initFilters()/Filter.init()) behavior as-is for now but consider whether its
exceptions should be handled differently if you want init to be resilient.
- Around line 23-30: Concurrent calls to addGlobalFilter can overwrite
sortedGlobalFilters because each thread snapshots globalFilters separately; to
fix, guard the add + recompute sequence with a single lock (e.g., synchronize
the addGlobalFilter method or use a dedicated mutex) so that updating
globalFilters and then recomputing sortedGlobalFilters happens atomically.
Ensure the synchronized block covers globalFilters.add(new
FilterRegistration(...)) and the subsequent stream/sort/map/collect into
sortedGlobalFilters (referencing addGlobalFilter, globalFilters,
sortedGlobalFilters, and FilterRegistration).
In `@src/main/java/org/juv25d/Server/Server.java`:
- Line 31: The code in Server.java creates a plaintext ServerSocket (see the
Server class and the ServerSocket instantiation using the port variable), which
sends traffic unencrypted; update the Server class to either (a) document the
assumption that TLS termination is performed upstream (reverse proxy/load
balancer) in the class Javadoc and any deployment/readme, or (b) switch to an
SSLServerSocket by creating an SSLServerSocketFactory and using it to create an
SSLServerSocket (replacing the ServerSocket instantiation) and loading the
keystore/SSLContext appropriately for production; pick one approach and apply
the change consistently where the ServerSocket is constructed.
- Around line 26-29: Move shutdown hook registration out of start() to a single
guarded location: register the Runtime.getRuntime().addShutdownHook(...) once
(e.g., in the Server constructor) or add an AtomicBoolean field (e.g.,
shutdownHookRegistered) and check-and-set it before calling addShutdownHook in
start(), so pipeline.destroyFilters() is not registered multiple times; modify
Server's constructor or start() to perform this single-registration guard and
reference the existing pipeline.destroyFilters() call inside the hook.
In `@src/main/java/org/juv25d/Server/ServerBuilder.java`:
- Around line 49-65: Add a one-time "built" guard to ServerBuilder: introduce a
private boolean built field, check at the start of build() and throw an
IllegalStateException (e.g., "ServerBuilder already built") if built is true,
then set built = true before or immediately after calling
pipeline.initFilters(); this ensures pipeline.initFilters() (and Filter.init())
run only once and prevents double-initialization when build() is called multiple
times.
In `@src/test/java/org/juv25d/filter/FilterScannerTest.java`:
- Around line 14-50: The tests are missing coverage for a Filter implementation
that has no `@Global` or `@Route` annotation, which lets FilterScanner.register
silently ignore valid Filter objects; add a unit test named
shouldRejectFilterWithNoAnnotation that creates a mock Pipeline and an instance
of an UnannotatedFilter (implements Filter), then assertThrows
IllegalStateException when calling FilterScanner.register(filter, pipeline) and
verifyNoInteractions(pipeline); this ensures FilterScanner.register correctly
rejects Filter instances without either `@Global` or `@Route` annotations.
---
Nitpick comments:
In `@src/main/java/org/juv25d/App.java`:
- Around line 38-52: The current .addFilter(...) calls in App.java suggest
registration order matters but FilterScanner and Pipeline.addGlobalFilter()
actually drive execution via `@Global`(order=...), so update the codebase to make
ordering explicit: annotate each global filter class (RedirectFilter, IpFilter,
LoggingFilter, RateLimitingFilter) with a clear `@Global`(order = X) value
reflecting the intended execution sequence and/or add a concise in-file comment
above the builder block listing those intended order values (e.g.,
RedirectFilter -> order N, IpFilter -> order N+1, LoggingFilter -> order N+2,
RateLimitingFilter -> order N+3) so future maintainers know which numeric orders
to use; verify FilterScanner and Pipeline.addGlobalFilter() will pick up those
annotation values.
In `@src/main/java/org/juv25d/Server/Server.java`:
- Around line 26-29: The shutdown hook calls pipeline.destroyFilters() while
virtual threads in the accept loop may still be processing requests; change
shutdown to first wait for in-flight handlers to finish by adding a concurrency
tracker (e.g., an AtomicInteger or a Semaphore/CountDownLatch) that is
incremented at the start of each request handler in the accept loop and
decremented when the handler completes, then in the
Runtime.getRuntime().addShutdownHook block call await with a reasonable timeout
on that tracker before invoking pipeline.destroyFilters(); update the
accept-loop handler logic (where virtual threads are spawned) to use the tracker
so destroyFilters() runs only after in-flight requests drain (or timeout).
In `@src/main/java/org/juv25d/Server/ServerBuilder.java`:
- Around line 22-25: The ServerBuilder.port(int port) setter currently accepts
any int and defers invalid-port failures to runtime; add eager validation in
ServerBuilder.port (or in build() if you prefer centralized checks) to ensure
port is either 0 or in range 1..65535 and throw an IllegalArgumentException with
a clear message for out-of-range values, otherwise assign this.port and return
this; also update any Javadoc/comments on ServerBuilder.port and adjust
callers/tests if they relied on silent acceptance.
- Around line 37-40: The addFilter method currently forwards null to
FilterScanner.register, which silently no-ops; update
ServerBuilder.addFilter(Filter filter) to explicitly guard against null by
validating the parameter (e.g., if filter == null) and throw a clear runtime
exception (IllegalArgumentException or NullPointerException) with a descriptive
message before calling FilterScanner.register, so misconfigurations are surfaced
immediately; reference ServerBuilder.addFilter, FilterScanner.register, and the
Filter parameter/pipeline variable when making the change.
In `@src/test/java/org/juv25d/Server/ServerBuilderTest.java`:
- Around line 63-67: The TestGlobalFilter.doFilter(HttpRequest request,
HttpResponse response, FilterChain chain) currently swallows the call; modify
this method to forward the request and response to the next filter by invoking
chain.doFilter(request, response) so downstream filters and the terminal handler
are executed—update the doFilter implementation to call chain.doFilter(...)
using the provided FilterChain parameter.
- Around line 19-47: Add tests exercising FilterScanner conflict detection and
the addFilterIf paths: create a filter class annotated with both `@Global` and
`@Route` and assert ServerBuilder.build() throws IllegalStateException (verify
FilterScanner detects the conflict), create a filter class with no annotation
and assert build() does not register or initialize it (silently ignored), add
tests calling ServerBuilder.addFilterIf(false, () -> supplier) and assert the
supplier is never invoked and no filter is registered, and add tests calling
ServerBuilder.addFilterIf(true, () -> supplier) and assert the supplier is
invoked and the filter is registered and initialized (use the existing
TestGlobalFilter/initialized AtomicBoolean pattern and reference ServerBuilder,
FilterScanner, addFilterIf, and TestGlobalFilter to locate code).
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/main/java/org/juv25d/Pipeline.java (2)
17-18:globalFiltersandrouteFiltersshould remainfinal.Both fields are initialized at declaration and never reassigned; removing
finalis unnecessary and weakens the immutability guarantee provided by the JMM for final fields.♻️ Proposed fix
- private List<FilterRegistration> globalFilters = new CopyOnWriteArrayList<>(); - private Map<String, List<FilterRegistration>> routeFilters = new ConcurrentHashMap<>(); + private final List<FilterRegistration> globalFilters = new CopyOnWriteArrayList<>(); + private final Map<String, List<FilterRegistration>> routeFilters = new ConcurrentHashMap<>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Pipeline.java` around lines 17 - 18, In class Pipeline, restore the immutability guarantee by marking the fields globalFilters and routeFilters as final again; since both are initialized at declaration and never reassigned, update the declarations of globalFilters (CopyOnWriteArrayList<FilterRegistration>) and routeFilters (ConcurrentHashMap<String, List<FilterRegistration>>) to include the final modifier so the Java Memory Model treats them as properly immutable references.
86-88:initFiltershas no per-filter error isolation, unlikedestroyFilters.A single failing
init()aborts all subsequent filter initialization. This is the symmetric counterpart of thedestroyFiltersissue that was resolved in this PR. While it is arguable that init failures should propagate (signaling a fatal misconfiguration), at minimum the partially-initialized state should be logged clearly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Pipeline.java` around lines 86 - 88, initFilters currently calls Filter::init for all filters without per-filter isolation so one exception aborts the rest; update initFilters to iterate over getAllFilters() and call each filter.init() inside a try-catch that logs the filter identity (e.g., class or name) and the caught exception (using the same logger used in destroyFilters) and continues initializing remaining filters; optionally collect exceptions and after the loop either rethrow a combined exception or return status, but at minimum ensure per-filter exceptions are logged and do not stop subsequent init calls.src/main/java/org/juv25d/Server/Server.java (1)
24-27: Lambda body insideaddShutdownHookis not indented consistently.♻️ Proposed fix
Runtime.getRuntime().addShutdownHook(new Thread(() -> { - logger.info("Shutting down server..."); - pipeline.destroyFilters(); + logger.info("Shutting down server..."); + pipeline.destroyFilters(); }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Server/Server.java` around lines 24 - 27, The shutdown hook lambda passed to Runtime.getRuntime().addShutdownHook has inconsistent indentation; reformat the lambda body so its statements are consistently indented (align the logger.info("Shutting down server..."); and pipeline.destroyFilters(); lines inside the new Thread(() -> { ... }) block), preserving the existing calls to logger and pipeline.destroyFilters and keeping the surrounding addShutdownHook/new Thread invocation unchanged.
🤖 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/Pipeline.java`:
- Around line 28-34: The addRouteFilter method performs a non-atomic compound
operation on a CopyOnWriteArrayList (routeFilters.computeIfAbsent(...) ->
registrations.add(...) -> registrations.sort(...)), which can race with other
threads and with createChain; fix by making the add+sort atomic: locate
addRouteFilter and ensure the mutation is done inside a single synchronized
block (e.g., synchronize on the registrations list or a dedicated lock for that
pattern) when creating the new FilterRegistration and sorting, so no concurrent
insertion can occur between add and sort; keep using FilterRegistration and
CopyOnWriteArrayList but guard the compound operation to preserve priority order
for subsequent createChain calls.
- Around line 47-74: createChain currently passes the field router (which may be
null if setRouter was never called) into FilterChainImpl; add a fail-fast null
check at the start of createChain and throw an IllegalStateException with a
clear message (e.g., "router not initialized - call setRouter(...) before
createChain") if router is null so FilterChainImpl and subsequent
doFilter/router.resolve(...) never receive a null router; reference the router
field and the createChain method (and mention setRouter for context) when
implementing the check.
In `@src/main/java/org/juv25d/Server/Server.java`:
- Line 17: Remove the unused AtomicBoolean guard by deleting the field
declaration shutdownHookRegistered in class Server and also remove its unused
import (java.util.concurrent.atomic.AtomicBoolean) from the top of the file;
ensure no other code references shutdownHookRegistered (search for
"shutdownHookRegistered") and if found, remove or refactor those references
accordingly since the constructor-only shutdown hook already prevents
duplicates.
- Line 1: Update the package declaration and directory names from uppercase
Server to lowercase server: change the package statement in Server.java and
ServerBuilder.java from "package org.juv25d.Server;" to "package
org.juv25d.server;" and move/rename the physical directories from
src/main/java/org/juv25d/Server to src/main/java/org/juv25d/server and likewise
for src/test/java/org/juv25d/Server; ensure any imports or references to
org.juv25d.Server throughout the codebase are updated to org.juv25d.server so
Server.java, ServerBuilder.java and tests compile with the new package path.
---
Duplicate comments:
In `@src/main/java/org/juv25d/Server/Server.java`:
- Line 32: The Server.java currently creates a plain ServerSocket via "new
ServerSocket(port, 64)" which exposes plaintext transport (CWE-319); replace
this with an SSL/TLS server socket by using an SSLServerSocket from
SSLServerSocketFactory (or configure an SSLContext and call
getServerSocketFactory()) and initialize it with the appropriate
keystore/credentials, enabled protocols and cipher suites before accepting
connections; update any references to the "serverSocket" variable and the Server
class accept/close logic to use the SSLServerSocket type so all inbound traffic
is encrypted.
---
Nitpick comments:
In `@src/main/java/org/juv25d/Pipeline.java`:
- Around line 17-18: In class Pipeline, restore the immutability guarantee by
marking the fields globalFilters and routeFilters as final again; since both are
initialized at declaration and never reassigned, update the declarations of
globalFilters (CopyOnWriteArrayList<FilterRegistration>) and routeFilters
(ConcurrentHashMap<String, List<FilterRegistration>>) to include the final
modifier so the Java Memory Model treats them as properly immutable references.
- Around line 86-88: initFilters currently calls Filter::init for all filters
without per-filter isolation so one exception aborts the rest; update
initFilters to iterate over getAllFilters() and call each filter.init() inside a
try-catch that logs the filter identity (e.g., class or name) and the caught
exception (using the same logger used in destroyFilters) and continues
initializing remaining filters; optionally collect exceptions and after the loop
either rethrow a combined exception or return status, but at minimum ensure
per-filter exceptions are logged and do not stop subsequent init calls.
In `@src/main/java/org/juv25d/Server/Server.java`:
- Around line 24-27: The shutdown hook lambda passed to
Runtime.getRuntime().addShutdownHook has inconsistent indentation; reformat the
lambda body so its statements are consistently indented (align the
logger.info("Shutting down server..."); and pipeline.destroyFilters(); lines
inside the new Thread(() -> { ... }) block), preserving the existing calls to
logger and pipeline.destroyFilters and keeping the surrounding
addShutdownHook/new Thread invocation unchanged.
| @@ -1,25 +1,36 @@ | |||
| package org.juv25d; | |||
| package org.juv25d.Server; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all Java files in or under directories named 'Server' and show package declarations
fd -t f '\.java$' | xargs head -3 | grep -E '(^==|^package)' | head -50Repository: ithsjava25/project-webserver-juv25d
Length of output: 2190
🏁 Script executed:
#!/bin/bash
# Also check the actual directory structure of the Server directory
find . -type d -name 'Server' 2>/dev/nullRepository: ithsjava25/project-webserver-juv25d
Length of output: 146
🌐 Web query:
Java package naming conventions lowercase
💡 Result:
- Use all-lowercase package names (no uppercase). This avoids confusion with type names (which typically start uppercase) and matches common Java style. [1], [2]
- For widely distributed packages, use reversed Internet domain name + project parts, e.g.
com.example.myapp.feature. The leading domain components are lowercase (e.g.,com,org,uk). [1] - Avoid underscores and separators; if you have multiple words, concatenate in lowercase, e.g.
com.example.deepspace(notdeepSpaceordeep_space). [2] - Don’t start your own packages with
java(reserved for the Java platform). [1]
Examples
- Good:
com.acme.payments.api,org.example.tools.json - Bad:
com.Acme.Payments,com.acme.payment_api,java.mycompany.app
Sources: Oracle Java Language Specification (package naming) [1]; OpenJDK style guide / common Java style (lowercase, no underscores) [2].
[1] (docs.oracle.com)
[2] (cr.openjdk.org)
Citations:
- 1: https://docs.oracle.com/javase/specs/jls/se6/html/names.html?utm_source=openai
- 2: https://cr.openjdk.org/~alundblad/styleguide/index-v6.html?utm_source=openai
Package name org.juv25d.Server violates Java naming conventions — must be all lowercase.
Java Language Specification and OpenJDK style guide require package names to use only lowercase letters. The package should be org.juv25d.server, and the physical directory structure must match accordingly (rename Server directory to server in both src/main/java/org/juv25d/ and src/test/java/org/juv25d/).
This affects Server.java and ServerBuilder.java.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/juv25d/Server/Server.java` at line 1, Update the package
declaration and directory names from uppercase Server to lowercase server:
change the package statement in Server.java and ServerBuilder.java from "package
org.juv25d.Server;" to "package org.juv25d.server;" and move/rename the physical
directories from src/main/java/org/juv25d/Server to
src/main/java/org/juv25d/server and likewise for
src/test/java/org/juv25d/Server; ensure any imports or references to
org.juv25d.Server throughout the codebase are updated to org.juv25d.server so
Server.java, ServerBuilder.java and tests compile with the new package path.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/juv25d/Pipeline.java (1)
60-69: Extract duplicate sort-and-deduplicate logic into a private helper.The sort +
LinkedHashSet-style deduplication block is copy-pasted verbatim betweencreateChain(Lines 60–69) andgetAllFilters(Lines 110–119). Extracting it keeps future changes consistent.♻️ Proposed refactor
+ private List<Filter> toOrderedFilters(List<FilterRegistration> registrations) { + Collections.sort(registrations); + List<Filter> result = new ArrayList<>(); + Set<Filter> seen = new HashSet<>(); + for (FilterRegistration reg : registrations) { + if (seen.add(reg.filter())) { + result.add(reg.filter()); + } + } + return result; + }Then in
createChain:- Collections.sort(collected); - - List<Filter> finalFilters = new ArrayList<>(); - Set<Filter> seen = new HashSet<>(); - - for (FilterRegistration reg : collected) { - if (seen.add(reg.filter())) { - finalFilters.add(reg.filter()); - } - } - - return new FilterChainImpl(finalFilters, router); + return new FilterChainImpl(toOrderedFilters(collected), currentRouter);And in
getAllFilters:- Collections.sort(all); - - List<Filter> result = new ArrayList<>(); - Set<Filter> seen = new HashSet<>(); - - for (FilterRegistration reg : all) { - if (seen.add(reg.filter())) { - result.add(reg.filter()); - } - } - - return result; + return toOrderedFilters(all);Also applies to: 105-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Pipeline.java` around lines 60 - 69, Extract the repeated "sort then dedupe" logic into a private helper method (e.g. sortAndDeduplicateFilters) that accepts the List<FilterRegistration> collected and returns List<Filter>; inside the helper sort the registrations, iterate using a Set<Filter> seen to preserve insertion order and build the final List<Filter>, and replace the duplicated blocks in createChain and getAllFilters with calls to this new private method (refer to FilterRegistration, collected, createChain, getAllFilters, finalFilters, and seen when locating and replacing the code).
🤖 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/Pipeline.java`:
- Around line 41-71: createChain currently reads the router field twice (check
and later use), opening a TOCTOU race; fix by reading the router into a local
final variable at the top of createChain (e.g., final Router localRouter =
this.router), perform the null check against localRouter, and pass localRouter
into the FilterChainImpl constructor instead of re-reading the field; ensure all
uses of router inside createChain reference that local variable.
---
Duplicate comments:
In `@src/main/java/org/juv25d/Pipeline.java`:
- Around line 20-28: The concurrency issue noted earlier is resolved: both
addGlobalFilter(Filter,int) and addRouteFilter(Filter,int,String) now perform a
single thread-safe mutation using CopyOnWriteArrayList.add, so no further
changes are required; keep the deferred sorting logic in
createChain()/getAllFilters() to perform ordering at read-time rather than
during mutation to avoid races.
---
Nitpick comments:
In `@src/main/java/org/juv25d/Pipeline.java`:
- Around line 60-69: Extract the repeated "sort then dedupe" logic into a
private helper method (e.g. sortAndDeduplicateFilters) that accepts the
List<FilterRegistration> collected and returns List<Filter>; inside the helper
sort the registrations, iterate using a Set<Filter> seen to preserve insertion
order and build the final List<Filter>, and replace the duplicated blocks in
createChain and getAllFilters with calls to this new private method (refer to
FilterRegistration, collected, createChain, getAllFilters, finalFilters, and
seen when locating and replacing the code).
Comment on lines
41
to
+71
| public FilterChainImpl createChain(HttpRequest request) { | ||
| List<Filter> filters = new ArrayList<>(); | ||
| filters.addAll(sortedGlobalFilters); | ||
| String path = request.path(); | ||
| List<FilterRegistration> exactMatches = routeFilters.get(path); | ||
| if (exactMatches != null) { | ||
| exactMatches.stream() | ||
| .sorted() | ||
| .map(FilterRegistration::filter) | ||
| .forEach(filters::add); | ||
| if (router == null) { | ||
| throw new IllegalStateException("Router not set"); | ||
| } | ||
|
|
||
| String path = request.path(); | ||
|
|
||
| List<FilterRegistration> collected = new ArrayList<>(); | ||
|
|
||
| collected.addAll(globalFilters); | ||
|
|
||
| for (Map.Entry<String, List<FilterRegistration>> entry : routeFilters.entrySet()) { | ||
| String pattern = entry.getKey(); | ||
| if (pattern.endsWith("*") && path.startsWith(pattern.substring(0, pattern.length() - 1))) { | ||
| entry.getValue().stream() | ||
| .sorted() | ||
| .map(FilterRegistration::filter) | ||
| .forEach(filters::add); | ||
|
|
||
| if (matches(pattern, path)) { | ||
| collected.addAll(entry.getValue()); | ||
| } | ||
| } | ||
| return new FilterChainImpl(filters, router); // Pass router instead of plugin | ||
|
|
||
| Collections.sort(collected); | ||
|
|
||
| List<Filter> finalFilters = new ArrayList<>(); | ||
| Set<Filter> seen = new HashSet<>(); | ||
|
|
||
| for (FilterRegistration reg : collected) { | ||
| if (seen.add(reg.filter())) { | ||
| finalFilters.add(reg.filter()); | ||
| } | ||
| } | ||
|
|
||
| return new FilterChainImpl(finalFilters, router); |
There was a problem hiding this comment.
TOCTOU: router field read twice — capture to a local variable.
this.router is checked for null at Line 42 then re-read from the volatile field implicitly at Line 71. A concurrent setRouter(newRouter) call between those two reads would silently attach a different router to the constructed chain. Capture the field once at the top of the method to eliminate the window.
🛡️ Proposed fix
public FilterChainImpl createChain(HttpRequest request) {
- if (router == null) {
+ Router currentRouter = this.router;
+ if (currentRouter == null) {
throw new IllegalStateException("Router not set");
}
String path = request.path();
List<FilterRegistration> collected = new ArrayList<>();
collected.addAll(globalFilters);
for (Map.Entry<String, List<FilterRegistration>> entry : routeFilters.entrySet()) {
String pattern = entry.getKey();
if (matches(pattern, path)) {
collected.addAll(entry.getValue());
}
}
Collections.sort(collected);
List<Filter> finalFilters = new ArrayList<>();
Set<Filter> seen = new HashSet<>();
for (FilterRegistration reg : collected) {
if (seen.add(reg.filter())) {
finalFilters.add(reg.filter());
}
}
- return new FilterChainImpl(finalFilters, router);
+ return new FilterChainImpl(finalFilters, currentRouter);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public FilterChainImpl createChain(HttpRequest request) { | |
| List<Filter> filters = new ArrayList<>(); | |
| filters.addAll(sortedGlobalFilters); | |
| String path = request.path(); | |
| List<FilterRegistration> exactMatches = routeFilters.get(path); | |
| if (exactMatches != null) { | |
| exactMatches.stream() | |
| .sorted() | |
| .map(FilterRegistration::filter) | |
| .forEach(filters::add); | |
| if (router == null) { | |
| throw new IllegalStateException("Router not set"); | |
| } | |
| String path = request.path(); | |
| List<FilterRegistration> collected = new ArrayList<>(); | |
| collected.addAll(globalFilters); | |
| for (Map.Entry<String, List<FilterRegistration>> entry : routeFilters.entrySet()) { | |
| String pattern = entry.getKey(); | |
| if (pattern.endsWith("*") && path.startsWith(pattern.substring(0, pattern.length() - 1))) { | |
| entry.getValue().stream() | |
| .sorted() | |
| .map(FilterRegistration::filter) | |
| .forEach(filters::add); | |
| if (matches(pattern, path)) { | |
| collected.addAll(entry.getValue()); | |
| } | |
| } | |
| return new FilterChainImpl(filters, router); // Pass router instead of plugin | |
| Collections.sort(collected); | |
| List<Filter> finalFilters = new ArrayList<>(); | |
| Set<Filter> seen = new HashSet<>(); | |
| for (FilterRegistration reg : collected) { | |
| if (seen.add(reg.filter())) { | |
| finalFilters.add(reg.filter()); | |
| } | |
| } | |
| return new FilterChainImpl(finalFilters, router); | |
| public FilterChainImpl createChain(HttpRequest request) { | |
| Router currentRouter = this.router; | |
| if (currentRouter == null) { | |
| throw new IllegalStateException("Router not set"); | |
| } | |
| String path = request.path(); | |
| List<FilterRegistration> collected = new ArrayList<>(); | |
| collected.addAll(globalFilters); | |
| for (Map.Entry<String, List<FilterRegistration>> entry : routeFilters.entrySet()) { | |
| String pattern = entry.getKey(); | |
| if (matches(pattern, path)) { | |
| collected.addAll(entry.getValue()); | |
| } | |
| } | |
| Collections.sort(collected); | |
| List<Filter> finalFilters = new ArrayList<>(); | |
| Set<Filter> seen = new HashSet<>(); | |
| for (FilterRegistration reg : collected) { | |
| if (seen.add(reg.filter())) { | |
| finalFilters.add(reg.filter()); | |
| } | |
| } | |
| return new FilterChainImpl(finalFilters, currentRouter); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/juv25d/Pipeline.java` around lines 41 - 71, createChain
currently reads the router field twice (check and later use), opening a TOCTOU
race; fix by reading the router into a local final variable at the top of
createChain (e.g., final Router localRouter = this.router), perform the null
check against localRouter, and pass localRouter into the FilterChainImpl
constructor instead of re-reading the field; ensure all uses of router inside
createChain reference that local variable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/org/juv25d/Server/Server.java (1)
22-25: Lambda body should be indented one level inside the braces.The shutdown hook logic itself is correct (constructor placement addresses the prior issue), but the lambda body is unindented.
✏️ Formatting fix
Runtime.getRuntime().addShutdownHook(new Thread(() -> { - logger.info("Shutting down server..."); - pipeline.destroyFilters(); + logger.info("Shutting down server..."); + pipeline.destroyFilters(); }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Server/Server.java` around lines 22 - 25, The lambda passed to Runtime.getRuntime().addShutdownHook(new Thread(...)) has its body misindented; reformat the lambda in the Server constructor so the statements logger.info("Shutting down server..."); and pipeline.destroyFilters(); are indented one level inside the lambda braces (the block given to new Thread), preserving the same code and semantics in the Runtime.getRuntime().addShutdownHook/new Thread(lambda) expression.src/main/java/org/juv25d/App.java (1)
22-23: Share a singleStaticFilesPlugininstance for/and/*.Both registrations construct separate instances of an apparently stateless plugin. A single instance is sufficient.
♻️ Proposed simplification
+ StaticFilesPlugin staticFilesPlugin = new 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 22 - 23, The code registers two separate StaticFilesPlugin instances via router.registerPlugin("/", new StaticFilesPlugin()) and router.registerPlugin("/*", new StaticFilesPlugin()); instead create a single StaticFilesPlugin instance (e.g., final StaticFilesPlugin staticFilesPlugin = new StaticFilesPlugin()) and pass that same instance to both router.registerPlugin("/", staticFilesPlugin) and router.registerPlugin("/*", staticFilesPlugin") to avoid redundant construction of an apparently stateless plugin.
🤖 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/config/FilterConfiguration.java`:
- Around line 16-20: Replace the hardcoded List<RedirectRule> in
FilterConfiguration with rules loaded from the application's configuration: add
a redirect-rules block in application-properties.yml (entries with
from/to/status), expose those as a List<RedirectRule> via ConfigLoader (or a
getter like getRedirectRules()), and change FilterConfiguration to obtain
redirect rules from ConfigLoader instead of using the inline new
RedirectRule(...) list (remove the placeholder rule to example.com). Ensure
FilterConfiguration uses the provided list (and preserves handling for wildcards
and HTTP status codes) and update any constructor or bean method signatures to
accept ConfigLoader or the injected List<RedirectRule>.
- Line 24: The IpFilter instance registered via addFilter in FilterConfiguration
is created with empty allowList and denyList, making it a no-op and adding
overhead; either remove the addFilter(new IpFilter(Set.of(), Set.of()))
registration entirely from FilterConfiguration or replace it with a properly
configured IpFilter by populating the allowList and/or denyList with the
intended IP rules (or pass a configuration source into the IpFilter constructor)
so that IpFilter's filtering logic is effective.
---
Duplicate comments:
In `@src/main/java/org/juv25d/Server/Server.java`:
- Line 1: The package declaration in Server.java uses an uppercase segment
"org.juv25d.Server" which violates Java package naming conventions; rename the
package declaration to "org.juv25d.server" and move/rename the corresponding
directory from Server to server, update any import statements or references to
Server (e.g., package declaration in Server.java and any other classes that
import org.juv25d.Server) to the new lowercase package, and ensure build/IDE
configuration is refreshed so the new package path is recognized.
- Line 30: The code is creating a plaintext ServerSocket (ServerSocket
serverSocket = new ServerSocket(port, 64)), which must be replaced with an
SSL/TLS server socket; update the Server class to obtain an SSLServerSocket from
SSLServerSocketFactory (configure with a loaded KeyStore/KeyManagerFactory or
existing server SSLContext), replace the new ServerSocket(...) with
SSLServerSocket creation, enable only secure TLS protocols/cipher suites and
require client auth if needed, and ensure proper keystore loading and exception
handling so the server communicates over encrypted channels instead of
plaintext.
---
Nitpick comments:
In `@src/main/java/org/juv25d/App.java`:
- Around line 22-23: The code registers two separate StaticFilesPlugin instances
via router.registerPlugin("/", new StaticFilesPlugin()) and
router.registerPlugin("/*", new StaticFilesPlugin()); instead create a single
StaticFilesPlugin instance (e.g., final StaticFilesPlugin staticFilesPlugin =
new StaticFilesPlugin()) and pass that same instance to both
router.registerPlugin("/", staticFilesPlugin) and router.registerPlugin("/*",
staticFilesPlugin") to avoid redundant construction of an apparently stateless
plugin.
In `@src/main/java/org/juv25d/Server/Server.java`:
- Around line 22-25: The lambda passed to
Runtime.getRuntime().addShutdownHook(new Thread(...)) has its body misindented;
reformat the lambda in the Server constructor so the statements
logger.info("Shutting down server..."); and pipeline.destroyFilters(); are
indented one level inside the lambda braces (the block given to new Thread),
preserving the same code and semantics in the
Runtime.getRuntime().addShutdownHook/new Thread(lambda) expression.
Comment on lines
+16
to
+20
| 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) | ||
| ); |
There was a problem hiding this comment.
Hardcoded redirect rules include a placeholder URL that will redirect real users to example.com.
/temp → https://example.com/temporary (line 18) is clearly a placeholder, but it is wired into the live filter chain. Any request to /temp will issue a 302 to the IANA-reserved example.com domain. The other two rules (/old-page, /docs/*) are also baked in — they cannot be changed without recompiling.
Since ConfigLoader already reads from application-properties.yml, the redirect rules should follow the same pattern rather than being hardcoded here.
♻️ Suggested approach — load rules from ConfigLoader
Add a redirect-rules block to application-properties.yml:
redirect-rules:
- from: "/old-page"
to: "/new-page"
status: 301
- from: "/docs/*"
to: "/documentation/"
status: 301Then expose them via ConfigLoader and consume them in FilterConfiguration:
- 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)
- );
-
- builder
- .addFilter(new RedirectFilter(redirectRules))
+ List<RedirectRule> redirectRules = config.getRedirectRules();
+ if (!redirectRules.isEmpty()) {
+ builder.addFilter(new RedirectFilter(redirectRules));
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/juv25d/config/FilterConfiguration.java` around lines 16 -
20, Replace the hardcoded List<RedirectRule> in FilterConfiguration with rules
loaded from the application's configuration: add a redirect-rules block in
application-properties.yml (entries with from/to/status), expose those as a
List<RedirectRule> via ConfigLoader (or a getter like getRedirectRules()), and
change FilterConfiguration to obtain redirect rules from ConfigLoader instead of
using the inline new RedirectRule(...) list (remove the placeholder rule to
example.com). Ensure FilterConfiguration uses the provided list (and preserves
handling for wildcards and HTTP status codes) and update any constructor or bean
method signatures to accept ConfigLoader or the injected List<RedirectRule>.
|
|
||
| builder | ||
| .addFilter(new RedirectFilter(redirectRules)) | ||
| .addFilter(new IpFilter(Set.of(), Set.of())) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*.java" -type f | xargs grep -l "class IpFilter"Repository: ithsjava25/project-webserver-juv25d
Length of output: 126
🏁 Script executed:
cat -n ./src/main/java/org/juv25d/filter/IpFilter.javaRepository: ithsjava25/project-webserver-juv25d
Length of output: 2106
Remove the unconfigured IpFilter instance or configure it with actual IP rules.
With both allowList and denyList empty, the filter falls through to the default case (line 32) and allows all requests. This makes it a no-op filter that adds processing overhead without providing any filtering benefit. Either configure it with actual whitelist/blacklist rules or remove the registration entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/juv25d/config/FilterConfiguration.java` at line 24, The
IpFilter instance registered via addFilter in FilterConfiguration is created
with empty allowList and denyList, making it a no-op and adding overhead; either
remove the addFilter(new IpFilter(Set.of(), Set.of())) registration entirely
from FilterConfiguration or replace it with a properly configured IpFilter by
populating the allowList and/or denyList with the intended IP rules (or pass a
configuration source into the IpFilter constructor) so that IpFilter's filtering
logic is effective.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/java/org/juv25d/filter/GlobalFilterTests.java (1)
25-29:⚠️ Potential issue | 🟡 MinorPath registration doesn't match the request path — register
"/*"instead of"/"to catch all routes.
SimpleRouter.resolvehas a fallback tonotFoundPluginand never returns null, so there's no NPE risk. However, the registered path"/"(exact match) won't match the request path"/anything". Since"/"doesn't end with"/*", it won't be treated as a wildcard either. The test will execute but withnotFoundPlugininstead of the intendedNoOpPlugin. Change the registration torouter.registerPlugin("/*", new NoOpPlugin())to ensure the catch-all pattern matches all paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/filter/GlobalFilterTests.java` around lines 25 - 29, The test registers NoOpPlugin for path "/" which is an exact match and won't match "/anything", causing SimpleRouter.resolve to return the notFoundPlugin; update the registration in GlobalFilterTests to use the catch-all pattern by calling SimpleRouter.registerPlugin with "/*" and NoOpPlugin so the request path "/anything" matches; ensure the change is applied where router is created and used in the execute(pipeline, "/anything") test invocation.src/main/java/org/juv25d/Connections/DefaultConnectionHandlerFactory.java (1)
9-23:⚠️ Potential issue | 🟡 Minor
DefaultConnectionHandlerFactorystores aPipelinefield that is never read.The constructor accepts and stores
pipeline(line 14-17), butcreate()(line 21) uses thepipelineparameter from the interface method instead. The field is dead state. Either remove the field (and the constructor parameter) or change the interface socreatedoesn't accept aPipelineand uses the stored instance.♻️ Option A — remove the unused field
public class DefaultConnectionHandlerFactory implements ConnectionHandlerFactory { private final HttpParser httpParser; private final Logger logger; - private final Pipeline pipeline; - public DefaultConnectionHandlerFactory(HttpParser httpParser, Logger logger, Pipeline pipeline) { + public DefaultConnectionHandlerFactory(HttpParser httpParser, Logger logger) { this.httpParser = httpParser; this.logger = logger; - this.pipeline = pipeline; } `@Override` public Runnable create(Socket socket, Pipeline pipeline) { return new ConnectionHandler(socket, httpParser, logger, pipeline); } }♻️ Option B — use the stored field, simplify the interface
// ConnectionHandlerFactory.java public interface ConnectionHandlerFactory { - Runnable create(Socket socket, Pipeline pipeline); + Runnable create(Socket socket); } // DefaultConnectionHandlerFactory.java `@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); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Connections/DefaultConnectionHandlerFactory.java` around lines 9 - 23, The stored Pipeline field in DefaultConnectionHandlerFactory is unused; remove the private final Pipeline pipeline field and the Pipeline parameter from the constructor (adjust the constructor signature and assignments accordingly) so the factory only keeps HttpParser and Logger, and keep the create(Socket socket, Pipeline pipeline) method as-is to pass the pipeline parameter into new ConnectionHandler(socket, httpParser, logger, pipeline).
🧹 Nitpick comments (7)
src/main/java/org/juv25d/Server/Pipeline.java (2)
20-28: No null-guard onfilter/patternparameters inaddGlobalFilterandaddRouteFilter.Passing a
nullfilter silently registers it and will throw an NPE later during chain execution or lifecycle calls. Fail-fast with a null check, similar tosetRouter.🛡️ Proposed fix (addGlobalFilter example)
public void addGlobalFilter(Filter filter, int order) { + Objects.requireNonNull(filter, "Filter must not be null"); globalFilters.add(new FilterRegistration(filter, order, null)); } public void addRouteFilter(Filter filter, int order, String pattern) { + Objects.requireNonNull(filter, "Filter must not be null"); + Objects.requireNonNull(pattern, "Pattern must not be null"); routeFilters .computeIfAbsent(pattern, k -> new CopyOnWriteArrayList<>()) .add(new FilterRegistration(filter, order, pattern)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Server/Pipeline.java` around lines 20 - 28, add explicit null-guards for the parameters in addGlobalFilter and addRouteFilter to fail fast: check that the Filter argument is non-null in both methods (and that pattern is non-null in addRouteFilter) and throw the same kind of exception used by setRouter (e.g., IllegalArgumentException or NullPointerException with a clear message) instead of registering a null; this will prevent later NPEs during filter execution and matches existing validation style.
48-69: Duplicated sort-and-deduplicate logic betweencreateChainandgetAllFilters.The "collect registrations → sort → deduplicate into
List<Filter>" block is copy-pasted in both methods. Extract a shared helper to reduce duplication and keep the two paths consistent.♻️ Sketch
+ private List<Filter> dedup(List<FilterRegistration> registrations) { + Collections.sort(registrations); + List<Filter> result = new ArrayList<>(); + Set<Filter> seen = new HashSet<>(); + for (FilterRegistration reg : registrations) { + if (seen.add(reg.filter())) { + result.add(reg.filter()); + } + } + return result; + }Also applies to: 105-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Server/Pipeline.java` around lines 48 - 69, The duplicated "collect → sort → deduplicate" logic in createChain and getAllFilters should be extracted into a single helper method (e.g., buildFilterListFromRegistrations(Collection<FilterRegistration> regs, String path) or similar) that encapsulates the existing steps: gather applicable FilterRegistration objects (using matches where needed), sort the registrations (preserving the current sort via Collections.sort(collected)), then deduplicate into a List<Filter> by iterating registrations and adding reg.filter() only if not already seen (using a Set<Filter> seen). Replace the copy-pasted blocks in createChain and getAllFilters with calls to this new helper (keep using FilterRegistration, Filter, matches, and the existing sorting behavior so semantics remain identical).src/main/java/org/juv25d/Server/ServerBuilder.java (2)
36-39:addFilterrequires the filter to be annotated with@Globalor@Route— document this contract.
FilterScanner.registerthrowsIllegalStateExceptionif the filter lacks the required annotation. This implicit requirement may surprise callers. A Javadoc note onaddFilterwould clarify the expectation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Server/ServerBuilder.java` around lines 36 - 39, Document that ServerBuilder.addFilter(Filter filter) requires the provided Filter to be annotated with either `@Global` or `@Route` because FilterScanner.register(filter, pipeline) will throw IllegalStateException if those annotations are missing; update the Javadoc on addFilter to state this contract, mention the specific annotations (`@Global`, `@Route`), and note that callers should ensure their Filter implementations carry one of these annotations to avoid the IllegalStateException thrown by FilterScanner.register.
21-23: No port-range validation — invalid values will fail late atServerSocketconstruction.A negative or out-of-range port will only fail when
new ServerSocket(port, 64)is called duringstart(). Fail-fast in the builder for better developer experience.🛡️ Proposed fix
public ServerBuilder port(int port) { + if (port < 0 || port > 65535) { + throw new IllegalArgumentException("Port must be between 0 and 65535"); + } this.port = port; return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Server/ServerBuilder.java` around lines 21 - 23, Add fail-fast validation in ServerBuilder.port(int): check that the provided port is within valid TCP/UDP range 0–65535 and throw an IllegalArgumentException with a clear message if it's out of range; keep returning this on success. Update the ServerBuilder.port method (and any callers if needed) so invalid values are rejected immediately rather than letting new ServerSocket(port, 64) in start() fail later.src/main/java/org/juv25d/Server/Server.java (1)
21-24: Shutdown hook indentation is inconsistent with the enclosing lambda.The lambda body should be indented one level deeper than the
Runtime.getRuntime()call for readability.♻️ Proposed fix
Runtime.getRuntime().addShutdownHook(new Thread(() -> { - logger.info("Shutting down server..."); - pipeline.destroyFilters(); + logger.info("Shutting down server..."); + pipeline.destroyFilters(); }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Server/Server.java` around lines 21 - 24, The shutdown hook lambda passed to Runtime.getRuntime().addShutdownHook has inconsistent indentation; reformat the lambda body so its statements (logger.info("Shutting down server..."); and pipeline.destroyFilters();) are indented one level deeper than the Runtime.getRuntime() call for readability. Locate the Runtime.getRuntime().addShutdownHook(...) expression and adjust the indentation of the inner lambda block containing logger.info and pipeline.destroyFilters() to match the enclosing style.src/main/java/org/juv25d/filter/config/FilterConfiguration.java (1)
22-25:IpFilteris placed afterRedirectFilter— consider security-first ordering.With the current order, a request from a blocked IP first hits redirect logic before the IP check can reject it. Putting
IpFilterfirst short-circuits all further processing for blocked clients.♻️ Proposed ordering
builder - .addFilter(new RedirectFilter(redirectRules)) .addFilter(new IpFilter(Set.of(), Set.of())) + .addFilter(new RedirectFilter(redirectRules)) .addFilter(new LoggingFilter())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/config/FilterConfiguration.java` around lines 22 - 25, The filter ordering is insecure because RedirectFilter is added before IpFilter; move the IP-based access control earlier so blocked clients are rejected before any redirect logic runs. Update the builder sequence where addFilter is called (the calls constructing RedirectFilter, IpFilter, LoggingFilter) to add new IpFilter(Set.of(), Set.of()) before new RedirectFilter(redirectRules) while keeping LoggingFilter last so IpFilter short-circuits processing for blocked IPs.src/test/java/org/juv25d/filter/FilterScannerTest.java (1)
44-50: Add a null-input guard test to document expected behavior.
FilterScanner.register()will throw anNPEif called withnullas the first argument (line 10:instance.getClass()). The current test only covers a non-FilterObject, not null input. Adding a test would lock down the expected behavior—either thatnullis silently ignored or that an exception is explicitly expected.✅ Suggested test
`@Test` void shouldHandleNullInput() { Pipeline pipeline = mock(Pipeline.class); assertThrows(NullPointerException.class, () -> FilterScanner.register(null, pipeline)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/filter/FilterScannerTest.java` around lines 44 - 50, The test suite is missing coverage for null input to FilterScanner.register which currently throws a NullPointerException at instance.getClass(); add a unit test in FilterScannerTest (e.g., shouldHandleNullInput) that creates a mocked Pipeline and asserts that calling FilterScanner.register(null, pipeline) throws NullPointerException so the expected behavior is documented by tests.
🤖 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/Server/Pipeline.java`:
- Around line 87-89: initFilters() currently calls
getAllFilters().forEach(Filter::init) without per-filter error handling, so if
any Filter.init throws the whole initialization stops; change it to iterate over
getAllFilters() and call init() for each filter inside a try/catch, mirroring
destroyFilters() behavior: catch exceptions from filter.init(), log the error
(using the same logger/mechanism used in destroyFilters()) with context
including the filter identity, and continue to initialize the remaining filters
so one faulty filter cannot prevent others from initializing.
---
Outside diff comments:
In `@src/main/java/org/juv25d/Connections/DefaultConnectionHandlerFactory.java`:
- Around line 9-23: The stored Pipeline field in DefaultConnectionHandlerFactory
is unused; remove the private final Pipeline pipeline field and the Pipeline
parameter from the constructor (adjust the constructor signature and assignments
accordingly) so the factory only keeps HttpParser and Logger, and keep the
create(Socket socket, Pipeline pipeline) method as-is to pass the pipeline
parameter into new ConnectionHandler(socket, httpParser, logger, pipeline).
In `@src/test/java/org/juv25d/filter/GlobalFilterTests.java`:
- Around line 25-29: The test registers NoOpPlugin for path "/" which is an
exact match and won't match "/anything", causing SimpleRouter.resolve to return
the notFoundPlugin; update the registration in GlobalFilterTests to use the
catch-all pattern by calling SimpleRouter.registerPlugin with "/*" and
NoOpPlugin so the request path "/anything" matches; ensure the change is applied
where router is created and used in the execute(pipeline, "/anything") test
invocation.
---
Duplicate comments:
In `@src/main/java/org/juv25d/Connections/ConnectionHandlerFactory.java`:
- Around line 1-3: The package declaration uses mixed case; change the package
line in ConnectionHandlerFactory from "package org.juv25d.Connections;" to the
lowercase "package org.juv25d.connections;" and update any imports referencing
the server package (e.g., change "import org.juv25d.Server.Pipeline;" to "import
org.juv25d.server.Pipeline;"); also rename the source directory to match the new
lowercase package (move files into org/juv25d/connections) and update any other
references to ConnectionHandlerFactory or Pipeline across the codebase to use
the lowercase package names.
In `@src/main/java/org/juv25d/filter/config/FilterConfiguration.java`:
- Line 24: The IpFilter is being instantiated with empty allowList/denyList
(Set.of()) in FilterConfiguration which makes it a no-op and wastes processing
time; either remove the .addFilter(new IpFilter(Set.of(), Set.of()))
registration from FilterConfiguration or replace the empty sets with the
intended configured IP rules (e.g., inject or read allow/deny collections and
pass them into new IpFilter(allowList, denyList)), ensuring the IpFilter
constructor receives meaningful data instead of Set.of().
- Around line 16-20: The FilterConfiguration currently contains hardcoded
RedirectRule entries (redirectRules and new RedirectRule("/temp", ...)); replace
this static list by loading redirect rules from the central configuration source
(e.g., ConfigLoader or application-properties.yml). Modify FilterConfiguration
to call the existing ConfigLoader (or bind a properties class) to obtain the
list of RedirectRule objects and populate redirectRules from that source instead
of using new RedirectRule(...) literals, and remove the placeholder entries
("/temp", "/old-page", "/docs/*") so rules come solely from configuration.
In `@src/main/java/org/juv25d/Server/Pipeline.java`:
- Around line 41-71: The createChain method reads this.router twice causing a
TOCTOU race; capture the router into a local variable (e.g., Router routerLocal
= this.router) before the null check, validate routerLocal for null, and then
use routerLocal when constructing and returning the new FilterChainImpl; update
all references to router in createChain (including the final FilterChainImpl
constructor call) to use the captured local variable and leave setRouter
unchanged.
In `@src/main/java/org/juv25d/Server/ServerBuilder.java`:
- Around line 48-64: build() is not idempotent because pipeline.initFilters() is
called every time; add a one-time guard in ServerBuilder (e.g., a private
boolean filtersInitialized or an AtomicBoolean) and update build() to check the
guard before calling pipeline.initFilters(), setting the guard true immediately
after the first successful init; reference the existing build(), pipeline, and
pipeline.initFilters() to locate where to add the guard so subsequent calls to
build() skip re-initializing filters.
---
Nitpick comments:
In `@src/main/java/org/juv25d/filter/config/FilterConfiguration.java`:
- Around line 22-25: The filter ordering is insecure because RedirectFilter is
added before IpFilter; move the IP-based access control earlier so blocked
clients are rejected before any redirect logic runs. Update the builder sequence
where addFilter is called (the calls constructing RedirectFilter, IpFilter,
LoggingFilter) to add new IpFilter(Set.of(), Set.of()) before new
RedirectFilter(redirectRules) while keeping LoggingFilter last so IpFilter
short-circuits processing for blocked IPs.
In `@src/main/java/org/juv25d/Server/Pipeline.java`:
- Around line 20-28: add explicit null-guards for the parameters in
addGlobalFilter and addRouteFilter to fail fast: check that the Filter argument
is non-null in both methods (and that pattern is non-null in addRouteFilter) and
throw the same kind of exception used by setRouter (e.g.,
IllegalArgumentException or NullPointerException with a clear message) instead
of registering a null; this will prevent later NPEs during filter execution and
matches existing validation style.
- Around line 48-69: The duplicated "collect → sort → deduplicate" logic in
createChain and getAllFilters should be extracted into a single helper method
(e.g., buildFilterListFromRegistrations(Collection<FilterRegistration> regs,
String path) or similar) that encapsulates the existing steps: gather applicable
FilterRegistration objects (using matches where needed), sort the registrations
(preserving the current sort via Collections.sort(collected)), then deduplicate
into a List<Filter> by iterating registrations and adding reg.filter() only if
not already seen (using a Set<Filter> seen). Replace the copy-pasted blocks in
createChain and getAllFilters with calls to this new helper (keep using
FilterRegistration, Filter, matches, and the existing sorting behavior so
semantics remain identical).
In `@src/main/java/org/juv25d/Server/Server.java`:
- Around line 21-24: The shutdown hook lambda passed to
Runtime.getRuntime().addShutdownHook has inconsistent indentation; reformat the
lambda body so its statements (logger.info("Shutting down server..."); and
pipeline.destroyFilters();) are indented one level deeper than the
Runtime.getRuntime() call for readability. Locate the
Runtime.getRuntime().addShutdownHook(...) expression and adjust the indentation
of the inner lambda block containing logger.info and pipeline.destroyFilters()
to match the enclosing style.
In `@src/main/java/org/juv25d/Server/ServerBuilder.java`:
- Around line 36-39: Document that ServerBuilder.addFilter(Filter filter)
requires the provided Filter to be annotated with either `@Global` or `@Route`
because FilterScanner.register(filter, pipeline) will throw
IllegalStateException if those annotations are missing; update the Javadoc on
addFilter to state this contract, mention the specific annotations (`@Global`,
`@Route`), and note that callers should ensure their Filter implementations carry
one of these annotations to avoid the IllegalStateException thrown by
FilterScanner.register.
- Around line 21-23: Add fail-fast validation in ServerBuilder.port(int): check
that the provided port is within valid TCP/UDP range 0–65535 and throw an
IllegalArgumentException with a clear message if it's out of range; keep
returning this on success. Update the ServerBuilder.port method (and any callers
if needed) so invalid values are rejected immediately rather than letting new
ServerSocket(port, 64) in start() fail later.
In `@src/test/java/org/juv25d/filter/FilterScannerTest.java`:
- Around line 44-50: The test suite is missing coverage for null input to
FilterScanner.register which currently throws a NullPointerException at
instance.getClass(); add a unit test in FilterScannerTest (e.g.,
shouldHandleNullInput) that creates a mocked Pipeline and asserts that calling
FilterScanner.register(null, pipeline) throws NullPointerException so the
expected behavior is documented by tests.
Comment on lines
+87
to
+89
| public void initFilters() { | ||
| getAllFilters().forEach(Filter::init); | ||
| } |
There was a problem hiding this comment.
initFilters() lacks per-filter error handling, unlike destroyFilters().
If any filter's init() throws, subsequent filters are never initialized, leaving the pipeline in a partially initialized state. destroyFilters() already wraps each call in a try/catch — apply the same pattern here so that one faulty filter doesn't prevent the rest from initializing.
🛡️ Proposed fix
public void initFilters() {
- getAllFilters().forEach(Filter::init);
+ for (Filter filter : getAllFilters()) {
+ try {
+ filter.init();
+ } catch (Exception e) {
+ System.err.println(
+ "Error initializing filter " +
+ filter.getClass().getName() +
+ ": " + e.getMessage()
+ );
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void initFilters() { | |
| getAllFilters().forEach(Filter::init); | |
| } | |
| public void initFilters() { | |
| for (Filter filter : getAllFilters()) { | |
| try { | |
| filter.init(); | |
| } catch (Exception e) { | |
| System.err.println( | |
| "Error initializing filter " + | |
| filter.getClass().getName() + | |
| ": " + e.getMessage() | |
| ); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/juv25d/Server/Pipeline.java` around lines 87 - 89,
initFilters() currently calls getAllFilters().forEach(Filter::init) without
per-filter error handling, so if any Filter.init throws the whole initialization
stops; change it to iterate over getAllFilters() and call init() for each filter
inside a try/catch, mirroring destroyFilters() behavior: catch exceptions from
filter.init(), log the error (using the same logger/mechanism used in
destroyFilters()) with context including the filter identity, and continue to
initialize the remaining filters so one faulty filter cannot prevent others from
initializing.
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
1 participant
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.
Closing this PR since it became messy due to multiple auto-generated fixes.
I will open a new, clean PR with the same changes.
Refactors the server bootstrap and request processing architecture.
Introduces annotation-based filter registration (@global, @route), ordered filter execution, and enforced filter lifecycle management.
Moves HttpParser, DefaultConnectionHandlerFactory, and pipeline initialization into ServerBuilder, establishing a clear composition root and improving separation of concerns.
Adds FilterConfiguration and FilterScanner to centralize filter wiring and simplify application bootstrap.
Includes unit tests.
Summary by CodeRabbit
Release Notes