Add RedirectFilter with configurable 301/302 rules#9
Add RedirectFilter with configurable 301/302 rules#9Ericthilen wants to merge 20 commits intomainfrom
Conversation
Add ci workflow for github actions.
* build: configure pom.xml with needed plugin/tools. Setup Java 25 environment with JUnit 5, Mockito, JaCoCo, Pitest, and Spotless * fix: add missing test scope for awaitility
|
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 pull request introduces a complete HTTP/TCP web server implementation in Java. The changes include a configuration-driven server core that accepts client connections, parses HTTP requests, serves static files with security protections and MIME type detection, implements a filter chain architecture for extensibility, and provides Docker containerization with automated CI/CD workflows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Around line 27-34: The release workflow's "Build and push" step uses
docker/build-push-action@v6.18.0 but there is no Dockerfile in the context so
the job will fail; fix by either adding a Dockerfile at the repository root (or
update the action's context and file path) that builds the application and keeps
the step using steps.meta.outputs.tags and steps.meta.outputs.labels, or
temporarily remove/guard the "Build and push" step (the step named "Build and
push") so the workflow defers push until a follow-up PR adds a Dockerfile (e.g.,
conditionally skip the step or set push: false).
In `@src/main/java/org/example/server/FilterChain.java`:
- Around line 15-18: The FilterChain constructor stores the caller's filters
array reference directly, allowing external mutation; update the
FilterChain(HttpFilter[] filters, TerminalHandler terminal) constructor to
defensively copy the incoming filters (e.g., Arrays.copyOf(filters,
filters.length) or filters.clone()) before assigning to the filters field while
still validating non-null, and keep the terminal assignment using
Objects.requireNonNull(terminal, "terminal").
In `@src/main/java/org/example/server/RedirectResponse.java`:
- Around line 3-10: Make the RedirectResponse class immutable and validate
constructor inputs: mark the fields location and statusCode as final, and in the
constructor (RedirectResponse(String location, int statusCode)) call
Objects.requireNonNull(location, "location") and validate statusCode is one of
the allowed values (e.g., 301 or 302), throwing IllegalArgumentException if not;
this prevents null locations and invalid status codes while keeping the object
effectively immutable.
In `@src/main/java/org/example/server/RedirectRulesLoader.java`:
- Around line 17-22: The regex patterns parsed in RedirectRulesLoader (when
trimmed.startsWith(REGEX_PREFIX)) are compiled raw via Pattern.compile(rawRegex)
which makes them unanchored and inconsistent with anchored wildcard/literal
rules; update the logic that handles REGEX_PREFIX to either wrap the rawRegex
with start/end anchors (prepend '^' and append '$') before compiling or provide
a clear opt-in (e.g., detect an explicit anchor flag) so regex rules behave
consistently with literal/wildcard matching; modify the block that extracts
rawRegex and calls Pattern.compile to apply the chosen anchoring behavior and
adjust the IllegalArgumentException message if needed to reflect the anchoring
policy.
In `@src/main/java/org/example/TcpServer.java`:
- Around line 19-23: The loop in TcpServer uses serverSocket.accept() and then
calls clientSocket.close() directly, which can leak the socket if an exception
occurs; update the accept/close handling to use a try-with-resources for the
Socket returned by serverSocket.accept() (e.g., wrap "Socket clientSocket =
serverSocket.accept()" in a try(...) block) so the socket is always closed, and
adjust surrounding exception handling as needed in the method containing the
loop to handle IOExceptions.
🧹 Nitpick comments (11)
src/main/java/org/example/App.java (1)
5-5: Hardcoded port and no integration with the filter pipeline.The port is hardcoded to 8080 — consider making it configurable via CLI args or an environment variable. Also,
TcpServer.start()currently just accepts and closes connections without routing them through theRedirectFilter/FilterChainpipeline introduced in this PR. Is the integration planned for a follow-up?.github/workflows/ci.yml (2)
14-14: Inconsistentactions/checkoutversion across workflows.This workflow uses
actions/checkout@v4, whilerelease.ymlusesactions/checkout@v6.0.2. Consider aligning them to avoid drift and ensure consistent behavior.
16-19:mvnis invoked before the JDK is set up.The "Get Java Version" step calls
mvn help:evaluateusing whatever JDK ships on the runner image, before the explicitsetup-javastep. This works today becauseubuntu-latestbundles a default JDK, but it's fragile — a runner image update could break this. Consider either (a) setting up a known baseline JDK first, then extracting the property, then setting up the target JDK, or (b) extracting the version with a simplegrep/xmllintfrompom.xmlinstead.Alternative: extract version without Maven
- name: Get Java Version run: | - JAVA_VERSION=$(mvn help:evaluate "-Dexpression=maven.compiler.release" -q -DforceStdout) + JAVA_VERSION=$(grep -oP '(?<=<maven.compiler.release>)\d+(?=</maven.compiler.release>)' pom.xml) echo "JAVA_VERSION=$JAVA_VERSION" >> $GITHUB_ENVsrc/main/java/org/example/TcpServer.java (2)
15-27: No shutdown mechanism — server runs until the process is killed.
start()blocks forever with no way to stop it gracefully. This complicates testing and clean shutdown. Consider adding avolatile boolean runningflag and astop()method that closes theServerSocketto break out ofaccept(). Theawaitilitytest dependency inpom.xmlsuggests async tests are anticipated, which would need a stoppable server.
18-18: Informational: unencrypted socket.Static analysis flags
new ServerSocket(port)as transmitting data in cleartext (CWE-319). This is acceptable for a development scaffold but should be addressed before handling real traffic (e.g., useSSLServerSocketor terminate TLS at a reverse proxy).src/main/java/org/example/server/RedirectRulesLoader.java (1)
34-45:wildcardToRegexwraps every single character in\Q…\E— works but is noisy.
Pattern.quote(String.valueOf(c))on each non-*character produces\Qx\Eper character. This is correct but generates unnecessarily verbose regex and allocates aStringper char. A simpler approach: accumulate literal runs and quote them in one call, or usePattern.quoteon segments split by*.Simpler alternative
private static String wildcardToRegex(String wildcard) { - StringBuilder sb = new StringBuilder(); - for (int i = 0; i < wildcard.length(); i++) { - char c = wildcard.charAt(i); - if (c == '*') { - sb.append(".*"); - } else { - sb.append(Pattern.quote(String.valueOf(c))); - } - } - return sb.toString(); + String[] parts = wildcard.split("\\*", -1); + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < parts.length; i++) { + if (i > 0) sb.append(".*"); + sb.append(Pattern.quote(parts[i])); + } + return sb.toString(); }pom.xml (1)
44-49:awaitilityversion is hardcoded — inconsistent with other dependency versions.Other test dependencies (
junit-jupiter,assertj-core,mockito) use<properties>for version management.awaitilityhas its version inlined. For consistency, extract it to a property.Proposed fix
Add to
<properties>:<awaitility.version>4.3.0</awaitility.version>Then reference it:
<dependency> <groupId>org.awaitility</groupId> <artifactId>awaitility</artifactId> - <version>4.3.0</version> + <version>${awaitility.version}</version> <scope>test</scope> </dependency>src/main/java/org/example/server/HttpRequest.java (1)
5-21: Consider using a Javarecord— you're on Java 25.Since the project targets Java 25 and this class is already immutable with record-style accessors (
method(),path()), it's a natural fit for arecord. This would eliminate the boilerplate while preserving the same API.Record equivalent
package org.example.server; import java.util.Objects; public record HttpRequest(String method, String path) { public HttpRequest { Objects.requireNonNull(method, "method"); Objects.requireNonNull(path, "path"); } }src/main/java/org/example/server/HttpResponse.java (1)
7-25: Consider using a case-insensitive map for HTTP headers.HTTP header names are case-insensitive per RFC 7230. A
LinkedHashMaptreats"Location"and"location"as distinct keys. If other filters or future code use different casing, headers could silently duplicate. ATreeMap(String.CASE_INSENSITIVE_ORDER)is a simple drop-in fix.♻️ Suggested change
- private final Map<String, String> headers = new LinkedHashMap<>(); + private final Map<String, String> headers = new java.util.TreeMap<>(String.CASE_INSENSITIVE_ORDER);src/test/java/org/example/server/RedirectFilterTest.java (1)
33-48: Consider verifying that the chain is also short-circuited on 302 redirects.The 301 test asserts
terminalCalledis false, but the 302 test doesn't check chain termination. Adding a similarAtomicBooleanassertion here would strengthen coverage and keep the tests consistent.src/main/java/org/example/server/FilterChain.java (1)
20-27: Terminal handler can be invoked more than once ifdoFilteris called after the chain is exhausted.Once
index >= filters.length, every subsequent call todoFilterre-invokes the terminal handler. If any filter or caller accidentally callsdoFiltertwice, the terminal runs multiple times with potentially unintended side effects. Consider adding a guard.🛡️ Suggested guard
public void doFilter(HttpRequest request, HttpResponse response) { if (index < filters.length) { HttpFilter current = filters[index++]; current.doFilter(request, response, this); return; } + if (index++ == filters.length) { // only invoke terminal once - terminal.handle(request, response); + terminal.handle(request, response); + } }
| public FilterChain(HttpFilter[] filters, TerminalHandler terminal) { | ||
| this.filters = Objects.requireNonNull(filters, "filters"); | ||
| this.terminal = Objects.requireNonNull(terminal, "terminal"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Defensively copy the filters array to prevent external mutation.
The constructor stores the caller's array reference directly. If the caller later modifies the array, the chain's behavior changes unexpectedly. Use Arrays.copyOf or .clone().
♻️ Suggested fix
+import java.util.Arrays;
+
public FilterChain(HttpFilter[] filters, TerminalHandler terminal) {
- this.filters = Objects.requireNonNull(filters, "filters");
+ this.filters = Arrays.copyOf(
+ Objects.requireNonNull(filters, "filters"), filters.length);
this.terminal = Objects.requireNonNull(terminal, "terminal");
}🤖 Prompt for AI Agents
In `@src/main/java/org/example/server/FilterChain.java` around lines 15 - 18, The
FilterChain constructor stores the caller's filters array reference directly,
allowing external mutation; update the FilterChain(HttpFilter[] filters,
TerminalHandler terminal) constructor to defensively copy the incoming filters
(e.g., Arrays.copyOf(filters, filters.length) or filters.clone()) before
assigning to the filters field while still validating non-null, and keep the
terminal assignment using Objects.requireNonNull(terminal, "terminal").
| public class RedirectResponse { | ||
| private String location; | ||
| private int statusCode; | ||
|
|
||
| public RedirectResponse(String location, int statusCode) { | ||
| this.location = location; | ||
| this.statusCode = statusCode; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fields should be final, and constructor should validate inputs.
location and statusCode are not final, making the object unintentionally mutable (even without setters, reflection or subclassing could mutate them). Also, unlike HttpRequest which uses Objects.requireNonNull, this class performs no validation — location can be null and statusCode can be any integer despite the PR constraining it to 301/302.
Proposed fix
public class RedirectResponse {
- private String location;
- private int statusCode;
+ private final String location;
+ private final int statusCode;
public RedirectResponse(String location, int statusCode) {
+ if (location == null || location.isBlank()) {
+ throw new IllegalArgumentException("location must not be blank");
+ }
+ if (statusCode != 301 && statusCode != 302) {
+ throw new IllegalArgumentException("statusCode must be 301 or 302");
+ }
this.location = location;
this.statusCode = statusCode;
}📝 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.
| public class RedirectResponse { | |
| private String location; | |
| private int statusCode; | |
| public RedirectResponse(String location, int statusCode) { | |
| this.location = location; | |
| this.statusCode = statusCode; | |
| } | |
| public class RedirectResponse { | |
| private final String location; | |
| private final int statusCode; | |
| public RedirectResponse(String location, int statusCode) { | |
| if (location == null || location.isBlank()) { | |
| throw new IllegalArgumentException("location must not be blank"); | |
| } | |
| if (statusCode != 301 && statusCode != 302) { | |
| throw new IllegalArgumentException("statusCode must be 301 or 302"); | |
| } | |
| this.location = location; | |
| this.statusCode = statusCode; | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/org/example/server/RedirectResponse.java` around lines 3 - 10,
Make the RedirectResponse class immutable and validate constructor inputs: mark
the fields location and statusCode as final, and in the constructor
(RedirectResponse(String location, int statusCode)) call
Objects.requireNonNull(location, "location") and validate statusCode is one of
the allowed values (e.g., 301 or 302), throwing IllegalArgumentException if not;
this prevents null locations and invalid status codes while keeping the object
effectively immutable.
| if (trimmed.startsWith(REGEX_PREFIX)) { | ||
| String rawRegex = trimmed.substring(REGEX_PREFIX.length()); | ||
| if (rawRegex.isBlank()) { | ||
| throw new IllegalArgumentException("regex sourcePath must not be blank"); | ||
| } | ||
| return Pattern.compile(rawRegex); |
There was a problem hiding this comment.
regex: patterns are not anchored — inconsistent with wildcard/literal behavior.
Patterns supplied via the regex: prefix are compiled as-is (no ^/$ anchoring), while wildcard and literal patterns are always anchored. This means regex:/old will match /old-page as well as /old, but the plain literal /old will only match exactly /old. This asymmetry is likely to surprise users.
If the intent is for regex: to allow partial matches, document it. Otherwise, consider anchoring all patterns consistently, or letting the rule author opt in to partial matching.
🤖 Prompt for AI Agents
In `@src/main/java/org/example/server/RedirectRulesLoader.java` around lines 17 -
22, The regex patterns parsed in RedirectRulesLoader (when
trimmed.startsWith(REGEX_PREFIX)) are compiled raw via Pattern.compile(rawRegex)
which makes them unanchored and inconsistent with anchored wildcard/literal
rules; update the logic that handles REGEX_PREFIX to either wrap the rawRegex
with start/end anchors (prepend '^' and append '$') before compiling or provide
a clear opt-in (e.g., detect an explicit anchor flag) so regex rules behave
consistently with literal/wildcard matching; modify the block that extracts
rawRegex and calls Pattern.compile to apply the chosen anchoring behavior and
adjust the IllegalArgumentException message if needed to reflect the anchoring
policy.
| while (true) { | ||
| Socket clientSocket = serverSocket.accept(); // block | ||
| System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress()); | ||
| clientSocket.close(); | ||
| } |
There was a problem hiding this comment.
Client socket is not closed in a try-with-resources — potential leak.
If anything throws between accept() and close() (e.g., an added handler in the future), the socket leaks. Use try-with-resources for safety.
Proposed fix
while (true) {
- Socket clientSocket = serverSocket.accept(); // block
- System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress());
- clientSocket.close();
+ try (Socket clientSocket = serverSocket.accept()) {
+ System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress());
+ }
}📝 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.
| while (true) { | |
| Socket clientSocket = serverSocket.accept(); // block | |
| System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress()); | |
| clientSocket.close(); | |
| } | |
| while (true) { | |
| try (Socket clientSocket = serverSocket.accept()) { | |
| System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/org/example/TcpServer.java` around lines 19 - 23, The loop in
TcpServer uses serverSocket.accept() and then calls clientSocket.close()
directly, which can leak the socket if an exception occurs; update the
accept/close handling to use a try-with-resources for the Socket returned by
serverSocket.accept() (e.g., wrap "Socket clientSocket = serverSocket.accept()"
in a try(...) block) so the socket is always closed, and adjust surrounding
exception handling as needed in the method containing the loop to handle
IOExceptions.
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (28)
src/main/java/org/example/httpparser/HttpParseRequestLine.java (2)
15-15: Unnecessary variable alias.
BufferedReader reader = br;serves no purpose — just usebrdirectly.♻️ Proposed fix
public void parseHttpRequest(BufferedReader br) throws IOException { - BufferedReader reader = br; - String requestLine = reader.readLine(); + String requestLine = br.readLine();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java` at line 15, In HttpParseRequestLine, remove the unnecessary alias "BufferedReader reader = br;" and update any references to "reader" to use "br" directly (or eliminate the variable use entirely where possible); ensure no other code depends on the "reader" symbol and delete the redundant declaration to avoid dead/duplicate variables.
11-11:debugis hardcoded tofalsewith no setter — the logging block is dead code.The
debugfield is alwaysfalseand there's no way to change it, making lines 34–38 unreachable. Either remove the dead code or expose a way to enable debug mode.Also applies to: 34-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java` at line 11, The debug flag in class HttpParseRequestLine is hardcoded false, making the logging block (lines 34–38) dead; fix by exposing and using a mutable debug control: add a public setter setDebug(boolean) (or a constructor parameter) and update any callers to enable debug when needed, then guard the existing logging block with this.debug so the logs can be emitted; alternatively, if debug logging is undesired, remove the debug field and the unreachable logging block (referencing debug, setDebug, and the logging statements in HttpParseRequestLine).src/main/java/org/example/filter/redirect/RedirectRulesLoader.java (2)
34-45:wildcardToRegexquotes each character individually — produces verbose patterns.Each non-
*character is separately wrapped in\Q...\E(e.g.,/foo→\Q/\E\Qf\E\Qo\E\Qo\E). While correct, this is needlessly verbose and produces hard-to-debug regex. Batching consecutive literal characters into a singlePattern.quote()call is cleaner.♻️ Suggested improvement
private static String wildcardToRegex(String wildcard) { StringBuilder sb = new StringBuilder(); + StringBuilder literal = new StringBuilder(); for (int i = 0; i < wildcard.length(); i++) { char c = wildcard.charAt(i); if (c == '*') { + if (literal.length() > 0) { + sb.append(Pattern.quote(literal.toString())); + literal.setLength(0); + } sb.append("[^/]*"); } else { - sb.append(Pattern.quote(String.valueOf(c))); + literal.append(c); } } + if (literal.length() > 0) { + sb.append(Pattern.quote(literal.toString())); + } return sb.toString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectRulesLoader.java` around lines 34 - 45, The wildcardToRegex method currently calls Pattern.quote for each non-'*' character which produces verbose regex; change it to accumulate consecutive literal characters into a buffer and call Pattern.quote once for each literal run, appending "[^/]*" for '*' characters as before; update the method (wildcardToRegex) to iterate chars, build a literal chunk string, flush it through Pattern.quote when you hit a '*' or end-of-string, and append the quoted chunk to the StringBuilder to produce a much cleaner regex.
10-31: Anchors^and$are redundant when used withMatcher.matches().
RedirectRule.matches()usesPattern.matcher(requestPath).matches(), which inherently requires the entire string to match. The^and$on Line 31 are unnecessary. Not a bug, but worth a note for clarity — either remove the anchors or document the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectRulesLoader.java` around lines 10 - 31, The compileSourcePattern method currently wraps the generated regex with "^" and "$" before compiling, which is redundant because RedirectRule.matches() uses Pattern.matcher(requestPath).matches() that already enforces full-string matching; update compileSourcePattern (and any callers like wildcardToRegex usage) to stop prepending "^" and "$" and simply return Pattern.compile(regex) for both wildcard and quoted paths, or alternatively add a comment in compileSourcePattern explaining the explicit anchors are unnecessary but intentionally kept — reference the compileSourcePattern and wildcardToRegex symbols and ensure unit tests for RedirectRule.matches() still validate the intended behavior after the change.src/main/java/org/example/filter/redirect/RedirectRule.java (1)
23-25:matches(null)will throw NPE fromPattern.matcher(null).If
requestPathis evernull, this will throw an unguardedNullPointerException. Consider whether callers can guarantee non-null, or add a defensive check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectRule.java` around lines 23 - 25, The matches method in RedirectRule currently calls sourcePattern.matcher(requestPath).matches() which will throw a NullPointerException if requestPath is null; update RedirectRule.matches to defensively handle null input (e.g., if requestPath == null return false or throw a clear IllegalArgumentException) before calling sourcePattern.matcher, referencing the matches method and sourcePattern.matcher to locate the change.src/main/java/org/example/http/MimeTypeDetector.java (1)
59-65: Consider extracting"application/octet-stream"to a named constant.The default MIME type string is used in multiple return paths. A
private static final String DEFAULT_MIME_TYPEconstant would reduce duplication and clarify intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/http/MimeTypeDetector.java` around lines 59 - 65, The default MIME string "application/octet-stream" is duplicated in detectMimeType and other return paths; extract it into a private static final String DEFAULT_MIME_TYPE constant (e.g., in class MimeTypeDetector) and replace literal usages in the detectMimeType(String filename) method (and any other methods in MimeTypeDetector that return the default) with DEFAULT_MIME_TYPE to improve clarity and avoid duplication.src/main/java/org/example/httpparser/HttpRequest.java (1)
11-41: Excessive indentation suggests copy-paste from a nested class.The entire class body is indented with 8 spaces instead of the standard 4. This is cosmetic but affects readability and suggests the code was extracted from an inner class without adjusting indentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpRequest.java` around lines 11 - 41, The HttpRequest class is indented with 8 spaces throughout (looks copied from a nested class); reformat the file to use the project standard 4-space indentation for the class body and its members (class HttpRequest, its constructor HttpRequest(...), and all getters getMethod/getPath/getVersion/getHeaders/getBody). Adjust the whitespace for the field declarations, constructor parameters/body and each getter, then run the project's Java formatter or IDE reformat action to ensure consistent indentation and styling.www/pageNotFound.html (1)
47-53: Consider adding a link back to the home page.A 404 page without navigation leaves users stranded. Adding a simple link (e.g.,
<a href="/">Go back home</a>) improves the user experience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@www/pageNotFound.html` around lines 47 - 53, Add a simple navigational anchor inside the existing <div class="container"> so users can return to the site—insert a link (e.g., an <a href="/">Go back home</a>) after the paragraph in the 404 markup (the elements h1, h2, p inside the container) and optionally give it a class like "home-link" for styling; update any CSS if needed to match site styles.Dockerfile (1)
3-6: Optimize Docker layer caching by copyingpom.xmlfirst.Currently
src/andpom.xmlare copied together before any Maven invocation. Any source change invalidates the dependency-download cache. Copyingpom.xmlfirst and resolving dependencies before copying source enables Docker to cache the dependency layer.♻️ Proposed optimization
FROM maven:3-eclipse-temurin-25-alpine AS build WORKDIR /build -COPY src/ src/ COPY pom.xml pom.xml +RUN mvn dependency:go-offline +COPY src/ src/ RUN mvn compile RUN mvn dependency:copy-dependencies -DincludeScope=compile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 3 - 6, Reorder Dockerfile steps to copy pom.xml and download dependencies before copying source so dependency resolution layer can be cached: first COPY pom.xml pom.xml, then RUN mvn dependency:copy-dependencies -DincludeScope=compile (or mvn -B dependency:go-offline) to populate the dependency layer, and only after that COPY src/ src/ and run mvn compile/build; update the existing RUN mvn compile and RUN mvn dependency:copy-dependencies invocations accordingly in the Dockerfile.src/test/java/org/example/http/MimeTypeDetectorTest.java (1)
145-145: Non-English comment in an otherwise English codebase.The Swedish comment "Parametriserad test för många filtyper på en gång" should be in English for consistency (e.g., "Parameterized test for many file types at once").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/http/MimeTypeDetectorTest.java` at line 145, The comment in MimeTypeDetectorTest contains a Swedish line "Parametriserad test för många filtyper på en gång"; update this to English for consistency (e.g., "Parameterized test for many file types at once") in the test class MimeTypeDetectorTest so the codebase uses a single language for comments; locate the comment near the parameterized test method in MimeTypeDetectorTest and replace the Swedish text with the English equivalent.src/main/java/org/example/filter/Filter.java (1)
7-14: Consider providing default no-op implementations forinit()anddestroy().Most filter implementations likely won't need lifecycle hooks. Adding
defaultimplementations reduces boilerplate for implementors.Proposed change
public interface Filter { - void init(); + default void init() {} void doFilter(HttpRequest request, HttpResponseBuilder response, FilterChain chain); - void destroy(); + default void destroy() {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/Filter.java` around lines 7 - 14, The Filter interface currently forces implementors to override lifecycle methods init() and destroy(), causing unnecessary boilerplate; change Filter by adding default no-op implementations for init() and destroy() (while keeping doFilter(HttpRequest request, HttpResponseBuilder response, FilterChain chain) abstract) so existing implementations can omit those methods—update the interface's init() and destroy() signatures to be default methods that do nothing and leave doFilter unchanged.src/main/java/org/example/filter/FilterChainImpl.java (2)
20-22: No defensive copy of thefilterslist.The constructor stores the caller's list reference directly. If the caller mutates the list after constructing the chain, behavior becomes unpredictable. Wrap it with
List.copyOf(filters)for safety, which also rejectsnullelements.Proposed fix
public FilterChainImpl(List<Filter> filters) { - this.filters = filters; + this.filters = List.copyOf(filters); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/FilterChainImpl.java` around lines 20 - 22, The constructor FilterChainImpl(List<Filter> filters) stores the caller's list reference directly which allows external mutation; change it to defensively copy the input by assigning this.filters = List.copyOf(filters) (which also enforces non-null and rejects null elements) so the internal filters list is immutable and not affected by caller changes; update the FilterChainImpl constructor to use List.copyOf and ensure the class field named filters remains compatible with an unmodifiable/immutable list.
29-31: TODO: terminal action when all filters are exhausted.The chain currently no-ops when all filters have run. This means the actual request handling (e.g., static file serving) never executes after filters complete. Consider accepting a terminal
Runnableor handler in the constructor to invoke at the end of the chain.Would you like me to propose an implementation for the terminal handler integration, or open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/FilterChainImpl.java` around lines 29 - 31, The chain currently no-ops when all filters are exhausted; add a terminal handler to FilterChainImpl by introducing a final Runnable (or Consumer<Request>/handler type) field (e.g., terminalHandler), initialize it via the FilterChainImpl constructor, and replace the empty else block in the method that advances the chain to call terminalHandler.run() (or invoke the handler) when index >= filters.size(); also null-check the handler (or require non-null in the constructor) so the terminal action always executes after all filters finish.src/main/java/org/example/httpparser/HttpParser.java (3)
40-45: Debug logging usesSystem.out.println; prefer a logging framework.The
debugflag isfalseby default with no setter, making this effectively dead code. If debug logging is needed, consider usingjava.util.loggingor SLF4J, or remove this block entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 40 - 45, The debug printing block in HttpParser uses System.out.println and a private debug flag that defaults to false with no setter, so this code is dead and not using a logging framework; update the class (HttpParser) to either remove the block or replace it with a proper logger (e.g., java.util.logging.Logger or SLF4J) and expose control via a setter or configuration method (e.g., setDebug(boolean) or configureLogger), and change the prints to logger.debug/info calls that reference headersMap and "Host" appropriately so runtime logging is configurable and framework-based.
53-55:getHeadersMap()exposes the mutable internal map.Callers can mutate the parser's internal state through the returned reference. Return an unmodifiable view instead.
Proposed fix
public Map<String, String> getHeadersMap() { - return headersMap; + return Collections.unmodifiableMap(headersMap); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 53 - 55, getHeadersMap() in HttpParser currently returns the mutable internal headersMap, letting callers mutate parser state; change it to return an unmodifiable view (e.g., Collections.unmodifiableMap(headersMap) or Map.copyOf(headersMap)) from the HttpParser#getHeadersMap method so callers cannot modify the internal map, and keep the internal field mutable for parser operations.
16-20:setReadersilently ignores subsequent calls.The
nullguard on line 17 means a second call with a differentInputStreamis silently discarded. This can mask bugs if anHttpParserinstance is inadvertently reused with a new connection. Consider either throwing anIllegalStateExceptionon re-initialization, or allowing re-assignment (resetting the headers map as well).Option: throw on re-init
public void setReader(InputStream in) { - if (this.reader == null) { - this.reader = new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8)); + if (this.reader != null) { + throw new IllegalStateException("Reader already initialized"); } + this.reader = new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 16 - 20, The setReader method in HttpParser currently ignores subsequent calls because of the null guard on reader; change it to fail fast by throwing an IllegalStateException when reader is already set (e.g., if (this.reader != null) throw new IllegalStateException("HttpParser already initialized with a reader")); otherwise, if you prefer re-use semantics, allow re-assignment by replacing the reader assignment with unconditional reinitialization and also reset internal parsing state (e.g., clear the headers map field via headers.clear() and any other state) before setting this.reader to the new BufferedReader; update the setReader method accordingly.src/test/java/org/example/httpparser/HttpParserTest.java (2)
16-16: Test method nameTestHttpParserForHeadersstarts with an uppercase letter.Java convention uses camelCase for method names:
testHttpParserForHeaders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/httpparser/HttpParserTest.java` at line 16, Method name TestHttpParserForHeaders violates Java naming conventions; rename the test method declaration from TestHttpParserForHeaders to testHttpParserForHeaders and update any references/usages (e.g., in annotations or reflective lookups) to match the new camelCase name so the test framework still discovers and runs it.
9-10: Mixing AssertJ and JUnit assertion APIs.The file imports
assertThatfrom AssertJ (line 9) andassertNotNull/assertEqualsfrom JUnit (line 10). Pick one assertion library for consistency—preferably AssertJ throughout, since it's already used for most assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/httpparser/HttpParserTest.java` around lines 9 - 10, The test mixes AssertJ and JUnit assertions; in HttpParserTest remove the static import of JUnit assertions and convert all usages of JUnit methods (e.g., assertNotNull, assertEquals, assertTrue, assertFalse) to AssertJ equivalents using assertThat (e.g., assertThat(obj).isNotNull(), assertThat(actual).isEqualTo(expected), assertThat(cond).isTrue(), assertThat(cond).isFalse()). Ensure the file keeps the existing static import import static org.assertj.core.api.AssertionsForClassTypes.assertThat; and update every assertion site in HttpParserTest to use assertThat so imports and assertions are consistent.pom.xml (1)
113-115: SurefireargLinerelies onmaven-dependency-plugin:propertiesgoal for Mockito agent path.The expression
${org.mockito:mockito-core:jar}is resolved by thepropertiesgoal ofmaven-dependency-plugin(lines 97–107). This is a correct but fragile setup—if the dependency-plugin execution is removed or the Mockito dependency changes, the build will fail with an unresolved property. Consider adding a brief XML comment explaining this coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 113 - 115, The surefire argLine uses the property ${org.mockito:mockito-core:jar} which is produced by the maven-dependency-plugin execution (properties goal) and is fragile if that execution or the Mockito dependency changes; add a short XML comment near the <argLine> element explaining this coupling and warning that the property is provided by the maven-dependency-plugin:properties execution and must be kept in sync with the Mockito dependency (or replaced with a stable property), and optionally mention the plugin goal id (maven-dependency-plugin:properties) and the dependency coordinate (org.mockito:mockito-core) so future maintainers know where the value originates and how to fix it if unresolved.src/test/java/org/example/httpparser/HttpParseRequestLineTest.java (2)
8-10: Same assertion framework mixing asHttpParserTest.Both AssertJ (
assertThat,assertThatThrownBy) and JUnit (assertThrows) are used. For consistency, consider using one.assertThatThrownBycan fully replaceassertThrows+assertThaton the message.Example: consolidate lines 39-48 with AssertJ only
`@Test` void testParserThrowErrorWhenEmpty(){ InputStream in = new ByteArrayInputStream("".getBytes()); httpParseRequestLine.setReader(in); - Exception exception = assertThrows( - IOException.class, () -> httpParseRequestLine.parseRequest() - ); - - assertThat(exception.getMessage()).isEqualTo("HTTP Request Line is Null or Empty"); + assertThatThrownBy(() -> httpParseRequestLine.parseRequest()) + .isInstanceOf(IOException.class) + .hasMessage("HTTP Request Line is Null or Empty"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/httpparser/HttpParseRequestLineTest.java` around lines 8 - 10, The test imports mix AssertJ and JUnit assertions; consolidate to AssertJ only by removing JUnit assertions import and replacing uses of JUnit's assertThrows + subsequent assertThat on the exception message with a single assertThatThrownBy call; update tests in HttpParseRequestLineTest (and any methods that call assertThrows) to use assertThatThrownBy(...).isInstanceOf(...).hasMessageContaining(...) or equivalent AssertJ assertions so only assertThat/assertThatThrownBy from AssertJ are used.
20-31:getBytes()without explicit charset.Lines 24, 41, 53, 64 all use
testString.getBytes()(platform-default charset) instead ofgetBytes(StandardCharsets.UTF_8). Since the production code reads withStandardCharsets.UTF_8, tests should encode consistently to avoid platform-dependent failures.Example fix (apply similarly at lines 41, 53, 64)
- InputStream in = new ByteArrayInputStream(testString.getBytes()); + InputStream in = new ByteArrayInputStream(testString.getBytes(StandardCharsets.UTF_8));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/httpparser/HttpParseRequestLineTest.java` around lines 20 - 31, Tests in HttpParseRequestLineTest call testString.getBytes() (e.g., inside testParserWithTestRequestLine) which uses the platform default charset and can cause flaky behavior; update each test to encode the request byte streams with an explicit charset by replacing getBytes() with getBytes(StandardCharsets.UTF_8) in testParserWithTestRequestLine and the other test methods referenced (lines ~41, ~53, ~64) so the InputStream fed to httpParseRequestLine.setReader(...) matches the production UTF-8 decoding.src/main/java/org/example/http/HttpResponseBuilder.java (2)
45-48:setBody(byte[])stores the array reference without a defensive copy.If the caller mutates the array after calling
setBody, the built response will contain the mutated data. This is a minor concern for an internal builder but worth noting if the API surface grows.Proposed fix
public void setBody(byte[] body) { - this.bytebody = body; + this.bytebody = body != null ? body.clone() : null; this.body = ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/http/HttpResponseBuilder.java` around lines 45 - 48, setBody(byte[]) currently stores the caller's byte array reference in the bytebody field which allows external mutation; change setBody(byte[] body) to make a defensive copy (e.g., Arrays.copyOf(body, body.length) or body.clone()) before assigning to the bytebody field and keep resetting the string body to ""; also handle null input by setting bytebody to null. Ensure you update only the setBody method and reference the bytebody field and body field in your change.
64-67: Javadoc comment uses/*instead of/**— won't be picked up by documentation tools.Proposed fix
- /* + /** * Builds the complete HTTP response as a byte array and preserves binary content without corruption. * `@return` Complete HTTP response (headers + body) as byte[] */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/http/HttpResponseBuilder.java` around lines 64 - 67, The block comment above the response-building routine in HttpResponseBuilder is a regular C-style comment (/*) so Javadoc tools will ignore it; change it to a Javadoc comment (/** ... */) and ensure the `@return` tag is properly formatted inside that Javadoc so the method (e.g., the response-building method in class HttpResponseBuilder such as buildResponse or build) is documented correctly and the description notes that it returns the complete HTTP response as a byte[] preserving binary content.src/main/java/org/example/TcpServer.java (1)
20-22: Virtual thread exception goes unobserved.If
handleClientthrows, theRuntimeExceptionpropagates inside the virtual thread with noUncaughtExceptionHandler. The default handler prints to stderr, but the server has no visibility into per-connection failures. Consider catching and logging inside the lambda, or setting a thread-level uncaught exception handler.Proposed fix
- Thread.ofVirtual().start(() -> handleClient(clientSocket)); + Thread.ofVirtual().start(() -> { + try { + handleClient(clientSocket); + } catch (Exception e) { + System.err.println("Client handler failed: " + e.getMessage()); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/TcpServer.java` around lines 20 - 22, The virtual-thread lambda that starts handleClient via Thread.ofVirtual().start(() -> handleClient(clientSocket)) does not observe exceptions; wrap the call to handleClient(clientSocket) in a try/catch inside the lambda (or install a Thread.setDefaultUncaughtExceptionHandler/Thread.UncaughtExceptionHandler for these virtual threads) and log the exception with your server logger including clientSocket.getRemoteSocketAddress(), ensure the clientSocket is closed in a finally block to free resources, and propagate/record any relevant metrics or status for per-connection failures.src/main/java/org/example/ConnectionHandler.java (1)
10-11: Fields should beprivate.
clientandurihave package-private access but are only used within this class. Mark themprivateto enforce encapsulation.Proposed fix
- Socket client; - String uri; + private final Socket client; + private String uri;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/ConnectionHandler.java` around lines 10 - 11, Change the access modifiers of the fields in ConnectionHandler: make the package-private Socket client and String uri private (i.e., private Socket client; private String uri;). Ensure any external access is replaced with getters/setters if required (but if they are only used inside ConnectionHandler no further changes are needed) and rebuild to verify no visibility errors.src/main/java/org/example/StaticFileHandler.java (1)
10-13: Mutable instance fields (fileBytes,statusCode) for inter-method state are fragile.
handleGetRequestcommunicates results tosendGetRequestvia instance fields rather than a return value. This makes the class not thread-safe and tightly couples the two methods. Consider returning a small result record (e.g.,record FileResult(byte[] content, int status)) fromhandleGetRequest.Also,
WEB_ROOTis an instance field but named like astatic finalconstant — preferwebRootper Java naming conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/StaticFileHandler.java` around lines 10 - 13, The class uses mutable instance fields fileBytes and statusCode to pass data between handleGetRequest and sendGetRequest and exposes an instance field WEB_ROOT named like a constant; refactor by introducing an immutable result type (e.g., record FileResult(byte[] content, int status)) returned from handleGetRequest, update sendGetRequest to accept a FileResult (or its content and status) instead of reading instance fields, remove the fileBytes and statusCode fields, and rename WEB_ROOT to webRoot (and keep it final if intended immutable) updating all usages in StaticFileHandler.src/test/java/org/example/config/ConfigLoaderTest.java (1)
128-140: Inconsistent method naming and extra whitespace.
invalid_port_should_Throw_Exceptionmixes snake_case with PascalCase, unlike every other test in this file. There's also extraneous whitespace before().- void invalid_port_should_Throw_Exception () throws Exception { + void invalid_port_should_throw_exception() throws Exception {Also, the
throws Exceptiondeclaration is unnecessary here since no checked exception escapes —assertThatThrownBycatches it internally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/config/ConfigLoaderTest.java` around lines 128 - 140, Rename and clean up the test method invalid_port_should_Throw_Exception: change its name to follow the file's naming convention (e.g., invalidPortShouldThrowException or invalidPortThrowsException), remove the extraneous whitespace before the parentheses, and delete the unnecessary "throws Exception" declaration since assertThatThrownBy handles exceptions; leave the assertion using ConfigLoader.loadOnce(configFile) and assertThatThrownBy unchanged.src/main/java/org/example/config/AppConfig.java (1)
26-37: Default values are duplicated betweendefaults()andwithDefaultsApplied().The port
8080, root dir"./www", and logging level"INFO"are hardcoded in bothdefaults()andwithDefaultsApplied(). If one is updated without the other, they'll silently diverge. Consider extracting constants:♻️ Suggested refactor
+ private static final int DEFAULT_PORT = 8080; + private static final String DEFAULT_ROOT_DIR = "./www"; + public record ServerConfig( `@JsonProperty`("port") Integer port, `@JsonProperty`("rootDir") String rootDir ) { public static ServerConfig defaults() { - return new ServerConfig(8080, "./www"); + return new ServerConfig(DEFAULT_PORT, DEFAULT_ROOT_DIR); } public ServerConfig withDefaultsApplied() { - int p = (port == null ? 8080 : port); + int p = (port == null ? DEFAULT_PORT : port); if (p < 1 || p > 65535) { throw new IllegalArgumentException("Invalid port number: " + p + ". Port must be between 1 and 65535"); } - String rd = (rootDir == null || rootDir.isBlank()) ? "./www" : rootDir; + String rd = (rootDir == null || rootDir.isBlank()) ? DEFAULT_ROOT_DIR : rootDir; return new ServerConfig(p, rd); } }Same applies to
LoggingConfigwith"INFO".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/config/AppConfig.java` around lines 26 - 37, The defaults (port 8080, root dir "./www", and log level "INFO") are duplicated between ServerConfig.defaults() and withDefaultsApplied(), and similarly in LoggingConfig; extract shared constants (e.g. DEFAULT_PORT, DEFAULT_ROOT_DIR, DEFAULT_LOG_LEVEL) at the class level or a central AppConfig constants holder and use those constants inside ServerConfig.defaults(), ServerConfig.withDefaultsApplied(), and LoggingConfig to set and validate defaults (replace hardcoded 8080, "./www", and "INFO" with the new constants and reuse them for validation and return 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 `@Dockerfile`:
- Line 12: The Dockerfile currently copies compiled classes to root using the
COPY instruction "COPY --from=build /build/target/classes/ /", but the Java
classpath in the ENTRYPOINT expects classes under /app (classpath
"/app:/dependencies/*"), causing ClassNotFoundException; update the COPY to
place the build output into /app (so files from /build/target/classes/ arrive
under /app) so the runtime classpath matches the actual file location.
In `@README.md`:
- Around line 329-336: Update the README section that currently labels "Filter
Chain (Future Feature)" to document the new RedirectFilter, RedirectRule, and
RedirectRulesLoader: replace the "future feature" wording with a brief
description of RedirectFilter behavior, show how RedirectRule entries are
structured and loaded via RedirectRulesLoader, and mention the filter chain
remains extensible for adding more filters (e.g., authentication, logging,
compression); reference the RedirectFilter, RedirectRule, and
RedirectRulesLoader names so readers can find the implementations.
- Line 48: The README's repository clone command contains a duplicated keyword
("git clone git clone ..."); update the command text to a single correct
invocation (e.g., "git clone
https://github.com/ithsjava25/project-webserver-juv25g.git") so the README shows
a valid clone command.
- Around line 37-44: Remove the extra space in the level-2 heading so "## Quick
Start" becomes "## Quick Start", and add appropriate fenced-code block language
identifiers (e.g., text, bash) to each triple-backtick block present in
README.md—specifically the ASCII-art/server launch block under the Quick Start
heading and the other fenced blocks further down (the examples and command
snippets) so they render with proper syntax highlighting and satisfy markdown
lint rules; update only the heading text and the opening backticks for the
fenced blocks (no content changes).
- Line 12: The link text "[Features](`#features`) • [Quick Start](`#quick-start`) •
[Configuration](`#configuration`)" uses the wrong anchor for the "## ✨ Features"
heading; update the Features link to use the generated slug by changing
"(`#features`)" to "(`#-features`)" so the anchor matches the "## ✨ Features"
heading and the link navigates correctly.
In `@src/main/java/org/example/App.java`:
- Line 10: The code currently constructs Path configPath =
Path.of("src/main/resources/application.yml") in App, which fails in packaged
JARs; change App to accept an optional CLI argument for the config file path
(e.g., check args[] for a provided path and use that to build configPath) and if
no arg is provided, load the resource from the classpath instead (use
ClassLoader#getResourceAsStream) and pass that to ConfigLoader as a fallback;
ensure ConfigLoader is used unchanged where it already handles missing
files/defaults so the new logic tries the explicit file first, then the
classpath resource.
In `@src/main/java/org/example/config/ConfigLoader.java`:
- Around line 55-66: The createMapperFor method uses the old Jackson 2.x pattern
YAMLMapper.builder(new YAMLFactory()) which is incompatible with Jackson 3.x;
update both YAMLMapper.builder(new YAMLFactory()) calls in createMapperFor to
YAMLMapper.builder().build() (i.e., call builder() with no args and then
build()), remove the now-unused YAMLFactory import, and ensure the
JsonMapper.builder().build() branch remains unchanged.
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 27-35: The resolveTargetFile method in ConnectionHandler is
appending ".html" to any URI that doesn't end with ".html", which also mangles
assets like "/style.css" or "/image.png"; change the logic in resolveTargetFile
so it only appends ".html" for extensionless paths (e.g., check for absence of a
file extension by comparing lastIndexOf('.') to lastIndexOf('/') or by using a
regex that ensures there is no dot after the last slash) and leave URIs that
already have any extension unchanged; update references to this.uri assignment
accordingly so index root ("/") still maps to "index.html" and only true
extensionless paths get the ".html" suffix.
- Around line 17-25: runConnectionHandler currently parses the request and
directly calls StaticFileHandler.sendGetRequest, skipping the RedirectFilter and
FilterChain; modify runConnectionHandler to build and invoke the filter pipeline
after parsing (use HttpParser/parser to obtain request data and
resolveTargetFile to set uri), instantiate a FilterChain, add RedirectFilter
(and any other filters), then call the chain to process the request and let the
chain decide whether to forward to StaticFileHandler.sendGetRequest or perform a
redirect; ensure the code uses the same request/response objects (from
client.getInputStream()/getOutputStream()) and that resolveTargetFile/uri are
updated only after filters run if filters can mutate the target.
In `@src/main/java/org/example/httpparser/HttpRequest.java`:
- Around line 19-29: The HttpRequest constructor allows a null path which leads
to NPE when RedirectFilter/RedirectRule.matches() calls
Pattern.matcher(requestPath). Add null checks by calling
Objects.requireNonNull(...) on path, method, and version at the start of the
HttpRequest constructor (and import java.util.Objects if missing) so the
constructor enforces non-null contracts and fails fast with a clear message.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 58-66: sendGetRequest currently passes the raw uri to
setContentTypeFromFilename, causing MimeTypeDetector.detectMimeType to see query
strings/fragments (e.g., ".html?foo=bar") and return wrong MIME;
sanitize/normalize the URI before MIME detection by reusing the sanitized value
produced in handleGetRequest (or have handleGetRequest return the sanitized path
or store it on the instance), then call setContentTypeFromFilename with that
sanitized filename/path (ensuring query string and fragment are removed) so
Content-Type is derived from the cleaned filename rather than the original uri.
In `@src/main/java/org/example/TcpServer.java`:
- Around line 32-34: The catch block in TcpServer currently throws a
RuntimeException by concatenating the caught exception into the message, which
discards the original stack trace; change the throw to use the two-argument
constructor so the caught exception becomes the cause (e.g., replace the
existing throw in the client-handling catch with new RuntimeException("Error
handling client connection", e)) to preserve the original exception and stack
trace.
In `@src/test/java/org/example/http/HttpResponseBuilderTest.java`:
- Around line 269-284: The test build_handlesUnknownStatusCodes in
HttpResponseBuilderTest mistakenly treats 100 as an "unknown" status; update the
ValueSource in that parameterized test so it uses only non-standard codes (e.g.,
replace 100 with 299 or 499) to reflect the intent, keeping the test method name
and assertions the same; locate the ValueSource ints array in
build_handlesUnknownStatusCodes and swap the 100 entry for a truly non-standard
code.
In `@src/test/java/org/example/httpparser/HttpParserTest.java`:
- Around line 15-28: The test TestHttpParserForHeaders feeds a full HTTP request
but calls parseHttp() directly, so the request line ("GET /index.html...") is
not consumed properly; update the test to first call httpParser.parseRequest()
after setting the input stream (httpParser.setReader(...)) and then call
httpParser.parseHttp() so the request line is consumed by parseRequest() before
header parsing begins (adjust TestHttpParserForHeaders to invoke parseRequest()
then parseHttp()).
---
Nitpick comments:
In `@Dockerfile`:
- Around line 3-6: Reorder Dockerfile steps to copy pom.xml and download
dependencies before copying source so dependency resolution layer can be cached:
first COPY pom.xml pom.xml, then RUN mvn dependency:copy-dependencies
-DincludeScope=compile (or mvn -B dependency:go-offline) to populate the
dependency layer, and only after that COPY src/ src/ and run mvn compile/build;
update the existing RUN mvn compile and RUN mvn dependency:copy-dependencies
invocations accordingly in the Dockerfile.
In `@pom.xml`:
- Around line 113-115: The surefire argLine uses the property
${org.mockito:mockito-core:jar} which is produced by the maven-dependency-plugin
execution (properties goal) and is fragile if that execution or the Mockito
dependency changes; add a short XML comment near the <argLine> element
explaining this coupling and warning that the property is provided by the
maven-dependency-plugin:properties execution and must be kept in sync with the
Mockito dependency (or replaced with a stable property), and optionally mention
the plugin goal id (maven-dependency-plugin:properties) and the dependency
coordinate (org.mockito:mockito-core) so future maintainers know where the value
originates and how to fix it if unresolved.
In `@src/main/java/org/example/config/AppConfig.java`:
- Around line 26-37: The defaults (port 8080, root dir "./www", and log level
"INFO") are duplicated between ServerConfig.defaults() and
withDefaultsApplied(), and similarly in LoggingConfig; extract shared constants
(e.g. DEFAULT_PORT, DEFAULT_ROOT_DIR, DEFAULT_LOG_LEVEL) at the class level or a
central AppConfig constants holder and use those constants inside
ServerConfig.defaults(), ServerConfig.withDefaultsApplied(), and LoggingConfig
to set and validate defaults (replace hardcoded 8080, "./www", and "INFO" with
the new constants and reuse them for validation and return values).
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 10-11: Change the access modifiers of the fields in
ConnectionHandler: make the package-private Socket client and String uri private
(i.e., private Socket client; private String uri;). Ensure any external access
is replaced with getters/setters if required (but if they are only used inside
ConnectionHandler no further changes are needed) and rebuild to verify no
visibility errors.
In `@src/main/java/org/example/filter/Filter.java`:
- Around line 7-14: The Filter interface currently forces implementors to
override lifecycle methods init() and destroy(), causing unnecessary
boilerplate; change Filter by adding default no-op implementations for init()
and destroy() (while keeping doFilter(HttpRequest request, HttpResponseBuilder
response, FilterChain chain) abstract) so existing implementations can omit
those methods—update the interface's init() and destroy() signatures to be
default methods that do nothing and leave doFilter unchanged.
In `@src/main/java/org/example/filter/FilterChainImpl.java`:
- Around line 20-22: The constructor FilterChainImpl(List<Filter> filters)
stores the caller's list reference directly which allows external mutation;
change it to defensively copy the input by assigning this.filters =
List.copyOf(filters) (which also enforces non-null and rejects null elements) so
the internal filters list is immutable and not affected by caller changes;
update the FilterChainImpl constructor to use List.copyOf and ensure the class
field named filters remains compatible with an unmodifiable/immutable list.
- Around line 29-31: The chain currently no-ops when all filters are exhausted;
add a terminal handler to FilterChainImpl by introducing a final Runnable (or
Consumer<Request>/handler type) field (e.g., terminalHandler), initialize it via
the FilterChainImpl constructor, and replace the empty else block in the method
that advances the chain to call terminalHandler.run() (or invoke the handler)
when index >= filters.size(); also null-check the handler (or require non-null
in the constructor) so the terminal action always executes after all filters
finish.
In `@src/main/java/org/example/filter/redirect/RedirectRule.java`:
- Around line 23-25: The matches method in RedirectRule currently calls
sourcePattern.matcher(requestPath).matches() which will throw a
NullPointerException if requestPath is null; update RedirectRule.matches to
defensively handle null input (e.g., if requestPath == null return false or
throw a clear IllegalArgumentException) before calling sourcePattern.matcher,
referencing the matches method and sourcePattern.matcher to locate the change.
In `@src/main/java/org/example/filter/redirect/RedirectRulesLoader.java`:
- Around line 34-45: The wildcardToRegex method currently calls Pattern.quote
for each non-'*' character which produces verbose regex; change it to accumulate
consecutive literal characters into a buffer and call Pattern.quote once for
each literal run, appending "[^/]*" for '*' characters as before; update the
method (wildcardToRegex) to iterate chars, build a literal chunk string, flush
it through Pattern.quote when you hit a '*' or end-of-string, and append the
quoted chunk to the StringBuilder to produce a much cleaner regex.
- Around line 10-31: The compileSourcePattern method currently wraps the
generated regex with "^" and "$" before compiling, which is redundant because
RedirectRule.matches() uses Pattern.matcher(requestPath).matches() that already
enforces full-string matching; update compileSourcePattern (and any callers like
wildcardToRegex usage) to stop prepending "^" and "$" and simply return
Pattern.compile(regex) for both wildcard and quoted paths, or alternatively add
a comment in compileSourcePattern explaining the explicit anchors are
unnecessary but intentionally kept — reference the compileSourcePattern and
wildcardToRegex symbols and ensure unit tests for RedirectRule.matches() still
validate the intended behavior after the change.
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 45-48: setBody(byte[]) currently stores the caller's byte array
reference in the bytebody field which allows external mutation; change
setBody(byte[] body) to make a defensive copy (e.g., Arrays.copyOf(body,
body.length) or body.clone()) before assigning to the bytebody field and keep
resetting the string body to ""; also handle null input by setting bytebody to
null. Ensure you update only the setBody method and reference the bytebody field
and body field in your change.
- Around line 64-67: The block comment above the response-building routine in
HttpResponseBuilder is a regular C-style comment (/*) so Javadoc tools will
ignore it; change it to a Javadoc comment (/** ... */) and ensure the `@return`
tag is properly formatted inside that Javadoc so the method (e.g., the
response-building method in class HttpResponseBuilder such as buildResponse or
build) is documented correctly and the description notes that it returns the
complete HTTP response as a byte[] preserving binary content.
In `@src/main/java/org/example/http/MimeTypeDetector.java`:
- Around line 59-65: The default MIME string "application/octet-stream" is
duplicated in detectMimeType and other return paths; extract it into a private
static final String DEFAULT_MIME_TYPE constant (e.g., in class MimeTypeDetector)
and replace literal usages in the detectMimeType(String filename) method (and
any other methods in MimeTypeDetector that return the default) with
DEFAULT_MIME_TYPE to improve clarity and avoid duplication.
In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Around line 40-45: The debug printing block in HttpParser uses
System.out.println and a private debug flag that defaults to false with no
setter, so this code is dead and not using a logging framework; update the class
(HttpParser) to either remove the block or replace it with a proper logger
(e.g., java.util.logging.Logger or SLF4J) and expose control via a setter or
configuration method (e.g., setDebug(boolean) or configureLogger), and change
the prints to logger.debug/info calls that reference headersMap and "Host"
appropriately so runtime logging is configurable and framework-based.
- Around line 53-55: getHeadersMap() in HttpParser currently returns the mutable
internal headersMap, letting callers mutate parser state; change it to return an
unmodifiable view (e.g., Collections.unmodifiableMap(headersMap) or
Map.copyOf(headersMap)) from the HttpParser#getHeadersMap method so callers
cannot modify the internal map, and keep the internal field mutable for parser
operations.
- Around line 16-20: The setReader method in HttpParser currently ignores
subsequent calls because of the null guard on reader; change it to fail fast by
throwing an IllegalStateException when reader is already set (e.g., if
(this.reader != null) throw new IllegalStateException("HttpParser already
initialized with a reader")); otherwise, if you prefer re-use semantics, allow
re-assignment by replacing the reader assignment with unconditional
reinitialization and also reset internal parsing state (e.g., clear the headers
map field via headers.clear() and any other state) before setting this.reader to
the new BufferedReader; update the setReader method accordingly.
In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java`:
- Line 15: In HttpParseRequestLine, remove the unnecessary alias "BufferedReader
reader = br;" and update any references to "reader" to use "br" directly (or
eliminate the variable use entirely where possible); ensure no other code
depends on the "reader" symbol and delete the redundant declaration to avoid
dead/duplicate variables.
- Line 11: The debug flag in class HttpParseRequestLine is hardcoded false,
making the logging block (lines 34–38) dead; fix by exposing and using a mutable
debug control: add a public setter setDebug(boolean) (or a constructor
parameter) and update any callers to enable debug when needed, then guard the
existing logging block with this.debug so the logs can be emitted;
alternatively, if debug logging is undesired, remove the debug field and the
unreachable logging block (referencing debug, setDebug, and the logging
statements in HttpParseRequestLine).
In `@src/main/java/org/example/httpparser/HttpRequest.java`:
- Around line 11-41: The HttpRequest class is indented with 8 spaces throughout
(looks copied from a nested class); reformat the file to use the project
standard 4-space indentation for the class body and its members (class
HttpRequest, its constructor HttpRequest(...), and all getters
getMethod/getPath/getVersion/getHeaders/getBody). Adjust the whitespace for the
field declarations, constructor parameters/body and each getter, then run the
project's Java formatter or IDE reformat action to ensure consistent indentation
and styling.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 10-13: The class uses mutable instance fields fileBytes and
statusCode to pass data between handleGetRequest and sendGetRequest and exposes
an instance field WEB_ROOT named like a constant; refactor by introducing an
immutable result type (e.g., record FileResult(byte[] content, int status))
returned from handleGetRequest, update sendGetRequest to accept a FileResult (or
its content and status) instead of reading instance fields, remove the fileBytes
and statusCode fields, and rename WEB_ROOT to webRoot (and keep it final if
intended immutable) updating all usages in StaticFileHandler.
In `@src/main/java/org/example/TcpServer.java`:
- Around line 20-22: The virtual-thread lambda that starts handleClient via
Thread.ofVirtual().start(() -> handleClient(clientSocket)) does not observe
exceptions; wrap the call to handleClient(clientSocket) in a try/catch inside
the lambda (or install a
Thread.setDefaultUncaughtExceptionHandler/Thread.UncaughtExceptionHandler for
these virtual threads) and log the exception with your server logger including
clientSocket.getRemoteSocketAddress(), ensure the clientSocket is closed in a
finally block to free resources, and propagate/record any relevant metrics or
status for per-connection failures.
In `@src/test/java/org/example/config/ConfigLoaderTest.java`:
- Around line 128-140: Rename and clean up the test method
invalid_port_should_Throw_Exception: change its name to follow the file's naming
convention (e.g., invalidPortShouldThrowException or
invalidPortThrowsException), remove the extraneous whitespace before the
parentheses, and delete the unnecessary "throws Exception" declaration since
assertThatThrownBy handles exceptions; leave the assertion using
ConfigLoader.loadOnce(configFile) and assertThatThrownBy unchanged.
In `@src/test/java/org/example/http/MimeTypeDetectorTest.java`:
- Line 145: The comment in MimeTypeDetectorTest contains a Swedish line
"Parametriserad test för många filtyper på en gång"; update this to English for
consistency (e.g., "Parameterized test for many file types at once") in the test
class MimeTypeDetectorTest so the codebase uses a single language for comments;
locate the comment near the parameterized test method in MimeTypeDetectorTest
and replace the Swedish text with the English equivalent.
In `@src/test/java/org/example/httpparser/HttpParseRequestLineTest.java`:
- Around line 8-10: The test imports mix AssertJ and JUnit assertions;
consolidate to AssertJ only by removing JUnit assertions import and replacing
uses of JUnit's assertThrows + subsequent assertThat on the exception message
with a single assertThatThrownBy call; update tests in HttpParseRequestLineTest
(and any methods that call assertThrows) to use
assertThatThrownBy(...).isInstanceOf(...).hasMessageContaining(...) or
equivalent AssertJ assertions so only assertThat/assertThatThrownBy from AssertJ
are used.
- Around line 20-31: Tests in HttpParseRequestLineTest call
testString.getBytes() (e.g., inside testParserWithTestRequestLine) which uses
the platform default charset and can cause flaky behavior; update each test to
encode the request byte streams with an explicit charset by replacing getBytes()
with getBytes(StandardCharsets.UTF_8) in testParserWithTestRequestLine and the
other test methods referenced (lines ~41, ~53, ~64) so the InputStream fed to
httpParseRequestLine.setReader(...) matches the production UTF-8 decoding.
In `@src/test/java/org/example/httpparser/HttpParserTest.java`:
- Line 16: Method name TestHttpParserForHeaders violates Java naming
conventions; rename the test method declaration from TestHttpParserForHeaders to
testHttpParserForHeaders and update any references/usages (e.g., in annotations
or reflective lookups) to match the new camelCase name so the test framework
still discovers and runs it.
- Around line 9-10: The test mixes AssertJ and JUnit assertions; in
HttpParserTest remove the static import of JUnit assertions and convert all
usages of JUnit methods (e.g., assertNotNull, assertEquals, assertTrue,
assertFalse) to AssertJ equivalents using assertThat (e.g.,
assertThat(obj).isNotNull(), assertThat(actual).isEqualTo(expected),
assertThat(cond).isTrue(), assertThat(cond).isFalse()). Ensure the file keeps
the existing static import import static
org.assertj.core.api.AssertionsForClassTypes.assertThat; and update every
assertion site in HttpParserTest to use assertThat so imports and assertions are
consistent.
In `@www/pageNotFound.html`:
- Around line 47-53: Add a simple navigational anchor inside the existing <div
class="container"> so users can return to the site—insert a link (e.g., an <a
href="/">Go back home</a>) after the paragraph in the 404 markup (the elements
h1, h2, p inside the container) and optionally give it a class like "home-link"
for styling; update any CSS if needed to match site styles.
Dockerfile
Outdated
| EXPOSE 8080 | ||
| RUN addgroup -S appgroup && adduser -S appuser -G appgroup | ||
| WORKDIR /app/ | ||
| COPY --from=build /build/target/classes/ / |
There was a problem hiding this comment.
Classes copied to / but classpath expects /app — the app won't start.
COPY --from=build /build/target/classes/ / places compiled classes at the filesystem root (e.g., /org/example/App.class). The ENTRYPOINT classpath is /app:/dependencies/*, so Java will look for org.example.App under /app/org/example/App.class and fail with ClassNotFoundException.
🐛 Proposed fix
-COPY --from=build /build/target/classes/ /
+COPY --from=build /build/target/classes/ /app/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` at line 12, The Dockerfile currently copies compiled classes to
root using the COPY instruction "COPY --from=build /build/target/classes/ /",
but the Java classpath in the ENTRYPOINT expects classes under /app (classpath
"/app:/dependencies/*"), causing ClassNotFoundException; update the COPY to
place the build output into /app (so files from /build/target/classes/ arrive
under /app) so the runtime classpath matches the actual file location.
README.md
Outdated
|
|
||
| *A modern, high-performance HTTP web server built from scratch in Java* | ||
|
|
||
| [Features](#features) • [Quick Start](#quick-start) • [Configuration](#configuration) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat README.mdRepository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 9195
🌐 Web query:
GitHub anchor generation emoji heading rules
💡 Result:
GitHub’s auto-generated heading anchors treat emoji as punctuation, so emoji characters themselves don’t become part of the slug; they only influence whether a hyphen is inserted (via surrounding whitespace).
Rules (as observed/documented by reverse-engineering and GitHub-compatible slug implementations):
- Lowercase the heading text. [1]
- Define “word” characters as letters/digits/underscore/hyphen; everything else (including emoji) counts as punctuation. [1]
- Convert the heading into an anchor by:
- Removing punctuation-only runs without spaces/tabs (no hyphen inserted). [1]
- Replacing any run of spaces/tabs and/or punctuation that includes at least one space/tab with a single
-. [1]
What that means for emoji (examples):
## Hello 😄 World→#hello-world(emoji dropped; spaces cause-). (Consistent with the rule above. [1])## Hello😄World→#helloworld(emoji dropped; no spaces, so nothing becomes-). [1]## 😄 emoji→#-emoji(leading emoji dropped; the following space becomes a leading-). [2]
Duplicates:
- Repeated headings get
-1,-2, ... appended to keep IDs unique (e.g.,foo, thenfoo-1, thenfoo-2). [2]
Note on the actual HTML GitHub emits:
- GitHub typically prefixes the generated anchor element’s
idwithuser-content-(e.g.user-content-hello-world) while the permalink you use in Markdown/URLs is usually the unprefixed fragment#hello-world. [3]
Sources: [1] [2] [3]
Change #features anchor to #-features.
The heading ## ✨ Features generates the anchor #-features (not #features), because GitHub treats the emoji as punctuation; the space after the emoji produces a leading hyphen in the anchor slug. The current link will not navigate correctly.
Suggested fix
-[Features](`#features`) • [Quick Start](`#quick-start`) • [Configuration](`#configuration`)
+[Features](`#-features`) • [Quick Start](`#quick-start`) • [Configuration](`#configuration`)The other two links (#quick-start and #configuration) are correct.
📝 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.
| [Features](#features) • [Quick Start](#quick-start) • [Configuration](#configuration) | |
| [Features](`#-features`) • [Quick Start](`#quick-start`) • [Configuration](`#configuration`) |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 12-12: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 12, The link text "[Features](`#features`) • [Quick
Start](`#quick-start`) • [Configuration](`#configuration`)" uses the wrong anchor
for the "## ✨ Features" heading; update the Features link to use the generated
slug by changing "(`#features`)" to "(`#-features`)" so the anchor matches the "## ✨
Features" heading and the link navigates correctly.
README.md
Outdated
| ## Quick Start | ||
|
|
||
| ``` | ||
| ┌─────────────────────────────────────────────┐ | ||
| │ Ready to launch your web server? │ | ||
| │ Follow these simple steps! │ | ||
| └─────────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Several markdown lint issues flagged by static analysis.
- Line 37: Extra space after
##in heading (## Quick Start→## Quick Start). - Multiple fenced code blocks (Lines 39, 80, 87, 134, 234, 283, 340) lack a language identifier — add
text,bash, etc. as appropriate for proper syntax highlighting and accessibility.
Also applies to: 39-43, 80-82, 87-89, 134-157, 234-239, 283-290, 340-342
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 37-37: Multiple spaces after hash on atx style heading
(MD019, no-multiple-space-atx)
[warning] 39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 37 - 44, Remove the extra space in the level-2
heading so "## Quick Start" becomes "## Quick Start", and add appropriate
fenced-code block language identifiers (e.g., text, bash) to each
triple-backtick block present in README.md—specifically the ASCII-art/server
launch block under the Quick Start heading and the other fenced blocks further
down (the examples and command snippets) so they render with proper syntax
highlighting and satisfy markdown lint rules; update only the heading text and
the opening backticks for the fenced blocks (no content changes).
README.md
Outdated
| This works across all Java versions and forms the foundation of most Java programs. | ||
| ### 1. Clone the repository | ||
| ```bash | ||
| git clone git clone https://github.com/ithsjava25/project-webserver-juv25g.git |
There was a problem hiding this comment.
Duplicate git clone in the clone command.
-git clone git clone https://github.com/ithsjava25/project-webserver-juv25g.git
+git clone https://github.com/ithsjava25/project-webserver-juv25g.git📝 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.
| git clone git clone https://github.com/ithsjava25/project-webserver-juv25g.git | |
| git clone https://github.com/ithsjava25/project-webserver-juv25g.git |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 48, The README's repository clone command contains a
duplicated keyword ("git clone git clone ..."); update the command text to a
single correct invocation (e.g., "git clone
https://github.com/ithsjava25/project-webserver-juv25g.git") so the README shows
a valid clone command.
README.md
Outdated
| ### Filter Chain (Future Feature) | ||
|
|
||
| The project includes a filter chain interface for future extensibility: | ||
| - Request/response filtering | ||
| - Authentication | ||
| - Logging | ||
| - Compression | ||
|
|
There was a problem hiding this comment.
"Filter Chain (Future Feature)" is inaccurate — this PR adds a working RedirectFilter.
The README describes the filter chain as a future feature, but the PR itself introduces RedirectFilter, RedirectRule, and RedirectRulesLoader as working functionality. Update this section to document the redirect filter and its configuration, and mention that the filter chain is extensible for additional filters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 329 - 336, Update the README section that currently
labels "Filter Chain (Future Feature)" to document the new RedirectFilter,
RedirectRule, and RedirectRulesLoader: replace the "future feature" wording with
a brief description of RedirectFilter behavior, show how RedirectRule entries
are structured and loaded via RedirectRulesLoader, and mention the filter chain
remains extensible for adding more filters (e.g., authentication, logging,
compression); reference the RedirectFilter, RedirectRule, and
RedirectRulesLoader names so readers can find the implementations.
| public HttpRequest(String method, | ||
| String path, | ||
| String version, | ||
| Map<String, String> headers, | ||
| String body) { | ||
| this.method = method; | ||
| this.path = path; | ||
| this.version = version; | ||
| this.headers = headers != null ? Map.copyOf(headers) : Collections.emptyMap(); | ||
| this.body = body; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find HttpRequest class and examine its structure
find . -name "HttpRequest.java" -type fRepository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 215
🏁 Script executed:
# Search for RedirectRule and RedirectFilter to understand the flow
rg -n --type=java "class RedirectRule|class RedirectFilter" -A 5Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 4989
🏁 Script executed:
# Check how HttpRequest is instantiated in the codebase
rg -n --type=java 'new HttpRequest\(' -B 2 -A 2Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 2151
🏁 Script executed:
# Find RedirectRule.matches() to see how it uses the path
rg -n --type=java "matches\(" --context=5 | grep -A 10 -B 5 "path\|Path"Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 8719
🏁 Script executed:
cat -n src/main/java/org/example/httpparser/HttpRequest.javaRepository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 1592
🏁 Script executed:
# Check for `@NonNull/`@Nullable annotations in the httpparser package
rg -n "@NonNull|@Nullable|Objects.requireNonNull" src/main/java/org/example/httpparser/ -A 2 -B 2Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 90
🏁 Script executed:
# Check how the path is retrieved/accessed
rg -n "public.*path\(\)|getPath()" src/main/java/org/example/httpparser/ -A 3Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 463
🏁 Script executed:
# Check HttpParseRequestLine to see if it guarantees non-null path
cat -n src/main/java/org/example/httpparser/HttpParseRequestLine.javaRepository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 2290
🏁 Script executed:
# Check for any tests or places where null path might be passed
rg -n "HttpRequest.*null\|new HttpRequest\(" -A 5 src/ | head -50Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 90
🏁 Script executed:
# Check if there's validation in the parser
rg -n "path\s*=\s*null\|path.*null" src/main/java/org/example/httpparser/ -B 3 -A 3Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 90
🏁 Script executed:
# Check the complete RedirectFilter.doFilter() method for null checks
rg -n "class RedirectFilter" src/main/java/org/example/ -A 30 | grep -A 30 "doFilter"Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 4569
🏁 Script executed:
# Check RedirectRule.matches() implementation completely
rg -n "public boolean matches" src/main/java/org/example/ -B 2 -A 5Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 1480
Add null validation for path in the HttpRequest constructor.
The constructor accepts a nullable path parameter without validation. Since RedirectFilter passes the path directly to RedirectRule.matches(), which calls Pattern.matcher(requestPath).matches(), a null path will cause NullPointerException. Add Objects.requireNonNull() for path, method, and version to enforce non-null contracts at construction time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/httpparser/HttpRequest.java` around lines 19 - 29,
The HttpRequest constructor allows a null path which leads to NPE when
RedirectFilter/RedirectRule.matches() calls Pattern.matcher(requestPath). Add
null checks by calling Objects.requireNonNull(...) on path, method, and version
at the start of the HttpRequest constructor (and import java.util.Objects if
missing) so the constructor enforces non-null contracts and fails fast with a
clear message.
| public void sendGetRequest(OutputStream outputStream, String uri) throws IOException { | ||
| handleGetRequest(uri); | ||
| HttpResponseBuilder response = new HttpResponseBuilder(); | ||
| response.setStatusCode(statusCode); | ||
| // Use MimeTypeDetector instead of hardcoded text/html | ||
| response.setContentTypeFromFilename(uri); | ||
| response.setBody(fileBytes); | ||
| outputStream.write(response.build()); | ||
| outputStream.flush(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "StaticFileHandler.java" -o -name "MimeTypeDetector.java" -o -name "StaticFileHandlerTest.java" | head -20Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 278
🏁 Script executed:
find . -name "StaticFileHandler.java" -o -name "MimeTypeDetector.java" -o -name "StaticFileHandlerTest.java"Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 268
🏁 Script executed:
cat -n ./src/main/java/org/example/StaticFileHandler.javaRepository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 2818
🏁 Script executed:
cat -n ./src/main/java/org/example/http/MimeTypeDetector.javaRepository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 3201
🏁 Script executed:
cat -n ./src/test/java/org/example/StaticFileHandlerTest.javaRepository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 6628
🏁 Script executed:
cat -n ./src/main/java/org/example/http/HttpResponseBuilder.javaRepository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 4509
Content-Type is derived from the unsanitized URI — MIME detection breaks for URIs with query strings or fragments.
sendGetRequest passes the original uri (e.g., "index.html?foo=bar") to setContentTypeFromFilename. Since handleGetRequest sanitizes only its local uri variable without modifying the method parameter, the unsanitized string reaches MimeTypeDetector.detectMimeType(), which extracts the extension as ".html?foo=bar". This won't match any MIME type in the map, defaulting to application/octet-stream instead of text/html; charset=UTF-8.
The test sanitized_uris_should_return_200 passes URIs with query strings but only asserts the status code, not the Content-Type header, leaving this bug undetected.
Proposed fix — sanitize uri before MIME detection
public void sendGetRequest(OutputStream outputStream, String uri) throws IOException {
+ // Sanitize for Content-Type detection (strip query/fragment)
+ String sanitized = uri;
+ int q = sanitized.indexOf('?');
+ if (q >= 0) sanitized = sanitized.substring(0, q);
+ int h = sanitized.indexOf('#');
+ if (h >= 0) sanitized = sanitized.substring(0, h);
+
handleGetRequest(uri);
HttpResponseBuilder response = new HttpResponseBuilder();
response.setStatusCode(statusCode);
- response.setContentTypeFromFilename(uri);
+ response.setContentTypeFromFilename(sanitized);
response.setBody(fileBytes);
outputStream.write(response.build());
outputStream.flush();
}A cleaner approach would be to have handleGetRequest return a result object or store the sanitized URI as a field to avoid duplication.
📝 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.
| public void sendGetRequest(OutputStream outputStream, String uri) throws IOException { | |
| handleGetRequest(uri); | |
| HttpResponseBuilder response = new HttpResponseBuilder(); | |
| response.setStatusCode(statusCode); | |
| // Use MimeTypeDetector instead of hardcoded text/html | |
| response.setContentTypeFromFilename(uri); | |
| response.setBody(fileBytes); | |
| outputStream.write(response.build()); | |
| outputStream.flush(); | |
| public void sendGetRequest(OutputStream outputStream, String uri) throws IOException { | |
| // Sanitize for Content-Type detection (strip query/fragment) | |
| String sanitized = uri; | |
| int q = sanitized.indexOf('?'); | |
| if (q >= 0) sanitized = sanitized.substring(0, q); | |
| int h = sanitized.indexOf('#'); | |
| if (h >= 0) sanitized = sanitized.substring(0, h); | |
| handleGetRequest(uri); | |
| HttpResponseBuilder response = new HttpResponseBuilder(); | |
| response.setStatusCode(statusCode); | |
| response.setContentTypeFromFilename(sanitized); | |
| response.setBody(fileBytes); | |
| outputStream.write(response.build()); | |
| outputStream.flush(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/StaticFileHandler.java` around lines 58 - 66,
sendGetRequest currently passes the raw uri to setContentTypeFromFilename,
causing MimeTypeDetector.detectMimeType to see query strings/fragments (e.g.,
".html?foo=bar") and return wrong MIME; sanitize/normalize the URI before MIME
detection by reusing the sanitized value produced in handleGetRequest (or have
handleGetRequest return the sanitized path or store it on the instance), then
call setContentTypeFromFilename with that sanitized filename/path (ensuring
query string and fragment are removed) so Content-Type is derived from the
cleaned filename rather than the original uri.
| } catch (Exception e) { | ||
| throw new RuntimeException("Error handling client connection " + e); | ||
| } |
There was a problem hiding this comment.
Original exception stack trace is lost — use the two-arg RuntimeException constructor.
"Error handling client connection " + e stringifies the exception. The original cause and its stack trace are discarded, making debugging much harder.
Proposed fix
- throw new RuntimeException("Error handling client connection " + e);
+ throw new RuntimeException("Error handling client connection", e);📝 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.
| } catch (Exception e) { | |
| throw new RuntimeException("Error handling client connection " + e); | |
| } | |
| } catch (Exception e) { | |
| throw new RuntimeException("Error handling client connection", e); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/TcpServer.java` around lines 32 - 34, The catch
block in TcpServer currently throws a RuntimeException by concatenating the
caught exception into the message, which discards the original stack trace;
change the throw to use the two-argument constructor so the caught exception
becomes the cause (e.g., replace the existing throw in the client-handling catch
with new RuntimeException("Error handling client connection", e)) to preserve
the original exception and stack trace.
| // Unknown status codes | ||
| @ParameterizedTest | ||
| @ValueSource(ints = {999, 123, 777, 100, 600}) | ||
| @DisplayName("Should handle unknown status codes gracefully") | ||
| void build_handlesUnknownStatusCodes(int statusCode) { | ||
| HttpResponseBuilder builder = new HttpResponseBuilder(); | ||
| builder.setStatusCode(statusCode); | ||
| builder.setBody(""); | ||
|
|
||
| byte[] result = builder.build(); | ||
| String resultStr = asString(result); | ||
|
|
||
| assertThat(resultStr) | ||
| .startsWith("HTTP/1.1 " + statusCode) | ||
| .doesNotContain("OK"); | ||
| } |
There was a problem hiding this comment.
Status code 100 is a valid HTTP status code (Continue), not "unknown".
The test is titled "Should handle unknown status codes gracefully" but 100 is a standard HTTP status. The test may still pass (the builder likely won't emit "OK" for it), but it misrepresents the intent. Consider replacing 100 with a truly non-standard code (e.g., 299 or 499).
- `@ValueSource`(ints = {999, 123, 777, 100, 600})
+ `@ValueSource`(ints = {999, 123, 777, 299, 600})📝 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.
| // Unknown status codes | |
| @ParameterizedTest | |
| @ValueSource(ints = {999, 123, 777, 100, 600}) | |
| @DisplayName("Should handle unknown status codes gracefully") | |
| void build_handlesUnknownStatusCodes(int statusCode) { | |
| HttpResponseBuilder builder = new HttpResponseBuilder(); | |
| builder.setStatusCode(statusCode); | |
| builder.setBody(""); | |
| byte[] result = builder.build(); | |
| String resultStr = asString(result); | |
| assertThat(resultStr) | |
| .startsWith("HTTP/1.1 " + statusCode) | |
| .doesNotContain("OK"); | |
| } | |
| // Unknown status codes | |
| `@ParameterizedTest` | |
| `@ValueSource`(ints = {999, 123, 777, 299, 600}) | |
| `@DisplayName`("Should handle unknown status codes gracefully") | |
| void build_handlesUnknownStatusCodes(int statusCode) { | |
| HttpResponseBuilder builder = new HttpResponseBuilder(); | |
| builder.setStatusCode(statusCode); | |
| builder.setBody(""); | |
| byte[] result = builder.build(); | |
| String resultStr = asString(result); | |
| assertThat(resultStr) | |
| .startsWith("HTTP/1.1 " + statusCode) | |
| .doesNotContain("OK"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/example/http/HttpResponseBuilderTest.java` around lines 269
- 284, The test build_handlesUnknownStatusCodes in HttpResponseBuilderTest
mistakenly treats 100 as an "unknown" status; update the ValueSource in that
parameterized test so it uses only non-standard codes (e.g., replace 100 with
299 or 499) to reflect the intent, keeping the test method name and assertions
the same; locate the ValueSource ints array in build_handlesUnknownStatusCodes
and swap the 100 entry for a truly non-standard code.
| @Test | ||
| void TestHttpParserForHeaders() throws IOException { | ||
| String testInput = "GET /index.html HTTP/1.1\r\nHost: localhost\r\nContent-Type: text/plain\r\nUser-Agent: JUnit5\r\n\r\n"; | ||
| InputStream in = new ByteArrayInputStream(testInput.getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| httpParser.setReader(in); | ||
| httpParser.parseHttp(); | ||
|
|
||
| assertNotNull(httpParser.getHeadersMap()); | ||
| assertThat(httpParser.getHeadersMap().size()).isEqualTo(3); | ||
| assertThat(httpParser.getHeadersMap().get("Host")).contains("localhost"); | ||
| assertThat(httpParser.getHeadersMap().get("Content-Type")).contains("text/plain"); | ||
| assertThat(httpParser.getHeadersMap().get("User-Agent")).contains("JUnit5"); | ||
| } |
There was a problem hiding this comment.
Test parses headers without first consuming the request line.
parseHttp() is called without a preceding parseRequest() call. The request line "GET /index.html HTTP/1.1" is fed to the header parser and happens to be skipped because it has no colon—but this is accidental. A proper test should call parseRequest() first (as the real usage flow would), then parseHttp(), so the request line is consumed before header parsing begins. Otherwise this test silently breaks if the request line format ever contains a colon.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/example/httpparser/HttpParserTest.java` around lines 15 -
28, The test TestHttpParserForHeaders feeds a full HTTP request but calls
parseHttp() directly, so the request line ("GET /index.html...") is not consumed
properly; update the test to first call httpParser.parseRequest() after setting
the input stream (httpParser.setReader(...)) and then call
httpParser.parseHttp() so the request line is consumed by parseRequest() before
header parsing begins (adjust TestHttpParserForHeaders to invoke parseRequest()
then parseHttp()).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/example/filter/redirect/RedirectFilter.java`:
- Line 33: The Location header is set directly from rule.getTargetUrl() without
stripping CR/LF, enabling HTTP response splitting; update the code that sets the
header in RedirectFilter (the response.setHeader("Location",
rule.getTargetUrl()) call) to sanitize the target URL by removing or replacing
CR and LF characters (same approach used for path.replaceAll("[\\r\\n]", "_")),
or ensure RedirectRule's constructor/validator strips CR/LF from its targetUrl;
locate RedirectRule and add equivalent replaceAll("[\\r\\n]", "_") sanitization
on targetUrl at construction time if not already present, or sanitize inside the
RedirectFilter before calling response.setHeader.
- Around line 29-36: Restore the missing guard so the redirect only runs when a
rule matches: wrap the existing redirect logic inside an if (rule.matches(path))
{ ... } block in RedirectFilter (the for loop iterating RedirectRule rule :
rules), ensuring the opening and closing braces are balanced so
chain.doFilter(request, response) remains inside doFilter and the method
compiles; keep the sanitizedPath/logging, response.setStatusCode(...),
response.setHeader("Location", ...), and return; only executed when
rule.matches(path) is true.
| final String sanitizedPath = path.replaceAll("[\\r\\n]", "_"); | ||
| LOG.info(() -> "Redirecting " + sanitizedPath + " -> " + rule.getTargetUrl() + " (" + rule.getStatusCode() + ")"); | ||
| response.setStatusCode(rule.getStatusCode()); | ||
| response.setHeader("Location", rule.getTargetUrl()); |
There was a problem hiding this comment.
targetUrl set in Location header without CRLF sanitization — potential HTTP response splitting
The request path is sanitized (path.replaceAll("[\\r\\n]", "_")), but rule.getTargetUrl() is injected directly into the Location header with no equivalent stripping. If RedirectRule does not validate or sanitize the URL on construction, an attacker who can influence redirect targets could inject arbitrary headers.
Verify whether RedirectRule strips CR/LF from targetUrl at construction time:
#!/bin/bash
# Locate RedirectRule and check for CR/LF handling in targetUrl
fd -e java RedirectRule --exec rg -n "targetUrl|replaceAll|\\\\r|\\\\n|CRLF|sanitize" {}If no sanitization is present, apply the same guard used for path:
🛡️ Proposed fix — sanitize targetUrl before setting the header
- response.setHeader("Location", rule.getTargetUrl());
+ response.setHeader("Location", rule.getTargetUrl().replaceAll("[\\r\\n]", "_"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/filter/redirect/RedirectFilter.java` at line 33,
The Location header is set directly from rule.getTargetUrl() without stripping
CR/LF, enabling HTTP response splitting; update the code that sets the header in
RedirectFilter (the response.setHeader("Location", rule.getTargetUrl()) call) to
sanitize the target URL by removing or replacing CR and LF characters (same
approach used for path.replaceAll("[\\r\\n]", "_")), or ensure RedirectRule's
constructor/validator strips CR/LF from its targetUrl; locate RedirectRule and
add equivalent replaceAll("[\\r\\n]", "_") sanitization on targetUrl at
construction time if not already present, or sanitize inside the RedirectFilter
before calling response.setHeader.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/org/example/filter/redirect/RedirectRulesLoader.java (3)
15-15: Usestrip()consistently instead oftrim().Line 15 uses
trim()while line 18 usesstrip().trim()only removes ASCII control characters (<= '\u0020'), whereasstrip()is Unicode-aware and is the modern Java replacement.♻️ Proposed fix
- String trimmed = sourcePath.trim(); + String trimmed = sourcePath.strip();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectRulesLoader.java` at line 15, In RedirectRulesLoader replace the use of String trimmed = sourcePath.trim() with the Unicode-aware strip variant to match the rest of the class (use sourcePath.strip()); update any related variables or usages of trimmed if needed so the code consistently uses strip() instead of trim() across the class/method that handles sourcePath.
34-45: Batch consecutive literal characters into a singlePattern.quotecall.The current loop calls
Pattern.quote(String.valueOf(c))for every non-wildcard character individually, producing verbose output like\Q/\E\Qa\E\Qp\E\Qi\Einstead of\Q/api\E. Collecting runs of literal characters and quoting them in one call is cleaner and avoids redundant open/close\Q…\Epairs.♻️ Proposed refactor
- private static String wildcardToRegex(String wildcard) { - StringBuilder sb = new StringBuilder(); - for (int i = 0; i < wildcard.length(); i++) { - char c = wildcard.charAt(i); - if (c == '*') { - sb.append("[^/]*"); - } else { - sb.append(Pattern.quote(String.valueOf(c))); - } - } - return sb.toString(); - } + private static String wildcardToRegex(String wildcard) { + StringBuilder sb = new StringBuilder(); + StringBuilder literal = new StringBuilder(); + for (int i = 0; i < wildcard.length(); i++) { + char c = wildcard.charAt(i); + if (c == '*') { + if (!literal.isEmpty()) { + sb.append(Pattern.quote(literal.toString())); + literal.setLength(0); + } + sb.append("[^/]*"); + } else { + literal.append(c); + } + } + if (!literal.isEmpty()) { + sb.append(Pattern.quote(literal.toString())); + } + return sb.toString(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectRulesLoader.java` around lines 34 - 45, The wildcardToRegex method currently quotes each non-wildcard character individually; refactor wildcardToRegex to batch consecutive literal characters into a single substring and call Pattern.quote once per run: iterate the input, accumulate a start index (or StringBuilder) for literal runs, when you hit '*' flush the accumulated literal via Pattern.quote(sb.toString()) to the main StringBuilder, append "[^/]*" for '*' and continue, and after the loop flush any remaining literal run; this produces fewer \Q...\E segments while preserving the existing '*' → "[^/]*" behavior.
17-23: Raw regex branch silently omits anchors; consider adding them for parity.The literal and wildcard branches both wrap their result with
^...$, but theregex:branch does not. This is not a functional bug today —RedirectRule.matches()usesMatcher.matches()which anchors the full input implicitly. However, if the call site is ever changed toMatcher.find(), the raw regex branch would silently change semantics to substring matching while the other branches continue to anchor. Wrapping raw input with anchors (or at least documenting the omission) makes the three branches consistent and resilient to future changes.♻️ Proposed fix
- return Pattern.compile(rawRegex); + return Pattern.compile("^(?:" + rawRegex + ")$");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectRulesLoader.java` around lines 17 - 23, The regex branch in RedirectRulesLoader currently compiles rawRegex without anchors, making it inconsistent with the literal/wildcard branches that wrap results with ^...$; change the Pattern.compile(rawRegex) call to compile("^" + rawRegex + "$") so the raw regex branch is anchored the same way (keep the existing blank check for rawRegex and use REGEX_PREFIX/rawRegex as the referenced symbols).src/main/java/org/example/filter/redirect/RedirectRule.java (1)
17-19: Consider supporting 307 and 308 redirect codes.The constraint
statusCode != 301 && statusCode != 302rules out 307 (Temporary Redirect) and 308 (Permanent Redirect), both of which are widely used standard codes. If the server is intended to evolve beyond a simple redirect filter, accepting all semantically valid redirect codes(301, 302, 303, 307, 308)avoids a follow-up breaking change.♻️ Proposed fix
- if (statusCode != 301 && statusCode != 302) { - throw new IllegalArgumentException("statusCode must be 301 or 302"); + if (statusCode != 301 && statusCode != 302 && statusCode != 303 + && statusCode != 307 && statusCode != 308) { + throw new IllegalArgumentException("statusCode must be a valid redirect code (301, 302, 303, 307, or 308)"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectRule.java` around lines 17 - 19, The constructor/validation in RedirectRule currently rejects any status code other than 301 or 302; update the validation logic in RedirectRule (the statusCode check/throw) to allow the full set of standard redirect codes (301, 302, 303, 307, 308) and update the IllegalArgumentException message accordingly; locate the check that compares statusCode (the if block throwing IllegalArgumentException) and change the condition to reject only values not in {301,302,303,307,308} and ensure the exception text lists the supported codes.
🤖 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/example/filter/redirect/RedirectRule.java`:
- Around line 27-29: The matches() method currently calls
sourcePattern.matcher(requestPath) which will throw a NullPointerException if
requestPath is null; update matches(String requestPath) in RedirectRule to guard
against null by returning false when requestPath is null (i.e., check
requestPath == null before calling sourcePattern.matcher(...).matches()),
ensuring sourcePattern is still used only when requestPath is non-null.
---
Nitpick comments:
In `@src/main/java/org/example/filter/redirect/RedirectRule.java`:
- Around line 17-19: The constructor/validation in RedirectRule currently
rejects any status code other than 301 or 302; update the validation logic in
RedirectRule (the statusCode check/throw) to allow the full set of standard
redirect codes (301, 302, 303, 307, 308) and update the IllegalArgumentException
message accordingly; locate the check that compares statusCode (the if block
throwing IllegalArgumentException) and change the condition to reject only
values not in {301,302,303,307,308} and ensure the exception text lists the
supported codes.
In `@src/main/java/org/example/filter/redirect/RedirectRulesLoader.java`:
- Line 15: In RedirectRulesLoader replace the use of String trimmed =
sourcePath.trim() with the Unicode-aware strip variant to match the rest of the
class (use sourcePath.strip()); update any related variables or usages of
trimmed if needed so the code consistently uses strip() instead of trim() across
the class/method that handles sourcePath.
- Around line 34-45: The wildcardToRegex method currently quotes each
non-wildcard character individually; refactor wildcardToRegex to batch
consecutive literal characters into a single substring and call Pattern.quote
once per run: iterate the input, accumulate a start index (or StringBuilder) for
literal runs, when you hit '*' flush the accumulated literal via
Pattern.quote(sb.toString()) to the main StringBuilder, append "[^/]*" for '*'
and continue, and after the loop flush any remaining literal run; this produces
fewer \Q...\E segments while preserving the existing '*' → "[^/]*" behavior.
- Around line 17-23: The regex branch in RedirectRulesLoader currently compiles
rawRegex without anchors, making it inconsistent with the literal/wildcard
branches that wrap results with ^...$; change the Pattern.compile(rawRegex) call
to compile("^" + rawRegex + "$") so the raw regex branch is anchored the same
way (keep the existing blank check for rawRegex and use REGEX_PREFIX/rawRegex as
the referenced symbols).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pom.xmlsrc/main/java/org/example/filter/redirect/RedirectFilter.javasrc/main/java/org/example/filter/redirect/RedirectRule.javasrc/main/java/org/example/filter/redirect/RedirectRulesLoader.javasrc/test/java/org/example/filter/redirect/RedirectFilterTest.java
💤 Files with no reviewable changes (1)
- pom.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/example/filter/redirect/RedirectFilter.java
- src/test/java/org/example/filter/redirect/RedirectFilterTest.java
| public boolean matches(String requestPath) { | ||
| return sourcePattern.matcher(requestPath).matches(); | ||
| } |
There was a problem hiding this comment.
matches() will throw NPE when requestPath is null.
Pattern.matcher(null) throws NullPointerException. If any upstream component passes a null path (e.g., a malformed HTTP request where the path could not be parsed), the exception will bubble up the filter chain uncaught instead of cleanly returning false.
🛡️ Proposed fix
public boolean matches(String requestPath) {
- return sourcePattern.matcher(requestPath).matches();
+ if (requestPath == null) return false;
+ return sourcePattern.matcher(requestPath).matches();
}📝 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.
| public boolean matches(String requestPath) { | |
| return sourcePattern.matcher(requestPath).matches(); | |
| } | |
| public boolean matches(String requestPath) { | |
| if (requestPath == null) return false; | |
| return sourcePattern.matcher(requestPath).matches(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/filter/redirect/RedirectRule.java` around lines 27
- 29, The matches() method currently calls sourcePattern.matcher(requestPath)
which will throw a NullPointerException if requestPath is null; update
matches(String requestPath) in RedirectRule to guard against null by returning
false when requestPath is null (i.e., check requestPath == null before calling
sourcePattern.matcher(...).matches()), ensuring sourcePattern is still used only
when requestPath is non-null.
feat: add regex support and enforce 301/302 redirects
Ericthilen
force-pushed
the
URL-Redirect-Filter
branch
from
ebfe583 to
42f23cd
Compare
5 participants
Implements a RedirectFilter that intercepts incoming requests and redirects them based on configurable redirect rules.
Requirements covered
Tests
Issue #33
Summary by CodeRabbit
New Features
Documentation
Tests