Conversation
Add ci workflow for github actions.
* build: configure pom.xml with needed plugin/tools. Setup Java 25 environment with JUnit 5, Mockito, JaCoCo, Pitest, and Spotless * fix: add missing test scope for awaitility
* Dockerfile that builds an image in a docker container then runs in another docker container Current implementation uses Temporary App.class reference before relevant file is created to start server. * Fixed unverified commit * Set up non-root user and updated Dockerfile to use user. Fixed file path to use /app/ instead of /app/org/example to prevent unnessary nesting of packages.
* added parser class for headers * refactoring * refactoring * refactoring, added getHeadersMap * refactoring * refactoring code, adding debug boolean * added unit tests for HttpParser.java class * refactoring and all green tests for class * refactoring the code * refactoring the bufferedReader * refactoring with coderabbit feedback for header put to merge * refactoring with coderabbit feedback for header put to merge, changed headersmap * refactoring from reviewer input * refactoring for code rabbit comments
* Add HttpResponseBuilder and its tests Signed-off-by: JohanHiths <johan.hietala@iths.se> * Implement minimal HTTP response builder Signed-off-by: JohanHiths <johan.hietala@iths.se> * update coderabbit * more coderabbit fixes Signed-off-by: JohanHiths <johan.hietala@iths.se> * Bytte från httptest till http efter rekommendationer från Xeutos * code rabbit fix * fixed more code rabbit problems --------- Signed-off-by: JohanHiths <johan.hietala@iths.se>
* added parser class for headers * refactoring * refactoring * refactoring, added getHeadersMap * refactoring * adding class for parsing requestLine (method, path, version). Has getters/setters for the three. * refactoring code, private setters * added tests for throwing error * refactoring the bufferedReader * refactoring, adding check for requestLineArray.length() is less or equals to two with added unit test * refactoring, adding check for requestLineArray.length() is less or equals to two with added unit test * refactoring, adding test for null value of input stream * refactoring from coderabbit input * refactoring test class. * refactoring, adding debug boolean flag for logger lines * refactoring and testing buffered reader stream * refactoring, changed class HttpParser.java to extend HttpParseRequestLine.java and making them share the same buffered reader. Added method to set the reader and then methods to call the HttpParser * refactoring, changed class HttpParser.java to extend HttpParseRequestLine.java and making them share the same buffered reader. Added method to set the reader and then methods to call the HttpParser
* Add Bucket4j dependency in pom file * Extract Bucket4j version into a property and update version in dependency for consistency
* Add support for serving static files - Introduced `StaticFileHandler` to serve static files from the `www` directory. - Enhanced `HttpResponseBuilder` to support byte array bodies. - Made `setReader` method public in `HttpParser` to improve flexibility. - Code tested via Insomnia by coding a temporary method for multithreads in TcpServer.java, this is now removed before future implementations of ConnectionHandler Co-authored-by: Daniel Fahlén xsz300@gmail.com * Fix typo in `HttpResponseBuilder` (contentLength) and refine body handling logic Co-authored-by: Daniel Fahlén xsz300@gmail.com * Add basic HTML structure to `index.html` in `www` directory * Make `handleGetRequest` method private in `StaticFileHandler` to encapsulate functionality
* Updates pom.xml, with jackson-dependencies, for config file * Updates pom.xml, removes jackson-annotations:2.20, because it was apparently unnecessary.
* * Move HTTP handling logic from TcpServer to a dedicated ConnectionTask class * Implement virtual thread execution for better scalability * Add regex-based URI routing to support clean URLs and default to index.html * Ensure sockets are properly closed Co-authored-by: Caroline Nordbrandt <caroline_nordbradt@hotmail.com> * change thrown exception to runtime instead with appropiet message in tcpServer. --------- Co-authored-by: Caroline Nordbrandt <caroline_nordbradt@hotmail.com>
📝 WalkthroughWalkthroughThis PR introduces a complete TCP web server implementation in Java with HTTP request parsing, static file serving, and containerization support. Changes include new server classes (TcpServer, ConnectionHandler, StaticFileHandler), HTTP handling components (HttpParser, HttpResponseBuilder, HttpParseRequestLine), GitHub Actions CI/CD workflows, Docker multi-stage build, Maven dependencies, unit tests, and static assets. Changes
Sequence DiagramsequenceDiagram
participant Client
participant TcpServer
participant ConnectionHandler
participant HttpParser
participant StaticFileHandler
participant HttpResponseBuilder
Client->>TcpServer: TCP Connect (port 8080)
TcpServer->>ConnectionHandler: new ConnectionHandler(socket)
ConnectionHandler->>HttpParser: setReader(inputStream)
ConnectionHandler->>HttpParser: parseRequest()
HttpParser->>HttpParser: parse request line
ConnectionHandler->>HttpParser: parseHttp()
HttpParser->>HttpParser: parse headers
alt URI == "/health"
ConnectionHandler->>HttpResponseBuilder: new HttpResponseBuilder()
HttpResponseBuilder->>HttpResponseBuilder: setStatusCode(200)<br/>setBody("OK")
HttpResponseBuilder->>Client: write HTTP 200 response
else URI for static file
ConnectionHandler->>StaticFileHandler: sendGetRequest(outputStream, uri)
StaticFileHandler->>StaticFileHandler: handleGetRequest(uri)<br/>read file bytes
StaticFileHandler->>HttpResponseBuilder: new HttpResponseBuilder()
HttpResponseBuilder->>HttpResponseBuilder: setBody(fileBytes)<br/>build response
HttpResponseBuilder->>Client: write HTTP response
end
Client->>TcpServer: TCP Close
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (13)
src/test/java/org/example/http/HttpResponseBuilderTest.java (1)
7-44: Missing coverage for thebyte[]body path andbuild()idempotency.The two existing tests only exercise
setBody(String). ThesetBody(byte[])branch inbuild()is untested. Additionally, there is no test confirming that callingbuild()twice on the same instance produces identical output (which would catch the mutation bug noted inHttpResponseBuilder).Consider adding:
- A test calling
setBody(byte[])and verifying the response body andContent-Length.- A test calling
build()twice and asserting both results are equal.🤖 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 7 - 44, Add two unit tests in HttpResponseBuilderTest: one that calls HttpResponseBuilder.setBody(byte[]) with a known byte array (including a multi-byte UTF-8 character if desired) and asserts the built response (via build()) contains the expected Content-Length and body bytes, referencing HttpResponseBuilder and setBody(byte[]); and another test that calls build() twice on the same HttpResponseBuilder instance and asserts the two returned Strings are identical to verify build() is idempotent (reference build() and HttpResponseBuilder). Ensure the byte[] test verifies Content-Length equals the actual byte length and that the response contains the raw body content.src/main/java/org/example/ConnectionHandler.java (2)
10-11: Fields should beprivate.
clientanduriare package-private by default.uriis only used internally viaresolveTargetFile;clientshould not be accessible outsideConnectionHandler.✏️ Proposed fix
- Socket client; - String uri; + private final Socket client; + private String uri;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/ConnectionHandler.java` around lines 10 - 11, Make the fields in ConnectionHandler private: change the package-private Socket client and String uri to private fields; update any internal uses (e.g., resolveTargetFile method) to access these private fields directly within the class and ensure no external code relies on package access to client or uri (refactor callers if needed).
17-42: No unit test covers the/healthendpoint — the primary feature of this PR.
ConnectionHandler.runConnectionHandler()is the integration point for/health, but no test exercises it. Consider adding a test that feeds a"GET /health HTTP/1.1\r\n\r\n"stream via a mock or real socket pair and asserts the JSON response body and200 OKstatus.Would you like me to draft a
ConnectionHandlerTestcovering the/healthendpoint and the static file fallback path?🤖 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 17 - 42, Add unit tests exercising ConnectionHandler.runConnectionHandler to cover the new /health branch: create a test (e.g., ConnectionHandlerTest) that constructs a ConnectionHandler with a mock or socket pair input stream containing "GET /health HTTP/1.1\r\n\r\n", invokes runConnectionHandler, and asserts the output stream contains a "HTTP/1.1 200 OK" status line and the JSON body {"status":"ok"}; also add a second test that sends a GET for a non-/health path and verifies resolveTargetFile is used and StaticFileHandler.sendGetRequest is invoked with the expected uri (use mocks or stubs for StaticFileHandler and override resolveTargetFile if needed) so both the /health shortcut and the static-file fallback are covered.src/test/java/org/example/httpparser/HttpParserTest.java (2)
32-32: Specify charset explicitly on empty-input test.
"".getBytes()is inconsistent with the explicitStandardCharsets.UTF_8used on lines 18 and 43.✏️ Proposed fix
- InputStream in = new ByteArrayInputStream("".getBytes()); + InputStream in = new ByteArrayInputStream("".getBytes(StandardCharsets.UTF_8));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/httpparser/HttpParserTest.java` at line 32, The empty-input test uses new ByteArrayInputStream("".getBytes()) without a charset while other tests use StandardCharsets.UTF_8; update the creation in HttpParserTest so the InputStream is built with an explicit charset (use StandardCharsets.UTF_8) when calling getBytes on the empty string (modify the line creating the ByteArrayInputStream/InputStream variable) to ensure consistent encoding across tests.
16-16: Test method name violates camelCase convention.
TestHttpParserForHeadersuses PascalCase while all other test methods in this class usetest...camelCase. Rename totestHttpParserForHeaders.✏️ Proposed fix
- void TestHttpParserForHeaders() throws IOException { + void testHttpParserForHeaders() throws IOException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/httpparser/HttpParserTest.java` at line 16, Rename the test method TestHttpParserForHeaders to follow camelCase by changing its name to testHttpParserForHeaders in the HttpParserTest class; update the method declaration and any internal or external references (calls, listeners, or annotations) that refer to TestHttpParserForHeaders so they point to testHttpParserForHeaders to keep naming consistent with other tests.src/test/java/org/example/httpparser/HttpParseRequestLineTest.java (1)
13-13: Misleading variable name — should reflect its actual type.
httpParseRequestLineis typed asHttpParser; the name implies it's aHttpParseRequestLine. Rename tohttpParserorparserfor clarity.✏️ Proposed fix
- private HttpParser httpParseRequestLine; + private HttpParser httpParser; `@BeforeEach` void setUp() { - httpParseRequestLine = new HttpParser(); + httpParser = new HttpParser(); }(and update all references in the test methods)
🤖 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` at line 13, Rename the misleading test field httpParseRequestLine (declared as HttpParser) to a clearer name such as httpParser (or parser) in the HttpParseRequestLineTest class and update every reference to that field inside the test methods; ensure the declaration "private HttpParser httpParseRequestLine;" is changed to "private HttpParser httpParser;" and all usages in setup and test methods are updated accordingly to avoid name/type confusion.src/main/java/org/example/httpparser/HttpParseRequestLine.java (2)
15-15: Redundant local variable; use the parameter directly.✏️ Proposed fix
- BufferedReader reader = br; - String requestLine = reader.readLine(); + String requestLine = br.readLine();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java` at line 15, In HttpParseRequestLine replace the redundant local variable declaration "BufferedReader reader = br" by using the method/constructor parameter "br" directly: remove the "reader" field/variable and update all references to use "br" (e.g., inside parse logic or methods in HttpParseRequestLine) so there is no unnecessary aliasing of the BufferedReader parameter.
11-12:debugflag is dead code — no external way to enable it.Neither
HttpParseRequestLinenor its subclassHttpParserexposes a setter fordebug; the logger block is permanently unreachable. Either add asetDebug(boolean)or remove the flag and rely on the existingjava.util.logging.Logger.🤖 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 11 - 12, The private boolean field debug in HttpParseRequestLine is unused and cannot be toggled externally; either remove the dead field and rely solely on the existing java.util.logging.Logger (logger) or add a public setter to allow enabling it from outside (for example add setDebug(boolean) in HttpParseRequestLine and ensure HttpParser subclasses or callers call it as needed); update any conditional branches that check the debug field to use logger.isLoggable(Level.FINE) or the new setter-controlled flag consistently so behavior is reachable.src/main/java/org/example/httpparser/HttpParser.java (3)
16-20:setReadersilently ignores subsequent calls — surprising contract.Once a
BufferedReaderis set, callingsetReaderagain (e.g., when reusing the object in tests) is silently a no-op. Either document this restriction with a comment or throwIllegalStateExceptionto fail fast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 16 - 20, The setReader method in HttpParser currently ignores subsequent calls which is surprising; change its behavior to fail fast by checking the reader field and throwing an IllegalStateException if it's already set (e.g., in HttpParser.setReader(InputStream) throw new IllegalStateException("reader already set") when this.reader != null) so callers/tests that attempt to reset the reader get an explicit error; update any tests that relied on resetting to either create a new HttpParser or adjust to the new contract.
40-45: Debug block usesSystem.out.printlninstead of the class logger.
HttpParseRequestLinealready has aLoggerinstance. UsingSystem.out.printlnfor debug output in the subclass is inconsistent and not suppressible via standard logging configuration. Use the inherited logger (or declare one here) withLevel.FINE.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 40 - 45, Replace the System.out.println debug block in HttpParser with the class logger at Level.FINE: use the existing Logger from HttpParseRequestLine (or declare a private static final Logger if needed) and call logger.isLoggable(Level.FINE) before logging; then log the Host and each header via logger.log(Level.FINE, ...) (or logger.fine(...) consistently) using headersMap and preserving the current debug conditional or remove the debug flag in favor of logger.isLoggable(Level.FINE).
53-55:getHeadersMap()exposes the mutable internal map.Callers can modify
headersMapdirectly through the returned reference. Return an unmodifiable view.✏️ Proposed fix
+import java.util.Collections; ... public Map<String, String> getHeadersMap() { - return headersMap; + return Collections.unmodifiableMap(headersMap); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 53 - 55, getHeadersMap() currently returns the internal mutable headersMap allowing callers to modify class state; change the method to return an unmodifiable view (e.g., wrap headersMap with Collections.unmodifiableMap(...) or use Map.copyOf(...)) so callers cannot mutate the internal headersMap while preserving read access; update the getHeadersMap method implementation and import Collections if needed.src/main/java/org/example/TcpServer.java (2)
18-18: PlainServerSockettransmits data in cleartext.The static analysis flag is valid for production deployments: a plain
ServerSocketexposes all traffic to network interception. For a learning project this may be acceptable, but consider documenting the limitation and, before production use, replace withSSLServerSocketFactory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/TcpServer.java` at line 18, The TcpServer currently constructs a plain ServerSocket (try (ServerSocket serverSocket = new ServerSocket(port))) which sends cleartext; replace or make optional use of an SSLServerSocket from SSLServerSocketFactory (or inject a ServerSocketFactory) to enable TLS for production—update TcpServer to create an SSLServerSocket via SSLServerSocketFactory.getDefault() (or accept an SSLContext/ServerSocketFactory parameter), load server certs/truststore where appropriate, and document the current cleartext behavior as a limitation until TLS is enabled.
16-21: ReplaceSystem.out.printlnwith a proper logger; be mindful of PII.The class uses
System.out.printlnfor server startup and per-connection logging. Line 21 logs the remote socket address (IP + port) which is PII. Switch tojava.util.logging.Logger(already used inHttpParseRequestLine) and guard or omit the IP-logging line.🤖 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 16 - 21, Replace the System.out.println calls in TcpServer with a java.util.logging.Logger instance (same style as used in HttpParseRequestLine); create a private static final Logger LOGGER = Logger.getLogger(TcpServer.class.getName()) and use LOGGER.info for the startup message instead of System.out.println, and for the per-connection log avoid logging PII by removing or guarding the remote address: either log only a non-PII event like "Client connected" with an anonymized identifier or conditionally log clientSocket.getRemoteSocketAddress() behind a configuration flag (e.g., logRemoteAddresses) so the IP:port is never logged by default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 31-32: The CI step named "Test with Maven" currently runs "mvn -B
test --file pom.xml", which stops before the verify phase so plugins bound to
verify (e.g., spotless:check and maven-failsafe-plugin) never run; update the
command used in that step to run the verify phase (e.g., use "mvn -B verify
--file pom.xml") or at minimum append the spotless check ("mvn -B test
spotless:check --file pom.xml") so that spotless:check and integration tests
executed by maven-failsafe-plugin are enforced in CI.
In `@Dockerfile`:
- Around line 10-11: Move the USER instruction so it appears before the
ENTRYPOINT for clarity and convention: place USER appuser above ENTRYPOINT
["java", "-classpath", "/app", "org.example.App"] (ensure any file permissions
or ownership for /app and runtime artifacts are already set for appuser before
switching users). Update the Dockerfile so USER precedes ENTRYPOINT and verify
org.example.App runs correctly as appuser.
- Around line 3-5: The Dockerfile fails to include static assets and
dependencies: ensure the build stage packages the app with dependencies and
copies the www static directory into the final image; replace mvn compile with a
full package (e.g., mvn -DskipTests package) so target/*.jar contains all
compiled classes and dependency jars (or use the maven-assembly/shade plugin),
COPY the produced jar into the runtime image, and add COPY www/ www/ (or COPY
--from=build /build/target/www/ www/) so StaticFileHandler can find "www"; also
adjust the runtime classpath to run the packaged jar (java -jar
/app/app-with-deps.jar) instead of relying on /app classpath.
In `@pom.xml`:
- Around line 175-184: The Spotless execution is bound to the verify phase
(spotless:check goal inside the <execution> with <phase>verify</phase>) but CI
invokes mvn -B test so formatting isn't enforced; either change the <phase> in
the pom.xml execution from verify to test so spotless:check runs during mvn
test, or leave the pom as-is and update the CI invocation to run mvn -B verify
instead; locate the <execution> block containing the check goal and modify the
<phase> value or update the CI command accordingly.
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 24-38: Remove the developer scaffolding and instructional Swedish
comments around the health-check handling in ConnectionHandler; keep the
health-check logic intact (the block that sets requestedUri = parser.getUri(),
checks requestedUri.equals("/health"), builds responseBody/header, writes to
client.getOutputStream(), flushes, and returns) but delete the comment lines
starting with "// --- DIN ÄNDRING..." and the inline "// Avslutar här..." so
only production-relevant comments (if any) remain.
- Around line 44-51: ConnectionHandler.resolveTargetFile currently appends
“.html” but does not strip or validate ../ sequences, and
StaticFileHandler.sendGetRequest constructs new File(WEB_ROOT, uri) without
normalization, allowing path traversal; fix by normalizing and validating the
resolved path inside StaticFileHandler.sendGetRequest (use
Paths.get(WEB_ROOT).resolve(uri).normalize()), verify the normalized path
startsWith(Paths.get(WEB_ROOT)) and reject with an IOException or access-denied
response if it does not, and also ensure ConnectionHandler.resolveTargetFile
normalizes or rejects suspicious URIs (e.g., remove leading slash and disallow
any .. segments) before returning the final uri so both resolveTargetFile and
sendGetRequest cannot be tricked by traversal sequences.
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Line 49: The code in HttpResponseBuilder uses
REASON_PHRASES.getOrDefault(statusCode, "OK") which can produce incorrect reason
phrases (e.g., "503 OK"); change the default fallback to a neutral value like
"Unknown" and update the REASON_PHRASES map to include missing common codes
(301, 302, 403, 429, 503) or replace the map lookup with a more complete lookup
routine; locate the REASON_PHRASES map and the line that sets the local variable
reason in HttpResponseBuilder and adjust the getOrDefault fallback and/or enrich
the map entries accordingly.
- Around line 38-60: The build() method currently mutates object state by
calling setBody(...) when bytebody is present; instead, compute local variables
(e.g., byte[] effectiveBytes and String effectiveBody) without altering
this.body or this.bytebody: if bytebody != null use effectiveBytes = bytebody
and effectiveBody = new String(bytebody, StandardCharsets.UTF_8), otherwise use
effectiveBody = this.body and effectiveBytes =
effectiveBody.getBytes(StandardCharsets.UTF_8); compute contentLength from
effectiveBytes.length, write headers and effectiveBody into the StringBuilder,
and never call setBody or modify fields so build() is idempotent (refer to
build(), setBody, body, bytebody, and contentLength in the diff).
In `@src/main/java/org/example/StaticFileHandler.java`:
- Line 28: The response currently hardcodes Content-Type to "text/html;
charset=utf-8" in StaticFileHandler (where response.setHeaders(Map.of(...)) is
called); change this to compute the MIME type from the requested URI/file before
calling response.setHeaders: use java.nio.file.Files.probeContentType(path) (or
a small extension->mime lookup map) to determine the contentType, append a
charset (e.g., "; charset=utf-8") only for text/* types, and fall back to
"application/octet-stream" (or "text/plain; charset=utf-8") if probeContentType
returns null; then set response.setHeaders(Map.of("Content-Type",
computedContentType)).
- Around line 18-22: The handleGetRequest method currently constructs new
File(WEB_ROOT, uri) directly, allowing path traversal; fix by URL-decoding the
incoming uri, resolving the candidate File's canonical path and comparing it
against the canonical path of WEB_ROOT to ensure the file is inside the web root
(reject request if it doesn't startWith the WEB_ROOT canonical path), and only
then read bytes into fileBytes after verifying file.isFile() and file.canRead();
update handleGetRequest to return an error/404/403 instead of reading files
outside the web root.
- Around line 30-31: The code uses PrintWriter (new PrintWriter(outputStream,
true)) and println(response.build()), which uses platform default encoding and
appends a newline; replace this by encoding the full response String to UTF-8
bytes (e.g., response.build().getBytes(StandardCharsets.UTF_8)), write those
bytes directly to outputStream and flush so no extra newline is added, and
ensure the Content-Length calculation (wherever set) uses the UTF-8 byte length
to match what you write; update references in StaticFileHandler where
PrintWriter and println are used.
- Line 14: The instance field fileBytes in StaticFileHandler causes a race when
handleGetRequest/sendGetRequest are called concurrently; remove the fileBytes
field and instead allocate a local byte[] inside sendGetRequest (and return or
use it directly within that call path), update handleGetRequest/sendGetRequest
signatures/usage as needed so each request works with its own local byte[]
(referencing fileBytes, StaticFileHandler, handleGetRequest, and sendGetRequest
to locate the change), and ensure no other methods rely on the removed instance
field.
In `@src/main/java/org/example/TcpServer.java`:
- Around line 22-34: The virtual-thread lambda started with
Thread.ofVirtual().start(() -> handleClient(clientSocket)) currently lets
exceptions go to the default uncaught-exception handler; change thread creation
to set an explicit uncaught-exception handler (via
Thread.ofVirtual().unstarted()/builder or
Thread.currentThread().setUncaughtExceptionHandler on the created thread) that
logs the error and any context so failures are observable, and update the catch
in handleClient(Socket) to throw new RuntimeException("Error handling client
connection", e) (pass the caught exception as the cause rather than
concatenating it) so the original stack trace from
ConnectionHandler.runConnectionHandler() is preserved.
In `@www/index.html`:
- Around line 3-6: The closing head tag has extra indentation compared to the
opening <head> tag; locate the <head> and </head> tags in the HTML and remove
the four leading spaces before </head> so the closing tag is aligned with the
opening <head> tag (ensure both tags start at the same column).
---
Nitpick comments:
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 10-11: Make the fields in ConnectionHandler private: change the
package-private Socket client and String uri to private fields; update any
internal uses (e.g., resolveTargetFile method) to access these private fields
directly within the class and ensure no external code relies on package access
to client or uri (refactor callers if needed).
- Around line 17-42: Add unit tests exercising
ConnectionHandler.runConnectionHandler to cover the new /health branch: create a
test (e.g., ConnectionHandlerTest) that constructs a ConnectionHandler with a
mock or socket pair input stream containing "GET /health HTTP/1.1\r\n\r\n",
invokes runConnectionHandler, and asserts the output stream contains a "HTTP/1.1
200 OK" status line and the JSON body {"status":"ok"}; also add a second test
that sends a GET for a non-/health path and verifies resolveTargetFile is used
and StaticFileHandler.sendGetRequest is invoked with the expected uri (use mocks
or stubs for StaticFileHandler and override resolveTargetFile if needed) so both
the /health shortcut and the static-file fallback are covered.
In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Around line 16-20: The setReader method in HttpParser currently ignores
subsequent calls which is surprising; change its behavior to fail fast by
checking the reader field and throwing an IllegalStateException if it's already
set (e.g., in HttpParser.setReader(InputStream) throw new
IllegalStateException("reader already set") when this.reader != null) so
callers/tests that attempt to reset the reader get an explicit error; update any
tests that relied on resetting to either create a new HttpParser or adjust to
the new contract.
- Around line 40-45: Replace the System.out.println debug block in HttpParser
with the class logger at Level.FINE: use the existing Logger from
HttpParseRequestLine (or declare a private static final Logger if needed) and
call logger.isLoggable(Level.FINE) before logging; then log the Host and each
header via logger.log(Level.FINE, ...) (or logger.fine(...) consistently) using
headersMap and preserving the current debug conditional or remove the debug flag
in favor of logger.isLoggable(Level.FINE).
- Around line 53-55: getHeadersMap() currently returns the internal mutable
headersMap allowing callers to modify class state; change the method to return
an unmodifiable view (e.g., wrap headersMap with
Collections.unmodifiableMap(...) or use Map.copyOf(...)) so callers cannot
mutate the internal headersMap while preserving read access; update the
getHeadersMap method implementation and import Collections if needed.
In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java`:
- Line 15: In HttpParseRequestLine replace the redundant local variable
declaration "BufferedReader reader = br" by using the method/constructor
parameter "br" directly: remove the "reader" field/variable and update all
references to use "br" (e.g., inside parse logic or methods in
HttpParseRequestLine) so there is no unnecessary aliasing of the BufferedReader
parameter.
- Around line 11-12: The private boolean field debug in HttpParseRequestLine is
unused and cannot be toggled externally; either remove the dead field and rely
solely on the existing java.util.logging.Logger (logger) or add a public setter
to allow enabling it from outside (for example add setDebug(boolean) in
HttpParseRequestLine and ensure HttpParser subclasses or callers call it as
needed); update any conditional branches that check the debug field to use
logger.isLoggable(Level.FINE) or the new setter-controlled flag consistently so
behavior is reachable.
In `@src/main/java/org/example/TcpServer.java`:
- Line 18: The TcpServer currently constructs a plain ServerSocket (try
(ServerSocket serverSocket = new ServerSocket(port))) which sends cleartext;
replace or make optional use of an SSLServerSocket from SSLServerSocketFactory
(or inject a ServerSocketFactory) to enable TLS for production—update TcpServer
to create an SSLServerSocket via SSLServerSocketFactory.getDefault() (or accept
an SSLContext/ServerSocketFactory parameter), load server certs/truststore where
appropriate, and document the current cleartext behavior as a limitation until
TLS is enabled.
- Around line 16-21: Replace the System.out.println calls in TcpServer with a
java.util.logging.Logger instance (same style as used in HttpParseRequestLine);
create a private static final Logger LOGGER =
Logger.getLogger(TcpServer.class.getName()) and use LOGGER.info for the startup
message instead of System.out.println, and for the per-connection log avoid
logging PII by removing or guarding the remote address: either log only a
non-PII event like "Client connected" with an anonymized identifier or
conditionally log clientSocket.getRemoteSocketAddress() behind a configuration
flag (e.g., logRemoteAddresses) so the IP:port is never logged by default.
In `@src/test/java/org/example/http/HttpResponseBuilderTest.java`:
- Around line 7-44: Add two unit tests in HttpResponseBuilderTest: one that
calls HttpResponseBuilder.setBody(byte[]) with a known byte array (including a
multi-byte UTF-8 character if desired) and asserts the built response (via
build()) contains the expected Content-Length and body bytes, referencing
HttpResponseBuilder and setBody(byte[]); and another test that calls build()
twice on the same HttpResponseBuilder instance and asserts the two returned
Strings are identical to verify build() is idempotent (reference build() and
HttpResponseBuilder). Ensure the byte[] test verifies Content-Length equals the
actual byte length and that the response contains the raw body content.
In `@src/test/java/org/example/httpparser/HttpParseRequestLineTest.java`:
- Line 13: Rename the misleading test field httpParseRequestLine (declared as
HttpParser) to a clearer name such as httpParser (or parser) in the
HttpParseRequestLineTest class and update every reference to that field inside
the test methods; ensure the declaration "private HttpParser
httpParseRequestLine;" is changed to "private HttpParser httpParser;" and all
usages in setup and test methods are updated accordingly to avoid name/type
confusion.
In `@src/test/java/org/example/httpparser/HttpParserTest.java`:
- Line 32: The empty-input test uses new ByteArrayInputStream("".getBytes())
without a charset while other tests use StandardCharsets.UTF_8; update the
creation in HttpParserTest so the InputStream is built with an explicit charset
(use StandardCharsets.UTF_8) when calling getBytes on the empty string (modify
the line creating the ByteArrayInputStream/InputStream variable) to ensure
consistent encoding across tests.
- Line 16: Rename the test method TestHttpParserForHeaders to follow camelCase
by changing its name to testHttpParserForHeaders in the HttpParserTest class;
update the method declaration and any internal or external references (calls,
listeners, or annotations) that refer to TestHttpParserForHeaders so they point
to testHttpParserForHeaders to keep naming consistent with other tests.
| - name: Test with Maven | ||
| run: mvn -B test --file pom.xml No newline at end of file |
There was a problem hiding this comment.
mvn -B test stops before the verify phase — Spotless and Failsafe never execute in CI.
The spotless:check goal is bound to verify in pom.xml (line 177) and integration tests use maven-failsafe-plugin which also runs in verify. Neither will fire with mvn test. Consider updating to mvn -B verify (or at a minimum mvn -B test spotless:check) to enforce formatting and run integration tests in CI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 31 - 32, The CI step named "Test with
Maven" currently runs "mvn -B test --file pom.xml", which stops before the
verify phase so plugins bound to verify (e.g., spotless:check and
maven-failsafe-plugin) never run; update the command used in that step to run
the verify phase (e.g., use "mvn -B verify --file pom.xml") or at minimum append
the spotless check ("mvn -B test spotless:check --file pom.xml") so that
spotless:check and integration tests executed by maven-failsafe-plugin are
enforced in CI.
| COPY src/ src/ | ||
| COPY pom.xml pom.xml | ||
| RUN mvn compile |
There was a problem hiding this comment.
www/ static assets are never copied into the image — StaticFileHandler will always fail.
The build stage only copies src/ and pom.xml, and the runtime stage only copies compiled classes. StaticFileHandler resolves files under the relative path "www" at runtime, but that directory is not present in the image, so every static-file request will throw an IOException.
mvn compile also only compiles sources; dependency JARs remain in ~/.m2 and are not staged under /build/target/. At runtime -classpath /app contains only your compiled classes — any reference to bucket4j or Jackson classes will cause a NoClassDefFoundError at startup.
🔧 Proposed fix
FROM maven:3-eclipse-temurin-25-alpine AS build
WORKDIR /build
COPY src/ src/
+COPY www/ www/
COPY pom.xml pom.xml
-RUN mvn compile
+RUN mvn package -DskipTests
+RUN mvn dependency:copy-dependencies -DoutputDirectory=target/dependency
FROM eclipse-temurin:25-jre-alpine
+WORKDIR /app
RUN addgroup -S appgroup && adduser -S appuser -G appgroup
COPY --from=build /build/target/classes/ /app/
+COPY --from=build /build/target/dependency/ /app/libs/
+COPY --from=build /build/www/ /app/www/
-ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
+ENTRYPOINT ["java", "-classpath", "/app:/app/libs/*", "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 3 - 5, The Dockerfile fails to include static assets
and dependencies: ensure the build stage packages the app with dependencies and
copies the www static directory into the final image; replace mvn compile with a
full package (e.g., mvn -DskipTests package) so target/*.jar contains all
compiled classes and dependency jars (or use the maven-assembly/shade plugin),
COPY the produced jar into the runtime image, and add COPY www/ www/ (or COPY
--from=build /build/target/www/ www/) so StaticFileHandler can find "www"; also
adjust the runtime classpath to run the packaged jar (java -jar
/app/app-with-deps.jar) instead of relying on /app classpath.
| ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"] | ||
| USER appuser |
There was a problem hiding this comment.
USER placed after ENTRYPOINT — non-idiomatic order.
While Docker applies the final USER instruction as the container's default user regardless of where it appears relative to ENTRYPOINT/CMD, convention is to place USER before ENTRYPOINT for clarity.
🔧 Proposed fix
COPY --from=build /build/target/classes/ /app/
+USER appuser
ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
-USER appuser📝 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.
| ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"] | |
| USER appuser | |
| USER appuser | |
| ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 10 - 11, Move the USER instruction so it appears
before the ENTRYPOINT for clarity and convention: place USER appuser above
ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"] (ensure any file
permissions or ownership for /app and runtime artifacts are already set for
appuser before switching users). Update the Dockerfile so USER precedes
ENTRYPOINT and verify org.example.App runs correctly as appuser.
| <executions> | ||
| <execution> | ||
| <phase>verify</phase> | ||
| <goals> | ||
| <goal> | ||
| check | ||
| </goal> | ||
| </goals> | ||
| </execution> | ||
| </executions> |
There was a problem hiding this comment.
spotless:check is bound to verify but CI only runs mvn test — style is never enforced.
The Spotless execution is bound to the verify phase (Line 177), but .github/workflows/ci.yml runs mvn -B test which stops before verify. This means Spotless will silently pass on every CI run regardless of formatting violations.
Either bind Spotless to the test phase or update the CI command to mvn -B verify.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pom.xml` around lines 175 - 184, The Spotless execution is bound to the
verify phase (spotless:check goal inside the <execution> with
<phase>verify</phase>) but CI invokes mvn -B test so formatting isn't enforced;
either change the <phase> in the pom.xml execution from verify to test so
spotless:check runs during mvn test, or leave the pom as-is and update the CI
invocation to run mvn -B verify instead; locate the <execution> block containing
the check goal and modify the <phase> value or update the CI command
accordingly.
| // --- DIN ÄNDRING FÖR ISSUE #75 BÖRJAR HÄR --- | ||
| String requestedUri = parser.getUri(); | ||
| if (requestedUri.equals("/health")) { | ||
| String responseBody = "{\"status\": \"ok\"}"; | ||
| String header = "HTTP/1.1 200 OK\r\n" + | ||
| "Content-Type: application/json\r\n" + | ||
| "Content-Length: " + responseBody.length() + "\r\n" + | ||
| "\r\n"; | ||
|
|
||
| client.getOutputStream().write(header.getBytes()); | ||
| client.getOutputStream().write(responseBody.getBytes()); | ||
| client.getOutputStream().flush(); | ||
| return; // Avslutar här så vi inte letar efter filer i onödan | ||
| } | ||
| // --- DIN ÄNDRING SLUTAR HÄR --- |
There was a problem hiding this comment.
Remove developer scaffolding comments before merge.
The Swedish // --- DIN ÄNDRING ... --- and // Avslutar här ... comments are instructional notes, not production documentation, and should be removed.
🤖 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 24 - 38,
Remove the developer scaffolding and instructional Swedish comments around the
health-check handling in ConnectionHandler; keep the health-check logic intact
(the block that sets requestedUri = parser.getUri(), checks
requestedUri.equals("/health"), builds responseBody/header, writes to
client.getOutputStream(), flushes, and returns) but delete the comment lines
starting with "// --- DIN ÄNDRING..." and the inline "// Avslutar här..." so
only production-relevant comments (if any) remain.
| private void handleGetRequest(String uri) throws IOException { | ||
|
|
||
| File file = new File(WEB_ROOT, uri); | ||
| fileBytes = Files.readAllBytes(file.toPath()); | ||
|
|
There was a problem hiding this comment.
Path traversal vulnerability — user-controlled URI is not sanitised before file access.
new File(WEB_ROOT, uri) concatenates the user-supplied URI directly onto the web root. A URI like ../../etc/passwd or %2F..%2F..%2Fetc%2Fpasswd resolves to a path outside www/, giving the caller arbitrary read access to the filesystem.
Always compare the canonical path of the resolved file against the canonical path of WEB_ROOT:
🔒 Proposed fix
private void handleGetRequest(String uri) throws IOException {
- File file = new File(WEB_ROOT, uri);
+ File root = new File(WEB_ROOT).getCanonicalFile();
+ File file = new File(root, uri).getCanonicalFile();
+ if (!file.getPath().startsWith(root.getPath() + File.separator)
+ && !file.getPath().equals(root.getPath())) {
+ throw new IOException("Forbidden path: " + uri);
+ }
fileBytes = Files.readAllBytes(file.toPath());
}📝 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.
| private void handleGetRequest(String uri) throws IOException { | |
| File file = new File(WEB_ROOT, uri); | |
| fileBytes = Files.readAllBytes(file.toPath()); | |
| private void handleGetRequest(String uri) throws IOException { | |
| File root = new File(WEB_ROOT).getCanonicalFile(); | |
| File file = new File(root, uri).getCanonicalFile(); | |
| if (!file.getPath().startsWith(root.getPath() + File.separator) | |
| && !file.getPath().equals(root.getPath())) { | |
| throw new IOException("Forbidden path: " + uri); | |
| } | |
| fileBytes = Files.readAllBytes(file.toPath()); |
🤖 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 18 - 22, The
handleGetRequest method currently constructs new File(WEB_ROOT, uri) directly,
allowing path traversal; fix by URL-decoding the incoming uri, resolving the
candidate File's canonical path and comparing it against the canonical path of
WEB_ROOT to ensure the file is inside the web root (reject request if it doesn't
startWith the WEB_ROOT canonical path), and only then read bytes into fileBytes
after verifying file.isFile() and file.canRead(); update handleGetRequest to
return an error/404/403 instead of reading files outside the web root.
| public void sendGetRequest(OutputStream outputStream, String uri) throws IOException{ | ||
| handleGetRequest(uri); | ||
| HttpResponseBuilder response = new HttpResponseBuilder(); | ||
| response.setHeaders(Map.of("Content-Type", "text/html; charset=utf-8")); |
There was a problem hiding this comment.
Content-Type is hardcoded to text/html regardless of file extension.
Every file — CSS, JavaScript, JSON, images — will be served as text/html; charset=utf-8. Browsers will misinterpret non-HTML resources, breaking stylesheets, scripts, and images. Derive the MIME type from the URI extension (e.g., via Files.probeContentType() or a manual lookup map) before setting the Content-Type header.
🤖 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 28, The response
currently hardcodes Content-Type to "text/html; charset=utf-8" in
StaticFileHandler (where response.setHeaders(Map.of(...)) is called); change
this to compute the MIME type from the requested URI/file before calling
response.setHeaders: use java.nio.file.Files.probeContentType(path) (or a small
extension->mime lookup map) to determine the contentType, append a charset
(e.g., "; charset=utf-8") only for text/* types, and fall back to
"application/octet-stream" (or "text/plain; charset=utf-8") if probeContentType
returns null; then set response.setHeaders(Map.of("Content-Type",
computedContentType)).
| PrintWriter writer = new PrintWriter(outputStream, true); | ||
| writer.println(response.build()); |
There was a problem hiding this comment.
PrintWriter uses the platform default encoding and println() appends a spurious newline after the response.
Two distinct bugs here:
new PrintWriter(outputStream, true)encodes characters using the JVM's platform default charset (not guaranteed to be UTF-8). If the body or headers contain non-ASCII characters, the bytes written to the socket will not match theContent-Lengthcomputed in UTF-8.writer.println(response.build())appends a platform line-separator (\non Linux) after the complete HTTP response, including after the already-written body. This adds stray bytes beyond the declaredContent-Length, which can confuse strict HTTP clients.
Write the response directly as UTF-8 bytes instead:
🐛 Proposed fix
- PrintWriter writer = new PrintWriter(outputStream, true);
- writer.println(response.build());
+ outputStream.write(response.build().getBytes(java.nio.charset.StandardCharsets.UTF_8));
+ outputStream.flush();📝 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.
| PrintWriter writer = new PrintWriter(outputStream, true); | |
| writer.println(response.build()); | |
| outputStream.write(response.build().getBytes(java.nio.charset.StandardCharsets.UTF_8)); | |
| outputStream.flush(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/StaticFileHandler.java` around lines 30 - 31, The
code uses PrintWriter (new PrintWriter(outputStream, true)) and
println(response.build()), which uses platform default encoding and appends a
newline; replace this by encoding the full response String to UTF-8 bytes (e.g.,
response.build().getBytes(StandardCharsets.UTF_8)), write those bytes directly
to outputStream and flush so no extra newline is added, and ensure the
Content-Length calculation (wherever set) uses the UTF-8 byte length to match
what you write; update references in StaticFileHandler where PrintWriter and
println are used.
| Thread.ofVirtual().start(() -> handleClient(clientSocket)); | ||
| } | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Failed to start TCP server", 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); | ||
| } |
There was a problem hiding this comment.
Two related exception-handling problems.
-
Swallowed virtual-thread exceptions (Line 22): A
RuntimeExceptionthrown inside the lambda propagates to the virtual thread's uncaught-exception handler, which by default merely prints to stderr. The server continues accepting connections with no observable failure signal. At minimum, set an explicit uncaught-exception handler on the virtual thread. -
Lost cause (Line 33):
"Error handling client connection " + econverts the exception to itstoString()representation and discards the original stack trace. Pass it as thecauseargument instead.
🔧 Proposed fix
- Thread.ofVirtual().start(() -> handleClient(clientSocket));
+ Thread.ofVirtual()
+ .uncaughtExceptionHandler((t, ex) ->
+ logger.log(Level.SEVERE, "Unhandled error in client thread", ex))
+ .start(() -> handleClient(clientSocket));- throw new RuntimeException("Error handling client connection " + 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 22 - 34, The
virtual-thread lambda started with Thread.ofVirtual().start(() ->
handleClient(clientSocket)) currently lets exceptions go to the default
uncaught-exception handler; change thread creation to set an explicit
uncaught-exception handler (via Thread.ofVirtual().unstarted()/builder or
Thread.currentThread().setUncaughtExceptionHandler on the created thread) that
logs the error and any context so failures are observable, and update the catch
in handleClient(Socket) to throw new RuntimeException("Error handling client
connection", e) (pass the caught exception as the cause rather than
concatenating it) so the original stack trace from
ConnectionHandler.runConnectionHandler() is preserved.
| <head> | ||
| <meta charset="UTF-8"> | ||
| <title>Welcome</title> | ||
| </head> |
There was a problem hiding this comment.
Mismatched <head> indentation.
</head> (Line 6) has four extra spaces of indentation compared to the opening <head> (Line 3).
🔧 Proposed fix
<head>
<meta charset="UTF-8">
<title>Welcome</title>
- </head>
+</head>📝 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.
| <head> | |
| <meta charset="UTF-8"> | |
| <title>Welcome</title> | |
| </head> | |
| <head> | |
| <meta charset="UTF-8"> | |
| <title>Welcome</title> | |
| </head> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@www/index.html` around lines 3 - 6, The closing head tag has extra
indentation compared to the opening <head> tag; locate the <head> and </head>
tags in the HTML and remove the four leading spaces before </head> so the
closing tag is aligned with the opening <head> tag (ensure both tags start at
the same column).
Closes #75
Summary by CodeRabbit
Release Notes
New Features
DevOps & Infrastructure
Tests
Dependencies