Feature/core di architecture - #114
Merged
Merged
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/java/org/juv25d/filter/ClassScanner.java (1)
37-48: Non-file resource warning correctly added (past issue addressed).Both previously flagged issues are now resolved in this version: the context classloader is threaded through
scanDir, and non-file resources emit a warning instead of silently returning empty.One minor style nit: the
java.util.logging.Loggeris fully-qualified inline. Consider adding it to the imports at the top of the file.♻️ Move Logger to imports
import java.io.File; import java.net.URL; import java.util.*; +import java.util.logging.Logger;- java.util.logging.Logger.getLogger(ClassScanner.class.getName()) + Logger.getLogger(ClassScanner.class.getName())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/ClassScanner.java` around lines 37 - 48, Move the fully-qualified java.util.logging.Logger usage into imports and use the short name in the else branch: add "import java.util.logging.Logger;" at the top of the ClassScanner file and replace "java.util.logging.Logger.getLogger(ClassScanner.class.getName())" with "Logger.getLogger(ClassScanner.class.getName())" in the code that logs skipped non-file resources (the else block that references resource.getProtocol(), resource and calls scanDir/basePackage/classes/cl).src/main/java/org/juv25d/di/Container.java (1)
108-108:createcould return<T>instead ofObjectto eliminate unchecked casts at call sites.Every caller (e.g., line 186:
(T) create(type), andFilterFactory) must cast the return value. Making the signature generic removes those casts.♻️ Proposed refactor
- public Object create(Class<?> clazz) { + `@SuppressWarnings`("unchecked") + public <T> T create(Class<T> clazz) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/di/Container.java` at line 108, The create method currently returns Object causing unchecked casts at call sites (e.g., callers doing (T) create(type) and usages in FilterFactory); change the signature to a generic form — declare create as <T> T create(Class<T> clazz) — and update any internal references to use Class<T> and return T so callers no longer need unchecked casts; adjust any places that relied on raw Class<?> to supply the proper generic Class<T> where needed.
🤖 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/di/Container.java`:
- Around line 284-319: canResolve currently lets RuntimeExceptions from
findBestConstructor bubble up, which aborts constructor selection; wrap the call
to findBestConstructor(type) inside canResolve in a try-catch that catches
RuntimeException (or Exception) and returns false on error so failed nested
resolution does not propagate; this change in canResolve ensures canResolveAll
and the Stream.filter used in findBestConstructor continue evaluating other
constructors instead of aborting.
- Around line 144-152: The resolve method (and similarly canResolve) only
follows one-level bindings so chained mappings (e.g., bind(A,B); bind(B,C))
break; change the single-step if that checks bindings to a loop that follows
bindings until a non-bound type is reached: use a while
(bindings.containsKey(type)) { type = (Class<T>) bindings.get(type); } and
maintain a seen-set to detect cycles and throw a clear RuntimeException on cycle
detection; update both resolve and canResolve (and any helper used by
isConstructable checks) to use this chained-resolution approach so
isConstructable is evaluated on the final concrete type.
In `@src/main/java/org/juv25d/filter/ClassScanner.java`:
- Line 64: The Javadoc for ClassScanner.scanDir incorrectly declares "@throws
ClassNotFoundException" even though scanDir catches ClassNotFoundException
internally (see catch at around the scanDir class-loading block). Remove the
misleading `@throws` tag from the scanDir Javadoc (or replace it with a note that
ClassNotFoundException is handled internally) so the doc accurately reflects
that scanDir does not propagate that exception.
---
Duplicate comments:
In `@src/main/java/org/juv25d/di/Container.java`:
- Line 182: The package-boundary check using
type.getPackageName().startsWith(basePackage) in Container must be tightened to
avoid matching sibling packages; update both occurrences (the if that currently
checks type.getPackageName().startsWith(basePackage) and the check inside
canResolve) to accept packages equal to basePackage or that start with
basePackage + "." instead of plain startsWith; i.e., replace the loose
startsWith(basePackage) with a boundary-aware test (equals(basePackage) ||
startsWith(basePackage + ".")) so only classes in the package or its subpackages
are allowed.
---
Nitpick comments:
In `@src/main/java/org/juv25d/di/Container.java`:
- Line 108: The create method currently returns Object causing unchecked casts
at call sites (e.g., callers doing (T) create(type) and usages in
FilterFactory); change the signature to a generic form — declare create as <T> T
create(Class<T> clazz) — and update any internal references to use Class<T> and
return T so callers no longer need unchecked casts; adjust any places that
relied on raw Class<?> to supply the proper generic Class<T> where needed.
In `@src/main/java/org/juv25d/filter/ClassScanner.java`:
- Around line 37-48: Move the fully-qualified java.util.logging.Logger usage
into imports and use the short name in the else branch: add "import
java.util.logging.Logger;" at the top of the ClassScanner file and replace
"java.util.logging.Logger.getLogger(ClassScanner.class.getName())" with
"Logger.getLogger(ClassScanner.class.getName())" in the code that logs skipped
non-file resources (the else block that references resource.getProtocol(),
resource and calls scanDir/basePackage/classes/cl).
This reverts commit d6d096f.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/juv25d/filter/RateLimitingFilter.java (1)
94-94: Expand the single-line guard for readability.The logic is correct, but cramming the condition, body, and return onto one line makes it harder to scan and set a breakpoint on.
♻️ Proposed style fix
- if (!enabled) {chain.doFilter(req, res);return;} + if (!enabled) { + chain.doFilter(req, res); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java` at line 94, The single-line guard in RateLimitingFilter that currently reads "if (!enabled) {chain.doFilter(req, res);return;}" should be expanded into a multi-line if block inside the doFilter method of class RateLimitingFilter: put the condition, the chain.doFilter(req, res) call, and the return statement each on their own lines and wrap the body with braces so it's easy to read and to set breakpoints.src/main/java/org/juv25d/di/Container.java (1)
224-224: Rename parameterfilterClasstoclazzortargetClass.The method is fully general-purpose; the
filterClassname leaks its filter-specific origin and misleads readers of this container-level method.♻️ Proposed rename
- private Constructor<?> findBestConstructor(Class<?> filterClass) { - Constructor<?>[] constructors = filterClass.getDeclaredConstructors(); + private Constructor<?> findBestConstructor(Class<?> targetClass) { + Constructor<?>[] constructors = targetClass.getDeclaredConstructors(); // Prefer `@Inject` List<Constructor<?>> injectCtors = Arrays.stream(constructors) .filter(c -> c.isAnnotationPresent(Inject.class)) .toList(); if (injectCtors.size() > 1) { - throw new RuntimeException("Multiple `@Inject` constructors in " + filterClass.getName()); + throw new RuntimeException("Multiple `@Inject` constructors in " + targetClass.getName()); } ... .orElseThrow(() -> - new RuntimeException("No suitable constructor found for " + filterClass.getName()) + new RuntimeException("No suitable constructor found for " + targetClass.getName()) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/di/Container.java` at line 224, Rename the parameter filterClass in the method findBestConstructor to a more general name such as clazz or targetClass to avoid leaking a filter-specific term into a container-level utility; update the method signature (findBestConstructor(Class<?> clazz)) and all internal references and any call sites that pass or reference filterClass to use the new name so the method intent is clearer and compilation remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/juv25d/di/Container.java`:
- Line 186: The package check in Container (the if using
type.getPackageName().startsWith(basePackage)) can match similarly named
packages; change it to enforce a package boundary by checking either equality or
that the package starts with basePackage + '.' (i.e.,
packageName.equals(basePackage) || packageName.startsWith(basePackage + "."));
apply the same fix to the analogous check in canResolve at the other occurrence
so both checks only permit true subpackages of basePackage.
- Around line 148-156: The current one-hop binding lookup in resolve (and
similarly in canResolve) only replaces type once via bindings.get(type) causing
chained bindings like bind(A,B); bind(B,C) to stop early; update the logic in
the resolve method (and canResolve) to follow the bindings map repeatedly until
you reach a concrete class (type not present as a key) or detect a cycle: loop
while bindings.containsKey(type) { type = (Class) bindings.get(type); } but
track visited keys to throw a clear error on circular bindings; ensure the final
resolved type is used for construction/instantiation.
---
Nitpick comments:
In `@src/main/java/org/juv25d/di/Container.java`:
- Line 224: Rename the parameter filterClass in the method findBestConstructor
to a more general name such as clazz or targetClass to avoid leaking a
filter-specific term into a container-level utility; update the method signature
(findBestConstructor(Class<?> clazz)) and all internal references and any call
sites that pass or reference filterClass to use the new name so the method
intent is clearer and compilation remains consistent.
In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java`:
- Line 94: The single-line guard in RateLimitingFilter that currently reads "if
(!enabled) {chain.doFilter(req, res);return;}" should be expanded into a
multi-line if block inside the doFilter method of class RateLimitingFilter: put
the condition, the chain.doFilter(req, res) call, and the return statement each
on their own lines and wrap the body with braces so it's easy to read and to set
breakpoints.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors startup to use a lightweight DI container and Bootstrap.init; adds Container, FilterRegistry/FilterScanner/FilterMatcher, FilterFactory, config classes, DI-based RouterConfig, and test coverage for Container and FilterRegistry. Pipeline now uses FilterMatcher and Router injected from the container. Changes
Sequence Diagram(s)sequenceDiagram
participant App as "App"
participant Container as "Container"
participant Bootstrap as "Bootstrap"
participant ClassScanner as "ClassScanner"
participant FilterScanner as "FilterScanner"
participant Registry as "FilterRegistry"
participant Factory as "FilterFactory"
participant Router as "Router"
participant Pipeline as "Pipeline"
App->>Container: create() & bind(Router → SimpleRouter)
App->>Bootstrap: Bootstrap.init(container, "org.juv25d")
Bootstrap->>Registry: new FilterRegistry()
Bootstrap->>Factory: new FilterFactory(container)
Bootstrap->>ClassScanner: findClasses("org.juv25d")
ClassScanner-->>FilterScanner: discovered classes
Bootstrap->>FilterScanner: scan(basePackage, Registry, Factory)
FilterScanner->>Factory: factory.create(filterClass)
Factory->>Container: get(filterClass)
Container-->>Factory: filter instance
FilterScanner->>Registry: registerGlobal/registerRoute(...)
Bootstrap->>Container: get(Router.class)
Container-->>Bootstrap: Router (RouterConfig runs)
Bootstrap->>Pipeline: new Pipeline(new FilterMatcher(Registry), Router)
Bootstrap-->>App: Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related issues
Possibly related PRs
Suggested labels
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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
Author
|
Would be nice if someone reviewed this ASAP so I can stop solving 1000 conflicts and growing this PR even more by adding more code to it xD |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
src/main/java/org/juv25d/filter/FilterRegistry.java (1)
52-53: Use aLoggerinstead ofSystem.errfor destruction errors.
System.err.printlnbypasses the structured logging infrastructure used everywhere else (ServerLogging), making these errors invisible in log aggregators and inconsistent in format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/FilterRegistry.java` around lines 52 - 53, Replace the System.err.println call in FilterRegistry (the destruction error block that prints "Error destroying filter " + filter.getClass().getName() + ": " + e.getMessage()) with the project's structured logger: obtain the ServerLogging logger for FilterRegistry and call the appropriate error method, passing a clear message that includes filter.getClass().getName() and the exception e as the throwable so the stacktrace and structured metadata are recorded (e.g., ServerLogging.getLogger(FilterRegistry.class).error("Error destroying filter {}", filter.getClass().getName(), e)).src/main/java/org/juv25d/filter/RedirectFilter.java (1)
47-50: Eliminate constructor duplication via constructor chaining.The no-arg constructor duplicates the
loggerassignment. Delegating to the existing constructor keeps initialization in one place.♻️ Proposed refactor
public RedirectFilter() { - this.rules = new RedirectConfig().rules(); - this.logger = Logger.getLogger(RedirectFilter.class.getName()); + this(new RedirectConfig().rules()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/RedirectFilter.java` around lines 47 - 50, The no-arg RedirectFilter constructor duplicates logger initialization; refactor it to delegate to the existing constructor by calling this(...) with the results of new RedirectConfig().rules() and Logger.getLogger(RedirectFilter.class.getName()) so all initialization lives in one constructor (remove the duplicated this.rules/this.logger assignments in the no-arg constructor).src/main/java/org/juv25d/filter/RateLimitingFilter.java (1)
94-94: Expand the early-return guard to separate statements.All three statements on one line is harder to read and step through in a debugger.
♻️ Proposed refactor
- if (!enabled) {chain.doFilter(req, res);return;} + if (!enabled) { + chain.doFilter(req, res); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java` at line 94, The single-line guard in RateLimitingFilter#doFilter is hard to read and debug; split it into separate statements so the condition, the call to chain.doFilter(req, res), and the return are on their own lines. Locate the if (!enabled) {chain.doFilter(req, res);return;} statement in the doFilter method and refactor it to a multi-line form with the boolean check, the chain.doFilter invocation, and the return each on its own line for readability and easier debugging.src/main/java/org/juv25d/filter/FilterFactory.java (1)
13-15: UnsafeClass<?>parameter and unchecked cast — useClass<? extends Filter>instead.With
Class<?>the compiler cannot enforce thatclazzis aFiltersubtype, and the explicit(Filter)cast can throwClassCastExceptionat runtime with a misuse. Narrowing the parameter toClass<? extends Filter>enforces the contract at the call site and eliminates the need for the cast.♻️ Proposed fix
- public Filter create(Class<?> clazz) { - return (Filter) container.get(clazz); + public Filter create(Class<? extends Filter> clazz) { + return container.get(clazz); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/FilterFactory.java` around lines 13 - 15, Change the create method signature to accept a bounded type parameter so callers must pass a Filter subtype: update FilterFactory.create to take Class<? extends Filter> (instead of Class<?>), call container.get(clazz) with that parameter, and return the result as a Filter without an unchecked cast; this removes the explicit (Filter) cast and enforces the contract at compile time while keeping the use of container.get intact.src/main/java/org/juv25d/di/Inject.java (1)
5-7: Consider adding@Documentedfor Javadoc visibility.The standard
javax.inject.Injectannotation is defined as@Documented, meaning it appears in the Javadoc of annotated constructors. Without it, consumers of this annotation won't see it in generated API docs.♻️ Proposed addition
import java.lang.annotation.*; +@Documented `@Retention`(RetentionPolicy.RUNTIME) `@Target`(ElementType.CONSTRUCTOR) public `@interface` Inject { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/di/Inject.java` around lines 5 - 7, The Inject annotation is missing `@Documented` so it won't appear in generated Javadoc; update the Inject declaration (annotation type Inject) to be annotated with `@Documented` and add the required import (java.lang.annotation.Documented) so the annotation is included in API docs and visible on annotated constructors.src/main/java/org/juv25d/App.java (1)
19-23: Extract the repeated"org.juv25d"base-package string to a named constant.The same literal appears in both the
Containerconstructor andBootstrap.init(). A typo or inconsistency between the two would be silently ignored (container and scanner would scan different packages).♻️ Proposed refactor
+ private static final String BASE_PACKAGE = "org.juv25d"; + Container container = new Container("org.juv25d"); + Container container = new Container(BASE_PACKAGE); container.bind(org.juv25d.router.Router.class, SimpleRouter.class); - Pipeline pipeline = Bootstrap.init(container, "org.juv25d"); + Pipeline pipeline = Bootstrap.init(container, BASE_PACKAGE);🤖 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 19 - 23, Extract the repeated package literal into a named constant in App: define a private static final String BASE_PACKAGE = "org.juv25d" and replace the two uses of the literal in the Container constructor call (new Container("org.juv25d")) and the Bootstrap.init call (Bootstrap.init(container, "org.juv25d")) with BASE_PACKAGE so both the container and bootstrap use the same constant.src/main/java/org/juv25d/router/RouterConfig.java (1)
16-16: ReplaceSystem.out.printlnwith the application logger.The rest of the application uses
ServerLogging.getLogger(). UsingSystem.out.printlnhere bypasses log-level control and structured output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/router/RouterConfig.java` at line 16, Replace the System.out.println call in RouterConfig with the application logger: obtain the logger via ServerLogging.getLogger() and log the same message at the appropriate level (e.g., info) instead of printing to stdout; update the RouterConfig location where System.out.println("Router configured") appears to call logger.info("Router configured") (or logger.debug/info per context) so log-level control and structured logging are used.src/main/java/org/juv25d/Bootstrap.java (1)
34-34: Side-effect-onlycontainer.get(RouterConfig.class)— add a clarifying comment.Line 34 retrieves
RouterConfigpurely for its constructor side-effects (presumably configuring routes on theRouter). The discarded return value makes this look accidental. A brief comment would prevent future readers from deleting it as dead code.Proposed fix
Router router = container.get(Router.class); - container.get(org.juv25d.router.RouterConfig.class); + // Eagerly resolve RouterConfig so its constructor registers routes on the Router + container.get(org.juv25d.router.RouterConfig.class);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/Bootstrap.java` at line 34, The call container.get(org.juv25d.router.RouterConfig.class) is used only for constructor side-effects and its return is discarded; add a concise inline comment next to that call (or above it) explaining that RouterConfig is being instantiated to trigger route registration/other side-effects (so it must not be removed as dead code), referencing RouterConfig and the container.get(...) call so future maintainers understand the intent.src/main/java/org/juv25d/filter/FilterScanner.java (1)
54-55: UseLoggerinstead ofSystem.err.printlnfor consistency.
Bootstrap.javausesjava.util.logging.LoggerandClassScannerusesLogger.getLogger(...). This class writes toSystem.errdirectly, which bypasses log-level configuration, formatting, and log aggregation.Proposed fix
+import java.util.logging.Logger; + public class FilterScanner { + private static final Logger logger = Logger.getLogger(FilterScanner.class.getName()); // ... - System.err.println("Filter " + filterClass.getName() - + " has both `@Global` and `@Route` — skipping. Use only one."); + logger.warning("Filter " + filterClass.getName() + + " has both `@Global` and `@Route` — skipping. Use only one."); // ... - System.err.println("Skipping filter: " - + filterClass.getName() + " due to " + e.getMessage()); + logger.warning("Skipping filter: " + + filterClass.getName() + " due to " + e.getMessage());Also applies to: 89-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/FilterScanner.java` around lines 54 - 55, Replace the System.err.println calls in FilterScanner with java.util.logging.Logger-based logging: add a private static final Logger (e.g., Logger.getLogger(FilterScanner.class.getName())) to the FilterScanner class and use its appropriate log level method (warning or severe) to log the messages currently printed at the two locations (the "has both `@Global` and `@Route` — skipping" message and the other occurrence around lines 89-90), ensuring messages include the filterClass.getName() text exactly as before.src/main/java/org/juv25d/di/Container.java (1)
224-248: Parameter namedfilterClassin a general-purpose DI method.
findBestConstructor(Class<?> filterClass)andisConstructable(Class<?> filterClass)use filter-specific naming despite being generic container methods. Consider renaming toclazzortypefor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/di/Container.java` around lines 224 - 248, Rename the misleading parameter filterClass to a generic name like clazz or type in the DI utility methods findBestConstructor and isConstructable so the intent is clear; update the parameter name in the method signatures and all internal references (e.g., uses in findBestConstructor, isConstructable, and any callers) to the chosen name and ensure imports/annotations remain unchanged so compilation still succeeds.src/main/java/org/juv25d/filter/ClassScanner.java (1)
80-80:Class.forName(className, true, cl)eagerly initializes classes during scanning.The
trueparameter triggers static initializers for every discovered class, which may have side effects (opening connections, registering drivers, etc.) during what should be a passive discovery phase. Consider usingfalseto defer initialization until the class is actually needed:Proposed fix
- classes.add(Class.forName(className, true, cl)); + classes.add(Class.forName(className, false, cl));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/filter/ClassScanner.java` at line 80, In ClassScanner replace the eager class loading call so static initializers are not run during scanning: locate the line that calls Class.forName(className, true, cl) (inside the code that adds to classes) and change the initialize flag from true to false (i.e., use Class.forName(className, false, cl)) so discovered classes are loaded without triggering static initializers; keep adding the resulting Class objects to the classes collection as before.
🤖 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/FilterConfig.java`:
- Around line 9-11: The constructor FilterConfig(Map<String, String> params)
currently stores the incoming map by reference and allows null to be accepted;
update the constructor to fail fast and make a defensive copy: call
Objects.requireNonNull(params, "params must not be null") (or otherwise throw an
NPE) and assign this.params = new HashMap<>(params) (or Map.copyOf(params) if
you want an unmodifiable snapshot and are okay with its null-value behavior) so
subsequent calls to get() use a stable, non-null map that the caller cannot
mutate.
In `@src/main/java/org/juv25d/filter/FilterRegistry.java`:
- Around line 26-28: getRouteFilters() returns an unmodifiable map but leaves
the inner lists in the mutable field routes exposed, allowing callers to mutate
internal state (e.g., getRouteFilters().get("/pattern").add(...)); update
getRouteFilters() to return a defensive copy that wraps each
List<FilterRegistration> with Collections.unmodifiableList (matching the
approach used by getGlobalFilters()), so callers cannot modify the inner lists
and must go through registerRoute to change routing filters.
---
Duplicate comments:
In `@src/main/java/org/juv25d/App.java`:
- Around line 35-37: The shutdown hook currently only logs and never invokes
FilterRegistry.shutdown(), so filters' destroy() lifecycle never runs; add a
shutdown path by adding a public shutdown() (or implement Closeable.close())
method on Pipeline that delegates to its internal FilterRegistry.shutdown()
(ensuring the registry reference used in Bootstrap.init()/FilterMatcher/Pipeline
is accessible), and then update the App shutdown hook to call
pipeline.shutdown() (or pipeline.close()) so each Filter.destroy() is invoked on
JVM exit.
In `@src/main/java/org/juv25d/Bootstrap.java`:
- Around line 14-36: FilterRegistry created in Bootstrap.init is only local and
never exposed for lifecycle shutdown; register it in the DI container (e.g.,
container.register(FilterRegistry.class, registry) or equivalent) immediately
after construction so shutdown/destroy can be invoked, or modify the API of init
to return both the Pipeline and the FilterRegistry (or a composite object) so
callers can access and shut it down; update any callers of init and the Pipeline
construction (references: Bootstrap.init, FilterRegistry, FilterMatcher,
Pipeline) to use the chosen approach so registry.shutdown()/destroy() can be
invoked during application shutdown.
In `@src/main/java/org/juv25d/config/RedirectConfig.java`:
- Around line 9-15: The RedirectConfig.rules() currently returns hard-coded
RedirectRule instances (e.g., "/old-page", "https://example.com/temporary") —
change it to load rules from the shared configuration source (same pattern as
RateLimitConfig) by calling ConfigLoader (or the existing config accessor) to
fetch redirect rules and map them into RedirectRule objects inside
RedirectConfig.rules(); keep a small in-code fallback (empty list or default)
only if ConfigLoader returns null/empty, and remove the static placeholders so
rules are configurable at runtime via external properties.
In `@src/main/java/org/juv25d/di/Container.java`:
- Around line 148-151: The binding resolution only follows one level; update the
Container.resolve method to repeatedly follow bindings from the initial type
until you reach a type that has no further binding or isConstructable succeeds:
loop while bindings.containsKey(currentType) and set currentType = (Class<?>)
bindings.get(currentType), using a visited/set to detect cycles and break with a
clear error if a cycle is found, then call isConstructable on the final resolved
type; reference the Container.resolve logic, the bindings map and
isConstructable to implement this chained-resolution and cycle detection.
- Line 186: The package-match check in Container (the if using
type.getPackageName().startsWith(basePackage)) wrongly matches prefixes like
"org.juv25dmalicious"; update the condition to enforce a package boundary by
checking either exact equality or that the package name starts with basePackage
+ "." (e.g., packageName.equals(basePackage) ||
packageName.startsWith(basePackage + ".")); also handle possible null from
type.getPackageName() if applicable. Replace the current startsWith(basePackage)
usage with this stricter check in the method/class containing that if.
- Around line 288-321: canResolve uses
type.getPackageName().startsWith(basePackage) which incorrectly matches sibling
packages; update the package boundary check in canResolve (and make the same
change in resolve) to ensure an exact package or proper subpackage match by
checking either type.getPackageName().equals(basePackage) ||
type.getPackageName().startsWith(basePackage + ".") (or equivalent logic that
compares basePackage + "." as the prefix), leaving the rest of canResolve
(constructor discovery, recursion, and cycle protection) intact.
In `@src/main/java/org/juv25d/filter/ClassScanner.java`:
- Around line 58-65: The Javadoc for ClassScanner.scanDir incorrectly declares
"@throws ClassNotFoundException" although scanDir catches ClassNotFoundException
internally (see the catch at line ~81) and does not propagate it; update the
Javadoc for the scanDir method to remove the "@throws ClassNotFoundException"
tag (or replace it with a note that classes that fail to load are skipped and
logged) so the documentation matches the implementation.
In `@src/main/java/org/juv25d/filter/FilterMatcher.java`:
- Around line 36-44: In FilterMatcher.matches, the wildcard branch that checks
pattern.endsWith("/*") currently only strips the '*' leaving the trailing '/' so
"/api/*" won't match "/api"; change the logic to treat the wildcard as matching
the base path itself and any subpaths: when pattern endsWith("/*",) derive the
base by removing the entire "/*" suffix (not just '*') and return true if path
equals that base OR if path starts with base + "/" (i.e., a subpath). Update the
matches method accordingly to use this base-equality-or-subpath check.
In `@src/main/java/org/juv25d/filter/FilterRegistry.java`:
- Around line 31-32: Replace the non-atomic volatile check-then-set on
isShutdown with an AtomicBoolean to make shutdown idempotent: change the
isShutdown field to an AtomicBoolean (e.g., AtomicBoolean isShutdown = new
AtomicBoolean(false)), add the java.util.concurrent.atomic.AtomicBoolean import,
and in the shutdown code replace "if (isShutdown) return; isShutdown = true;"
with a single atomic compareAndSet check such as "if
(!isShutdown.compareAndSet(false, true)) return;" so only the first thread
proceeds to run the destroy loop.
In `@src/main/java/org/juv25d/filter/IpFilter.java`:
- Around line 22-26: The no-arg IpFilter() constructor currently instantiates a
new IpFilterConfig directly, preventing DI from providing a custom config;
change the constructor to accept an IpFilterConfig parameter (e.g.,
IpFilter(IpFilterConfig config)) and assign this.whitelist = config.whitelist()
and this.blacklist = config.blacklist() so the container can inject overrides of
IpFilterConfig and users can register custom subclasses.
In `@src/main/java/org/juv25d/filter/RedirectFilter.java`:
- Line 29: The Javadoc line referencing pipeline.addFilter(new
RedirectFilter(rules)) is stale; update or remove it in the RedirectFilter class
Javadoc (the comment near the RedirectFilter declaration) so it no longer points
to a non-existent pipeline.addFilter API—either replace with the current usage
pattern for registering RedirectFilter or simply delete that example line to
avoid misleading docs.
---
Nitpick comments:
In `@src/main/java/org/juv25d/App.java`:
- Around line 19-23: Extract the repeated package literal into a named constant
in App: define a private static final String BASE_PACKAGE = "org.juv25d" and
replace the two uses of the literal in the Container constructor call (new
Container("org.juv25d")) and the Bootstrap.init call (Bootstrap.init(container,
"org.juv25d")) with BASE_PACKAGE so both the container and bootstrap use the
same constant.
In `@src/main/java/org/juv25d/Bootstrap.java`:
- Line 34: The call container.get(org.juv25d.router.RouterConfig.class) is used
only for constructor side-effects and its return is discarded; add a concise
inline comment next to that call (or above it) explaining that RouterConfig is
being instantiated to trigger route registration/other side-effects (so it must
not be removed as dead code), referencing RouterConfig and the
container.get(...) call so future maintainers understand the intent.
In `@src/main/java/org/juv25d/di/Container.java`:
- Around line 224-248: Rename the misleading parameter filterClass to a generic
name like clazz or type in the DI utility methods findBestConstructor and
isConstructable so the intent is clear; update the parameter name in the method
signatures and all internal references (e.g., uses in findBestConstructor,
isConstructable, and any callers) to the chosen name and ensure
imports/annotations remain unchanged so compilation still succeeds.
In `@src/main/java/org/juv25d/di/Inject.java`:
- Around line 5-7: The Inject annotation is missing `@Documented` so it won't
appear in generated Javadoc; update the Inject declaration (annotation type
Inject) to be annotated with `@Documented` and add the required import
(java.lang.annotation.Documented) so the annotation is included in API docs and
visible on annotated constructors.
In `@src/main/java/org/juv25d/filter/ClassScanner.java`:
- Line 80: In ClassScanner replace the eager class loading call so static
initializers are not run during scanning: locate the line that calls
Class.forName(className, true, cl) (inside the code that adds to classes) and
change the initialize flag from true to false (i.e., use
Class.forName(className, false, cl)) so discovered classes are loaded without
triggering static initializers; keep adding the resulting Class objects to the
classes collection as before.
In `@src/main/java/org/juv25d/filter/FilterFactory.java`:
- Around line 13-15: Change the create method signature to accept a bounded type
parameter so callers must pass a Filter subtype: update FilterFactory.create to
take Class<? extends Filter> (instead of Class<?>), call container.get(clazz)
with that parameter, and return the result as a Filter without an unchecked
cast; this removes the explicit (Filter) cast and enforces the contract at
compile time while keeping the use of container.get intact.
In `@src/main/java/org/juv25d/filter/FilterRegistry.java`:
- Around line 52-53: Replace the System.err.println call in FilterRegistry (the
destruction error block that prints "Error destroying filter " +
filter.getClass().getName() + ": " + e.getMessage()) with the project's
structured logger: obtain the ServerLogging logger for FilterRegistry and call
the appropriate error method, passing a clear message that includes
filter.getClass().getName() and the exception e as the throwable so the
stacktrace and structured metadata are recorded (e.g.,
ServerLogging.getLogger(FilterRegistry.class).error("Error destroying filter
{}", filter.getClass().getName(), e)).
In `@src/main/java/org/juv25d/filter/FilterScanner.java`:
- Around line 54-55: Replace the System.err.println calls in FilterScanner with
java.util.logging.Logger-based logging: add a private static final Logger (e.g.,
Logger.getLogger(FilterScanner.class.getName())) to the FilterScanner class and
use its appropriate log level method (warning or severe) to log the messages
currently printed at the two locations (the "has both `@Global` and `@Route` —
skipping" message and the other occurrence around lines 89-90), ensuring
messages include the filterClass.getName() text exactly as before.
In `@src/main/java/org/juv25d/filter/RateLimitingFilter.java`:
- Line 94: The single-line guard in RateLimitingFilter#doFilter is hard to read
and debug; split it into separate statements so the condition, the call to
chain.doFilter(req, res), and the return are on their own lines. Locate the if
(!enabled) {chain.doFilter(req, res);return;} statement in the doFilter method
and refactor it to a multi-line form with the boolean check, the chain.doFilter
invocation, and the return each on its own line for readability and easier
debugging.
In `@src/main/java/org/juv25d/filter/RedirectFilter.java`:
- Around line 47-50: The no-arg RedirectFilter constructor duplicates logger
initialization; refactor it to delegate to the existing constructor by calling
this(...) with the results of new RedirectConfig().rules() and
Logger.getLogger(RedirectFilter.class.getName()) so all initialization lives in
one constructor (remove the duplicated this.rules/this.logger assignments in the
no-arg constructor).
In `@src/main/java/org/juv25d/router/RouterConfig.java`:
- Line 16: Replace the System.out.println call in RouterConfig with the
application logger: obtain the logger via ServerLogging.getLogger() and log the
same message at the appropriate level (e.g., info) instead of printing to
stdout; update the RouterConfig location where System.out.println("Router
configured") appears to call logger.info("Router configured") (or
logger.debug/info per context) so log-level control and structured logging are
used.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/juv25d/router/RouterConfig.java (2)
12-12: InjectRouter(abstraction) instead ofSimpleRouter(concrete type)
App.javaalready bindsRouter.class → SimpleRouter.classin the container. InjectingSimpleRouterdirectly bypasses that binding and couplesRouterConfigto the implementation, defeating the purpose of the DI abstraction layer. If the implementation is ever swapped, this constructor silently continues to resolve the concrete class regardless of the registered binding.♻️ Proposed refactor
-import org.juv25d.router.SimpleRouter; +import org.juv25d.router.Router; ... `@Inject` -public RouterConfig(SimpleRouter router) { +public RouterConfig(Router router) {You'll also need to ensure
RouterexposesregisterPlugin; if it doesn't, add the method to the interface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/router/RouterConfig.java` at line 12, Change RouterConfig's constructor to accept the Router abstraction instead of the concrete SimpleRouter: update the RouterConfig(SimpleRouter router) constructor to RouterConfig(Router router) and use that Router reference internally; also ensure the Router interface declares the registerPlugin method (add registerPlugin(...) to Router if missing) so RouterConfig can call router.registerPlugin without depending on SimpleRouter.
20-20: ReplaceSystem.out.printlnwith the project loggerThe rest of the codebase (e.g.,
MetricPlugin) usesServerLogging.getLogger(). UsingSystem.out.printlnbypasses the logging infrastructure (level filtering, formatting, rotation, etc.).♻️ Proposed refactor
+import org.juv25d.logging.ServerLogging; +import java.util.logging.Logger; public class RouterConfig { + private static final Logger logger = ServerLogging.getLogger(); `@Inject` public RouterConfig(SimpleRouter router) { router.registerPlugin("/metric", new MetricPlugin()); router.registerPlugin("/health", new HealthCheckPlugin()); router.registerPlugin("/", new StaticFilesPlugin()); router.registerPlugin("/*", new StaticFilesPlugin()); router.registerPlugin("/notfound", new NotFoundPlugin()); - System.out.println("Router configured"); + logger.info("Router configured"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/router/RouterConfig.java` at line 20, Replace the direct System.out.println call in the RouterConfig class with the project's logger: add a class-level logger (e.g., private static final var LOGGER = ServerLogging.getLogger(RouterConfig.class)) and replace System.out.println("Router configured") with LOGGER.info("Router configured"); ensure you import ServerLogging and use the same logger naming/level conventions as MetricPlugin so logging integrates with the project's infrastructure.
🤖 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/router/RouterConfig.java`:
- Line 18: The current registration router.registerPlugin("/notfound", new
NotFoundPlugin()) creates a literal debug endpoint rather than a global 404
fallback; either (A) make it the catch‑all 404 by registering NotFoundPlugin as
the wildcard/fallback handler used by RouterConfig (replace the literal
"/notfound" registration with the router's global fallback registration or the
"/*" catch-all so StaticFilesPlugin/StaticFileHandler won't consume unknown
paths), or (B) if you intend a named debug endpoint, rename the route or the
class (e.g., DebugNotFoundEndpoint or /debug/notfound) to avoid implying it is
the app’s general 404 handler; update references to NotFoundPlugin,
router.registerPlugin, StaticFilesPlugin, and StaticFileHandler accordingly.
---
Nitpick comments:
In `@src/main/java/org/juv25d/router/RouterConfig.java`:
- Line 12: Change RouterConfig's constructor to accept the Router abstraction
instead of the concrete SimpleRouter: update the RouterConfig(SimpleRouter
router) constructor to RouterConfig(Router router) and use that Router reference
internally; also ensure the Router interface declares the registerPlugin method
(add registerPlugin(...) to Router if missing) so RouterConfig can call
router.registerPlugin without depending on SimpleRouter.
- Line 20: Replace the direct System.out.println call in the RouterConfig class
with the project's logger: add a class-level logger (e.g., private static final
var LOGGER = ServerLogging.getLogger(RouterConfig.class)) and replace
System.out.println("Router configured") with LOGGER.info("Router configured");
ensure you import ServerLogging and use the same logger naming/level conventions
as MetricPlugin so logging integrates with the project's infrastructure.
bamsemats
previously approved these changes
Feb 23, 2026
|
Sorry about the constant merge conflicts you've had to deal with. This looks like a solid addition. I'll keep an eye on this PR to try and get it merged asap after the new conflicts are resolved. Feel free to ping me on discord when it's ready. |
kristinaxm
force-pushed
the
feature/core-di-architecture
branch
from
February 23, 2026 14:53
2ca169c to
f73ebb0
Compare
bamsemats
approved these changes
Feb 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #113 (: Hopefully..
Summary by CodeRabbit
New Features
Refactor
Tests