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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CI and release workflows, a multi-stage Dockerfile, upgrades Java to 25, introduces a TcpServer and HTTP parsing/response utilities (HttpParser, HttpParseRequestLine, HttpResponseBuilder, MimeTypeDetector), and adds unit tests plus Maven plugins/dependencies for testing, mutation testing, and formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as TcpServer
participant Parser as HttpParser
participant Builder as HttpResponseBuilder
participant Detector as MimeTypeDetector
Client->>Server: TCP connect (port 8080)
Server->>Server: accept()
Server->>Parser: provide InputStream
Parser->>Parser: parse request-line (method, uri, version)
Parser->>Parser: parse headers -> headersMap
Parser-->>Server: parsed request data
Server->>Builder: build response for request
Builder->>Detector: detect MIME type from URI/filename
Detector-->>Builder: return Content-Type
Builder-->>Server: formatted HTTP/1.1 response
Server->>Client: write response and close connection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@Dockerfile`:
- Around line 9-10: The Dockerfile currently copies only /build/target/classes/
and sets ENTRYPOINT to run org.example.App, which omits runtime dependencies and
leads to ClassNotFoundException; update the build stage to produce and include
runtime dependencies (e.g., run maven-dependency-plugin:copy-dependencies or
build a fat/uber JAR with maven-shade-plugin or maven-assembly-plugin) and then
modify the final image to either copy the dependencies directory and add them to
the classpath (e.g., include /build/target/dependency/* on the classpath) or
copy the fat JAR and run it (adjusting ENTRYPOINT to java -jar <fat-jar-name> or
java -classpath "/app:/app/dependency/*" org.example.App) so that classes and
all runtime jars (like bucket4j) are available at runtime.
In `@pom.xml`:
- Around line 22-26: Remove the unused Bucket4j dependency from the POM: delete
the <dependency> block that references groupId "com.bucket4j" and artifactId
"bucket4j_jdk17-core" (version set via ${bucket4j.version}), or move it out of
this PR and add it only in the PR that implements rate-limiting; ensure no other
POM references the ${bucket4j.version} property remains orphaned.
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 37-46: The REASON_PHRASES map and build() method currently use
REASON_PHRASES.getOrDefault(statusCode, "OK"), which returns "OK" for any
unmapped status code and can produce incorrect status lines (e.g., HTTP/1.1 403
OK); update the fallback to a neutral phrase such as "Unknown" (or construct a
fallback using the numeric statusCode) by replacing the getOrDefault default
value, and/or expand REASON_PHRASES to include the additional expected codes so
build() uses correct reason phrases for statusCode.
- Around line 44-56: build() unconditionally appends hardcoded "Content-Length"
and "Connection" headers after iterating headers, which causes duplicates when
callers set those keys; update build() to detect existing header keys
(case-insensitively) in the headers map and skip appending the hardcoded values
if present (e.g., check headers.keySet() lower-cased or use headers.containsKey
for exact matches), otherwise compute Content-Length from
body.getBytes(StandardCharsets.UTF_8).length and append Connection: close only
when not already provided; reference build(), headers, body and the
Content-Length/Connection header names to locate the change.
In `@src/main/java/org/example/http/MimeTypeDetector.java`:
- Line 74: The call in MimeTypeDetector that converts the filename extension to
lower case uses the locale-sensitive toLowerCase(); replace
filename.substring(lastDot).toLowerCase() with
filename.substring(lastDot).toLowerCase(Locale.ROOT) (import java.util.Locale if
needed) so extension matching uses a stable, locale-independent lowercasing for
keys like ".ico".
In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Around line 16-20: The setReader method currently ignores subsequent calls due
to "if (this.reader == null)"; change it to fail fast instead of silently
ignoring by throwing an IllegalStateException when reader is already set, e.g.
in setReader(InputStream in) check if this.reader != null and throw new
IllegalStateException("reader already set") otherwise assign this.reader = new
BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8)); reference the
setReader method and the reader field when making this change.
- Around line 22-46: parseHttp() currently reads and silently discards the
request line when parseRequest() wasn't called first; update parseHttp() to
detect whether the request line has already been parsed (e.g.,
getMethod()/getUri()/getVersion() are null) and if not delegate to
parseRequest() to consume and populate the request line before consuming
headers, or explicitly check the first read line for a request-line pattern
(methods like GET/POST or ending with "HTTP/1.1") and treat it as the request
line rather than a header; ensure parseHttp() no longer swallows that line so
getMethod()/getUri()/getVersion() are set and header parsing still behaves
correctly (refer to parseHttp(), parseRequest(), getMethod(), getUri(),
getVersion()).
In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java`:
- Around line 25-31: The version token in HttpParseRequestLine is not validated:
after setVersion(requestLineArray[2]) accept any value; update the parsing to
validate the version string (e.g., match against the pattern "HTTP/\\d\\.\\d")
and throw an IOException if it doesn't match; modify the logic around
setVersion(...) in the same block (referencing HttpParseRequestLine, setVersion,
and requestLineArray[2]) so only a version matching the expected regex is
accepted and invalid versions produce an error.
In `@src/main/java/org/example/TcpServer.java`:
- Around line 19-23: Wrap the Socket returned by serverSocket.accept() in a
try-with-resources block to ensure clientSocket is always closed even if
printing the remote address throws; specifically replace the current raw
accept() + System.out.println(...) + clientSocket.close() sequence in TcpServer
with a try (Socket clientSocket = serverSocket.accept()) {
System.out.println("Client connected: " +
clientSocket.getRemoteSocketAddress()); } so the clientSocket is auto-closed on
exceptions and normal exit.
In `@src/test/java/org/example/httpparser/HttpParserTest.java`:
- Around line 15-28: The test TestHttpParserForHeaders calls parseHttp() but
never parseRequest(), so the request line (GET /index.html HTTP/1.1) is ignored
and getMethod()/getUri()/getVersion() remain null; update the test to call
httpParser.parseRequest() before httpParser.parseHttp() (or add a new test that
runs parseRequest() then parseHttp()) and add assertions that
httpParser.getMethod(), httpParser.getUri(), and httpParser.getVersion() match
"GET", "/index.html", and "HTTP/1.1" respectively to validate full parsing flow.
🧹 Nitpick comments (10)
src/main/java/org/example/App.java (1)
5-5: Consider making the port configurable.The port is hardcoded to
8080. For flexibility (e.g., in CI, Docker, or when the port is already in use), consider reading from an environment variable or command-line argument with8080as the default.pom.xml (1)
51-56: Minor: Inline version forawaitility— consider extracting to a property for consistency.Other dependency versions (
junit.jupiter.version,assertj.core.version,mockito.version,bucket4j.version) are managed via<properties>, butawaitilityhas its version hardcoded at4.3.0.Dockerfile (2)
3-5: Improve Docker layer caching by copyingpom.xmlfirst.Currently, any source change invalidates the dependency download layer. Copy
pom.xmland resolve dependencies before copyingsrc/so that dependency layers are cached across builds.Proposed fix
FROM maven:3-eclipse-temurin-25-alpine AS build WORKDIR /build -COPY src/ src/ COPY pom.xml pom.xml -RUN mvn compile +RUN mvn dependency:go-offline -B +COPY src/ src/ +RUN mvn compile -o
10-11: AddEXPOSE 8080for documentation and tooling.The
TcpServerlistens on port 8080. AddingEXPOSE 8080beforeENTRYPOINTdocuments the expected port and enables automatic port mapping withdocker run -P.Proposed fix
COPY --from=build /build/target/classes/ /app/ +EXPOSE 8080 ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"] USER appusersrc/test/java/org/example/http/HttpResponseBuilderTest.java (1)
75-124: Minor overlap between the two parameterized tests.
setContentTypeFromFilename_detectsVariousTypesandsetContentTypeFromFilename_allCasesshare several identical cases (index.html,style.css,logo.png,doc.pdf). Consider consolidating into a single parameterized test that covers both common types and edge cases, to reduce duplication and maintenance overhead.src/main/java/org/example/httpparser/HttpParseRequestLine.java (2)
11-11:debugis alwaysfalse— dead code.The
debugfield has no setter or any way to be set totrue, making the logging block on lines 34-38 unreachable. Either add a way to enable debug mode or remove the dead code.Also applies to: 34-38
14-15: Unnecessary local variable aliasing.
BufferedReader reader = br;just aliases the parameter. Usebrdirectly (or rename the parameter toreader).Proposed fix
- public void parseHttpRequest(BufferedReader br) throws IOException { - BufferedReader reader = br; - String requestLine = reader.readLine(); + public void parseHttpRequest(BufferedReader reader) throws IOException { + String requestLine = reader.readLine();src/main/java/org/example/httpparser/HttpParser.java (1)
53-55:getHeadersMap()returns the mutable internal map.Callers can add, remove, or modify headers, breaking encapsulation. Return an unmodifiable view or a defensive copy.
Proposed fix
public Map<String, String> getHeadersMap() { - return headersMap; + return Map.copyOf(headersMap); }src/test/java/org/example/httpparser/HttpParseRequestLineTest.java (1)
24-24: Use explicit charset ingetBytes().
testString.getBytes()uses the platform default charset, which may differ across environments. UseStandardCharsets.UTF_8for consistency with the rest of the codebase.src/test/java/org/example/httpparser/HttpParserTest.java (1)
16-16: Test method nameTestHttpParserForHeadersstarts with uppercase.Java/JUnit convention is
lowerCamelCasefor method names. Other tests in this file use the correct convention (e.g.,testParseHttp_EmptyInput). Rename for consistency.
| COPY --from=build /build/target/classes/ /app/ | ||
| ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"] |
There was a problem hiding this comment.
Classpath includes only compiled classes — runtime dependencies will be missing.
The COPY only brings /build/target/classes/ into the image. If any non-test dependency (e.g., bucket4j) is used at runtime, it won't be on the classpath and the app will fail with ClassNotFoundException. Consider using maven-dependency-plugin:copy-dependencies or packaging as a fat JAR instead.
Alternative: use maven-jar-plugin with dependency copy
-COPY --from=build /build/target/classes/ /app/
-ENTRYPOINT ["java", "-classpath", "/app", "org.example.App"]
+COPY --from=build /build/target/classes/ /app/classes/
+COPY --from=build /build/target/dependency/ /app/lib/
+ENTRYPOINT ["java", "-classpath", "/app/classes:/app/lib/*", "org.example.App"]In the build stage, add a step to copy dependencies:
RUN mvn compile
+RUN mvn dependency:copy-dependencies -DoutputDirectory=target/dependency -DincludeScope=runtime🤖 Prompt for AI Agents
In `@Dockerfile` around lines 9 - 10, The Dockerfile currently copies only
/build/target/classes/ and sets ENTRYPOINT to run org.example.App, which omits
runtime dependencies and leads to ClassNotFoundException; update the build stage
to produce and include runtime dependencies (e.g., run
maven-dependency-plugin:copy-dependencies or build a fat/uber JAR with
maven-shade-plugin or maven-assembly-plugin) and then modify the final image to
either copy the dependencies directory and add them to the classpath (e.g.,
include /build/target/dependency/* on the classpath) or copy the fat JAR and run
it (adjusting ENTRYPOINT to java -jar <fat-jar-name> or java -classpath
"/app:/app/dependency/*" org.example.App) so that classes and all runtime jars
(like bucket4j) are available at runtime.
| <dependency> | ||
| <groupId>com.bucket4j</groupId> | ||
| <artifactId>bucket4j_jdk17-core</artifactId> | ||
| <version>${bucket4j.version}</version> | ||
| </dependency> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -i 'bucket4j' --type=javaRepository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 90
Remove bucket4j dependency or defer to the PR that uses it.
This dependency is not used anywhere in the codebase and appears to be added prematurely. If it's intended for future rate-limiting work, add it in the PR where it's actually implemented to keep the dependency tree lean.
🤖 Prompt for AI Agents
In `@pom.xml` around lines 22 - 26, Remove the unused Bucket4j dependency from the
POM: delete the <dependency> block that references groupId "com.bucket4j" and
artifactId "bucket4j_jdk17-core" (version set via ${bucket4j.version}), or move
it out of this PR and add it only in the PR that implements rate-limiting;
ensure no other POM references the ${bucket4j.version} property remains
orphaned.
| return octet; | ||
| } | ||
|
|
||
| String extension = filename.substring(lastDot).toLowerCase(); |
There was a problem hiding this comment.
Use Locale.ROOT in toLowerCase() to avoid locale-sensitive behavior.
On systems with a Turkish locale, "FILE.ICO".toLowerCase() produces "fıle.ico" (dotless ı), which won't match the map key ".ico". Since file extensions are ASCII, Locale.ROOT is the correct choice.
Proposed fix
- String extension = filename.substring(lastDot).toLowerCase();
+ String extension = filename.substring(lastDot).toLowerCase(java.util.Locale.ROOT);📝 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.
| String extension = filename.substring(lastDot).toLowerCase(); | |
| String extension = filename.substring(lastDot).toLowerCase(java.util.Locale.ROOT); |
🤖 Prompt for AI Agents
In `@src/main/java/org/example/http/MimeTypeDetector.java` at line 74, The call in
MimeTypeDetector that converts the filename extension to lower case uses the
locale-sensitive toLowerCase(); replace
filename.substring(lastDot).toLowerCase() with
filename.substring(lastDot).toLowerCase(Locale.ROOT) (import java.util.Locale if
needed) so extension matching uses a stable, locale-independent lowercasing for
keys like ".ico".
| protected void setReader(InputStream in) { | ||
| if (this.reader == null) { | ||
| this.reader = new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8)); | ||
| } | ||
| } |
There was a problem hiding this comment.
setReader silently ignores subsequent calls.
The if (this.reader == null) guard means a second call with a different InputStream is silently ignored. This could lead to confusing bugs if the parser is reused. Consider either allowing re-initialization or throwing if already set.
🤖 Prompt for AI Agents
In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 16 - 20,
The setReader method currently ignores subsequent calls due to "if (this.reader
== null)"; change it to fail fast instead of silently ignoring by throwing an
IllegalStateException when reader is already set, e.g. in setReader(InputStream
in) check if this.reader != null and throw new IllegalStateException("reader
already set") otherwise assign this.reader = new BufferedReader(new
InputStreamReader(in, StandardCharsets.UTF_8)); reference the setReader method
and the reader field when making this change.
| public void parseHttp() throws IOException { | ||
| String headerLine; | ||
|
|
||
| while ((headerLine = reader.readLine()) != null) { | ||
| if (headerLine.isEmpty()) { | ||
| break; | ||
| } | ||
|
|
||
| int valueSeparator = headerLine.indexOf(':'); | ||
| if (valueSeparator <= 0) { | ||
| continue; | ||
| } | ||
|
|
||
| String key = headerLine.substring(0, valueSeparator).trim(); | ||
| String value = headerLine.substring(valueSeparator + 1).trim(); | ||
|
|
||
| headersMap.merge(key, value, (existing, incoming) -> existing +", " + incoming); | ||
| } | ||
| if (debug) { | ||
| System.out.println("Host: " + headersMap.get("Host")); | ||
| for (String key : headersMap.keySet()) { | ||
| System.out.println(key + ": " + headersMap.get(key)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
parseHttp() silently consumes the request line as an invalid header.
In TestHttpParserForHeaders, the input starts with "GET /index.html HTTP/1.1\r\n...". Since parseHttp() is called without first calling parseRequest(), the request line is read as a header line, fails the colon check (indexOf(':') <= 0), and is silently discarded. This works by coincidence but means:
- The request line is lost —
getMethod()/getUri()/getVersion()will be null. - If a future request line happens to contain a colon (e.g., in the URI), it could be misinterpreted as a header.
Consider documenting and enforcing that parseRequest() must be called before parseHttp(), or have parseHttp() delegate to parseRequest() internally on the first line.
🤖 Prompt for AI Agents
In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 22 - 46,
parseHttp() currently reads and silently discards the request line when
parseRequest() wasn't called first; update parseHttp() to detect whether the
request line has already been parsed (e.g., getMethod()/getUri()/getVersion()
are null) and if not delegate to parseRequest() to consume and populate the
request line before consuming headers, or explicitly check the first read line
for a request-line pattern (methods like GET/POST or ending with "HTTP/1.1") and
treat it as the request line rather than a header; ensure parseHttp() no longer
swallows that line so getMethod()/getUri()/getVersion() are set and header
parsing still behaves correctly (refer to parseHttp(), parseRequest(),
getMethod(), getUri(), getVersion()).
| } else { | ||
| setMethod(requestLineArray[0]); | ||
| if (!getMethod().matches("^[A-Z]+$")){ | ||
| throw new IOException("Invalid HTTP method"); | ||
| } | ||
| setUri(requestLineArray[1]); | ||
| setVersion(requestLineArray[2]); |
There was a problem hiding this comment.
No validation on the HTTP version token.
After validating the method is uppercase-only, the version (e.g., HTTP/1.1) is accepted without any check. A request like GET / GARBAGE would be parsed successfully. Consider validating the version matches an expected pattern (e.g., HTTP/\\d\\.\\d).
🤖 Prompt for AI Agents
In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java` around lines
25 - 31, The version token in HttpParseRequestLine is not validated: after
setVersion(requestLineArray[2]) accept any value; update the parsing to validate
the version string (e.g., match against the pattern "HTTP/\\d\\.\\d") and throw
an IOException if it doesn't match; modify the logic around setVersion(...) in
the same block (referencing HttpParseRequestLine, setVersion, and
requestLineArray[2]) so only a version matching the expected regex is accepted
and invalid versions produce an error.
| while (true) { | ||
| Socket clientSocket = serverSocket.accept(); // block | ||
| System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress()); | ||
| clientSocket.close(); | ||
| } |
There was a problem hiding this comment.
Use try-with-resources for clientSocket to prevent resource leaks.
If System.out.println on Line 21 throws (e.g., encoding error with getRemoteSocketAddress()), the socket will leak.
Proposed fix
while (true) {
- Socket clientSocket = serverSocket.accept(); // block
- System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress());
- clientSocket.close();
+ try (Socket clientSocket = serverSocket.accept()) {
+ System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress());
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while (true) { | |
| Socket clientSocket = serverSocket.accept(); // block | |
| System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress()); | |
| clientSocket.close(); | |
| } | |
| while (true) { | |
| try (Socket clientSocket = serverSocket.accept()) { | |
| System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/org/example/TcpServer.java` around lines 19 - 23, Wrap the
Socket returned by serverSocket.accept() in a try-with-resources block to ensure
clientSocket is always closed even if printing the remote address throws;
specifically replace the current raw accept() + System.out.println(...) +
clientSocket.close() sequence in TcpServer with a try (Socket clientSocket =
serverSocket.accept()) { System.out.println("Client connected: " +
clientSocket.getRemoteSocketAddress()); } so the clientSocket is auto-closed on
exceptions and normal exit.
| @Test | ||
| void TestHttpParserForHeaders() throws IOException { | ||
| String testInput = "GET /index.html HTTP/1.1\r\nHost: localhost\r\nContent-Type: text/plain\r\nUser-Agent: JUnit5\r\n\r\n"; | ||
| InputStream in = new ByteArrayInputStream(testInput.getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| httpParser.setReader(in); | ||
| httpParser.parseHttp(); | ||
|
|
||
| assertNotNull(httpParser.getHeadersMap()); | ||
| assertThat(httpParser.getHeadersMap().size()).isEqualTo(3); | ||
| assertThat(httpParser.getHeadersMap().get("Host")).contains("localhost"); | ||
| assertThat(httpParser.getHeadersMap().get("Content-Type")).contains("text/plain"); | ||
| assertThat(httpParser.getHeadersMap().get("User-Agent")).contains("JUnit5"); | ||
| } |
There was a problem hiding this comment.
Test does not call parseRequest() before parseHttp().
The input includes a request line (GET /index.html HTTP/1.1), but only parseHttp() is called. The request line is silently discarded (no colon → skipped). This means getMethod(), getUri(), and getVersion() are all null after this test. Consider adding a test that calls both parseRequest() and parseHttp() in sequence to validate the intended full parsing flow.
🤖 Prompt for AI Agents
In `@src/test/java/org/example/httpparser/HttpParserTest.java` around lines 15 -
28, The test TestHttpParserForHeaders calls parseHttp() but never
parseRequest(), so the request line (GET /index.html HTTP/1.1) is ignored and
getMethod()/getUri()/getVersion() remain null; update the test to call
httpParser.parseRequest() before httpParser.parseHttp() (or add a new test that
runs parseRequest() then parseHttp()) and add assertions that
httpParser.getMethod(), httpParser.getUri(), and httpParser.getVersion() match
"GET", "/index.html", and "HTTP/1.1" respectively to validate full parsing flow.
* 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>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 54-64: The header presence checks in HttpResponseBuilder use
headers.containsKey(...) which is case-sensitive, so callers using different
header casing (e.g., "content-length") will produce duplicate headers; fix by
making header lookups case-insensitive: either change the headers map to a
case-insensitive map (e.g., new TreeMap(String.CASE_INSENSITIVE_ORDER) or
another case-insensitive implementation) where headers are stored (ensure
setHeader and any iteration still behave as needed), or replace the containsKey
checks with a case-insensitive existence test (e.g.,
headers.keySet().stream().anyMatch(k -> k.equalsIgnoreCase("Content-Length"))
and similarly for "Connection") so the guards in the block that appends
Content-Length/Connection detect headers regardless of casing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 62-72: The build() method currently corrupts binary payloads by
converting bytebody to a UTF-8 String via setBody; change the API so binary
bodies are preserved: either make build() return a byte[] (or a Response object
carrying header bytes and raw body) or add a new buildBytes() method that builds
the header as bytes (using StandardCharsets.UTF_8) and concatenates those header
bytes with the raw bytebody without calling setBody or new String(...); ensure
existing build() still works for text by using body.getBytes(UTF_8) but when
bytebody != null use bytebody directly and avoid any lossy conversions in
methods build(), setBody, and any code paths referencing body/bytebody.
🧹 Nitpick comments (2)
src/main/java/org/example/http/HttpResponseBuilder.java (1)
12-12: Nit: field namebytebodyshould bebyteBodyper Java camelCase convention.src/test/java/org/example/http/HttpResponseBuilderTest.java (1)
252-266: Consider adding a test forsetBody(byte[])to validate binary body handling.There are no tests exercising
setBody(byte[] body). Given the concerns with howbuild()handles binary data, adding a test that verifies round-trip fidelity for non-UTF-8 bytes would help surface the corruption issue early.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/java/org/example/http/HttpResponseBuilder.java (2)
64-67: Javadoc comment uses/*instead of/**.The block comment on line 64 should start with
/**to be recognized as Javadoc.Fix Javadoc opening tag
- /* + /** * Builds the complete HTTP response as a byte array and preserves binary content without corruption. * `@return` Complete HTTP response (headers + body) as byte[] */
45-48:setBody(byte[])does not defensively copy the input array.A caller can mutate the array after calling
setBody(byte[]), silently changing the response body. If this builder is only used internally and immediately built, this is acceptable — but worth noting if the API surface grows.Optional: defensive copy
public void setBody(byte[] body) { - this.bytebody = body; + this.bytebody = body != null ? body.clone() : null; this.body = ""; // Clear string body when setting byte body }src/test/java/org/example/http/HttpResponseBuilderTest.java (1)
238-242:.doesNotContain("OK")assertion is fragile.This assertion passes only because the body is
"". If the body or a header value ever contained the substring "OK", it would break. Consider asserting on the status line more precisely instead.More robust assertion
assertThat(resultStr) .contains("HTTP/1.1 " + statusCode + " " + expectedReason) - .contains("Location: " + location) - .doesNotContain("OK"); + .contains("Location: " + location); + // Verify "OK" is not in the status line + String statusLine = resultStr.lines().findFirst().orElse(""); + assertThat(statusLine).doesNotContain("OK");
gitnes94
force-pushed
the
feature/mime-type-detection
branch
from
b2107ae to
5a685f3
Compare
* Added basic YAML config-file. * Added class ConfigLoader with static classes for encapsulation * Added static metod loadOnce and step one of static method load * Added static method createMapperFor that checks for YAML or JSON-files before creating an ObjectMapper object. * implement ConfigLoader refs #13 * Added AppConfig.java record for config after coderabbit feedback * Updated ConfigLoader to use AppConfig record and jackson 3 * Added tests for ConfigLoader and reset cached method in ConfigLoader to ensure test isolation with static cache * Removed unused dependency. Minor readability tweaks in AppConfig. * Added check for illegal port numbers to withDefaultsApplied-method. * Added test for illegal port numbers.
* Add 404 error handling to `StaticFileHandler` with fallback page * Add test coverage for `StaticFileHandler` and improve constructor flexibility - Introduced a new constructor in `StaticFileHandler` to support custom web root paths, improving testability. - Added `StaticFileHandlerTest` to validate static file serving and error handling logic. * Add test for 404 handling in `StaticFileHandler` - Added a test to ensure nonexistent files return a 404 response. - Utilized a temporary directory and fallback file for improved test isolation. * Verify `Content-Type` header in `StaticFileHandlerTest` after running Pittest, mutations survived where setHeaders could be removed without test failure. * Remove unused `.btn` styles from `pageNotFound.html` * Improve 404 handling in `StaticFileHandler`: add fallback to plain text response if `pageNotFound.html` is missing, and fix typo in `pageNotFound.html`, after comments from CodeRabbit.
…o feature/mime-type-detection
Changed 'charset=utf-8' to 'charset=UTF-8' in StaticFileHandlerTest to match MimeTypeDetector output and align with HTTP RFC standard. Uppercase UTF-8 is the correct format per RFC 2616/7231.
12 participants
Implements MIME-type detection based on file extension
Resolves issue #8
What this PR does
MimeTypeDetectorutility class for detecting MIME types from file extensionsContent-Typeheaders including charset for text-based filesapplication/octet-streamfor unknown file typesChanges
src/main/java/org/example/http/MimeTypeDetector.javasrc/test/java/org/example/http/MimeTypeDetectorTest.javaHttpResponseBuilder.java- addedsetContentTypeFromFilename()methodTesting
How to test manually
www/directorySummary by CodeRabbit
New Features
Tests
Chores