Implement configurable filter pipeline (global + per‑route filters)

[Ericthilen]

Summary by CodeRabbit

• New Features
  • HTTP server and static-file serving with configurable server and logging defaults; request filter pipeline with global/route filters and ordering.
• CI / CD
  • Java build workflow and release-triggered multi-arch Docker publish.
• Chores
  • Java target bumped to 25; added formatting, mutation-test tooling, and test libraries.
• Tests
  • Extensive unit/integration tests for parser, response builder, static files, config loader, and filter pipeline.
• Breaking Change
  • Removed prior console application entry point.

[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 Java 25 tooling and CI/CD workflows; introduces an HTTP server stack (parser, response builder, connection/tcp server, static file handler), a configurable filter pipeline with routing and ordering, configuration loader/records, many unit tests, Dockerfile and static web assets; removes the previous App entrypoint and its simple tests.

Changes

Cohort / File(s)	Summary
CI/CD Workflows ​.github/workflows/ci.yml, ​.github/workflows/release.yml	Adds Maven-based CI that derives JDK version from Maven and runs compile/tests; adds release workflow to build and push multi-arch Docker image to GHCR.
Build & Packaging pom.xml, Dockerfile	Bumps maven.compiler.release 23→25, adds bucket4j, test deps (mockito, awaitility, jackson), pitest and spotless plugins; introduces multi-stage Dockerfile using Temurin 25.
Removed legacy entrypoint & tests src/main/java/org/example/App.java, src/test/java/org/example/AppTest.java, src/test/java/org/example/AppIT.java	Removes the prior App main class and its two simple test classes.
Filter pipeline & routing src/main/java/org/example/server/ConfigurableFilterPipeline.java, src/main/java/org/example/server/FilterRegistration.java, src/main/java/org/example/server/RoutePattern.java	New configurable filter pipeline that partitions registrations into global vs route-specific, orders them, composes a FilterChain, and executes against a TerminalHandler; adds a FilterRegistration record and RoutePattern utility.
HTTP primitives & parser src/main/java/org/example/httpparser/HttpParseRequestLine.java, src/main/java/org/example/httpparser/HttpParser.java, src/main/java/org/example/http/HttpResponseBuilder.java	Adds request-line parsing, header parsing, and an HttpResponseBuilder to assemble raw HTTP responses; includes readers, header maps, and UTF-8 length handling.
Server runtime & static file serving src/main/java/org/example/ConnectionHandler.java, src/main/java/org/example/StaticFileHandler.java, src/main/java/org/example/TcpServer.java	Adds ConnectionHandler to parse requests and serve static files, StaticFileHandler for file resolution and 404 fallback, and TcpServer to accept sockets and dispatch per-connection handling (virtual threads).
Configuration API & loader src/main/java/org/example/config/AppConfig.java, src/main/java/org/example/config/ConfigLoader.java, src/main/resources/application.yml	Adds AppConfig record with nested ServerConfig/LoggingConfig and defaults application; adds ConfigLoader with lazy cached loading, YAML/JSON mapper selection, tests reset hook; provides default application.yml.
Tests: parsing, response, config, static files, pipeline src/test/java/org/example/httpparser/..., src/test/java/org/example/http/HttpResponseBuilderTest.java, src/test/java/org/example/config/ConfigLoaderTest.java, src/test/java/org/example/StaticFileHandlerTest.java, src/test/java/org/example/server/ConfigurableFilterPipelineTest.java	Adds comprehensive unit tests covering request-line and header parsing, HttpResponseBuilder behavior (UTF-8 length), ConfigLoader behaviors and validations, StaticFileHandler success/404 flows, and many pipeline behaviors (ordering, route-matching, short-circuiting).
Static web assets www/index.html, www/pageNotFound.html	Adds index and custom 404 pages used by StaticFileHandler.
Formatting & tooling tests pom.xml (spotless, pitest entries)	Introduces formatting and mutation-testing plugins configured in build lifecycle.

Sequence Diagram

sequenceDiagram
    participant Client as Client
    participant Pipeline as ConfigurableFilterPipeline
    participant Filters as HttpFilter(s)
    participant Handler as TerminalHandler

    Client->>Pipeline: execute(request, handler)
    Pipeline->>Pipeline: partition registrations (global / route)
    Pipeline->>Pipeline: sort by order & build FilterChain
    Pipeline->>Filters: invoke first.filter.handle(request, chain)
    Filters->>Filters: filter may call chain.next(request) or return response
    Filters->>Handler: chain.next(request) when end reached
    Handler->>Handler: handle(request) -> HttpResponse
    Handler-->>Pipeline: HttpResponse (via chain returns)
    Pipeline-->>Client: HttpResponse

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hop through filters, order neat and spry,
I match the paths, decide to stop or let pass by.
Java leaps to twenty-five, containers take flight,
Old main retired — tests now vet the server's light. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 4.12% 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 title accurately describes the main change: implementing a configurable filter pipeline with support for both global and per-route filters, which directly matches the new ConfigurableFilterPipeline class and FilterRegistration record added in the changeset.

_{✏️ 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: 5

🤖 Fix all issues with AI agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.

In @.github/workflows/ci.yml:
- Around line 28-32: CI currently only runs mvn -B compile and mvn -B test so
the spotless-maven-plugin (bound to the verify phase) isn't executed; update the
workflow to enforce formatting by either adding a dedicated step that runs mvn
spotless:check (e.g., name "Spotless check" run mvn -B spotless:check) before
tests, or change the existing test step to run mvn -B verify (replacing mvn -B
test) so the plugin runs during the build; reference the mvn commands and
spotless-maven-plugin when making the change.
- Around line 16-19: The "Get Java Version" step invokes mvn (mvn help:evaluate)
before any JDK is guaranteed to be installed; add a preceding setup-java step
(e.g., using actions/setup-java with a known LTS like 21) to install the JDK
before running the "Get Java Version" step, or alternatively hardcode the
JAVA_VERSION value instead of calling mvn; update the workflow so setup-java
runs before the step that executes mvn help:evaluate to ensure a stable JDK
environment.

In @.github/workflows/release.yml:
- Line 26: Replace the hardcoded container image value under the images key
("ghcr.io/ithsjava25/webserver") with a repository-derived expression using the
GitHub Actions context (e.g., construct the image name from github.repository)
so the workflow becomes portable when the repo is forked or renamed; update the
images entry to use the derived value (ghcr.io/${{ github.repository }} or
similar) and ensure any tag/version suffix remains appended as needed.

In `@pom.xml`:
- Around line 44-49: Extract the hardcoded Awaitility version 4.3.0 into a Maven
property (e.g., awaitility.version) and update the <version> element of the
org.awaitility:awaitility dependency to reference that property
(${awaitility.version}) so it matches how JUnit/AssertJ/Mockito versions are
managed; add the new property to the existing <properties> block to centralize
test dependency versions.

In `@src/main/java/org/example/App.java`:
- Line 5: Replace the hardcoded port 8080 passed to new TcpServer(8080).start()
in App.main with a configurable value: read the port from args (e.g., args[0])
or from an environment variable (e.g., PORT) and parse/validate it, falling back
to a sensible default (e.g., 8080) if missing or invalid; ensure you use the
TcpServer(...) constructor and call start() with that resolved integer and add
error handling for NumberFormatException or missing values.

In `@src/main/java/org/example/server/ConfigurableFilterPipeline.java`:
- Around line 90-97: The getOrder(HttpFilter) method in
ConfigurableFilterPipeline is unreliable because it uses identity comparison
(reg.filter() == filter) against FilterRegistration entries; remove this method
and update its call sites to obtain the order from the corresponding
FilterRegistration directly (e.g., use the registration.order() for the
registration you already resolved) or use equality-based matching when matching
by filter is truly required; if the upstream refactor that avoids needing this
lookup is applied, delete getOrder and any references to it and rely on the new
registration-based flow instead.
- Around line 16-50: The execute() method should sort the FilterRegistration
objects (the registrations collection) by their order and then extract
HttpFilter instances, instead of placing HttpFilter into
globalFilters/routeFilters and calling getOrder() during comparisons; change the
logic in execute() to: collect matching FilterRegistration into two lists
(globalRegs and routeRegs) based on reg.isGlobal() and
matchesAny(reg.routePatterns(), request.path()), sort those lists with
Comparator.comparingInt(FilterRegistration::order) (or equivalent), then map
them to HttpFilter via reg.filter() and combine; remove reliance on getOrder()
for sorting so duplicate filter instances with different registrations preserve
their respective orders and sort complexity improves, leaving
buildChain(allFilters, handler).next(request) untouched.
- Around line 12-13: The constructor
ConfigurableFilterPipeline(List<FilterRegistration> registrations) stores the
incoming list directly, making the internal field registrations vulnerable to
external mutation; change the constructor to defensively copy and null-check the
argument (e.g., use List.copyOf(registrations) after Objects.requireNonNull) so
the pipeline holds an immutable snapshot of registrations and cannot be affected
by caller mutations.
- Around line 100-146: The file contains multiple top-level types that should be
split into their own files; extract HttpRequest, HttpResponse, HttpFilter,
FilterChain, and TerminalHandler into separate public classes/interfaces (create
HttpRequest.java, HttpResponse.java, HttpFilter.java, FilterChain.java,
TerminalHandler.java) preserving their constructors and method signatures, keep
them in the same package, remove those definitions from
ConfigurableFilterPipeline.java, and update any imports/usages (e.g., references
to HttpRequest.path(), HttpRequest.getMethod(), HttpResponse.getStatusCode(),
HttpResponse.getBody(), and the handle/next methods) across the codebase so
compilation and behavior remain unchanged.

In `@src/main/java/org/example/server/FilterRegistration.java`:
- Around line 5-13: The record FilterRegistration accepts a mutable List<String>
routePatterns which can be modified after construction; add a compact
constructor for FilterRegistration that replaces the incoming routePatterns with
an immutable copy using List.copyOf(routePatterns) while preserving the existing
null semantic (i.e., if routePatterns is null keep it null, otherwise assign
List.copyOf(routePatterns)); this ensures immutability of the routePatterns
field and retains behavior of isGlobal().

In `@src/main/java/org/example/TcpServer.java`:
- Around line 19-23: The accept loop in TcpServer currently calls
serverSocket.accept() and then clientSocket.close() outside any
try-with-resources, risking a socket leak if handling code throws; change the
loop to acquire the accepted Socket in a try-with-resources (e.g., try (Socket
clientSocket = serverSocket.accept()) { ... }) so the socket is always closed,
move the System.out.println and any request-handling logic inside that try
block, and leave serverSocket management unchanged; update the loop in the
TcpServer class (the accept/close section) accordingly.
- Around line 15-27: The start() method currently accepts connections serially
using ServerSocket.accept() and handles them inline, which will block other
clients; modify TcpServer.start() to hand each accepted Socket (clientSocket)
off to a worker thread or an ExecutorService (e.g., create a fixed/ cached
ExecutorService field) and submit a Runnable/Callable that processes the
connection and closes clientSocket, instead of processing/closing it on the
accept thread; ensure proper shutdown of the ExecutorService and that exceptions
from the worker are logged/handled.
- Line 18: The ServerSocket instantiation in TcpServer (new ServerSocket(port))
is flagged for unencrypted cleartext; update the code to support TLS before
production by replacing or wrapping ServerSocket creation with an
SSLServerSocket from SSLServerSocketFactory (or ensure TLS is terminated
upstream via a reverse proxy) — modify the TcpServer socket-creation logic
(where ServerSocket is constructed) to accept an
SSLServerSocketFactory/SSLContext or to document/validate that only
non-production/local use is allowed so TLS termination is enforced when exposed
to real traffic.

In `@src/test/java/org/example/ConfigurableFilterPipelineTest.java`:
- Line 1: The test class ConfigurableFilterPipelineTest is declared in package
org.example.server but resides in src/test/java/org/example/..., causing a
package/path mismatch and preventing access to package-private types like
HttpFilter, FilterChain, and FilterRegistration; fix by either updating the
package declaration in ConfigurableFilterPipelineTest to match its directory
(e.g., package org.example;) or move the file into the server package directory
so the package declaration org.example.server matches the file path and the test
can access the package-private types.
- Around line 26-29: The test currently discards the return value of
pipeline.execute; update the global_filter_runs test to capture the HttpResponse
returned by pipeline.execute (call to pipeline.execute(new HttpRequest(...), new
TestHandler(events))) and add an assertion that response.getStatusCode() == 200
(or assertEquals(200, response.getStatusCode())) to verify the handler responded
successfully.
- Around line 12-58: Add unit tests to cover route-specific filter behavior for
ConfigurableFilterPipeline: create tests that register FilterRegistration
entries with a non-null route pattern and assert that HttpFilter/TestFilter runs
only when HttpRequest path matches and is skipped when it doesn't; add a test
with multiple FilterRegistration entries having different order values to verify
execution order of filters; add a test combining global (route == null) and
route-specific filters to assert globals run before matching route filters; use
pipeline.execute(HttpRequest, TestHandler) and assert events list and
HttpResponse status codes as in existing tests to validate behavior.

🧹 Nitpick comments (11)

🤖 Fix all nitpicks with AI agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.

In @.github/workflows/ci.yml:
- Around line 16-19: The "Get Java Version" step invokes mvn (mvn help:evaluate)
before any JDK is guaranteed to be installed; add a preceding setup-java step
(e.g., using actions/setup-java with a known LTS like 21) to install the JDK
before running the "Get Java Version" step, or alternatively hardcode the
JAVA_VERSION value instead of calling mvn; update the workflow so setup-java
runs before the step that executes mvn help:evaluate to ensure a stable JDK
environment.

In @.github/workflows/release.yml:
- Line 26: Replace the hardcoded container image value under the images key
("ghcr.io/ithsjava25/webserver") with a repository-derived expression using the
GitHub Actions context (e.g., construct the image name from github.repository)
so the workflow becomes portable when the repo is forked or renamed; update the
images entry to use the derived value (ghcr.io/${{ github.repository }} or
similar) and ensure any tag/version suffix remains appended as needed.

In `@pom.xml`:
- Around line 44-49: Extract the hardcoded Awaitility version 4.3.0 into a Maven
property (e.g., awaitility.version) and update the <version> element of the
org.awaitility:awaitility dependency to reference that property
(${awaitility.version}) so it matches how JUnit/AssertJ/Mockito versions are
managed; add the new property to the existing <properties> block to centralize
test dependency versions.

In `@src/main/java/org/example/App.java`:
- Line 5: Replace the hardcoded port 8080 passed to new TcpServer(8080).start()
in App.main with a configurable value: read the port from args (e.g., args[0])
or from an environment variable (e.g., PORT) and parse/validate it, falling back
to a sensible default (e.g., 8080) if missing or invalid; ensure you use the
TcpServer(...) constructor and call start() with that resolved integer and add
error handling for NumberFormatException or missing values.

In `@src/main/java/org/example/server/ConfigurableFilterPipeline.java`:
- Around line 12-13: The constructor
ConfigurableFilterPipeline(List<FilterRegistration> registrations) stores the
incoming list directly, making the internal field registrations vulnerable to
external mutation; change the constructor to defensively copy and null-check the
argument (e.g., use List.copyOf(registrations) after Objects.requireNonNull) so
the pipeline holds an immutable snapshot of registrations and cannot be affected
by caller mutations.
- Around line 100-146: The file contains multiple top-level types that should be
split into their own files; extract HttpRequest, HttpResponse, HttpFilter,
FilterChain, and TerminalHandler into separate public classes/interfaces (create
HttpRequest.java, HttpResponse.java, HttpFilter.java, FilterChain.java,
TerminalHandler.java) preserving their constructors and method signatures, keep
them in the same package, remove those definitions from
ConfigurableFilterPipeline.java, and update any imports/usages (e.g., references
to HttpRequest.path(), HttpRequest.getMethod(), HttpResponse.getStatusCode(),
HttpResponse.getBody(), and the handle/next methods) across the codebase so
compilation and behavior remain unchanged.

In `@src/main/java/org/example/server/FilterRegistration.java`:
- Around line 5-13: The record FilterRegistration accepts a mutable List<String>
routePatterns which can be modified after construction; add a compact
constructor for FilterRegistration that replaces the incoming routePatterns with
an immutable copy using List.copyOf(routePatterns) while preserving the existing
null semantic (i.e., if routePatterns is null keep it null, otherwise assign
List.copyOf(routePatterns)); this ensures immutability of the routePatterns
field and retains behavior of isGlobal().

In `@src/main/java/org/example/TcpServer.java`:
- Around line 15-27: The start() method currently accepts connections serially
using ServerSocket.accept() and handles them inline, which will block other
clients; modify TcpServer.start() to hand each accepted Socket (clientSocket)
off to a worker thread or an ExecutorService (e.g., create a fixed/ cached
ExecutorService field) and submit a Runnable/Callable that processes the
connection and closes clientSocket, instead of processing/closing it on the
accept thread; ensure proper shutdown of the ExecutorService and that exceptions
from the worker are logged/handled.
- Line 18: The ServerSocket instantiation in TcpServer (new ServerSocket(port))
is flagged for unencrypted cleartext; update the code to support TLS before
production by replacing or wrapping ServerSocket creation with an
SSLServerSocket from SSLServerSocketFactory (or ensure TLS is terminated
upstream via a reverse proxy) — modify the TcpServer socket-creation logic
(where ServerSocket is constructed) to accept an
SSLServerSocketFactory/SSLContext or to document/validate that only
non-production/local use is allowed so TLS termination is enforced when exposed
to real traffic.

In `@src/test/java/org/example/ConfigurableFilterPipelineTest.java`:
- Around line 26-29: The test currently discards the return value of
pipeline.execute; update the global_filter_runs test to capture the HttpResponse
returned by pipeline.execute (call to pipeline.execute(new HttpRequest(...), new
TestHandler(events))) and add an assertion that response.getStatusCode() == 200
(or assertEquals(200, response.getStatusCode())) to verify the handler responded
successfully.
- Around line 12-58: Add unit tests to cover route-specific filter behavior for
ConfigurableFilterPipeline: create tests that register FilterRegistration
entries with a non-null route pattern and assert that HttpFilter/TestFilter runs
only when HttpRequest path matches and is skipped when it doesn't; add a test
with multiple FilterRegistration entries having different order values to verify
execution order of filters; add a test combining global (route == null) and
route-specific filters to assert globals run before matching route filters; use
pipeline.execute(HttpRequest, TestHandler) and assert events list and
HttpResponse status codes as in existing tests to validate behavior.

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

15-27: Single-threaded accept loop will serialize all connections.

Given the PR objective is to implement a filter pipeline for an HTTP server, each accepted connection should be dispatched to a separate thread (or handed to an ExecutorService) so that one slow client doesn't block all others.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/main/java/org/example/TcpServer.java` around lines 15 - 27, The start()
method currently accepts connections serially using ServerSocket.accept() and
handles them inline, which will block other clients; modify TcpServer.start() to
hand each accepted Socket (clientSocket) off to a worker thread or an
ExecutorService (e.g., create a fixed/ cached ExecutorService field) and submit
a Runnable/Callable that processes the connection and closes clientSocket,
instead of processing/closing it on the accept thread; ensure proper shutdown of
the ExecutorService and that exceptions from the worker are logged/handled.

---

18-18: Note on unencrypted socket (static analysis flag).

The static analysis tool flags new ServerSocket(port) for cleartext transmission (CWE-319). This is fine for local development, but keep in mind that TLS termination (via SSLServerSocketFactory or a reverse proxy) will be needed before exposing this to any real traffic.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/main/java/org/example/TcpServer.java` at line 18, The ServerSocket
instantiation in TcpServer (new ServerSocket(port)) is flagged for unencrypted
cleartext; update the code to support TLS before production by replacing or
wrapping ServerSocket creation with an SSLServerSocket from
SSLServerSocketFactory (or ensure TLS is terminated upstream via a reverse
proxy) — modify the TcpServer socket-creation logic (where ServerSocket is
constructed) to accept an SSLServerSocketFactory/SSLContext or to
document/validate that only non-production/local use is allowed so TLS
termination is enforced when exposed to real traffic.

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

5-5: Consider making the port configurable.

The port is hardcoded to 8080. Since the PR is about introducing configurability, consider reading it from args, an environment variable, or a config file to stay consistent with that theme.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/main/java/org/example/App.java` at line 5, Replace the hardcoded port
8080 passed to new TcpServer(8080).start() in App.main with a configurable
value: read the port from args (e.g., args[0]) or from an environment variable
(e.g., PORT) and parse/validate it, falling back to a sensible default (e.g.,
8080) if missing or invalid; ensure you use the TcpServer(...) constructor and
call start() with that resolved integer and add error handling for
NumberFormatException or missing values.

.github/workflows/release.yml (1)

26-26: Consider deriving the image name from the repository.

The image name is hardcoded. Using ghcr.io/${{ github.repository }} makes the workflow portable if the repo is forked or renamed.

Suggested change

-          images: ghcr.io/ithsjava25/webserver
+          images: ghcr.io/${{ github.repository }}

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In @.github/workflows/release.yml at line 26, Replace the hardcoded container
image value under the images key ("ghcr.io/ithsjava25/webserver") with a
repository-derived expression using the GitHub Actions context (e.g., construct
the image name from github.repository) so the workflow becomes portable when the
repo is forked or renamed; update the images entry to use the derived value
(ghcr.io/${{ github.repository }} or similar) and ensure any tag/version suffix
remains appended as needed.

.github/workflows/ci.yml (1)

16-19: mvn is invoked before setup-java installs the target JDK.

This step relies on the JDK pre-installed on the GitHub runner image. It works today because mvn help:evaluate only reads the POM property without compiling, but if the runner image changes (e.g., removes Maven or the default JDK), this step will break silently. Consider adding a bootstrap setup-java step with a known LTS version (e.g., 21) before this step, or hardcode/duplicate the version.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In @.github/workflows/ci.yml around lines 16 - 19, The "Get Java Version" step
invokes mvn (mvn help:evaluate) before any JDK is guaranteed to be installed;
add a preceding setup-java step (e.g., using actions/setup-java with a known LTS
like 21) to install the JDK before running the "Get Java Version" step, or
alternatively hardcode the JAVA_VERSION value instead of calling mvn; update the
workflow so setup-java runs before the step that executes mvn help:evaluate to
ensure a stable JDK environment.

pom.xml (1)

44-49: Minor: awaitility version is hardcoded unlike other test dependency versions.

Other test dependencies (JUnit, AssertJ, Mockito) use <properties> for version management. Consider extracting 4.3.0 into a property for consistency.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@pom.xml` around lines 44 - 49, Extract the hardcoded Awaitility version 4.3.0
into a Maven property (e.g., awaitility.version) and update the <version>
element of the org.awaitility:awaitility dependency to reference that property
(${awaitility.version}) so it matches how JUnit/AssertJ/Mockito versions are
managed; add the new property to the existing <properties> block to centralize
test dependency versions.

src/test/java/org/example/ConfigurableFilterPipelineTest.java (2)

26-29: Return value of execute is discarded — consider asserting the response.

In global_filter_runs, the HttpResponse is not captured. Asserting statusCode == 200 would strengthen the test.

♻️ Suggested fix

-        pipeline.execute(
+        HttpResponse response = pipeline.execute(
                 new HttpRequest("GET", "/home"),
                 new TestHandler(events)
         );
 
         assertEquals(
                 List.of("filter", "handler"),
                 events
         );
+        assertEquals(200, response.getStatusCode());

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/test/java/org/example/ConfigurableFilterPipelineTest.java` around lines
26 - 29, The test currently discards the return value of pipeline.execute;
update the global_filter_runs test to capture the HttpResponse returned by
pipeline.execute (call to pipeline.execute(new HttpRequest(...), new
TestHandler(events))) and add an assertion that response.getStatusCode() == 200
(or assertEquals(200, response.getStatusCode())) to verify the handler responded
successfully.

---

12-58: Test coverage is thin — no route-specific filter tests.

The PR title is "Implement configurable filter pipeline (global + per‑route filters)" but tests only exercise global filters. Consider adding tests for:

• Route-specific filters that match the request path
• Route-specific filters that do not match (should be skipped)
• Multiple filters with different order values (verify execution order)
• Mixed global + route filters (verify globals run before route filters)

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/test/java/org/example/ConfigurableFilterPipelineTest.java` around lines
12 - 58, Add unit tests to cover route-specific filter behavior for
ConfigurableFilterPipeline: create tests that register FilterRegistration
entries with a non-null route pattern and assert that HttpFilter/TestFilter runs
only when HttpRequest path matches and is skipped when it doesn't; add a test
with multiple FilterRegistration entries having different order values to verify
execution order of filters; add a test combining global (route == null) and
route-specific filters to assert globals run before matching route filters; use
pipeline.execute(HttpRequest, TestHandler) and assert events list and
HttpResponse status codes as in existing tests to validate behavior.

src/main/java/org/example/server/FilterRegistration.java (1)

5-13: Consider making a defensive copy of routePatterns to preserve immutability.

Records are value-based types, but the List<String> parameter can be mutated after construction. A compact constructor with List.copyOf() (which also rejects null elements) would enforce true immutability. Since null is used to signal "global" in the current design, you'd need to handle that:

♻️ Suggested defensive copy

 public record FilterRegistration(
         HttpFilter filter,
         int order,
         List<String> routePatterns
 ) {
+    public FilterRegistration {
+        routePatterns = routePatterns == null ? null : List.copyOf(routePatterns);
+    }
+
     public boolean isGlobal() {
         return routePatterns == null || routePatterns.isEmpty();
     }
 }

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/main/java/org/example/server/FilterRegistration.java` around lines 5 -
13, The record FilterRegistration accepts a mutable List<String> routePatterns
which can be modified after construction; add a compact constructor for
FilterRegistration that replaces the incoming routePatterns with an immutable
copy using List.copyOf(routePatterns) while preserving the existing null
semantic (i.e., if routePatterns is null keep it null, otherwise assign
List.copyOf(routePatterns)); this ensures immutability of the routePatterns
field and retains behavior of isGlobal().

src/main/java/org/example/server/ConfigurableFilterPipeline.java (2)

12-13: Constructor does not make a defensive copy of registrations.

If the caller mutates the list after constructing the pipeline, behavior becomes unpredictable. Consider List.copyOf(registrations) for safety.

♻️ Suggested fix

     public ConfigurableFilterPipeline(List<FilterRegistration> registrations) {
-        this.registrations = registrations;
+        this.registrations = List.copyOf(registrations);
     }

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/main/java/org/example/server/ConfigurableFilterPipeline.java` around
lines 12 - 13, The constructor
ConfigurableFilterPipeline(List<FilterRegistration> registrations) stores the
incoming list directly, making the internal field registrations vulnerable to
external mutation; change the constructor to defensively copy and null-check the
argument (e.g., use List.copyOf(registrations) after Objects.requireNonNull) so
the pipeline holds an immutable snapshot of registrations and cannot be affected
by caller mutations.

---

100-146: Multiple top-level types in a single file hinders discoverability and IDE navigation.

HttpRequest, HttpResponse, HttpFilter, FilterChain, and TerminalHandler are all foundational types that will be imported across the codebase. Placing each in its own file (consistent with the existing FilterRegistration.java and RoutePattern.java pattern) improves discoverability and follows standard Java conventions.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/main/java/org/example/server/ConfigurableFilterPipeline.java` around
lines 100 - 146, The file contains multiple top-level types that should be split
into their own files; extract HttpRequest, HttpResponse, HttpFilter,
FilterChain, and TerminalHandler into separate public classes/interfaces (create
HttpRequest.java, HttpResponse.java, HttpFilter.java, FilterChain.java,
TerminalHandler.java) preserving their constructors and method signatures, keep
them in the same package, remove those definitions from
ConfigurableFilterPipeline.java, and update any imports/usages (e.g., references
to HttpRequest.path(), HttpRequest.getMethod(), HttpResponse.getStatusCode(),
HttpResponse.getBody(), and the handle/next methods) across the codebase so
compilation and behavior remain unchanged.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Spotless format check is not enforced in CI.

The spotless-maven-plugin check is bound to the verify phase in pom.xml, but CI only runs compile and test. Formatting violations will go undetected. Either add a separate mvn spotless:check step or change the test step to run mvn -B verify.

Proposed fix: add a Spotless check step

       - name: Test with Maven
         run: mvn -B test --file pom.xml
+
+      - name: Check formatting with Spotless
+        run: mvn -B spotless:check --file pom.xml

📝 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

	- name: Compile with Maven
	run: mvn -B compile --file pom.xml
	- name: Test with Maven
	run: mvn -B test --file pom.xml
	- name: Compile with Maven
	run: mvn -B compile --file pom.xml
	- name: Test with Maven
	run: mvn -B test --file pom.xml
	- name: Check formatting with Spotless
	run: mvn -B spotless:check --file pom.xml

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In @.github/workflows/ci.yml around lines 28 - 32, CI currently only runs mvn -B
compile and mvn -B test so the spotless-maven-plugin (bound to the verify phase)
isn't executed; update the workflow to enforce formatting by either adding a
dedicated step that runs mvn spotless:check (e.g., name "Spotless check" run mvn
-B spotless:check) before tests, or change the existing test step to run mvn -B
verify (replacing mvn -B test) so the plugin runs during the build; reference
the mvn commands and spotless-maven-plugin when making the change.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

clientSocket is not protected by try-with-resources — potential resource leak.

If any code between accept() and close() throws (or when you later add request handling logic between them), the socket will leak. Wrap it in its own try-with-resources.

🐛 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());
+                    // TODO: handle request / response
+                }
         }

📝 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());
	clientSocket.close();
	}
	while (true) {
	try (Socket clientSocket = serverSocket.accept()) {
	System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress());
	// TODO: handle request / response
	}
	}

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/main/java/org/example/TcpServer.java` around lines 19 - 23, The accept
loop in TcpServer currently calls serverSocket.accept() and then
clientSocket.close() outside any try-with-resources, risking a socket leak if
handling code throws; change the loop to acquire the accepted Socket in a
try-with-resources (e.g., try (Socket clientSocket = serverSocket.accept()) {
... }) so the socket is always closed, move the System.out.println and any
request-handling logic inside that try block, and leave serverSocket management
unchanged; update the loop in the TcpServer class (the accept/close section)
accordingly.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the file path vs package declaration
echo "=== Finding ConfigurableFilterPipelineTest.java ==="
fd "ConfigurableFilterPipelineTest.java"

echo ""
echo "=== Package declaration in the file ==="
fd "ConfigurableFilterPipelineTest.java" --exec head -5 {}

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

Length of output: 355

---

🏁 Script executed:

#!/bin/bash
# Check the full test file to see imports and class usage
echo "=== Full ConfigurableFilterPipelineTest.java ==="
cat -n src/test/java/org/example/ConfigurableFilterPipelineTest.java

echo ""
echo "=== Check if org.example.server exists with relevant classes ==="
fd "ConfigurableFilterPipeline\.java"
echo ""
fd "HttpFilter\.java"
echo ""
fd "FilterChain\.java"

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

Length of output: 3465

---

🏁 Script executed:

#!/bin/bash
# Check the directory structure in test source
echo "=== Directory structure in src/test/java/org/example/ ==="
ls -la src/test/java/org/example/

echo ""
echo "=== Check if src/test/java/org/example/server/ exists ==="
ls -la src/test/java/org/example/server/ 2>&1 || echo "Directory does not exist"

echo ""
echo "=== List all Java files in test directory ==="
find src/test/java -name "*.java"

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

Length of output: 834

---

Package declaration does not match the file path — this code will not compile.

The file is located at src/test/java/org/example/ConfigurableFilterPipelineTest.java but declares package org.example.server;. In Java, the package declaration must match the directory structure relative to the source root. Move this file to src/test/java/org/example/server/ConfigurableFilterPipelineTest.java to match the declared package, which is required since the test uses unimported types (HttpFilter, FilterChain, FilterRegistration, etc.) that are package-private or default-access from the same package.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/test/java/org/example/ConfigurableFilterPipelineTest.java` at line 1, The
test class ConfigurableFilterPipelineTest is declared in package
org.example.server but resides in src/test/java/org/example/..., causing a
package/path mismatch and preventing access to package-private types like
HttpFilter, FilterChain, and FilterRegistration; fix by either updating the
package declaration in ConfigurableFilterPipelineTest to match its directory
(e.g., package org.example;) or move the file into the server package directory
so the package declaration org.example.server matches the file path and the test
can access the package-private types.

[coderabbitai]

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/test/java/org/example/server/ConfigurableFilterPipelineTest.java`:
- Around line 12-58: Add tests that exercise per-route registrations, ordering,
and short-circuiting: create FilterRegistration instances with non-null
RoutePattern matching and non-matching routes and assert only matching per-route
filters run (use TestFilter and TestHandler with pipeline.execute), add multiple
FilterRegistration entries with differing priority values to verify
ConfigurableFilterPipeline orders filters by priority, and add a stopping
TestFilter followed by another filter to assert the second filter is never
invoked and the pipeline returns the stopping filter's response status;
reference FilterRegistration, RoutePattern, HttpFilter/TestFilter,
ConfigurableFilterPipeline, TestHandler, and pipeline.execute when adding these
new test cases.
- Around line 26-29: The test currently ignores the return value of
pipeline.execute; capture its HttpResponse (the result of pipeline.execute when
invoked with new HttpRequest("GET", "/home") and new TestHandler(events)) and
add an assertion that response.getStatusCode() (or equivalent status accessor on
the returned object) equals 200 to ensure TestHandler was executed; update the
test method global_filter_runs to assign the result to a variable and assert
status == 200 alongside existing event checks.

---

Duplicate comments:
In `@src/test/java/org/example/server/ConfigurableFilterPipelineTest.java`:
- Line 1: The package declaration in ConfigurableFilterPipelineTest.java should
match its directory; ensure the file begins with "package org.example.server;"
(as shown) so the class ConfigurableFilterPipelineTest compiles under
src/test/java/org/example/server; if not present, add or update that package
line and re-run mvn/gradle tests to confirm the prior compilation error is
resolved.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

global_filter_runs discards the pipeline response — missing assertion on status code.

The return value of pipeline.execute() is ignored. You should assert the response is 200 to verify the handler was actually reached and produced the correct result, not just that events were recorded.

Proposed fix

-        pipeline.execute(
+        HttpResponse response = pipeline.execute(
                 new HttpRequest("GET", "/home"),
                 new TestHandler(events)
         );
 
+        assertEquals(200, response.getStatusCode());
         assertEquals(
                 List.of("filter", "handler"),
                 events
         );

📝 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

	pipeline.execute(
	new HttpRequest("GET", "/home"),
	new TestHandler(events)
	);
	HttpResponse response = pipeline.execute(
	new HttpRequest("GET", "/home"),
	new TestHandler(events)
	);
	assertEquals(200, response.getStatusCode());
	assertEquals(
	List.of("filter", "handler"),
	events
	);

🤖 Prompt for AI Agents

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

In `@src/test/java/org/example/server/ConfigurableFilterPipelineTest.java` around
lines 26 - 29, The test currently ignores the return value of pipeline.execute;
capture its HttpResponse (the result of pipeline.execute when invoked with new
HttpRequest("GET", "/home") and new TestHandler(events)) and add an assertion
that response.getStatusCode() (or equivalent status accessor on the returned
object) equals 200 to ensure TestHandler was executed; update the test method
global_filter_runs to assign the result to a variable and assert status == 200
alongside existing event checks.

[coderabbitai]

Actionable comments posted: 1

🤖 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/test/java/org/example/server/ConfigurableFilterPipelineTest.java`:
- Around line 102-123: The test
mixed_pipeline_runs_global_then_route_then_handler relies on stable ordering for
equal order values; make it deterministic by assigning distinct order values to
the FilterRegistration entries instead of both using 1: keep the global
TestFilter/FilterRegistration at order 1 and change the route
TestFilter/FilterRegistration to a higher order (e.g. 2) so
ConfigurableFilterPipeline (constructed from regs) will always execute "g1"
before "r1" and produce the asserted events list; update only the order argument
in the List.of(...) registrations for the TestFilter instances referenced in
this test.

---

Duplicate comments:
In `@src/test/java/org/example/server/ConfigurableFilterPipelineTest.java`:
- Around line 26-29: The tests currently discard the return value of
pipeline.execute (called with new HttpRequest("GET", "/home") and new
TestHandler(events)), so they don't verify the response status; capture the
response from pipeline.execute in each affected test (global_filter_runs,
route_specific_filter_runs_when_path_matches,
route_specific_filter_is_skipped_when_path_does_not_match,
mixed_pipeline_runs_global_then_route_then_handler, ordering_is_by_order_field)
and add an assertion that response.getStatus() (or the appropriate status
accessor on the returned response object) equals the expected status code (e.g.,
200) to ensure the pipeline didn't return an error response while still
recording events.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

mixed_pipeline test relies on insertion order when both filters share the same order value.

Both the global and route filter are registered with order 1 (lines 110–111). The test asserts ["g1", "r1", "handler"], which implicitly assumes that filters with equal order preserve their registration order (stable sort). If ConfigurableFilterPipeline uses an unstable sort, this test becomes flaky. Either document this as an intentional contract (stable ordering) or give the two filters distinct order values to make the test deterministic regardless of sort stability.

🤖 Prompt for AI Agents

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

In `@src/test/java/org/example/server/ConfigurableFilterPipelineTest.java` around
lines 102 - 123, The test mixed_pipeline_runs_global_then_route_then_handler
relies on stable ordering for equal order values; make it deterministic by
assigning distinct order values to the FilterRegistration entries instead of
both using 1: keep the global TestFilter/FilterRegistration at order 1 and
change the route TestFilter/FilterRegistration to a higher order (e.g. 2) so
ConfigurableFilterPipeline (constructed from regs) will always execute "g1"
before "r1" and produce the asserted events list; update only the order argument
in the List.of(...) registrations for the TestFilter instances referenced in
this test.

[coderabbitai]

Actionable comments posted: 16

🧹 Nitpick comments (16)

src/main/java/org/example/config/ConfigLoader.java (1)

38-53: load() is public and does not apply defaults — document the distinction.

loadOnce() calls withDefaultsApplied() but load() does not. This asymmetry is a subtle footgun for callers. Consider adding a brief Javadoc to load() stating that it returns the raw parsed config and that callers must call withDefaultsApplied() themselves if needed.

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/config/ConfigLoader.java` around lines 38 - 53, Add
a brief Javadoc to the public load(Path) method explaining it returns the raw
parsed AppConfig (or AppConfig.defaults() when file missing/null) and does not
apply defaults; explicitly state callers must invoke
AppConfig.withDefaultsApplied() themselves if they need defaults applied.
Reference the related methods in the comment—loadOnce() and
withDefaultsApplied()—so callers understand the asymmetry and when to use load()
vs loadOnce().

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

14-15: Redundant local variable.

reader is just an alias for the parameter br and serves no purpose. Use br directly.

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` around lines
14 - 15, In parseHttpRequest (class HttpParseRequestLine) remove the redundant
local alias "reader" (which is assigned from the parameter br) and use the
method parameter br directly; update all references of "reader" inside
parseHttpRequest to "br" and delete the unused BufferedReader reader declaration
to eliminate the needless variable.

---

30-31: No validation on URI or HTTP version.

uri and version are set without any validation. Consider at minimum validating that version matches the expected HTTP/x.x pattern to reject malformed requests early, and sanitizing the URI against path traversal (e.g., rejecting .. segments) before it reaches downstream handlers.

🤖 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` around lines
30 - 31, HttpParseRequestLine currently calls setUri(requestLineArray[1]) and
setVersion(requestLineArray[2]) without validation; update the parsing logic in
HttpParseRequestLine to validate the version with a strict pattern (e.g., match
^HTTP/\d+\.\d+$) and reject or throw a parse error for non-matching values, and
sanitize/validate the URI passed to setUri by normalizing the path and rejecting
paths containing path-traversal segments like ".." (or percent-encoded
equivalents) before accepting it; ensure you use the existing setUri and
setVersion entrypoints so downstream code continues to work.

---

11-11: debug is always false — dead code.

The debug field is initialized to false with no setter or constructor parameter to enable it. The logging block on lines 34–38 is unreachable. Either remove the dead code or expose a way to toggle 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 field in class HttpParseRequestLine is dead code because it’s always
false; either remove the field and the logging block that references it (the
unreachable logging statements) or make it configurable by adding a constructor
parameter or a public setter (e.g., add HttpParseRequestLine(boolean debug) or
setDebug(boolean debug)) and use that value when deciding whether to execute the
logging block; alternatively replace the ad-hoc boolean with a proper logger
check (logger.isDebugEnabled()) if a logging framework is available. Ensure you
update any callers to pass the debug flag if you add a constructor, or add the
setter usage where instances are created if you choose the setter approach.

pom.xml (1)

130-136: JaCoCo coverage is disabled (<skip>true</skip>).

Was this intentional? If coverage reporting is desired in CI, this skip flag will suppress it entirely.

🤖 Prompt for AI Agents

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

In `@pom.xml` around lines 130 - 136, The jacoco configuration currently sets
<skip>true</skip> in the jacoco-maven-plugin which disables coverage; remove or
change that setting so coverage runs in CI — update the jacoco-maven-plugin
configuration (artifactId: jacoco-maven-plugin, version: 0.8.14) to either
remove the <skip> flag or set <skip>false</skip> and ensure the plugin's report
and prepare-agent goals are enabled for the build profile used in CI.

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

16-44: Test coverage is minimal — consider expanding.

Only the happy path (string body with 200) and UTF-8 length are tested. Consider adding tests for:

• setBody(byte[]) path
• Custom status codes (e.g., 404) and their reason phrases
• Custom headers via setHeaders()
• Interaction between the two setBody overloads (string then byte[], and vice versa)

These would catch the mutual-exclusion and idempotency bugs noted in the production code review.

🤖 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 missing paths: write
tests that call setBody(byte[]) (in addition to setBody(String)) and verify
build() uses the provided raw bytes for Content-Length and body bytes; add tests
that set a non-200 status (e.g., 404 with its reason phrase) and assert the
status line in build(); add tests that call setHeaders() with custom headers and
assert they appear (and that Content-Length/header precedence is correct); and
add tests exercising both orders of setBody overloads (String then byte[] and
byte[] then String) to ensure mutual exclusion/idempotency between
setBody(String) and setBody(byte[]).

---

7-7: Minor: Extra leading whitespace on class declaration.

Line 7 has extra indentation before class. Since Spotless is configured in the build, this may be caught automatically, but worth fixing for consistency.

🤖 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 7,
Remove the extra leading whitespace before the class declaration for
HttpResponseBuilderTest so the line reads "class HttpResponseBuilderTest {" with
normal indentation; update the file's indentation to match the project's style
(and reformat or run Spotless if configured) to ensure the change passes
formatting checks.

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

40-45: Dead debug code using System.out.println — inconsistent with parent's Logger.

Same debug = false with no setter pattern as the parent class. Also, the parent uses java.util.logging.Logger while this class uses System.out.println. Remove or unify the debug approach.

🤖 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 block in HttpParser using a local boolean "debug" and
System.out.println is dead/inconsistent; remove the prints and either remove the
"debug" flag or replace it with java.util.logging.Logger usage: obtain a Logger
(java.util.logging.Logger) in HttpParser, replace System.out.println calls that
reference headersMap and "Host" with logger.log(Level.FINE, ...) (or Level.INFO
if appropriate), and guard with logger.isLoggable(Level.FINE) instead of the
debug flag so logging behavior matches the parent's logging approach.

---

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

Callers can add, remove, or modify headers in the returned map, breaking encapsulation. Return an unmodifiable view if mutation is not intended.

Proposed fix

 public Map<String, String> getHeadersMap() {
-    return headersMap;
+    return java.util.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 internal mutable headersMap from
HttpParser, exposing and allowing external mutation; change it to return an
unmodifiable view or immutable copy instead (e.g., return
Collections.unmodifiableMap(new HashMap<>(headersMap))) and add the necessary
import (java.util.Collections and java.util.HashMap) so callers cannot modify
internal state while preserving current signature; update any unit tests that
relied on mutating the returned map.

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

12-53: Consider adding tests for duplicate header merging and parseRequest() flow.

Current tests cover basic header parsing, empty input, and invalid lines. Missing scenarios include:

• Duplicate headers (verifying the , concatenation in headersMap.merge)
• Full request flow: parseRequest() followed by parseHttp(), then asserting both method/URI/version and headers
• Calling parseHttp() when reader is not set (expected failure)

🤖 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 12 -
53, Add unit tests in HttpParserTest to cover duplicate-header merging and full
request parsing: create a test that feeds input with repeated header names
(e.g., "X-Test: a\r\nX-Test: b\r\n\r\n") to HttpParser.parseHttp() and assert
that HttpParser.getHeadersMap() contains "X-Test" with the concatenated value
matching the merge behavior (", " separator); add a test that exercises
parseRequest() followed by parseHttp() using a full request line (method, URI,
version) plus headers and assert the parsed method/URI/version (from
parseRequest()/getMethod()/getUri()/getVersion() or equivalent) and headers map;
and add a test that calls parseHttp() when no reader is set and assert the
expected failure/exception to ensure proper error handling.

Dockerfile (1)

10-11: USER directive should precede ENTRYPOINT.

While Docker applies USER at container start regardless of declaration order, the convention is to place USER before ENTRYPOINT/CMD so readers immediately see the security context. Also, any RUN/COPY after USER would execute as that user, so ordering matters for consistency.

Proposed fix

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

🤖 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 declares
ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"] before USER
appuser; move the USER appuser line to appear before the ENTRYPOINT line so the
USER directive precedes ENTRYPOINT (ensure no RUN/COPY that must run as root are
placed after the new USER line), preserving the same ENTRYPOINT value and order
of other instructions.

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

10-11: Fields should be private.

client and uri have package-private access. Since they aren't accessed from other classes in the package, make them private to enforce encapsulation.

♻️ Proposed fix

-    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, Make
the fields in ConnectionHandler private to enforce encapsulation: change the
visibility of the Socket client and String uri fields to private (i.e., update
the declarations for client and uri in class ConnectionHandler) so they are not
package-private; if other class usage exists, provide private getters/setters
instead of leaving them non-private.

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

13-13: WEB_ROOT naming — use camelCase for instance fields.

Java convention reserves ALL_CAPS for static final constants. Since this is an instance field, rename to webRoot.

♻️ Proposed fix

-    private final String WEB_ROOT;
+    private final String webRoot;

(Update all references in the class accordingly.)

🤖 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 13, The instance
field WEB_ROOT in StaticFileHandler should follow Java camelCase conventions;
rename the field to webRoot and update all references within the class
(constructor, methods, any usages) to use webRoot instead of WEB_ROOT; ensure
any assignments, getters/setters or usages in methods like the constructor and
serve/static file path resolution are updated accordingly and the field remains
non-static and final.

---

14-15: Mutable instance fields fileBytes and statusCode make the class stateful.

These fields are used only to pass data from handleGetRequest to sendGetRequest. This couples the two methods via side-effects and would break if the class were ever shared across threads. Consider returning a response record/object from handleGetRequest instead.

🤖 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 14 - 15, The
class currently uses mutable instance fields fileBytes and statusCode to pass
data between handleGetRequest and sendGetRequest, creating statefulness and
thread-safety issues; refactor handleGetRequest to return an immutable response
object or record (e.g., FileResponse { byte[] bytes; int status }) and change
sendGetRequest to accept that FileResponse as a parameter, remove the fileBytes
and statusCode fields, and update callers to use the returned FileResponse from
handleGetRequest when invoking sendGetRequest.

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

8-10: Mixing AssertJ and JUnit assertion styles.

Tests inconsistently use assertThatThrownBy (AssertJ) in one test and assertThrows (JUnit) in others. Pick one style for consistency—preferably AssertJ throughout since you're already importing it.

♻️ Example: unify on AssertJ

-    `@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");
-    }
+    `@Test`
+    void testParserThrowErrorWhenEmpty(){
+        InputStream in = new ByteArrayInputStream("".getBytes());
+        httpParseRequestLine.setReader(in);
+        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 tests in HttpParseRequestLineTest mix JUnit and AssertJ
assertions; unify to AssertJ by replacing usages of JUnit's
assertThrows/assertEquals/assertTrue/etc. with AssertJ equivalents (e.g., use
assertThatThrownBy(...) for exceptions and assertThat(...) for value checks) in
the test methods and remove the static import of
org.junit.jupiter.api.Assertions; update any test methods referencing
assertThrows to use assertThatThrownBy and convert other JUnit assertions to
assertThat calls so all assertions consistently use AssertJ.

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

18-26: Unencrypted ServerSocket — acceptable for local dev, but note for production.

Static analysis flagged the use of a plain ServerSocket (CWE-319). For a learning/demo server this is fine, but for any production use you'd want SSLServerSocketFactory or a TLS-terminating reverse proxy in front.

🤖 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 18 - 26, The code uses
an unencrypted ServerSocket in the TCP server (in the block creating
ServerSocket and spawning threads that call handleClient), which is fine for
local demos but not production; update the server to use an SSLServerSocket
created from SSLServerSocketFactory (or clearly document and enforce use of a
TLS-terminating proxy) by replacing ServerSocket instantiation with an
SSLServerSocket from SSLServerSocketFactory.getDefault()/custom SSLContext, load
server key material/keystore, and accept client connections from that
SSLServerSocket so handleClient receives encrypted sockets; ensure exception
handling and logging remain intact for SSL setup failures.

🤖 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 1-5: The Dockerfile only runs "mvn compile" so third‑party JARs
are not produced/copied and runtime will throw ClassNotFoundException; also the
repo's www/ static assets are never copied. Replace the compile step with a
package step that produces a runnable artifact (e.g., RUN mvn package with a
shade/assembly plugin configured to create an uber/fat JAR), or alternatively
copy dependency JARs from the build Maven repository and set the runtime
classpath to include target/classes plus those jars (refer to the existing RUN
mvn compile and java -classpath /app org.example.App in the diff). Finally add a
COPY www/ /app/www/ (or similar) so StaticFileHandler can find
index.html/pageNotFound.html and ensure the final image runs the fat JAR (or
uses the adjusted classpath).

In `@pom.xml`:
- Around line 22-26: Remove the unused Bucket4j dependency block (groupId
com.bucket4j, artifactId bucket4j_jdk17-core, version ${bucket4j.version}) from
the pom so it is not declared as a compile dependency; if you intend to add
rate-limiting later instead change it to an appropriate optional/test scope or
add a clear TODO comment linking to the feature ticket, but do not leave the
compile-scoped dependency in the pom unused.

In `@src/main/java/org/example/config/AppConfig.java`:
- Around line 3-4: The AppConfig class is importing Jackson 2.x annotations
(com.fasterxml.jackson.annotation.*) which are incompatible with the project
Jackson 3.x dependency and the ObjectMapper in ConfigLoader; replace those
imports so AppConfig uses the Jackson 3.x annotations (tools.jackson.annotations
or the correct tools.jackson package equivalents) for `@JsonProperty` and
`@JsonIgnoreProperties` to ensure the ObjectMapper in ConfigLoader recognizes them
during deserialization. Locate AppConfig and update its import statements for
JsonProperty and JsonIgnoreProperties to the matching tools.jackson namespace
(keeping the annotation usage and field names unchanged), then run tests to
confirm mapping at the deserialization call in ConfigLoader works as expected.

In `@src/main/java/org/example/config/ConfigLoader.java`:
- Around line 55-66: createMapperFor: the code incorrectly calls
YAMLMapper.builder(new YAMLFactory()) (Jackson 3.x YAMLMapper.builder() takes no
factory) and silently falls back to YAML for unknown extensions; remove the
YAMLFactory argument from both YAMLMapper.builder() calls and instead of
returning a YAML mapper for unrecognized file extensions throw an
IllegalArgumentException with a clear message; update createMapperFor to use
YAMLMapper.builder().build() for .yml/.yaml, JsonMapper.builder().build() for
.json, and throw new IllegalArgumentException("Unsupported config file
extension: "+ name) for other cases.

In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 27-36: The resolveTargetFile method currently appends ".html" to
any URI that doesn't end with ".html", which incorrectly rewrites asset requests
like "/style.css" to "/style.css.html"; update resolveTargetFile to (1) treat
"/" or "" as "index.html", (2) detect if the URI already has a file extension
using a regex like "\\.[^/]+$" (hasExtension) and leave it unchanged, and (3)
only append ".html" when the URI has no extension (i.e., not hasExtension).
Reference: method resolveTargetFile and the field this.uri.

In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Line 49: Replace the misleading hardcoded default "OK" used when looking up
reason phrases in REASON_PHRASES; in HttpResponseBuilder change the getOrDefault
call to return a neutral default like "Unknown" (or ensure REASON_PHRASES
contains the full set of standard status codes) so that unmapped status codes
(e.g., 301, 403, 405) do not incorrectly show "OK" — update the line that
references REASON_PHRASES and the local variable reason accordingly.
- Around line 21-27: The two overloads setBody(String) and setBody(byte[]) leave
the other representation intact, so build() (which checks body.isEmpty()) can
pick the wrong payload; modify setBody(String) to clear bytebody (set bytebody =
null) and modify setBody(byte[]) to clear body (set body = "") so they are
mutually exclusive, ensuring build() uses the intended branch when either
setBody(String) or setBody(byte[]) is called.
- Around line 38-46: The build() method mutates internal state by calling
setBody(new String(bytebody,...)) and can corrupt binary data; make build()
side-effect-free by not modifying fields (do not call setBody or change
body/bytebody). Instead, compute contentLength from bytebody when bytebody !=
null (use bytebody.length) or from body.getBytes(UTF_8). Assemble the response
as bytes (use a ByteArrayOutputStream or similar) writing the headers as UTF_8
bytes and then writing bytebody directly when present, otherwise write body
bytes; ensure build() returns the final serialized bytes/string without altering
the HttpResponseBuilder's body or bytebody fields.

In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Around line 16-20: The setReader method in HttpParser currently ignores
subsequent calls because of the if (this.reader == null) guard; change setReader
to either throw an IllegalStateException if reader is already set (fail-fast) or
(preferably) allow reassignment by always replacing this.reader with a new
BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8)), close the
previous reader if non-null to avoid resource leaks, and reset any parsing state
such as headersMap (clear or reinitialize headersMap) so the parser is ready for
a new connection; update references to reader and headersMap accordingly in the
HttpParser class.
- Around line 22-25: parseHttp() and parseRequest() currently dereference the
instance field reader without ensuring it's initialized; add a precondition
check at the start of both methods (parseHttp() and parseRequest()) that
validates reader != null and throws a clear unchecked exception such as
IllegalStateException with a descriptive message (e.g., "reader not set; call
setReader(...) before parsing"), or alternatively initialize reader eagerly in
the constructor/field declaration if appropriate; use the setReader(...) method
name in the message to guide callers.

In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 27-42: The handleGetRequest method currently constructs File file
= new File(WEB_ROOT, uri) without sanitizing uri, enabling path traversal;
change this to canonicalize/normalize both WEB_ROOT and the resolved requested
path (e.g., use Path#toRealPath or File#getCanonicalPath on WEB_ROOT and the
resolved path for the File created from WEB_ROOT + uri) and verify the requested
path startsWith the canonical WEB_ROOT before reading bytes; if the check fails,
treat as not found or forbidden (set statusCode = 404 or 403 and use
pageNotFound.html or a fixed message) and avoid reading the fileBytes from
outside WEB_ROOT. Ensure this logic is applied in handleGetRequest and uses the
same canonical paths for comparisons to prevent symlink bypass.
- Around line 44-54: The response currently hardcodes "Content-Type: text/html"
in sendGetRequest which mislabels all files; update sendGetRequest to determine
the MIME type dynamically (e.g., use Files.probeContentType(path) or a small
extension→MIME map) after handleGetRequest has populated fileBytes and the file
path/filename, then call response.setHeaders(Map.of("Content-Type", mimeType));
ensure you fall back to "application/octet-stream" if probeContentType returns
null and keep using HttpResponseBuilder, statusCode, and fileBytes as before.

In `@src/main/java/org/example/TcpServer.java`:
- Around line 29-35: The catch in handleClient currently concatenates the
exception into a string and loses the original stack trace; change the throw to
use the two-argument RuntimeException constructor so the original exception is
preserved as the cause (i.e., throw new RuntimeException("Error handling client
connection", e)); update the block around ConnectionHandler and
runConnectionHandler to rethrow with the cause rather than stringifying the
exception.

In `@src/test/java/org/example/config/ConfigLoaderTest.java`:
- Around line 117-126: Replace the Swedish inline comment in the test method
invalid_yaml_fails with English for consistency: change "// saknar ':' efter
port" to an English comment like "// missing ':' after port" in the test that
writes "server:\n  port 8080\n" (located near the creation of Path configFile
and the Files.writeString call) so the test and surrounding codebase use a
single language.
- Around line 128-140: Rename the test method
invalid_port_should_Throw_Exception in ConfigLoaderTest to follow consistent
snake_case and remove the extra spaces before the parentheses; for example,
change it to invalid_port_should_throw_exception() (keep the `@Test` and
`@DisplayName` unchanged) so the method name uses lowercase for "throw" and
"exception" and has no trailing whitespace before "()".

In `@src/test/java/org/example/httpparser/HttpParserTest.java`:
- Around line 15-28: The test TestHttpParserForHeaders currently feeds a full
HTTP request (including the request line) into httpParser.parseHttp(), which
lets the request line be ignored by accident; either change the test to call
httpParser.parseRequest() before httpParser.parseHttp() to consume the request
line, or alter testInput to start at the headers only (remove the "GET /...
HTTP/1.1\r\n" line) so parseHttp() receives only header lines; also rename the
test method to camelCase (e.g., testHttpParserForHeaders) to follow Java test
naming conventions.

---

Duplicate comments:
In `@src/main/java/org/example/TcpServer.java`:
- Around line 20-22: No change required: the accepted client socket from
serverSocket.accept() is correctly passed into handleClient(...) which uses
ConnectionHandler (implements AutoCloseable) to manage and close the socket, so
leave the Thread.ofVirtual().start(() -> handleClient(clientSocket)) call as-is
and ensure ConnectionHandler continues to close the socket in its close()
implementation.

---

Nitpick comments:
In `@Dockerfile`:
- Around line 10-11: The Dockerfile currently declares ENTRYPOINT ["java",
"-classpath", "/app", "org.example.App"] before USER appuser; move the USER
appuser line to appear before the ENTRYPOINT line so the USER directive precedes
ENTRYPOINT (ensure no RUN/COPY that must run as root are placed after the new
USER line), preserving the same ENTRYPOINT value and order of other
instructions.

In `@pom.xml`:
- Around line 130-136: The jacoco configuration currently sets <skip>true</skip>
in the jacoco-maven-plugin which disables coverage; remove or change that
setting so coverage runs in CI — update the jacoco-maven-plugin configuration
(artifactId: jacoco-maven-plugin, version: 0.8.14) to either remove the <skip>
flag or set <skip>false</skip> and ensure the plugin's report and prepare-agent
goals are enabled for the build profile used in CI.

In `@src/main/java/org/example/config/ConfigLoader.java`:
- Around line 38-53: Add a brief Javadoc to the public load(Path) method
explaining it returns the raw parsed AppConfig (or AppConfig.defaults() when
file missing/null) and does not apply defaults; explicitly state callers must
invoke AppConfig.withDefaultsApplied() themselves if they need defaults applied.
Reference the related methods in the comment—loadOnce() and
withDefaultsApplied()—so callers understand the asymmetry and when to use load()
vs loadOnce().

In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 10-11: Make the fields in ConnectionHandler private to enforce
encapsulation: change the visibility of the Socket client and String uri fields
to private (i.e., update the declarations for client and uri in class
ConnectionHandler) so they are not package-private; if other class usage exists,
provide private getters/setters instead of leaving them non-private.

In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Around line 40-45: The debug block in HttpParser using a local boolean "debug"
and System.out.println is dead/inconsistent; remove the prints and either remove
the "debug" flag or replace it with java.util.logging.Logger usage: obtain a
Logger (java.util.logging.Logger) in HttpParser, replace System.out.println
calls that reference headersMap and "Host" with logger.log(Level.FINE, ...) (or
Level.INFO if appropriate), and guard with logger.isLoggable(Level.FINE) instead
of the debug flag so logging behavior matches the parent's logging approach.
- Around line 53-55: getHeadersMap() currently returns the internal mutable
headersMap from HttpParser, exposing and allowing external mutation; change it
to return an unmodifiable view or immutable copy instead (e.g., return
Collections.unmodifiableMap(new HashMap<>(headersMap))) and add the necessary
import (java.util.Collections and java.util.HashMap) so callers cannot modify
internal state while preserving current signature; update any unit tests that
relied on mutating the returned map.

In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java`:
- Around line 14-15: In parseHttpRequest (class HttpParseRequestLine) remove the
redundant local alias "reader" (which is assigned from the parameter br) and use
the method parameter br directly; update all references of "reader" inside
parseHttpRequest to "br" and delete the unused BufferedReader reader declaration
to eliminate the needless variable.
- Around line 30-31: HttpParseRequestLine currently calls
setUri(requestLineArray[1]) and setVersion(requestLineArray[2]) without
validation; update the parsing logic in HttpParseRequestLine to validate the
version with a strict pattern (e.g., match ^HTTP/\d+\.\d+$) and reject or throw
a parse error for non-matching values, and sanitize/validate the URI passed to
setUri by normalizing the path and rejecting paths containing path-traversal
segments like ".." (or percent-encoded equivalents) before accepting it; ensure
you use the existing setUri and setVersion entrypoints so downstream code
continues to work.
- Line 11: The debug field in class HttpParseRequestLine is dead code because
it’s always false; either remove the field and the logging block that references
it (the unreachable logging statements) or make it configurable by adding a
constructor parameter or a public setter (e.g., add HttpParseRequestLine(boolean
debug) or setDebug(boolean debug)) and use that value when deciding whether to
execute the logging block; alternatively replace the ad-hoc boolean with a
proper logger check (logger.isDebugEnabled()) if a logging framework is
available. Ensure you update any callers to pass the debug flag if you add a
constructor, or add the setter usage where instances are created if you choose
the setter approach.

In `@src/main/java/org/example/StaticFileHandler.java`:
- Line 13: The instance field WEB_ROOT in StaticFileHandler should follow Java
camelCase conventions; rename the field to webRoot and update all references
within the class (constructor, methods, any usages) to use webRoot instead of
WEB_ROOT; ensure any assignments, getters/setters or usages in methods like the
constructor and serve/static file path resolution are updated accordingly and
the field remains non-static and final.
- Around line 14-15: The class currently uses mutable instance fields fileBytes
and statusCode to pass data between handleGetRequest and sendGetRequest,
creating statefulness and thread-safety issues; refactor handleGetRequest to
return an immutable response object or record (e.g., FileResponse { byte[]
bytes; int status }) and change sendGetRequest to accept that FileResponse as a
parameter, remove the fileBytes and statusCode fields, and update callers to use
the returned FileResponse from handleGetRequest when invoking sendGetRequest.

In `@src/main/java/org/example/TcpServer.java`:
- Around line 18-26: The code uses an unencrypted ServerSocket in the TCP server
(in the block creating ServerSocket and spawning threads that call
handleClient), which is fine for local demos but not production; update the
server to use an SSLServerSocket created from SSLServerSocketFactory (or clearly
document and enforce use of a TLS-terminating proxy) by replacing ServerSocket
instantiation with an SSLServerSocket from
SSLServerSocketFactory.getDefault()/custom SSLContext, load server key
material/keystore, and accept client connections from that SSLServerSocket so
handleClient receives encrypted sockets; ensure exception handling and logging
remain intact for SSL setup failures.

In `@src/test/java/org/example/http/HttpResponseBuilderTest.java`:
- Around line 16-44: Add unit tests for HttpResponseBuilder to cover the missing
paths: write tests that call setBody(byte[]) (in addition to setBody(String))
and verify build() uses the provided raw bytes for Content-Length and body
bytes; add tests that set a non-200 status (e.g., 404 with its reason phrase)
and assert the status line in build(); add tests that call setHeaders() with
custom headers and assert they appear (and that Content-Length/header precedence
is correct); and add tests exercising both orders of setBody overloads (String
then byte[] and byte[] then String) to ensure mutual exclusion/idempotency
between setBody(String) and setBody(byte[]).
- Line 7: Remove the extra leading whitespace before the class declaration for
HttpResponseBuilderTest so the line reads "class HttpResponseBuilderTest {" with
normal indentation; update the file's indentation to match the project's style
(and reformat or run Spotless if configured) to ensure the change passes
formatting checks.

In `@src/test/java/org/example/httpparser/HttpParseRequestLineTest.java`:
- Around line 8-10: The tests in HttpParseRequestLineTest mix JUnit and AssertJ
assertions; unify to AssertJ by replacing usages of JUnit's
assertThrows/assertEquals/assertTrue/etc. with AssertJ equivalents (e.g., use
assertThatThrownBy(...) for exceptions and assertThat(...) for value checks) in
the test methods and remove the static import of
org.junit.jupiter.api.Assertions; update any test methods referencing
assertThrows to use assertThatThrownBy and convert other JUnit assertions to
assertThat calls so all assertions consistently use AssertJ.

In `@src/test/java/org/example/httpparser/HttpParserTest.java`:
- Around line 12-53: Add unit tests in HttpParserTest to cover duplicate-header
merging and full request parsing: create a test that feeds input with repeated
header names (e.g., "X-Test: a\r\nX-Test: b\r\n\r\n") to HttpParser.parseHttp()
and assert that HttpParser.getHeadersMap() contains "X-Test" with the
concatenated value matching the merge behavior (", " separator); add a test that
exercises parseRequest() followed by parseHttp() using a full request line
(method, URI, version) plus headers and assert the parsed method/URI/version
(from parseRequest()/getMethod()/getUri()/getVersion() or equivalent) and
headers map; and add a test that calls parseHttp() when no reader is set and
assert the expected failure/exception to ensure proper error handling.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Runtime dependencies are missing from the final image.

mvn compile only compiles source code into target/classes/. Third-party dependency JARs (Jackson, Bucket4j, etc.) are not included. At runtime, java -classpath /app org.example.App will fail with ClassNotFoundException for any library class.

Additionally, the www/ directory (containing static web assets like index.html and pageNotFound.html) lives at the repo root and is never copied into the build context or the final image, so the StaticFileHandler won't find its web root.

Consider using mvn package to produce a fat/uber JAR (with a shade or assembly plugin), or at minimum copy dependency JARs and adjust the classpath. Also add COPY www/ /app/www/ (or similar) to include static assets.

Proposed sketch (dependency-copy approach)

 FROM maven:3-eclipse-temurin-25-alpine AS build
 WORKDIR /build
 COPY src/ src/
 COPY pom.xml pom.xml
-RUN mvn compile
+COPY www/ www/
+RUN mvn dependency:copy-dependencies -DoutputDirectory=target/libs && mvn compile

 FROM eclipse-temurin:25-jre-alpine
 RUN addgroup -S appgroup && adduser -S appuser -G appgroup
-COPY --from=build /build/target/classes/ /app/
-ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
 USER appuser
+COPY --from=build /build/target/classes/ /app/classes/
+COPY --from=build /build/target/libs/ /app/libs/
+COPY --from=build /build/www/ /app/www/
+ENTRYPOINT ["java", "-classpath", "/app/classes:/app/libs/*", "org.example.App"]

🤖 Prompt for AI Agents

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

In `@Dockerfile` around lines 1 - 5, The Dockerfile only runs "mvn compile" so
third‑party JARs are not produced/copied and runtime will throw
ClassNotFoundException; also the repo's www/ static assets are never copied.
Replace the compile step with a package step that produces a runnable artifact
(e.g., RUN mvn package with a shade/assembly plugin configured to create an
uber/fat JAR), or alternatively copy dependency JARs from the build Maven
repository and set the runtime classpath to include target/classes plus those
jars (refer to the existing RUN mvn compile and java -classpath /app
org.example.App in the diff). Finally add a COPY www/ /app/www/ (or similar) so
StaticFileHandler can find index.html/pageNotFound.html and ensure the final
image runs the fat JAR (or uses the adjusted classpath).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for bucket4j imports and usage in Java source files
echo "=== Searching for bucket4j imports and usage ==="
rg -i "bucket4j|import.*bucket" --type java -n

echo ""
echo "=== Checking pom.xml for Bucket4j references ==="
grep -n "bucket4j" pom.xml

echo ""
echo "=== Searching in all file types for bucket4j mentions ==="
rg -i "bucket4j" -n --max-count=20

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

Length of output: 729

---

Remove unused Bucket4j dependency from pom.xml.

Bucket4j is declared as a compile-scoped dependency but is not imported or used anywhere in the codebase. Either remove the dependency if it's not needed, or defer adding it until the rate-limiting functionality is actually implemented.

🤖 Prompt for AI Agents

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

In `@pom.xml` around lines 22 - 26, Remove the unused Bucket4j dependency block
(groupId com.bucket4j, artifactId bucket4j_jdk17-core, version
${bucket4j.version}) from the pom so it is not declared as a compile dependency;
if you intend to add rate-limiting later instead change it to an appropriate
optional/test scope or add a clear TODO comment linking to the feature ticket,
but do not leave the compile-scoped dependency in the pom unused.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check which Jackson packages are declared as dependencies
echo "=== pom.xml dependencies ==="
fd pom.xml --exec cat {} \; 2>/dev/null | grep -A2 -B2 -i "jackson" | head -50

echo ""
echo "=== build.gradle dependencies ==="
fd build.gradle --exec cat {} \; 2>/dev/null | grep -i "jackson" | head -30

echo ""
echo "=== All Jackson imports in Java files ==="
rg -n "import (com\.fasterxml\.jackson|tools\.jackson)" --type=java

echo ""
echo "=== List of Java files in config directory ==="
fd "\.java$" src/main/java/org/example/config --exec cat -n {} \; 2>/dev/null | head -100

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

Length of output: 5188

---

Critical: Jackson namespace mismatch — annotations won't be recognized by the mapper.

AppConfig imports from com.fasterxml.jackson (Jackson 2.x), but the project declares Jackson 3.x as a dependency (tools.jackson.core:jackson-databind:3.0.3). ConfigLoader correctly uses tools.jackson, but its ObjectMapper will not recognize @JsonProperty and @JsonIgnoreProperties annotations from the incompatible com.fasterxml.jackson package. Deserialization of AppConfig at line 48 will silently fail to apply field mappings.

Fix

-import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
-import com.fasterxml.jackson.annotation.JsonProperty;
+import tools.jackson.annotation.JsonIgnoreProperties;
+import tools.jackson.annotation.JsonProperty;

📝 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

	import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
	import com.fasterxml.jackson.annotation.JsonProperty;
	import tools.jackson.annotation.JsonIgnoreProperties;
	import tools.jackson.annotation.JsonProperty;

🤖 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 3 - 4, The
AppConfig class is importing Jackson 2.x annotations
(com.fasterxml.jackson.annotation.*) which are incompatible with the project
Jackson 3.x dependency and the ObjectMapper in ConfigLoader; replace those
imports so AppConfig uses the Jackson 3.x annotations (tools.jackson.annotations
or the correct tools.jackson package equivalents) for `@JsonProperty` and
`@JsonIgnoreProperties` to ensure the ObjectMapper in ConfigLoader recognizes them
during deserialization. Locate AppConfig and update its import statements for
JsonProperty and JsonIgnoreProperties to the matching tools.jackson namespace
(keeping the annotation usage and field names unchanged), then run tests to
confirm mapping at the deserialization call in ConfigLoader works as expected.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "ConfigLoader.java" | head -5

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

Length of output: 160

---

🏁 Script executed:

find . -type f \( -name "pom.xml" -o -name "build.gradle" -o -name "build.gradle.kts" \) | head -5

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

Length of output: 117

---

🌐 Web query:

Jackson 3.x tools.jackson YAMLMapper builder API constructor

💡 Result:

In Jackson 3.x (the tools.jackson.* packages), the recommended way to create a YAML mapper is via the builder (mappers are intended to be configured immutably and then build()-ed):

import tools.jackson.dataformat.yaml.YAMLMapper;

YAMLMapper mapper = YAMLMapper.builder()
    // .enable(...) / .disable(...) / .configure(...)
    .build();

This works because YAMLMapper exposes a YAMLMapper.Builder (since 3.0) that extends the shared MapperBuilder API, whose purpose is to construct configured mapper instances in Jackson 3.x. [1][2]

You’ll also see real-world Jackson 3 migrations switching to exactly YAMLMapper.builder().build() with tools.jackson.dataformat.yaml.YAMLMapper. [3]

Note: the YAMLMapper.Builder constructor you may see in Javadoc (Builder(YAMLMapper m)) is for the builder implementation itself; normal usage is YAMLMapper.builder() rather than instantiating the builder directly. [2]

Sources:
[1] https://javadoc.io/static/tools.jackson.core/jackson-databind/3.0.0/tools.jackson.databind/tools/jackson/databind/cfg/MapperBuilder.html
[2] https://fasterxml.github.io/jackson-dataformats-text/javadoc/yaml/2.10/com/fasterxml/jackson/dataformat/yaml/YAMLMapper.Builder.html
[3] https://www.mail-archive.com/commits%40logging.apache.org/msg52345.html

---

🏁 Script executed:

cat -n ./src/main/java/org/example/config/ConfigLoader.java

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

Length of output: 2737

---

🏁 Script executed:

cat ./pom.xml | grep -A 5 -B 5 "jackson"

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

Length of output: 613

---

Fix Jackson 3.x API usage: YAMLMapper.builder() does not take YAMLFactory argument.

In Jackson 3.0.3 (tools.jackson), YAMLMapper manages its factory internally. Remove the YAMLFactory argument from both builder() calls on lines 59 and 64. Additionally, the silent fallback to YAML for unrecognized extensions (line 63–64) masks misconfiguration; throw IllegalArgumentException instead to surface the error early.

Proposed fix

 private static ObjectMapper createMapperFor(Path configPath) {
     String name = configPath.getFileName().toString().toLowerCase();

     if (name.endsWith(".yml") || name.endsWith(".yaml")) {
-        return YAMLMapper.builder(new YAMLFactory()).build();
+        return YAMLMapper.builder().build();

     } else if (name.endsWith(".json")) {
         return JsonMapper.builder().build();
-    } else  {
-        return YAMLMapper.builder(new YAMLFactory()).build();
+    } else {
+        throw new IllegalArgumentException(
+            "Unsupported config file format: " + name + ". Use .yml, .yaml, or .json");
     }
 }

🤖 Prompt for AI Agents

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

In `@src/main/java/org/example/config/ConfigLoader.java` around lines 55 - 66,
createMapperFor: the code incorrectly calls YAMLMapper.builder(new
YAMLFactory()) (Jackson 3.x YAMLMapper.builder() takes no factory) and silently
falls back to YAML for unknown extensions; remove the YAMLFactory argument from
both YAMLMapper.builder() calls and instead of returning a YAML mapper for
unrecognized file extensions throw an IllegalArgumentException with a clear
message; update createMapperFor to use YAMLMapper.builder().build() for
.yml/.yaml, JsonMapper.builder().build() for .json, and throw new
IllegalArgumentException("Unsupported config file extension: "+ name) for other
cases.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Regex on line 30 appends .html to all non-HTML URIs, including CSS, JS, images, etc.

The negative lookahead ^(?!.*\\.html$).*$ matches any URI that doesn't end in .html. Requests for /style.css, /script.js, or /image.png would be rewritten to /style.css.html, /script.js.html, etc., resulting in 404s for all non-HTML static assets.

Consider checking whether the URI already has an extension instead:

🐛 Proposed fix — only append .html to extension-less URIs

     private void resolveTargetFile(String uri) {
-        if (uri.matches("/$")) { //matches(/)
+        if (uri.equals("/")) {
             this.uri = "index.html";
-        } else if (uri.matches("^(?!.*\\.html$).*$")) {
-            this.uri = uri.concat(".html");
+        } else if (!uri.contains(".")) {
+            this.uri = uri.substring(1).concat(".html");
         } else {
-            this.uri = uri;
+            this.uri = uri.substring(1);
         }
-
     }

📝 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

	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;
	}
	}
	private void resolveTargetFile(String uri) {
	if (uri.equals("/")) {
	this.uri = "index.html";
	} else if (!uri.contains(".")) {
	this.uri = uri.substring(1).concat(".html");
	} else {
	this.uri = uri.substring(1);
	}
	}

🤖 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 - 36, The
resolveTargetFile method currently appends ".html" to any URI that doesn't end
with ".html", which incorrectly rewrites asset requests like "/style.css" to
"/style.css.html"; update resolveTargetFile to (1) treat "/" or "" as
"index.html", (2) detect if the URI already has a file extension using a regex
like "\\.[^/]+$" (hasExtension) and leave it unchanged, and (3) only append
".html" when the URI has no extension (i.e., not hasExtension). Reference:
method resolveTargetFile and the field this.uri.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Hardcoded Content-Type: text/html for all responses.

Every served file—including potential future CSS, JS, or image assets—will be labelled text/html. Use Files.probeContentType() or a simple extension→MIME map to derive the correct type dynamically.

♻️ Proposed fix — derive Content-Type from file extension

+    private String resolveContentType(String fileName) {
+        if (fileName.endsWith(".css"))  return "text/css; charset=utf-8";
+        if (fileName.endsWith(".js"))   return "application/javascript; charset=utf-8";
+        if (fileName.endsWith(".png"))  return "image/png";
+        if (fileName.endsWith(".jpg"))  return "image/jpeg";
+        return "text/html; charset=utf-8";
+    }

Then in sendGetRequest:

-        response.setHeaders(Map.of("Content-Type", "text/html; charset=utf-8"));
+        response.setHeaders(Map.of("Content-Type", resolveContentType(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 void sendGetRequest(OutputStream outputStream, String uri) throws IOException{
	handleGetRequest(uri);
	HttpResponseBuilder response = new HttpResponseBuilder();
	response.setStatusCode(statusCode);
	response.setHeaders(Map.of("Content-Type", "text/html; charset=utf-8"));
	response.setBody(fileBytes);
	PrintWriter writer = new PrintWriter(outputStream, true);
	writer.println(response.build());
	}
	private String resolveContentType(String fileName) {
	if (fileName.endsWith(".css")) return "text/css; charset=utf-8";
	if (fileName.endsWith(".js")) return "application/javascript; charset=utf-8";
	if (fileName.endsWith(".png")) return "image/png";
	if (fileName.endsWith(".jpg")) return "image/jpeg";
	return "text/html; charset=utf-8";
	}
	public void sendGetRequest(OutputStream outputStream, String uri) throws IOException{
	handleGetRequest(uri);
	HttpResponseBuilder response = new HttpResponseBuilder();
	response.setStatusCode(statusCode);
	response.setHeaders(Map.of("Content-Type", resolveContentType(uri)));
	response.setBody(fileBytes);
	PrintWriter writer = new PrintWriter(outputStream, true);
	writer.println(response.build());
	}

🤖 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 44 - 54, The
response currently hardcodes "Content-Type: text/html" in sendGetRequest which
mislabels all files; update sendGetRequest to determine the MIME type
dynamically (e.g., use Files.probeContentType(path) or a small extension→MIME
map) after handleGetRequest has populated fileBytes and the file path/filename,
then call response.setHeaders(Map.of("Content-Type", mimeType)); ensure you fall
back to "application/octet-stream" if probeContentType returns null and keep
using HttpResponseBuilder, statusCode, and fileBytes as before.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Exception cause is lost — use the two-arg RuntimeException constructor.

"Error handling client connection " + e stringifies the exception, discarding the original stack trace. Pass e as the cause parameter so callers/loggers can inspect the full chain.

🐛 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.

Suggested change

	private void handleClient(Socket client) {
	try (ConnectionHandler connectionHandler = new ConnectionHandler(client)) {
	connectionHandler.runConnectionHandler();
	} catch (Exception e) {
	throw new RuntimeException("Error handling client connection " + e);
	}
	}
	private void handleClient(Socket client) {
	try (ConnectionHandler connectionHandler = new ConnectionHandler(client)) {
	connectionHandler.runConnectionHandler();
	} 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 29 - 35, The catch in
handleClient currently concatenates the exception into a string and loses the
original stack trace; change the throw to use the two-argument RuntimeException
constructor so the original exception is preserved as the cause (i.e., throw new
RuntimeException("Error handling client connection", e)); update the block
around ConnectionHandler and runConnectionHandler to rethrow with the cause
rather than stringifying the exception.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Non-English comment on Line 121.

// saknar ':' efter port is Swedish. Use English for consistency across the codebase, e.g., // missing ':' after port.

🤖 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 117 -
126, Replace the Swedish inline comment in the test method invalid_yaml_fails
with English for consistency: change "// saknar ':' efter port" to an English
comment like "// missing ':' after port" in the test that writes "server:\n 
port 8080\n" (located near the creation of Path configFile and the
Files.writeString call) so the test and surrounding codebase use a single
language.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix method name formatting and casing.

Line 130: invalid_port_should_Throw_Exception () has excessive whitespace before the parentheses and mixes camelCase (Throw, Exception) into an otherwise snake_case convention. Suggest:

-    void invalid_port_should_Throw_Exception    () throws Exception {
+    void invalid_port_should_throw_exception() throws Exception {

🤖 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 the test method invalid_port_should_Throw_Exception in
ConfigLoaderTest to follow consistent snake_case and remove the extra spaces
before the parentheses; for example, change it to
invalid_port_should_throw_exception() (keep the `@Test` and `@DisplayName`
unchanged) so the method name uses lowercase for "throw" and "exception" and has
no trailing whitespace before "()".

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Test feeds request line through the header parser — works by accident.

TestHttpParserForHeaders passes the full HTTP request (GET /index.html HTTP/1.1\r\n...) directly to parseHttp() without first calling parseRequest(). The request line is silently discarded because it contains no colon. The test passes, but it doesn't reflect real usage and is fragile — if parsing logic changes to be stricter, this test will break for the wrong reason.

Either call parseRequest() first (to consume the request line), or strip the request line from the test input.

Proposed fix (strip request line from input)

-    String testInput = "GET /index.html HTTP/1.1\r\nHost: localhost\r\nContent-Type: text/plain\r\nUser-Agent: JUnit5\r\n\r\n";
+    String testInput = "Host: localhost\r\nContent-Type: text/plain\r\nUser-Agent: JUnit5\r\n\r\n";

Also: the method name TestHttpParserForHeaders uses PascalCase — Java convention for test methods is camelCase (e.g., testHttpParserForHeaders).

📝 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

	@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");
	}
	`@Test`
	void TestHttpParserForHeaders() throws IOException {
	String testInput = "Host: 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");
	}

🤖 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 currently feeds a full HTTP request
(including the request line) into httpParser.parseHttp(), which lets the request
line be ignored by accident; either change the test to call
httpParser.parseRequest() before httpParser.parseHttp() to consume the request
line, or alter testInput to start at the headers only (remove the "GET /...
HTTP/1.1\r\n" line) so parseHttp() receives only header lines; also rename the
test method to camelCase (e.g., testHttpParserForHeaders) to follow Java test
naming conventions.