Server‑side caching Issue36

[Boppler12]

Summary by CodeRabbit

• New Features

  • HTTP web server with configurable port, static file serving, request parsing, and multi-threaded handling
  • In-memory response cache and custom 404 page

• Infrastructure

  • CI pipeline for Java build & tests on push/PR
  • Automated Docker image build and publish for multi-arch releases
  • Dockerized runtime image

• Documentation

  • Comprehensive README with setup, configuration, and deployment guidance

• Tests

  • Extensive unit tests for HTTP parsing, response building, caching, MIME detection, and config loader

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a Java HTTP web server: TCP acceptor with virtual threads, HTTP parsing and response building, static-file serving with an LRU cache, YAML/JSON configuration loader with validation, test coverage, Dockerfile, and GitHub Actions workflows for CI and image release.

Changes

Cohort / File(s)	Summary
CI/CD & Build Infrastructure ​.github/workflows/ci.yml, ​.github/workflows/release.yml, Dockerfile, pom.xml	Added GitHub Actions CI and release workflows; multi-stage Dockerfile; bumped maven.compiler.release to 25; added dependencies (Bucket4J, Mockito, Awaitility, Jackson) and plugins (pitest, spotless).
Server Core Components src/main/java/org/example/App.java, src/main/java/org/example/TcpServer.java, src/main/java/org/example/ConnectionHandler.java, src/main/java/org/example/StaticFileHandler.java	Implemented TCP server using virtual threads, connection handling, request parsing, routing to static-file handler, URI sanitization, path-traversal protection, and response streaming with shared cache.
HTTP Protocol Support src/main/java/org/example/httpparser/..., src/main/java/org/example/http/...	Added request-line parser, full header parsing, immutable HttpRequest, HttpResponseBuilder (binary-safe, Content-Length handling), and MimeTypeDetector.
Caching & Filtering Framework src/main/java/org/example/CacheFilter.java, src/main/java/org/example/filter/...	Introduced thread-safe LRU cache with size and byte limits, cache stats API, and a simple Filter/FilterChain abstraction with FilterChainImpl.
Configuration Management src/main/java/org/example/config/AppConfig.java, src/main/java/org/example/config/ConfigLoader.java, src/main/resources/application.yml	Added AppConfig records with nested ServerConfig and LoggingConfig, defaults and validation; ConfigLoader for YAML/JSON with caching and test reset; default port 8080 and rootDir ./www.
Static Content www/index.html, www/pageNotFound.html	Added homepage and styled 404 page under the www/ web root.
Tests src/test/java/org/example/http/..., src/test/java/org/example/httpparser/..., src/test/java/org/example/config/..., src/test/java/org/example/StaticFileHandlerTest.java	Comprehensive unit tests for response building, MIME detection, request/header parsing, static file serving (including concurrency and sanitization), and config loading (defaults, validation, caching).
Documentation README.md	Complete rewrite to document the HTTP web server, configuration, build/run instructions, and deployment guidance.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client
    participant Server as TcpServer
    participant Handler as ConnectionHandler
    participant Parser as HttpParser
    participant Static as StaticFileHandler
    participant Cache as CacheFilter
    participant FS as FileSystem

    Client->>Server: TCP connect / send request
    Server->>Server: accept & spawn virtual thread
    Server->>Handler: handle(Socket)
    Handler->>Parser: parse request line & headers
    Handler->>Static: sendGetRequest(uri)
    Static->>Static: sanitize URI & check traversal
    Static->>Cache: getOrFetch(uri)
    alt cache hit
        Cache-->>Static: return bytes
    else cache miss
        Cache->>FS: read file bytes
        FS-->>Cache: file bytes
        Cache-->>Static: stored & returned bytes
    end
    Static->>Handler: build HTTP response bytes
    Handler->>Client: write response (headers + body)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 I hopped in the bytes, a tiny server sprite,
Threads sprung like springs to serve pages at night.
Cache kept things snug, YAML set the way,
Docker packed dreams for deployment day.
— a happy rabbit, nibbling code carrots 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.18% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'Server‑side caching Issue36' is partially related to the changeset. The main focus of the changes is implementing a complete HTTP web server with caching, but the title only emphasizes caching and includes an issue reference, not capturing the broader scope of implementing core server functionality like request parsing, static file serving, configuration management, and CI/CD workflows.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 17

🧹 Nitpick comments (15)

Dockerfile (1)

10-11: Place USER before ENTRYPOINT for clarity.

While Docker applies the last USER directive at container start regardless of ordering, the conventional practice is to switch to the non-root user before ENTRYPOINT so the intent is immediately clear to readers.

Proposed reorder

-ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
 USER appuser
+ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 10 - 11, The Dockerfile currently sets ENTRYPOINT
before switching user, which obscures intent; reorder the directives so the USER
instruction appears before ENTRYPOINT (move USER appuser above ENTRYPOINT
["java", "-classpath", "/app", "org.example.App"]) so the container will clearly
run as the non-root user and the intent is obvious to readers; ensure no other
instructions depend on the original order and keep the exact ENTRYPOINT and USER
values unchanged.

src/test/java/org/example/http/HttpResponseBuilderTest.java (2)

5-5: Prefer the standard Assertions entry point for AssertJ.

AssertionsForClassTypes is an internal entry point with a narrower API surface. The idiomatic import is:

import static org.assertj.core.api.Assertions.assertThat;

🤖 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` at line 5, The
test imports the internal AssertJ entry point AssertionsForClassTypes; replace
the static import "import static
org.assertj.core.api.AssertionsForClassTypes.assertThat;" with the public API
"import static org.assertj.core.api.Assertions.assertThat;" so
HttpResponseBuilderTest uses the standard Assertions entry point (update the
import at the top of the file where assertThat is referenced).

---

16-44: Consider adding test coverage for the byte[] body path and error status codes.

The current tests only exercise the setBody(String) overload with status 200. The setBody(byte[]) code path (lines 41-43 in the builder) and non-200 status codes (404, 500, etc.) are untested. Given that binary body handling has a correctness issue (see HttpResponseBuilder review), a test here would help catch the regression.

🤖 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 16
- 44, Add unit tests for HttpResponseBuilder to cover the setBody(byte[]) path
and non-200 status codes: create tests that call
HttpResponseBuilder.setBody(byte[]) with a multi-byte or binary payload and
assert build() includes the correct Content-Length (UTF-8 byte length or raw
byte length), that the body bytes are preserved in the output and that the
header/body separator ("\r\n\r\n") is present; also add tests that call
HttpResponseBuilder.setStatus(404) and setStatus(500) (or equivalent API) and
assert the status line in build() contains "HTTP/1.1 404" / "HTTP/1.1 500" so
non-200 responses are validated. Ensure one test uses a non-text binary array to
catch encoding/regression issues in the byte[] handling.

.github/workflows/ci.yml (1)

14-14: Inconsistent actions/checkout version across workflows.

This workflow uses actions/checkout@v4 while release.yml uses actions/checkout@v6.0.2. Consider pinning both to the same version for consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml at line 14, CI workflow is using
actions/checkout@v4 which is inconsistent with release.yml's
actions/checkout@v6.0.2; update the uses: entry for the checkout step (the
actions/checkout reference in the CI workflow) to the same pinned version used
in release.yml (or choose a single canonical version and update both workflows)
so both workflows consistently reference the same actions/checkout tag.

pom.xml (1)

133-136: JaCoCo is configured with <skip>true</skip>, disabling coverage reporting.

Given the investment in test infrastructure (unit tests, pitest, surefire/failsafe), disabling JaCoCo seems unintentional. If coverage reporting is desired, remove or set to false.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pom.xml` around lines 133 - 136, The JaCoCo plugin configuration currently
sets <skip>true</skip> (version 0.8.14) which disables coverage reporting; edit
the pom.xml JaCoCo plugin block to remove the <skip> element or set it to
<skip>false</skip> so JaCoCo runs during the build (ensure this change is
applied in the same plugin configuration that contains the
<version>0.8.14</version> entry and any goals/bindings related to report or
prepare-agent).

src/main/java/org/example/httpparser/HttpParseRequestLine.java (2)

15-15: Remove the redundant local variable.

reader is identical to br; using br directly simplifies the code.

♻️ Proposed fix

-        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,
The local variable "reader" in HttpParseRequestLine is redundant (it just
aliases "br"); remove the declaration "BufferedReader reader = br;" and update
any subsequent uses of "reader" in methods like parseRequestLine (or wherever
referenced) to use "br" directly so the code references the original
BufferedReader variable instead of the duplicate.

---

11-11: debug flag is always false and has no setter — the logging block is unreachable dead code.

HttpParseRequestLine provides no way to enable debug mode, so logger.info(…) is never called. Either add a protected setDebug(boolean) mutator or remove the field and the guarded block.

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 private boolean field debug in class HttpParseRequestLine is always false
and never set, making the logger.info(...) block (around the guarded lines
34-38) dead code; either add a protected mutator like setDebug(boolean debug) to
allow enabling debug mode before parsing (and use that field to gate the
logger.info calls), or remove the debug field and the guarded logging block
entirely to eliminate dead code—update any constructors or tests that must
enable debug to call the new setDebug method if you choose the mutator approach.

src/main/java/org/example/StaticFileHandler.java (1)

35-35: Content-Type is hardcoded as text/html regardless of file extension.

Serving a .css, .js, or image file with Content-Type: text/html will cause browsers to misinterpret the response. Consider deriving the MIME type from the file extension (e.g., via Files.probeContentType or a small lookup map).

🤖 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` at line 35, The response
Content-Type is hardcoded in StaticFileHandler causing wrong MIME for non-HTML
assets; update the code around response.setHeaders(...) in the StaticFileHandler
class to determine the MIME type from the file being served (e.g., use
Files.probeContentType(Path) or a small extension-to-MIME lookup) and set the
Content-Type header to that value (fall back to application/octet-stream or
text/plain; charset=utf-8 if probe returns null) before calling
response.setHeaders.

src/main/java/org/example/httpparser/HttpParser.java (2)

53-55: getHeadersMap() exposes internal mutable state.

Callers receive a live reference to the backing HashMap and can freely mutate it. 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() currently returns the mutable backing field headersMap from
HttpParser, exposing internal mutable state; change its implementation to return
an unmodifiable view or a defensive copy (e.g., return
Collections.unmodifiableMap(headersMap) or new HashMap<>(headersMap)) so callers
cannot mutate the parser's internal headersMap while preserving read access.

---

40-45: Use Logger for debug output instead of System.out.println.

The parent class already holds a Logger instance. Mixing System.out here is inconsistent and makes runtime log capture impossible.

♻️ Proposed fix

-        if (debug) {
-            System.out.println("Host: " + headersMap.get("Host"));
-            for (String key : headersMap.keySet()) {
-                System.out.println(key + ": " + headersMap.get(key));
-            }
-        }
+        if (debug) {
+            logger.fine("Host: " + headersMap.get("Host"));
+            headersMap.forEach((k, v) -> logger.fine(k + ": " + v));
+        }

🤖 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,
Replace the System.out.println debug prints in HttpParser with the parent's
Logger usage: remove System.out.println calls inside the debug block and call
the existing Logger (use logger.debug(...) or logger.info(...) as appropriate)
to log the Host and each header from headersMap; also replace the manual if
(debug) check with logger.isDebugEnabled() guard before iterating headersMap to
avoid mixing System.out and to leverage the parent's logging configuration.

src/test/java/org/example/httpparser/HttpParseRequestLineTest.java (1)

33-36: Null test depends on NPE propagation from InputStreamReader, not an explicit guard.

setReader(null) currently throws because new InputStreamReader(null, UTF_8) throws NullPointerException internally. If setReader ever adds an early explicit Objects.requireNonNull(in) guard, the test continues to pass — but if it silently swallows null (e.g. a null-check guard that returns early), the test would fail. This is fragile. Consider testing the documented precondition explicitly, or adding a Objects.requireNonNull(in, "InputStream must not be null") guard in setReader to make the contract explicit and self-documenting.

🤖 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 33 - 36, The test is fragile because it relies on InputStreamReader
throwing NPE; make the contract explicit by adding a null-check guard in the
implementation or updating the test to assert the documented precondition:
either add Objects.requireNonNull(in, "InputStream must not be null") at the
start of HttpParseRequestLine.setReader(InputStream in) (so setReader explicitly
throws NPE with a clear message) or change the test
HttpParseRequestLineTest.testParserThrowErrorWhenNull to assert that
setReader(null) throws a NullPointerException with the expected message (e.g.,
using isInstanceOf(NullPointerException.class).hasMessageContaining("InputStream
must not be null")).

src/main/java/org/example/App.java (1)

5-5: Hardcoded port and no error handling on server startup.

If the port is already in use or the server fails to bind, an unhandled exception will crash the JVM with no user-friendly output. At minimum, catch IOException and log the failure. The port should also be externalizable (e.g., via args[0] or an env var) for deployment flexibility.

♻️ Proposed fix

+import java.io.IOException;
+import java.util.logging.Logger;

 public class App {
+    private static final Logger logger = Logger.getLogger(App.class.getName());
+
     public static void main(String[] args) {
-        new TcpServer(8080).start();
+        int port = args.length > 0 ? Integer.parseInt(args[0]) : 8080;
+        try {
+            new TcpServer(port).start();
+        } catch (IOException e) {
+            logger.severe("Failed to start server on port " + port + ": " + 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/App.java` at line 5, Replace the hardcoded inline
server startup with externalized port parsing and proper error handling: read
the port from args[0] or an environment variable (fallback to 8080), parse it to
an int with validation, construct TcpServer with that port and wrap the call to
start() in a try/catch that catches IOException (and optionally
NumberFormatException) and logs a clear error to System.err or the project
logger before exiting with a non‑zero status; update the App class main method
(where new TcpServer(8080).start() is called) to implement this behavior.

src/main/java/org/example/TcpServer.java (3)

15-27: No graceful shutdown mechanism.

The while (true) loop has no exit path short of killing the process. Consider registering a shutdown hook (or a volatile boolean running flag checked each iteration) so the server can be stopped cleanly in tests and controlled deployments.

♻️ Suggested refactor

+private volatile boolean running = false;
+
 public void start() {
     System.out.println("Starting TCP server on port " + port);
+    running = true;
     try (ServerSocket serverSocket = new ServerSocket(port)) {
-        while (true) {
+        Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+            running = false;
+            try { serverSocket.close(); } catch (IOException ignored) {}
+        }));
+        while (running) {
             Socket clientSocket = serverSocket.accept();

🤖 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 15 - 27, The start()
method spins forever in a while (true) and lacks a graceful shutdown; add a
volatile boolean running flag checked each loop and expose a stop() method that
sets running = false and closes the ServerSocket, or register a
Runtime.getRuntime().addShutdownHook to call stop(); update start() to use while
(running) and ensure handleClient(clientSocket) stays the same, and make
ServerSocket a field so stop() can close it, catching/ignoring SocketException
from close to break accept().

---

29-35: Rethrowing inside a fire-and-forget virtual thread adds noise without benefit.

The RuntimeException thrown from handleClient is caught by the virtual thread's uncaught exception handler — no caller in that thread receives it. Logging and returning achieves the same (or better) observability without the overhead of constructing and propagating a wrapper exception, and keeps the intent explicit.

♻️ Suggested refactor

 private void handleClient(Socket client) {
     try (ConnectionHandler connectionHandler = new ConnectionHandler(client)) {
         connectionHandler.runConnectionHandler();
     } catch (Exception e) {
-        throw new RuntimeException("Error handling client connection", e);
+        System.err.println("Error handling client connection: " + e.getMessage());
+        e.printStackTrace();
     }
 }

(Or use a proper logging framework — see the note below.)

🤖 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 29 - 35, The
handleClient method currently catches Exception and rethrows a new
RuntimeException, which is unnecessary for fire-and-forget virtual threads;
change the catch block to log the exception and return instead of throwing to
avoid noise and extra allocation. In the catch for handleClient, call your
logger (or System.err) with a clear message including the caught Exception (e)
and do not rethrow; keep the try-with-resources that constructs
ConnectionHandler and still call runConnectionHandler() as before. Ensure you
reference the ConnectionHandler and its runConnectionHandler() invocation when
making the change so only the exception handling behavior is modified.

---

16-16: System.out.println used for all diagnostic output.

Consider replacing with java.util.logging.Logger (already on the classpath) or SLF4J, which provide structured output, log levels, and easy redirection to files/monitoring systems.

Also applies to: 21-21

🤖 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` at line 16, Replace the ad-hoc
System.out.println diagnostic prints in the TcpServer class with a
java.util.logging.Logger instance: add a private static final Logger (e.g.,
LOGGER = Logger.getLogger(TcpServer.class.getName())) to the class and change
the two System.out.println calls (the "Starting TCP server on port ..." and the
other diagnostic at line 21) to use LOGGER at the appropriate level (INFO for
startup messages); ensure you import java.util.logging.Logger and use
descriptive log messages consistent with the original text.

🤖 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`:
- Around line 5-10: The runtime image will miss dependency JARs and the www
static folder; change the build stage to produce and collect runtime
dependencies (e.g., replace RUN mvn compile with RUN mvn package and run the
maven-dependency-plugin:copy-dependencies or build a fat JAR with the
shade/assembly plugin) so dependency JARs are available, adjust the runtime COPY
from the build stage to include those dependency JARs (or copy the fat JAR)
instead of only target/classes (see COPY --from=build /build/target/classes/),
add COPY www/ /app/www/ and set WORKDIR /app so StaticFileHandler's relative
"www" path resolves, and update ENTRYPOINT/classpath invocation (ENTRYPOINT
["java", "-classpath", "/app:...dependencies...", "org.example.App"] or point to
the fat JAR) accordingly.

In `@pom.xml`:
- Around line 22-26: The declared Maven dependency artifactId
'bucket4j_jdk17-core' (groupId com.bucket4j) appears unused in production
sources; inspect usages across the repository (including config loaders,
integration tests, and any non-Java resources) to confirm, and then either
remove this <dependency> entry from pom.xml or change its scope to test/provided
as appropriate; update pom.xml by deleting or modifying the dependency and run
mvn dependency:analyze and a full build/test to ensure nothing breaks before
committing.

In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 10-11: The fields client and uri in ConnectionHandler are
package-private; make them private by changing their declarations to private
Socket client; and private String uri; and then update any direct package
accesses to use new accessors or constructor parameters—add/getClient(),
add/getUri() or modify existing constructor/methods in ConnectionHandler to
operate on the private fields so no external code accesses them directly.
- Around line 27-35: resolveTargetFile currently preserves the leading '/' for
non-root URIs causing StaticFileHandler's new File(WEB_ROOT, uri) to treat uri
as absolute and bypass the WEB_ROOT; change resolveTargetFile to: if uri equals
"/" set this.uri="index.html", else remove any leading '/' from uri, then if the
resulting uri does not end with ".html" append ".html", otherwise keep it —
avoid the regex use and operate with simple string methods so new File(WEB_ROOT,
this.uri) correctly resolves under WEB_ROOT.

In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 48-57: HttpResponseBuilder.build() currently emits all
user-supplied headers from the headers map and then unconditionally appends a
Content-Length header, causing a duplicate if the caller set Content-Length via
setHeaders; modify build() to check the headers map for a case-insensitive
"Content-Length" key (or remove it) before appending the automatic
Content-Length: either remove any "Content-Length" entry from headers prior to
headers.forEach(...) or skip appending the auto Content-Length when headers
contains that key, ensuring you reference the headers field and the build()
method when making the change.
- Line 49: The REASON_PHRASES lookup in HttpResponseBuilder currently uses
REASON_PHRASES.getOrDefault(statusCode, "OK") which yields misleading "OK" for
unknown/ error codes; change the default to a neutral value like "Unknown" (or
expand REASON_PHRASES to include the specific codes you use) so the reason
variable is set via REASON_PHRASES.getOrDefault(statusCode, "Unknown"); update
any tests or usages expecting "OK" when statusCode is unmapped accordingly.
- Around line 38-60: The build() method is corrupting binary responses by
converting bytebody into a UTF-8 String and returning a text representation;
change the API to build raw bytes (or add a new method writeTo(OutputStream)) so
the response body is written as the original byte[] without charset conversion
and Content-Length is computed from that byte[] (refer to bytebody, body,
setBody and build()); update StaticFileHandler to write the response headers and
the bytebody directly to the socket OutputStream instead of using
PrintWriter.println(response.build()); also use
REASON_PHRASES.getOrDefault(statusCode, "") (or a more appropriate default like
an empty string) so non-2xx status codes don't show "OK", and remove the stray
// comment.

In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Line 12: HttpParser currently defines a private boolean field named debug that
duplicates and shadows the identically-named flag in HttpParseRequestLine;
remove the private debug declaration from HttpParser, change the debug field in
HttpParseRequestLine to protected (or package-private) so the child and parent
share the same flag, and update any logging/printing locations to reference that
single shared field (e.g., keep uses inside HttpParseRequestLine and parent
Logger-guarded blocks but remove the duplicate System.out guarded by the removed
field).

In `@src/main/java/org/example/StaticFileHandler.java`:
- Line 15: The FileCache instance is currently created per StaticFileHandler
(private final FileCache cache = new FileCache()), so the cache is discarded
each request; change the design so the FileCache is shared across
requests—either make the field a static shared instance (e.g., private static
final FileCache cache) in StaticFileHandler so cache.contains(uri) sees prior
entries, or better, add a FileCache parameter to StaticFileHandler's constructor
and create a single FileCache in the outer server (e.g., in TcpServer) and pass
that same instance down through ConnectionHandler.runConnectionHandler() into
each StaticFileHandler to ensure a global, reusable cache.
- Around line 37-38: The PrintWriter in StaticFileHandler is using the platform
default encoding; change construction to explicitly use UTF-8 by wrapping the
outputStream with an OutputStreamWriter created with StandardCharsets.UTF_8 (and
then a PrintWriter) so the bytes match the Content-Type charset; update the
writer creation that currently references outputStream so
writer.println(response.build()) writes UTF-8-encoded data.
- Around line 26-27: The code uses new File(WEB_ROOT, uri) which will ignore
WEB_ROOT when uri is an absolute path (leading '/'), causing requests to bypass
the web root; update resolveTargetFile (or handleGetRequest) to remove any
leading '/' from the URI before constructing the File (e.g., normalize uri so
"/" -> "index.html" and other URIs never start with '/'), and then perform
canonicalization/validation of the resulting File against WEB_ROOT to prevent
path traversal (use File#getCanonicalPath to ensure the target resides under
WEB_ROOT) — adjust references in StaticFileHandler to use the normalized uri and
validate the File before calling Files.readAllBytes.
- Around line 19-30: The code currently lets Files.readAllBytes throw a generic
IOException causing no HTTP response on missing files; modify handleGetRequest
to explicitly detect a missing file (check File.exists() or File.isFile()) and
throw a FileNotFoundException (include the uri in the message) instead of
letting a generic IOException propagate, then update sendGetRequest to catch
FileNotFoundException and write a proper HTTP/1.1 404 Not Found response back to
the client (headers + optional body) before closing the connection; references:
handleGetRequest, sendGetRequest, runConnectionHandler.

In `@src/main/java/org/example/TcpServer.java`:
- Line 18: The code opens a plain ServerSocket in TcpServer (try (ServerSocket
serverSocket = new ServerSocket(port))) which allows cleartext transmission;
change it to create an SSLServerSocket from an SSLServerSocketFactory/SSLContext
instead: load or reference the keystore/SSLContext configuration, initialize an
SSLContext (or obtain SSLServerSocketFactory via
SSLServerSocketFactory.getDefault()/SSLContext.getSocketFactory()), create an
SSLServerSocket bound to the same port, and replace usages of the ServerSocket
variable with the SSLServerSocket instance so all accepted sockets are
TLS-protected (ensure proper resource handling in the try-with-resources block).
- Around line 32-34: The catch block in TcpServer that currently does `throw new
RuntimeException("Error handling client connection " + e)` loses the original
exception type and stack trace; change it to throw the RuntimeException with the
original exception as the cause (i.e., construct the RuntimeException with a
message and pass the caught Exception e as the cause) or alternatively log the
error with the full exception (using the logger in the class) instead of
rethrowing inside the virtual thread so the original stack trace and cause chain
are preserved; locate the catch in the client-handling method in class TcpServer
and update it accordingly.
- Around line 19-26: The current catch lumps startup and runtime accept()
IOExceptions together causing the server to exit on transient errors; modify
TcpServer so the initial ServerSocket creation failure still throws (keep the
outer try for startup), but move the accept loop into its own try/catch: inside
the while(true) wrap serverSocket.accept() (and Thread.ofVirtual().start(() ->
handleClient(clientSocket))) in a small try that catches IOException, logs the
error and remote info, and continues the loop rather than rethrowing; only treat
exceptions from ServerSocket creation (before entering the loop) as fatal.
Reference: ServerSocket/ serverSocket.accept(), TcpServer class, and
handleClient().

In `@src/test/java/org/example/httpparser/HttpParserTest.java`:
- Around line 16-28: The test TestHttpParserForHeaders incorrectly relies on
parseHttp() to skip the request line; update the test to mirror real usage by
either calling parseRequest() before parseHttp() (invoke
httpParser.parseRequest() on the provided InputStream) or change the testInput
to contain only the header block (remove the "GET /index.html HTTP/1.1" request
line) so the test explicitly targets header parsing behavior in parseHttp();
adjust assertions only if needed after making one of these two changes.
- Line 16: Rename the test method TestHttpParserForHeaders to start with a
lowercase letter (e.g., testHttpParserForHeaders) to follow Java/JUnit naming
conventions; update the method declaration and any references to the method name
in the class HttpParserTest so the test still runs and compiles.

---

Nitpick comments:
In @.github/workflows/ci.yml:
- Line 14: CI workflow is using actions/checkout@v4 which is inconsistent with
release.yml's actions/checkout@v6.0.2; update the uses: entry for the checkout
step (the actions/checkout reference in the CI workflow) to the same pinned
version used in release.yml (or choose a single canonical version and update
both workflows) so both workflows consistently reference the same
actions/checkout tag.

In `@Dockerfile`:
- Around line 10-11: The Dockerfile currently sets ENTRYPOINT before switching
user, which obscures intent; reorder the directives so the USER instruction
appears before ENTRYPOINT (move USER appuser above ENTRYPOINT ["java",
"-classpath", "/app", "org.example.App"]) so the container will clearly run as
the non-root user and the intent is obvious to readers; ensure no other
instructions depend on the original order and keep the exact ENTRYPOINT and USER
values unchanged.

In `@pom.xml`:
- Around line 133-136: The JaCoCo plugin configuration currently sets
<skip>true</skip> (version 0.8.14) which disables coverage reporting; edit the
pom.xml JaCoCo plugin block to remove the <skip> element or set it to
<skip>false</skip> so JaCoCo runs during the build (ensure this change is
applied in the same plugin configuration that contains the
<version>0.8.14</version> entry and any goals/bindings related to report or
prepare-agent).

In `@src/main/java/org/example/App.java`:
- Line 5: Replace the hardcoded inline server startup with externalized port
parsing and proper error handling: read the port from args[0] or an environment
variable (fallback to 8080), parse it to an int with validation, construct
TcpServer with that port and wrap the call to start() in a try/catch that
catches IOException (and optionally NumberFormatException) and logs a clear
error to System.err or the project logger before exiting with a non‑zero status;
update the App class main method (where new TcpServer(8080).start() is called)
to implement this behavior.

In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Around line 53-55: getHeadersMap() currently returns the mutable backing field
headersMap from HttpParser, exposing internal mutable state; change its
implementation to return an unmodifiable view or a defensive copy (e.g., return
Collections.unmodifiableMap(headersMap) or new HashMap<>(headersMap)) so callers
cannot mutate the parser's internal headersMap while preserving read access.
- Around line 40-45: Replace the System.out.println debug prints in HttpParser
with the parent's Logger usage: remove System.out.println calls inside the debug
block and call the existing Logger (use logger.debug(...) or logger.info(...) as
appropriate) to log the Host and each header from headersMap; also replace the
manual if (debug) check with logger.isDebugEnabled() guard before iterating
headersMap to avoid mixing System.out and to leverage the parent's logging
configuration.

In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java`:
- Line 15: The local variable "reader" in HttpParseRequestLine is redundant (it
just aliases "br"); remove the declaration "BufferedReader reader = br;" and
update any subsequent uses of "reader" in methods like parseRequestLine (or
wherever referenced) to use "br" directly so the code references the original
BufferedReader variable instead of the duplicate.
- Line 11: The private boolean field debug in class HttpParseRequestLine is
always false and never set, making the logger.info(...) block (around the
guarded lines 34-38) dead code; either add a protected mutator like
setDebug(boolean debug) to allow enabling debug mode before parsing (and use
that field to gate the logger.info calls), or remove the debug field and the
guarded logging block entirely to eliminate dead code—update any constructors or
tests that must enable debug to call the new setDebug method if you choose the
mutator approach.

In `@src/main/java/org/example/StaticFileHandler.java`:
- Line 35: The response Content-Type is hardcoded in StaticFileHandler causing
wrong MIME for non-HTML assets; update the code around response.setHeaders(...)
in the StaticFileHandler class to determine the MIME type from the file being
served (e.g., use Files.probeContentType(Path) or a small extension-to-MIME
lookup) and set the Content-Type header to that value (fall back to
application/octet-stream or text/plain; charset=utf-8 if probe returns null)
before calling response.setHeaders.

In `@src/main/java/org/example/TcpServer.java`:
- Around line 15-27: The start() method spins forever in a while (true) and
lacks a graceful shutdown; add a volatile boolean running flag checked each loop
and expose a stop() method that sets running = false and closes the
ServerSocket, or register a Runtime.getRuntime().addShutdownHook to call stop();
update start() to use while (running) and ensure handleClient(clientSocket)
stays the same, and make ServerSocket a field so stop() can close it,
catching/ignoring SocketException from close to break accept().
- Around line 29-35: The handleClient method currently catches Exception and
rethrows a new RuntimeException, which is unnecessary for fire-and-forget
virtual threads; change the catch block to log the exception and return instead
of throwing to avoid noise and extra allocation. In the catch for handleClient,
call your logger (or System.err) with a clear message including the caught
Exception (e) and do not rethrow; keep the try-with-resources that constructs
ConnectionHandler and still call runConnectionHandler() as before. Ensure you
reference the ConnectionHandler and its runConnectionHandler() invocation when
making the change so only the exception handling behavior is modified.
- Line 16: Replace the ad-hoc System.out.println diagnostic prints in the
TcpServer class with a java.util.logging.Logger instance: add a private static
final Logger (e.g., LOGGER = Logger.getLogger(TcpServer.class.getName())) to the
class and change the two System.out.println calls (the "Starting TCP server on
port ..." and the other diagnostic at line 21) to use LOGGER at the appropriate
level (INFO for startup messages); ensure you import java.util.logging.Logger
and use descriptive log messages consistent with the original text.

In `@src/test/java/org/example/http/HttpResponseBuilderTest.java`:
- Line 5: The test imports the internal AssertJ entry point
AssertionsForClassTypes; replace the static import "import static
org.assertj.core.api.AssertionsForClassTypes.assertThat;" with the public API
"import static org.assertj.core.api.Assertions.assertThat;" so
HttpResponseBuilderTest uses the standard Assertions entry point (update the
import at the top of the file where assertThat is referenced).
- Around line 16-44: Add unit tests for HttpResponseBuilder to cover the
setBody(byte[]) path and non-200 status codes: create tests that call
HttpResponseBuilder.setBody(byte[]) with a multi-byte or binary payload and
assert build() includes the correct Content-Length (UTF-8 byte length or raw
byte length), that the body bytes are preserved in the output and that the
header/body separator ("\r\n\r\n") is present; also add tests that call
HttpResponseBuilder.setStatus(404) and setStatus(500) (or equivalent API) and
assert the status line in build() contains "HTTP/1.1 404" / "HTTP/1.1 500" so
non-200 responses are validated. Ensure one test uses a non-text binary array to
catch encoding/regression issues in the byte[] handling.

In `@src/test/java/org/example/httpparser/HttpParseRequestLineTest.java`:
- Around line 33-36: The test is fragile because it relies on InputStreamReader
throwing NPE; make the contract explicit by adding a null-check guard in the
implementation or updating the test to assert the documented precondition:
either add Objects.requireNonNull(in, "InputStream must not be null") at the
start of HttpParseRequestLine.setReader(InputStream in) (so setReader explicitly
throws NPE with a clear message) or change the test
HttpParseRequestLineTest.testParserThrowErrorWhenNull to assert that
setReader(null) throws a NullPointerException with the expected message (e.g.,
using isInstanceOf(NullPointerException.class).hasMessageContaining("InputStream
must not be null")).

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Get full Dockerfile content
cat -n Dockerfile

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 515

---

🏁 Script executed:

# Check pom.xml for dependencies and maven plugins
cat -n pom.xml

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 8522

---

🏁 Script executed:

# Find StaticFileHandler and examine how it resolves the www directory
find . -name "StaticFileHandler.java" -type f | head -5

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 158

---

🏁 Script executed:

# Check if www directory exists
find . -type d -name "www" | head -10

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 113

---

🏁 Script executed:

# List root-level directories and files to understand project structure
ls -la | head -20

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 672

---

🏁 Script executed:

# Examine StaticFileHandler to understand how it resolves the www directory path
cat -n src/main/java/org/example/StaticFileHandler.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 1680

---

🏁 Script executed:

# Check if www directory is referenced in App.java or other main code
find src/main/java -name "App.java" -type f -exec cat -n {} \;

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 287

---

🏁 Script executed:

# Look for any hardcoded paths or configuration related to www
rg -n "www" src/

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 209

---

Runtime image is missing dependency JARs and the www/ static content directory.

The build stage only runs mvn compile, which puts compiled classes under target/classes/ but does not download dependency JARs into that directory. The runtime stage copies only target/classes/ to /app/ and sets the classpath to /app. Since pom.xml declares non-test dependencies (bucket4j, jackson-databind, jackson-dataformat-yaml), the application will fail at runtime with ClassNotFoundException.

Additionally, StaticFileHandler uses a relative path "www" to locate static content. The Dockerfile does not copy the www/ directory into the image, so the application will fail to serve static files at runtime.

Consider using mvn package with the maven-dependency-plugin:copy-dependencies goal (or the shade/assembly plugin to create a fat JAR), add a COPY www/ /app/www/ step in the runtime stage, and set WORKDIR /app to ensure the relative path resolves correctly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 5 - 10, The runtime image will miss dependency JARs
and the www static folder; change the build stage to produce and collect runtime
dependencies (e.g., replace RUN mvn compile with RUN mvn package and run the
maven-dependency-plugin:copy-dependencies or build a fat JAR with the
shade/assembly plugin) so dependency JARs are available, adjust the runtime COPY
from the build stage to include those dependency JARs (or copy the fat JAR)
instead of only target/classes (see COPY --from=build /build/target/classes/),
add COPY www/ /app/www/ and set WORKDIR /app so StaticFileHandler's relative
"www" path resolves, and update ENTRYPOINT/classpath invocation (ENTRYPOINT
["java", "-classpath", "/app:...dependencies...", "org.example.App"] or point to
the fat JAR) accordingly.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -rn 'bucket4j|io\.github\.bucket4j|Bucket4j|Bandwidth|Refill|Bucket\.builder' --type=java src/

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 90

---

bucket4j_jdk17-core is declared as a compile-scope dependency but is not used in any Java source files.

Verify whether it's used in files outside this scope (configuration loaders, tests, etc.), or remove it if truly unused to reduce classpath and Docker image size.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pom.xml` around lines 22 - 26, The declared Maven dependency artifactId
'bucket4j_jdk17-core' (groupId com.bucket4j) appears unused in production
sources; inspect usages across the repository (including config loaders,
integration tests, and any non-Java resources) to confirm, and then either
remove this <dependency> entry from pom.xml or change its scope to test/provided
as appropriate; update pom.xml by deleting or modifying the dependency and run
mvn dependency:analyze and a full build/test to ensure nothing breaks before
committing.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

client and uri fields should be private.

Both are currently package-private, exposing ConnectionHandler's internal state to the entire package.

♻️ Proposed fix

-    Socket client;
-    String uri;
+    private Socket client;
+    private String uri;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	Socket client;
	String uri;
	private 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, The
fields client and uri in ConnectionHandler are package-private; make them
private by changing their declarations to private Socket client; and private
String uri; and then update any direct package accesses to use new accessors or
constructor parameters—add/getClient(), add/getUri() or modify existing
constructor/methods in ConnectionHandler to operate on the private fields so no
external code accesses them directly.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

resolveTargetFile preserves the leading / for all non-root URIs, causing a path-resolution bug in StaticFileHandler.

String.matches("/$") only matches the exact string "/". Every other URI (e.g. "/index.html", "/about") keeps its leading slash. When that value is later passed to new File(WEB_ROOT, uri) in StaticFileHandler, Java's File(parent, child) contract ignores parent when child is an absolute path (starts with /), so the www/ root is completely bypassed for every request other than /. This also makes the complex regex unnecessary — equivalent logic via simple string operations is clearer and eliminates the bug:

♻️ Proposed fix

     private void resolveTargetFile(String uri) {
-        if (uri.matches("/$")) { //matches(/)
-            this.uri = "index.html";
-        } else if (uri.matches("^(?!.*\\.html$).*$")) {
-            this.uri = uri.concat(".html");
-        } else {
-            this.uri = uri;
-        }
+        // Strip leading slash so new File(WEB_ROOT, uri) resolves relative to WEB_ROOT
+        String normalized = uri.startsWith("/") ? uri.substring(1) : uri;
+        if (normalized.isEmpty()) {
+            this.uri = "index.html";
+        } else if (!normalized.endsWith(".html")) {
+            this.uri = normalized + ".html";
+        } else {
+            this.uri = normalized;
+        }
     }

🤖 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 27 - 35,
resolveTargetFile currently preserves the leading '/' for non-root URIs causing
StaticFileHandler's new File(WEB_ROOT, uri) to treat uri as absolute and bypass
the WEB_ROOT; change resolveTargetFile to: if uri equals "/" set
this.uri="index.html", else remove any leading '/' from uri, then if the
resulting uri does not end with ".html" append ".html", otherwise keep it —
avoid the regex use and operate with simple string methods so new File(WEB_ROOT,
this.uri) correctly resolves under WEB_ROOT.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

rg -n 'HttpResponseBuilder|\.build\(\)' --type=java -C 5 -g '!*Test*'

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 2813

---

🏁 Script executed:

rg -n 'setBody|void|public' src/main/java/org/example/http/HttpResponseBuilder.java -A 3

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 1009

---

🏁 Script executed:

cat -n src/main/java/org/example/http/HttpResponseBuilder.java

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 2434

---

build() corrupts binary content by converting byte[] to String.

Line 43 converts bytebody to UTF-8 String, silently replacing invalid byte sequences with the Unicode replacement character (U+FFFD). This makes binary file serving broken—the Content-Length header (computed from original byte array) won't match the actual body bytes written. In StaticFileHandler.java line 36-38, binary file bytes are passed via setBody(fileBytes) and the response is written as text via PrintWriter.println(response.build()), corrupting any non-UTF-8-safe files (images, PDFs, fonts, etc.).

The response should be built as byte[] or written directly to OutputStream to preserve binary content.

Additionally:

• Line 49: Default reason phrase "OK" is misleading for error codes (e.g., 503 would show "HTTP/1.1 503 OK").
• Line 2: Stray // comment.

🤖 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 38 -
60, The build() method is corrupting binary responses by converting bytebody
into a UTF-8 String and returning a text representation; change the API to build
raw bytes (or add a new method writeTo(OutputStream)) so the response body is
written as the original byte[] without charset conversion and Content-Length is
computed from that byte[] (refer to bytebody, body, setBody and build()); update
StaticFileHandler to write the response headers and the bytebody directly to the
socket OutputStream instead of using PrintWriter.println(response.build()); also
use REASON_PHRASES.getOrDefault(statusCode, "") (or a more appropriate default
like an empty string) so non-2xx status codes don't show "OK", and remove the
stray // comment.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Static analysis: plain ServerSocket transmits data in cleartext (CWE-319).

The static analysis tool flags this as "Cleartext Transmission of Sensitive Information." For an internal/educational HTTP server this is likely intentional, but if the server ever handles any credentials or sensitive content in production, SSLServerSocket via SSLServerSocketFactory should be used instead.

🤖 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` at line 18, The code opens a plain
ServerSocket in TcpServer (try (ServerSocket serverSocket = new
ServerSocket(port))) which allows cleartext transmission; change it to create an
SSLServerSocket from an SSLServerSocketFactory/SSLContext instead: load or
reference the keystore/SSLContext configuration, initialize an SSLContext (or
obtain SSLServerSocketFactory via
SSLServerSocketFactory.getDefault()/SSLContext.getSocketFactory()), create an
SSLServerSocket bound to the same port, and replace usages of the ServerSocket
variable with the SSLServerSocket instance so all accepted sockets are
TLS-protected (ensure proper resource handling in the try-with-resources block).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Any transient IOException from accept() terminates the entire server loop.

The single catch (IOException e) wraps both startup failures (legitimate fatal) and runtime accept() errors (often transient) identically. A brief network blip or OS-level error causes the loop — and the server — to die permanently.

Consider distinguishing startup failure from runtime accept errors:

♻️ Proposed fix

 try (ServerSocket serverSocket = new ServerSocket(port)) {
     while (true) {
-        Socket clientSocket = serverSocket.accept();
-        System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress());
-        Thread.ofVirtual().start(() -> handleClient(clientSocket));
+        try {
+            Socket clientSocket = serverSocket.accept();
+            System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress());
+            Thread.ofVirtual().start(() -> handleClient(clientSocket));
+        } catch (IOException e) {
+            if (serverSocket.isClosed()) break; // intentional shutdown
+            System.err.println("Transient accept error, continuing: " + e.getMessage());
+        }
     }
 } catch (IOException e) {
     throw new RuntimeException("Failed to start TCP server", 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.

Suggested change

	while (true) {
	Socket clientSocket = serverSocket.accept(); // block
	System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress());
	Thread.ofVirtual().start(() -> handleClient(clientSocket));
	}
	} catch (IOException e) {
	throw new RuntimeException("Failed to start TCP server", e);
	}
	while (true) {
	try {
	Socket clientSocket = serverSocket.accept(); // block
	System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress());
	Thread.ofVirtual().start(() -> handleClient(clientSocket));
	} catch (IOException e) {
	if (serverSocket.isClosed()) break; // intentional shutdown
	System.err.println("Transient accept error, continuing: " + e.getMessage());
	}
	}
	} catch (IOException e) {
	throw new RuntimeException("Failed to start TCP server", 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 19 - 26, The current
catch lumps startup and runtime accept() IOExceptions together causing the
server to exit on transient errors; modify TcpServer so the initial ServerSocket
creation failure still throws (keep the outer try for startup), but move the
accept loop into its own try/catch: inside the while(true) wrap
serverSocket.accept() (and Thread.ofVirtual().start(() ->
handleClient(clientSocket))) in a small try that catches IOException, logs the
error and remote info, and continues the loop rather than rethrowing; only treat
exceptions from ServerSocket creation (before entering the loop) as fatal.
Reference: ServerSocket/ serverSocket.accept(), TcpServer class, and
handleClient().

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Original exception cause and stack trace are silently discarded.

"Error handling client connection " + e calls e.toString() — only the class name and message are embedded in the new exception's message string. The original stack trace, cause chain, and exception type are lost. Since this fires inside a virtual thread, the JVM invokes the thread's UncaughtExceptionHandler with the wrapper RuntimeException, which means whatever is printed to stderr shows only the synthetic wrapper's frames — not where the real error occurred.

Pass e as the cause argument instead (and consider simply logging rather than rethrowing, since no caller in the virtual thread catches this):

🐛 Proposed fix

 } catch (Exception e) {
-    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.

Suggested change

	} 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 that currently does `throw new RuntimeException("Error
handling client connection " + e)` loses the original exception type and stack
trace; change it to throw the RuntimeException with the original exception as
the cause (i.e., construct the RuntimeException with a message and pass the
caught Exception e as the cause) or alternatively log the error with the full
exception (using the logger in the class) instead of rethrowing inside the
virtual thread so the original stack trace and cause chain are preserved; locate
the catch in the client-handling method in class TcpServer and update it
accordingly.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

testHttpParserForHeaders silently relies on parseHttp() discarding the request line.

The input contains GET /index.html HTTP/1.1 as the first line, but only parseHttp() is called (not parseRequest() first). The request line is silently skipped because it contains no :, so valueSeparator <= 0 triggers continue. The test asserts size == 3 headers, which passes — but it implicitly validates a discard behaviour rather than the intended two-phase parseRequest() → parseHttp() flow used in production. Consider either (a) calling parseRequest() before parseHttp() to mirror real usage, or (b) feeding only headers as input to make the unit boundary explicit.

🤖 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 16 -
28, The test TestHttpParserForHeaders incorrectly relies on parseHttp() to skip
the request line; update the test to mirror real usage by either calling
parseRequest() before parseHttp() (invoke httpParser.parseRequest() on the
provided InputStream) or change the testInput to contain only the header block
(remove the "GET /index.html HTTP/1.1" request line) so the test explicitly
targets header parsing behavior in parseHttp(); adjust assertions only if needed
after making one of these two changes.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Test method name violates Java naming convention.

TestHttpParserForHeaders starts with an uppercase T. Java (and JUnit) convention requires test method names to start with a lowercase letter.

♻️ Proposed fix

-    void TestHttpParserForHeaders() throws IOException {
+    void testHttpParserForHeaders() throws IOException {

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	void TestHttpParserForHeaders() throws IOException {
	void testHttpParserForHeaders() throws IOException {

🤖 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, Rename
the test method TestHttpParserForHeaders to start with a lowercase letter (e.g.,
testHttpParserForHeaders) to follow Java/JUnit naming conventions; update the
method declaration and any references to the method name in the class
HttpParserTest so the test still runs and compiles.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/main/java/org/example/CacheFilter.java (1)

10-13: Replace System.out.println with a proper logger.

System.out.println provides no log-level control, no structured output, and no easy suppression in tests or production. Consider replacing with java.util.logging.Logger or an SLF4J facade.

♻️ Proposed refactor

+import java.util.logging.Logger;
 
 public class CacheFilter {
+    private static final Logger logger = Logger.getLogger(CacheFilter.class.getName());
     private final FileCache cache = new FileCache();
 
     public byte[] getOrFetch(String uri, FileProvider provider) throws IOException {
         if (cache.contains(uri)) {
-            System.out.println("✓ Cache hit for: " + uri);
+            logger.fine("Cache hit for: " + uri);
             return cache.get(uri);
         }
-        System.out.println("✗ Cache miss for: " + uri);
+        logger.fine("Cache miss for: " + uri);
         byte[] fileBytes = provider.fetch(uri);
         cache.put(uri, fileBytes);
         return fileBytes;
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/CacheFilter.java` around lines 10 - 13, The two
System.out.println calls in CacheFilter (the "✓ Cache hit for: " and "✗ Cache
miss for: " lines) should be replaced with a proper logger: add a private static
final logger (either SLF4J's LoggerFactory.getLogger(CacheFilter.class) or
java.util.logging.Logger.getLogger(CacheFilter.class.getName())) and use logger
methods instead of println (e.g., logger.debug/trace for hits and
logger.info/warn for misses), passing the URI as a parameter or using
parameterized logging to avoid string concatenation; update the code in the
CacheFilter method where cache.get(uri) is returned to use logger.<class/method
references>: CacheFilter, the cache-hit/cache-miss println calls.

🤖 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/CacheFilter.java`:
- Around line 8-16: The getOrFetch method has a non-atomic check-then-act on the
FileCache (uses cache.contains + cache.get/cache.put) causing races and
double-fetches under concurrent virtual threads; fix by making the compound
operation atomic: either synchronize the getOrFetch method (or a private lock
object) around the contains/get/fetch/put sequence, or switch FileCache to a
thread-safe ConcurrentHashMap and replace the contains/get/put pattern with
computeIfAbsent (wrapping provider.fetch to handle the checked IOException).
Also ensure the cache instance used by StaticFileHandler is shared (not
per-instance) so the chosen atomic approach is effective.

---

Duplicate comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Line 14: StaticFileHandler currently instantiates its own CacheFilter (and
embedded FileCache) per handler, so the cache is thrown away each request;
change this by making CacheFilter shared: either make the CacheFilter (or its
FileCache) a static field on StaticFileHandler or, preferably, create a single
CacheFilter instance in TcpServer and pass it through ConnectionHandler into
StaticFileHandler via constructor injection so getOrFetch can hit a shared cache
across requests; update constructors/signatures in StaticFileHandler and
ConnectionHandler accordingly and ensure references to CacheFilter and
getOrFetch use the injected/shared instance.
- Around line 16-26: sendGetRequest currently lets Files.readAllBytes propagate
through cacheFilter.getOrFetch causing no HTTP response on missing files; wrap
the cacheFilter.getOrFetch(...) call in a try/catch for IOException (and any
unchecked file access exceptions), and in the catch build and write a 404
response using HttpResponseBuilder (e.g., set status to 404 and a short "Not
Found" body) before returning; keep using response.build() and the PrintWriter
to send the error response so clients receive a proper HTTP 404 instead of a
connection reset.
- Around line 17-19: The current getOrFetch call in StaticFileHandler accepts a
raw URI path that may begin with a leading slash or contain ../ segments,
allowing absolute-path bypass and directory traversal; update the code that
calls cacheFilter.getOrFetch (and the related
ConnectionHandler.resolveTargetFile) to first normalize the requested path by
removing any leading '/' (or use Paths.get(WEB_ROOT).resolve(path).normalize())
and then resolve it against the WEB_ROOT directory, obtain the resulting
File/Path canonical (or normalized) path, and verify it starts with the
canonical WEB_ROOT before reading; if the final resolved path is outside
WEB_ROOT, reject the request (e.g., return 403) instead of reading the file.
Ensure you reference StaticFileHandler/cacheFilter.getOrFetch and
ConnectionHandler.resolveTargetFile when making these changes so the
normalized/validated path is what's passed to Files.readAllBytes.
- Around line 24-25: The current code in StaticFileHandler constructs a
PrintWriter with platform default encoding and uses
writer.println(response.build()), causing charset mismatch with bytebody.length
and adding an extra newline; replace this by writing the raw response bytes to
the socket using the same byte array used for Content-Length (e.g., write
bytebody to outputStream and flush) or, if using a writer, create an
OutputStreamWriter/BufferedWriter with StandardCharsets.UTF_8 and use
write()/flush() (do not use println); update the code around response.build(),
PrintWriter, and outputStream to ensure the bytes sent match bytebody.length and
no extra line separator is appended.

---

Nitpick comments:
In `@src/main/java/org/example/CacheFilter.java`:
- Around line 10-13: The two System.out.println calls in CacheFilter (the "✓
Cache hit for: " and "✗ Cache miss for: " lines) should be replaced with a
proper logger: add a private static final logger (either SLF4J's
LoggerFactory.getLogger(CacheFilter.class) or
java.util.logging.Logger.getLogger(CacheFilter.class.getName())) and use logger
methods instead of println (e.g., logger.debug/trace for hits and
logger.info/warn for misses), passing the URI as a parameter or using
parameterized logging to avoid string concatenation; update the code in the
CacheFilter method where cache.get(uri) is returned to use logger.<class/method
references>: CacheFilter, the cache-hit/cache-miss println calls.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Find and inspect FileCache implementation
fd "FileCache.java" --exec cat {}

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 633

---

🏁 Script executed:

# Find and inspect CacheFilter to verify the code snippet
fd "CacheFilter.java" --exec cat {}

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 765

---

🏁 Script executed:

# Find and inspect StaticFileHandler to see how cacheFilter is used
fd "StaticFileHandler.java" --exec cat {}

Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template

Length of output: 1020

---

Non-atomic check-then-act in getOrFetch — data race under concurrent virtual threads.

cache.contains(uri) and cache.get(uri) / cache.put(uri, bytes) are two separate operations with no lock between them. The check-then-act pattern means that two or more threads checking a given condition and acting on that information can create a race: if two threads call the method simultaneously, both may evaluate the contains check and proceed to the miss path concurrently. Once the per-instance issue in StaticFileHandler is fixed by making cacheFilter a shared (static or injected) instance, this race becomes active.

FileCache is backed by a plain HashMap, which is not thread-safe; concurrent put calls during a resize can corrupt internal state. Even with a thread-safe backing store, check-then-act and read-modify-write are compound actions that must be atomic to ensure thread safety. Duplicate provider.fetch calls for the same URI are at minimum wasteful (double I/O).

Fix: Synchronize getOrFetch, or restructure around ConcurrentHashMap with computeIfAbsent (wrapping the checked exception).

Proposed fix — synchronize the compound operation

-    public byte[] getOrFetch(String uri, FileProvider provider) throws IOException {
+    public synchronized byte[] getOrFetch(String uri, FileProvider provider) throws IOException {
         if (cache.contains(uri)) {
             System.out.println("✓ Cache hit for: " + uri);
             return cache.get(uri);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	public byte[] getOrFetch(String uri, FileProvider provider) throws IOException {
	if (cache.contains(uri)) {
	System.out.println("✓ Cache hit for: " + uri);
	return cache.get(uri);
	}
	System.out.println("✗ Cache miss for: " + uri);
	byte[] fileBytes = provider.fetch(uri);
	cache.put(uri, fileBytes);
	return fileBytes;
	public synchronized byte[] getOrFetch(String uri, FileProvider provider) throws IOException {
	if (cache.contains(uri)) {
	System.out.println("✓ Cache hit for: " + uri);
	return cache.get(uri);
	}
	System.out.println("✗ Cache miss for: " + uri);
	byte[] fileBytes = provider.fetch(uri);
	cache.put(uri, fileBytes);
	return fileBytes;
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/CacheFilter.java` around lines 8 - 16, The
getOrFetch method has a non-atomic check-then-act on the FileCache (uses
cache.contains + cache.get/cache.put) causing races and double-fetches under
concurrent virtual threads; fix by making the compound operation atomic: either
synchronize the getOrFetch method (or a private lock object) around the
contains/get/fetch/put sequence, or switch FileCache to a thread-safe
ConcurrentHashMap and replace the contains/get/put pattern with computeIfAbsent
(wrapping provider.fetch to handle the checked IOException). Also ensure the
cache instance used by StaticFileHandler is shared (not per-instance) so the
chosen atomic approach is effective.