Open
Conversation
…on made test fail. to handle various ETag formats; improve Javadocs and correct typos in `HttpCachingHeaders`
…lify `setNoCache` behavior.
…ag generation logic; refactor `HttpCachingHeaders` and `HttpResponseBuilder` for improved compliance and maintainability. # Conflicts: # src/main/java/org/example/http/CachingFilter.java # src/main/java/org/example/http/HttpResponseBuilder.java
…eader` method to `HttpResponseBuilder`.
…ization and handle invalid `If-Modified-Since` parsing gracefully.
…ization and handle invalid `If-Modified-Since` parsing gracefully.
…ation and handle invalid If-Modified-Since parsing gracefully.
…ization and handle invalid `If-Modified-Since` parsing gracefully.
…ation and handle invalid If-Modified-Since parsing gracefully.
Removed the getStatusCode method from HttpResponseBuilder.
…ETag generation, and add caching headers.
…ndling, and ensure consistent ETag and caching headers in responses. At the request of Martin Stenhagen and gvaguirres
…equest` to include `resolvedPath`, and refactor handlers and tests for consistent path handling.
…prove path conversion logic, and ensure accurate file system path generation.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds request path resolution via FileResolver and threads a new resolvedPath into HttpRequest; ConnectionHandler, StaticFileHandler, and many tests updated to use it. Introduces caching support (CachingFilter, HttpCachingHeaders), HttpResponseBuilder.addHeader, tests, and a TestFilterChain helper. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Conn as ConnectionHandler
participant Resolver as FileResolver
participant Cache as CachingFilter
participant Static as StaticFileHandler
participant FS as FileSystem
Client->>Conn: send HTTP request (URI)
Conn->>Resolver: resolvePath(uri)
Resolver-->>Conn: resolvedPath or null
Conn->>Cache: doFilter(HttpRequest(resolvedPath), response, chain)
Cache->>FS: stat/inspect file(resolvedPath)
alt Not modified (ETag or If-Modified-Since)
Cache-->>Client: 304 Not Modified + caching headers
else
Cache->>Static: chain.doFilter(request, response)
Static->>FS: read file(resolvedPath)
Static-->>Cache: file bytes / 200 OK
Cache->>Client: 200 OK + file + caching headers
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (12)
src/test/java/org/example/filter/IpFilterTest.java (1)
30-37: Consider resettingConfigLoaderbetween tests for isolation.
ConfigLoader.loadOncecaches the config globally. If other test classes load different configs, test execution order could affect results. Consider using@AfterEachor@BeforeEachto callConfigLoader.resetForTests().Example cleanup hook
`@AfterEach` void tearDown() { ConfigLoader.resetForTests(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/filter/IpFilterTest.java` around lines 30 - 37, Add a test teardown that resets global config to avoid cross-test pollution: in the IpFilterTest class (where setUp() calls ConfigLoader.loadOnce(...)) add an `@AfterEach` method that calls ConfigLoader.resetForTests() so each test runs with a clean config; alternatively call ConfigLoader.resetForTests() at the start of setUp() to guarantee isolation before invoking ConfigLoader.loadOnce().src/main/java/org/example/StaticFileHandler.java (1)
64-64: Unusedchainparameter.The
FilterChain chainparameter is declared but never used. Either invokechain.doFilter(request, response)to continue the chain, or remove the parameter if this handler is terminal.🤖 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 64, The handle(HttpRequest request, HttpResponseBuilder response, FilterChain chain) method declares a FilterChain chain but never uses it; either call chain.doFilter(request, response) inside handle to continue processing the filter chain (ensure correct exception handling and propagation) or remove the chain parameter and update all callers/signature references if this handler is meant to be terminal—locate the handle method in StaticFileHandler and apply the chosen change consistently across the codebase.src/main/java/org/example/ConnectionHandler.java (1)
51-52: Duplicate path resolution remains.The PR objective is to eliminate duplication, but path resolution is now performed twice:
- Line 51:
FileResolver.resolvePath(parser.getUri())computesresolvedPath- Line 77:
resolveTargetFile(parser.getUri())performs a separate resolution intothis.uriConsider using
request.getResolvedPath()inStaticFileHandler.sendGetRequestinstead of callingresolveTargetFile.Also applies to: 77-79
🤖 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 51 - 52, The code currently resolves the same path twice—via FileResolver.resolvePath(parser.getUri()) (producing resolvedPath) and later via resolveTargetFile(parser.getUri()) which sets this.uri; update StaticFileHandler.sendGetRequest to use the already-resolved value (request.getResolvedPath()) instead of calling resolveTargetFile/parser.getUri() again, remove the redundant resolveTargetFile call or its duplicated logic, and ensure this.uri (or equivalent field) is set from the single resolvedPath so all callers (including sendGetRequest) reuse that resolved value.src/main/java/org/example/http/HttpResponseBuilder.java (1)
96-102:addHeaderduplicatessetHeaderfunctionality.Both
addHeaderandsetHeader(lines 87-89) perform identical operations:this.headers.put(key, value). Consider removing one or differentiating their behavior (e.g.,addHeadercould append values for multi-value headers likeSet-Cookieinstead of replacing).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/http/HttpResponseBuilder.java` around lines 96 - 102, The addHeader method duplicates setHeader; either remove addHeader or change its behavior to append values for multi-value headers. If you choose to append, modify the headers storage used by HttpResponseBuilder (the headers Map) to hold multiple values (e.g., Map<String, List<String>> or Map<String, StringBuilder>), implement addHeader(String key, String value) to append the new value (for Set-Cookie add a new list entry, for regular headers optionally comma-join), and keep setHeader(String key, String value) to replace the header. Update any code paths using headers, setHeader, or addHeader to the new header value representation.src/main/java/org/example/FileResolver.java (1)
3-8: Unused imports.
FilterChain,HttpResponseBuilder,HttpRequest, andFileare imported but not used in this class.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/FileResolver.java` around lines 3 - 8, Remove the unused imports from FileResolver: eliminate FilterChain, HttpResponseBuilder, HttpRequest, and java.io.File from the import list so only actually used symbols (e.g., org.example.config.ConfigLoader) remain; update the import section in the FileResolver class to remove those unused entries and run an import/compile check to ensure no referenced symbols in FileResolver rely on them.src/test/java/org/example/http/TestFilterChain.java (1)
5-5: Stray comment/// /.This appears to be a typo or incomplete Javadoc. Remove or complete it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/http/TestFilterChain.java` at line 5, Remove the stray comment "/// /" in TestFilterChain.java (the top-of-file comment before the TestFilterChain class) — either delete the malformed line or replace it with a proper Javadoc/comment if documentation was intended; ensure the file header compiles cleanly and that the TestFilterChain class and its imports are unaffected by the removal.src/main/java/org/example/http/CachingFilter.java (1)
11-11: Unused import.
java.io.ObjectInputFilteris imported but not used.Proposed fix
-import java.io.ObjectInputFilter;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/http/CachingFilter.java` at line 11, Remove the unused import "java.io.ObjectInputFilter" from CachingFilter.java; locate the import statement at the top of the file and delete it (no code changes to classes or methods like CachingFilter are required), then run a quick compile or IDE auto-organize imports to ensure no other unused imports remain.src/test/java/org/example/http/CachingFilterTest.java (3)
13-13: Remove stray comment.The
/// //comment appears to be a leftover artifact.Proposed fix
-/// //🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/http/CachingFilterTest.java` at line 13, Remove the stray comment artifact "/// //" from the CachingFilterTest file; open the CachingFilterTest class and delete that standalone comment (it's not part of any test or method), ensuring no other code or formatting is changed in the CachingFilterTest class.
55-57: Test does not clean up created file.The test creates
www/ok.txtbut does not delete it after the test completes. This can cause test pollution and flaky behavior. Consider using@AfterEachcleanup or@TempDirfor test files.Suggested approach with `@AfterEach`
`@AfterEach` void cleanup() throws Exception { Files.deleteIfExists(Paths.get("www/ok.txt")); }Alternatively, use JUnit's
@TempDirannotation to manage temporary test files automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/http/CachingFilterTest.java` around lines 55 - 57, The test CachingFilterTest creates a persistent file via new File("www/ok.txt") and Files.writeString(...) but never removes it; add a cleanup to avoid test pollution by either switching the test to use JUnit's `@TempDir` for a temporary directory (replace "www/ok.txt" with a Path from the `@TempDir`) or add an `@AfterEach` method in CachingFilterTest that calls Files.deleteIfExists(Paths.get("www/ok.txt")) (or deletes the file's parent directory) to ensure the created file is removed after each test.
23-46: Consider adding test for 304 response scenario.The current tests only verify that the filter chain continues on cache misses. Consider adding tests that verify 304 responses are correctly returned when
If-None-MatchorIf-Modified-Sinceheaders match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/http/CachingFilterTest.java` around lines 23 - 46, Add tests that verify CachingFilter returns 304 when request headers indicate resource is not modified: create two new test methods (e.g., shouldReturn304WhenEtagMatches and shouldReturn304WhenLastModifiedMatches) that build an existing resource request using FileResolver.resolvePath for a file with a stable ETag/Last-Modified, construct HttpRequest including an If-None-Match (matching the resource ETag) and an If-Modified-Since (matching the resource last-modified) respectively, invoke CachingFilter.doFilter(request, response, chain) and assert that HttpResponseBuilder status is 304 and TestFilterChain.called is false; use the same classes referenced in the diff (CachingFilter, HttpRequest, HttpResponseBuilder, TestFilterChain) to locate and implement the tests.src/test/java/org/example/http/HttpCachingHeadersTest.java (2)
40-42: Misleading variable name.
etagValueis used to store theCache-Controlheader value, which is confusing. Consider renaming tocacheControlValuefor clarity. The same issue occurs on line 52.Suggested fix
- String etagValue = cachingHeaders.getHeaders().get("Cache-Control"); + String cacheControlValue = cachingHeaders.getHeaders().get("Cache-Control"); - assertThat(etagValue).isEqualTo("public, max-age=3600"); + assertThat(cacheControlValue).isEqualTo("public, max-age=3600");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/http/HttpCachingHeadersTest.java` around lines 40 - 42, Rename the misleading local variable etagValue in HttpCachingHeadersTest to cacheControlValue wherever it stores the result of cachingHeaders.getHeaders().get("Cache-Control") (and update the second occurrence that the reviewer noted), and update the corresponding assertThat(...) call to reference cacheControlValue instead; ensure the variable rename is applied in the test method(s) that read the Cache-Control header so names match purpose and assertions remain unchanged.
36-36: Test method name should follow camelCase convention.
CacheControlReturnsCorrectValuestarts with an uppercase letter. JUnit test method names should use camelCase starting with a lowercase letter for consistency.Suggested fix
- void CacheControlReturnsCorrectValue() { + void cacheControlReturnsCorrectValue() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/http/HttpCachingHeadersTest.java` at line 36, Rename the test method CacheControlReturnsCorrectValue to follow camelCase (e.g., cacheControlReturnsCorrectValue) to comply with JUnit naming conventions; update the method declaration in HttpCachingHeadersTest (method symbol CacheControlReturnsCorrectValue) and any internal references or test runners if referenced elsewhere, leaving the `@Test` annotation and method body unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/ConnectionHandler.java`:
- Line 23: The field `private HttpRequest request` in class ConnectionHandler is
declared but never used; either delete this unused field from ConnectionHandler
or, if the intent was to retain the last-built request, assign the constructed
HttpRequest to `this.request` where the request is created (e.g., inside the
method that builds/sends the HttpRequest) and ensure any subsequent code reads
from `this.request`; choose one option and remove any unused imports if you
delete the field.
In `@src/main/java/org/example/FileResolver.java`:
- Line 22: In FileResolver (the method that computes the path from requestPath),
avoid calling requestPath.substring(1) when requestPath is null or empty; add a
defensive check such as if (requestPath == null || requestPath.isEmpty()) treat
it as "/" (or directly map to "index.html"), otherwise only use substring(1)
when requestPath.length() > 1 (or when it startsWith("/") and length>1); update
the logic around the existing expression String path = requestPath.equals("/") ?
"index.html" : requestPath.substring(1) to handle null/empty and
single-character cases safely.
- Around line 21-26: FileResolver.resolvePath currently returns raw
request-derived paths, allowing ../ traversal; update resolvePath to normalize
and canonicalize against the server root and reject paths that escape it: create
File objects for the server root (from ConfigLoader.get().server().rootDir())
and the requested path (new File(rootFile, path)), call
getCanonicalFile()/getCanonicalPath() on both, verify the requested canonical
path startsWith the root canonical path (or is equal), and if validation fails
return null or throw a safe exception; return the validated canonical path on
success to provide defense-in-depth alongside StaticFileHandler.
In `@src/main/java/org/example/http/CachingFilter.java`:
- Around line 51-55: The 304 path currently returns before ETag/Last-Modified
are applied; update CachingFilter so cachingHeaders.getHeaders() (including ETag
and Last-Modified derived from eTag and lastModified) are added to the response
before the conditional early-returns. Specifically, compute eTag and
lastModified as you already do, then call
cachingHeaders.getHeaders().forEach(response::addHeader) prior to the checks
that use ifNoneMatch and ifModifiedSince, and do the same change for the second
conditional branch that sets 304; ensure you still call
response.setStatusCode(HttpResponseBuilder.SC_NOT_MODIFIED) and return after
adding headers.
- Around line 36-38: The code in CachingFilter incorrectly builds the file path
by assigning rootDir and path both from request.getResolvedPath() and calling
new File(rootDir, path), which duplicates the path; fix it by using the resolved
path directly—replace the dual variables and the two-argument File constructor
with a single File constructed from request.getResolvedPath() (or a local
resolvedPath variable) so CachingFilter uses the complete filesystem path
returned by HttpRequest.getResolvedPath().
- Around line 68-70: In CachingFilter replace the empty catch in the
If-Modified-Since parsing block with logging: catch the exception in the same
try around parsing (the block containing request.getHeader("If-Modified-Since"))
and call the class logger (or create one if missing) to log a warning or debug
message that includes the malformed header value and the exception (e.g., log
the header string and e.toString()); keep the original control flow after
logging so we still handle missing/invalid dates gracefully.
In `@src/main/java/org/example/http/HttpCachingHeaders.java`:
- Around line 45-53: The Javadoc for addETagHeader says it should add double
quotes but the method currently calls setHeader(ETAG, etag) directly; update
addETagHeader to ensure the header value is properly quoted per RFC 7232 by
wrapping the raw token with double quotes before calling setHeader(ETAG, ...),
but avoid double-quoting if the passed etag already starts and ends with quotes
and handle null/empty input gracefully; locate addETagHeader and the ETAG
constant and adjust the logic there (and callers only if behavior change
requires it).
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 66-68: StaticFileHandler is constructing the File with rootDir
plus request.getResolvedPath(), but request.getResolvedPath() already contains
the rootDir (and the method call result is being discarded), causing duplicated
prefixes like "www/www/..."; fix by removing the redundant rootDir usage and the
discarded call: obtain the resolved path once via request.getResolvedPath(), use
that resolvedPath directly to construct the File (or normalize/validate it for
path traversal), and assign to the existing 'file' variable instead of calling
request.getResolvedPath() without using its result.
- Around line 76-78: In StaticFileHandler, remove the redundant and
potentially-null call to response.setBody(fileBytes) inside the else branch (the
response.setBody(fileBytes) that runs before the error file is read) — keep
response.setStatusCode(HttpResponseBuilder.SC_NOT_FOUND) there, then read the
error file into fileBytes (or an explicitly named errorBytes) and call
response.setBody(...) only after the error content is loaded; ensure you
reference the existing response.setBody(fileBytes) and fileBytes variable so the
stale/null assignment is eliminated.
In `@src/test/java/org/example/http/CachingFilterTest.java`:
- Around line 53-66: In CachingFilterTest update the call to
FileResolver.resolvePath so it resolves the path for the actual test request
("/ok.txt") instead of "/does-not-exist"; locate the resolvedPath variable
assignment that calls FileResolver.resolvePath and change the argument to
"/ok.txt" so the HttpRequest (constructed with "/ok.txt") and the resolvedPath
used by the CachingFilter operate on the same file.
---
Nitpick comments:
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 51-52: The code currently resolves the same path twice—via
FileResolver.resolvePath(parser.getUri()) (producing resolvedPath) and later via
resolveTargetFile(parser.getUri()) which sets this.uri; update
StaticFileHandler.sendGetRequest to use the already-resolved value
(request.getResolvedPath()) instead of calling resolveTargetFile/parser.getUri()
again, remove the redundant resolveTargetFile call or its duplicated logic, and
ensure this.uri (or equivalent field) is set from the single resolvedPath so all
callers (including sendGetRequest) reuse that resolved value.
In `@src/main/java/org/example/FileResolver.java`:
- Around line 3-8: Remove the unused imports from FileResolver: eliminate
FilterChain, HttpResponseBuilder, HttpRequest, and java.io.File from the import
list so only actually used symbols (e.g., org.example.config.ConfigLoader)
remain; update the import section in the FileResolver class to remove those
unused entries and run an import/compile check to ensure no referenced symbols
in FileResolver rely on them.
In `@src/main/java/org/example/http/CachingFilter.java`:
- Line 11: Remove the unused import "java.io.ObjectInputFilter" from
CachingFilter.java; locate the import statement at the top of the file and
delete it (no code changes to classes or methods like CachingFilter are
required), then run a quick compile or IDE auto-organize imports to ensure no
other unused imports remain.
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 96-102: The addHeader method duplicates setHeader; either remove
addHeader or change its behavior to append values for multi-value headers. If
you choose to append, modify the headers storage used by HttpResponseBuilder
(the headers Map) to hold multiple values (e.g., Map<String, List<String>> or
Map<String, StringBuilder>), implement addHeader(String key, String value) to
append the new value (for Set-Cookie add a new list entry, for regular headers
optionally comma-join), and keep setHeader(String key, String value) to replace
the header. Update any code paths using headers, setHeader, or addHeader to the
new header value representation.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Line 64: The handle(HttpRequest request, HttpResponseBuilder response,
FilterChain chain) method declares a FilterChain chain but never uses it; either
call chain.doFilter(request, response) inside handle to continue processing the
filter chain (ensure correct exception handling and propagation) or remove the
chain parameter and update all callers/signature references if this handler is
meant to be terminal—locate the handle method in StaticFileHandler and apply the
chosen change consistently across the codebase.
In `@src/test/java/org/example/filter/IpFilterTest.java`:
- Around line 30-37: Add a test teardown that resets global config to avoid
cross-test pollution: in the IpFilterTest class (where setUp() calls
ConfigLoader.loadOnce(...)) add an `@AfterEach` method that calls
ConfigLoader.resetForTests() so each test runs with a clean config;
alternatively call ConfigLoader.resetForTests() at the start of setUp() to
guarantee isolation before invoking ConfigLoader.loadOnce().
In `@src/test/java/org/example/http/CachingFilterTest.java`:
- Line 13: Remove the stray comment artifact "/// //" from the CachingFilterTest
file; open the CachingFilterTest class and delete that standalone comment (it's
not part of any test or method), ensuring no other code or formatting is changed
in the CachingFilterTest class.
- Around line 55-57: The test CachingFilterTest creates a persistent file via
new File("www/ok.txt") and Files.writeString(...) but never removes it; add a
cleanup to avoid test pollution by either switching the test to use JUnit's
`@TempDir` for a temporary directory (replace "www/ok.txt" with a Path from the
`@TempDir`) or add an `@AfterEach` method in CachingFilterTest that calls
Files.deleteIfExists(Paths.get("www/ok.txt")) (or deletes the file's parent
directory) to ensure the created file is removed after each test.
- Around line 23-46: Add tests that verify CachingFilter returns 304 when
request headers indicate resource is not modified: create two new test methods
(e.g., shouldReturn304WhenEtagMatches and
shouldReturn304WhenLastModifiedMatches) that build an existing resource request
using FileResolver.resolvePath for a file with a stable ETag/Last-Modified,
construct HttpRequest including an If-None-Match (matching the resource ETag)
and an If-Modified-Since (matching the resource last-modified) respectively,
invoke CachingFilter.doFilter(request, response, chain) and assert that
HttpResponseBuilder status is 304 and TestFilterChain.called is false; use the
same classes referenced in the diff (CachingFilter, HttpRequest,
HttpResponseBuilder, TestFilterChain) to locate and implement the tests.
In `@src/test/java/org/example/http/HttpCachingHeadersTest.java`:
- Around line 40-42: Rename the misleading local variable etagValue in
HttpCachingHeadersTest to cacheControlValue wherever it stores the result of
cachingHeaders.getHeaders().get("Cache-Control") (and update the second
occurrence that the reviewer noted), and update the corresponding
assertThat(...) call to reference cacheControlValue instead; ensure the variable
rename is applied in the test method(s) that read the Cache-Control header so
names match purpose and assertions remain unchanged.
- Line 36: Rename the test method CacheControlReturnsCorrectValue to follow
camelCase (e.g., cacheControlReturnsCorrectValue) to comply with JUnit naming
conventions; update the method declaration in HttpCachingHeadersTest (method
symbol CacheControlReturnsCorrectValue) and any internal references or test
runners if referenced elsewhere, leaving the `@Test` annotation and method body
unchanged.
In `@src/test/java/org/example/http/TestFilterChain.java`:
- Line 5: Remove the stray comment "/// /" in TestFilterChain.java (the
top-of-file comment before the TestFilterChain class) — either delete the
malformed line or replace it with a proper Javadoc/comment if documentation was
intended; ensure the file header compiles cleanly and that the TestFilterChain
class and its imports are unaffected by the removal.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/org/example/ConnectionHandler.javasrc/main/java/org/example/FileResolver.javasrc/main/java/org/example/StaticFileHandler.javasrc/main/java/org/example/http/CachingFilter.javasrc/main/java/org/example/http/HttpCachingHeaders.javasrc/main/java/org/example/http/HttpResponseBuilder.javasrc/main/java/org/example/httpparser/HttpRequest.javasrc/test/java/org/example/filter/IpFilterTest.javasrc/test/java/org/example/http/CachingFilterTest.javasrc/test/java/org/example/http/HttpCachingHeadersTest.javasrc/test/java/org/example/http/TestFilterChain.java
Comment on lines
+68
to
+70
| } catch (Exception e) { | ||
|
|
||
| } |
There was a problem hiding this comment.
Empty catch block silently swallows parsing errors.
If If-Modified-Since header parsing fails, the exception is ignored without any logging. This makes debugging difficult. At minimum, log the exception or the malformed header value.
Proposed fix
} catch (Exception e) {
-
+ // Ignore malformed If-Modified-Since header; proceed without cache validation
+ // Consider logging: log.debug("Invalid If-Modified-Since header: {}", modifiedSince, e);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (Exception e) { | |
| } | |
| } catch (Exception e) { | |
| // Ignore malformed If-Modified-Since header; proceed without cache validation | |
| // Consider logging: log.debug("Invalid If-Modified-Since header: {}", modifiedSince, e); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/http/CachingFilter.java` around lines 68 - 70, In
CachingFilter replace the empty catch in the If-Modified-Since parsing block
with logging: catch the exception in the same try around parsing (the block
containing request.getHeader("If-Modified-Since")) and call the class logger (or
create one if missing) to log a warning or debug message that includes the
malformed header value and the exception (e.g., log the header string and
e.toString()); keep the original control flow after logging so we still handle
missing/invalid dates gracefully.
Comment on lines
+45
to
+53
| /** | ||
| * Helper method for setting ETag header value | ||
| * ETag values must be enclosed in double quotes "123" not 123 | ||
| * @param etag Raw, unquoted ETag token (e.g. {`@code` abc123}); double quotes | ||
| * are added automatically to comply with RFC 7232. | ||
| */ | ||
| public void addETagHeader(String etag) { | ||
| setHeader(ETAG, etag); | ||
| } |
There was a problem hiding this comment.
ETag quoting: Javadoc says quotes are added but implementation doesn't add them.
The Javadoc states "double quotes are added automatically to comply with RFC 7232", but the implementation just calls setHeader(ETAG, etag) without adding quotes.
🐛 Fix to match documented behavior
public void addETagHeader(String etag) {
- setHeader(ETAG, etag);
+ setHeader(ETAG, "\"" + etag + "\"");
}Alternatively, update the Javadoc if callers are expected to include quotes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/http/HttpCachingHeaders.java` around lines 45 - 53,
The Javadoc for addETagHeader says it should add double quotes but the method
currently calls setHeader(ETAG, etag) directly; update addETagHeader to ensure
the header value is properly quoted per RFC 7232 by wrapping the raw token with
double quotes before calling setHeader(ETAG, ...), but avoid double-quoting if
the passed etag already starts and ends with quotes and handle null/empty input
gracefully; locate addETagHeader and the ETAG constant and adjust the logic
there (and callers only if behavior change requires it).
…nd enhance caching headers in `CachingFilter`. Update tests accordingly.
…`..` and returning a 403 response. Ensure fallback body for error handling.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/test/java/org/example/http/CachingFilterTest.java (1)
33-41:⚠️ Potential issue | 🟠 MajorRequest path and
resolvedPathare crossed between the two tests.In the first test,
resolvedPathpoints to/ok.txtwhile the request path is/does-not-exist. In the second test, it is reversed. This weakens what each test actually validates.Also applies to: 58-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/http/CachingFilterTest.java` around lines 33 - 41, In CachingFilterTest the test data is crossed: the resolvedPath variable and the request path passed to the HttpRequest constructor are swapped between the two tests (symbols: resolvedPath, HttpRequest constructor, CachingFilterTest), so update each test so resolvedPath points to the same file as the request path used in that test (e.g. when testing an existing resource both request path and resolvedPath should be "/ok.txt"; when testing a missing resource both should be "/does-not-exist"); locate the two affected test blocks around the HttpRequest creation and swap the path arguments so each test validates the intended scenario.src/main/java/org/example/FileResolver.java (1)
31-45:⚠️ Potential issue | 🟠 MajorCanonicalize against the server root;
contains("..")is not a complete traversal defense.The guard on Line 34 is string-based and can miss normalization edge cases. Resolve to normalized paths and enforce
candidate.startsWith(root)before returning.🔒 Proposed hardening
+import java.nio.file.InvalidPathException; +import java.nio.file.Path; import java.nio.file.Paths; @@ - String path = requestPath.equals("/") ? "index.html" : requestPath.substring(1); - String rootDir = ConfigLoader.get().server().rootDir(); - - if (path.contains("..")) { + String path = requestPath.equals("/") ? "index.html" + : (requestPath.startsWith("/") ? requestPath.substring(1) : requestPath); + Path root = Paths.get(ConfigLoader.get().server().rootDir()).toAbsolutePath().normalize(); + Path candidate; + try { + candidate = root.resolve(path).normalize(); + } catch (InvalidPathException e) { + candidate = null; + } + + if (candidate == null || !candidate.startsWith(root)) { try { response.setBody(Files.readAllBytes(errorFile.toPath())); } catch (IOException e) { response.setBody("Forbidden".getBytes(StandardCharsets.UTF_8)); } response.setStatusCode(HttpResponseBuilder.SC_FORBIDDEN); return null; } - return rootDir + "/" + path; + return candidate.toString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/FileResolver.java` around lines 31 - 45, The current string-based traversal check in FileResolver should be replaced by path canonicalization: construct a candidate Path from ConfigLoader.get().server().rootDir() and the request-derived path (use Paths.get(rootDir, path) or equivalent), normalize or resolve it to a canonical/real path, then verify the canonical candidate startsWith the canonical root path before returning; if normalization fails or the candidate is outside the root, set response body to the errorFile contents (falling back to "Forbidden" on IOException), set response.setStatusCode(HttpResponseBuilder.SC_FORBIDDEN), and return null. Use the existing symbols (response, errorFile, HttpResponseBuilder.SC_FORBIDDEN, ConfigLoader.get().server().rootDir()) and handle IO/security exceptions when resolving paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/http/CachingFilter.java`:
- Around line 49-55: The ETag is being double-quoted because generateEtag()
already returns a quoted string and addETagHeader(eTag) wraps it again, causing
mismatches when comparing ifNoneMatch.equals(eTag); fix by ensuring only one
side adds quotes: either change addETagHeader to accept raw unquoted ETag
(remove its quoting) or change call sites (e.g., where
cachingHeaders.addETagHeader(eTag) is used) to pass an unquoted value (strip
surrounding quotes from generateEtag()), and normalize comparisons in the
If-None-Match check (use a helper that strips/normalizes quotes before
comparing) so generateEtag(), addETagHeader(), and the ifNoneMatch comparison
all consistently handle quoting.
- Around line 44-69: The code in CachingFilter uses file.lastModified()/length()
and runs ETag/If-Modified-Since logic even when the file is missing, causing
false 304 responses; before calling generateEtag(file) and any
cachingHeaders/304 short-circuit (the block referencing generateEtag,
lastModified, ifNoneMatch, modifiedSince and the subsequent
response.setStatusCode(HttpResponseBuilder.SC_NOT_MODIFIED) returns), first
check that the File actually exists and is a regular file (e.g., file.exists()
&& file.isFile()); if the file is missing, skip the caching validation/304
handling and let the request continue (chain.doFilter) so downstream code can
produce the correct 404 instead of returning 304. Ensure all uses of
lastModified and ETag logic are guarded by that existence check.
In `@src/test/java/org/example/http/CachingFilterTest.java`:
- Line 33: The calls to FileResolver.resolvePath currently use the old
single-argument form but the method now requires four parameters (String
requestPath, HttpResponseBuilder response, FilterChain chain, File errorFile);
update every call site (e.g., in CachingFilterTest, IpFilterTest,
ConnectionHandler) to supply the missing HttpResponseBuilder, FilterChain and an
appropriate File (or error sentinel) so the call matches
FileResolver.resolvePath(String, HttpResponseBuilder, FilterChain, File), or
alternatively add an overloaded resolvePath(String) wrapper that delegates to
the new signature; ensure you reference the FileResolver.resolvePath method and
the HttpResponseBuilder and FilterChain types when making the change so
compilation succeeds.
---
Duplicate comments:
In `@src/main/java/org/example/FileResolver.java`:
- Around line 31-45: The current string-based traversal check in FileResolver
should be replaced by path canonicalization: construct a candidate Path from
ConfigLoader.get().server().rootDir() and the request-derived path (use
Paths.get(rootDir, path) or equivalent), normalize or resolve it to a
canonical/real path, then verify the canonical candidate startsWith the
canonical root path before returning; if normalization fails or the candidate is
outside the root, set response body to the errorFile contents (falling back to
"Forbidden" on IOException), set
response.setStatusCode(HttpResponseBuilder.SC_FORBIDDEN), and return null. Use
the existing symbols (response, errorFile, HttpResponseBuilder.SC_FORBIDDEN,
ConfigLoader.get().server().rootDir()) and handle IO/security exceptions when
resolving paths.
In `@src/test/java/org/example/http/CachingFilterTest.java`:
- Around line 33-41: In CachingFilterTest the test data is crossed: the
resolvedPath variable and the request path passed to the HttpRequest constructor
are swapped between the two tests (symbols: resolvedPath, HttpRequest
constructor, CachingFilterTest), so update each test so resolvedPath points to
the same file as the request path used in that test (e.g. when testing an
existing resource both request path and resolvedPath should be "/ok.txt"; when
testing a missing resource both should be "/does-not-exist"); locate the two
affected test blocks around the HttpRequest creation and swap the path arguments
so each test validates the intended scenario.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/org/example/ConnectionHandler.javasrc/main/java/org/example/FileResolver.javasrc/main/java/org/example/StaticFileHandler.javasrc/main/java/org/example/http/CachingFilter.javasrc/main/java/org/example/http/HttpCachingHeaders.javasrc/test/java/org/example/http/CachingFilterTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/example/StaticFileHandler.java
- src/main/java/org/example/ConnectionHandler.java
| file.getParentFile().mkdirs(); | ||
| Files.writeString(file.toPath(), "hello"); | ||
|
|
||
| String resolvedPath = FileResolver.resolvePath("/ok.txt"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find FileResolver.resolvePath declarations
echo "=== FileResolver.resolvePath DECLARATIONS ==="
fd -e java FileResolver | xargs grep -n "public.*resolvePath" || echo "No matches found"
# Find all resolvePath call sites
echo -e "\n=== FileResolver.resolvePath CALL SITES ==="
rg -n "FileResolver\.resolvePath\s*\(" --type java -A1
# Check CachingFilterTest.java specifically for line 33 and 58
echo -e "\n=== CachingFilterTest.java context (lines 25-65) ==="
if [ -f "src/test/java/org/example/http/CachingFilterTest.java" ]; then
sed -n '25,65p' src/test/java/org/example/http/CachingFilterTest.java | cat -n
fiRepository: ithsjava25/project-webserver-juv25g
Length of output: 2823
FileResolver.resolvePath calls use incorrect signature—compile blocker affecting multiple test files.
The declared signature requires 4 parameters: resolvePath(String requestPath, HttpResponseBuilder response, FilterChain chain, File errorFile), but all call sites pass only the path string. This affects 5 call sites:
- Lines 33 and 58 in
CachingFilterTest.java - Lines 114 and 140 in
IpFilterTest.java - Line 51 in
ConnectionHandler.java
All must be updated to match the declaration or the method signature must be changed to match the usage pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/example/http/CachingFilterTest.java` at line 33, The calls
to FileResolver.resolvePath currently use the old single-argument form but the
method now requires four parameters (String requestPath, HttpResponseBuilder
response, FilterChain chain, File errorFile); update every call site (e.g., in
CachingFilterTest, IpFilterTest, ConnectionHandler) to supply the missing
HttpResponseBuilder, FilterChain and an appropriate File (or error sentinel) so
the call matches FileResolver.resolvePath(String, HttpResponseBuilder,
FilterChain, File), or alternatively add an overloaded resolvePath(String)
wrapper that delegates to the new signature; ensure you reference the
FileResolver.resolvePath method and the HttpResponseBuilder and FilterChain
types when making the change so compilation succeeds.
…l file path validation in `StaticFileHandler`, and simplify `FileResolver` parameters.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/main/java/org/example/StaticFileHandler.java (1)
64-94:handleGetRequestandhandleare duplicating file-serving logic and drifting.The two paths already differ in status/body root handling. Extract common sanitize/resolve/read/not-found logic into one private helper to prevent further divergence.
🤖 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 64 - 94, handleGetRequest and handle duplicate the sanitize/resolve/read/not-found logic; extract that common behavior into a single private helper (e.g., private Result resolveAndReadStatic(String resolvedPath) or serveStaticFile) inside StaticFileHandler and have both handleGetRequest and handle call it. The helper should: compute canonical root via ConfigLoader.get().server().rootDir(), canonicalize the requested path, return a result containing status code and body bytes, enforce the root containment check (403), read file bytes for existing files (200), and for missing files return 404 with pageNotFound.html fallback if present; then update handleGetRequest and handle to use the helper’s returned status/body and remove the duplicated file-handling code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/FileResolver.java`:
- Around line 27-44: The resolvePath method in FileResolver mixes HTTP response
construction (HttpResponseBuilder/error.html) with path resolution and signals
errors by returning null; remove any creation or modification of
HttpResponseBuilder and file-based error handling from resolvePath and instead
make it return a strict resolution result (e.g., non-null String path,
Optional<String>, or throw a dedicated checked/unchecked exception such as
PathResolutionException) when a forbidden or invalid path is detected (e.g.,
path.contains("..")). Update resolvePath to only compute and validate the
filesystem path using ConfigLoader.get().server().rootDir() and index logic
(index.html), and replace the null return with the chosen explicit error
mechanism; ensure all callers of resolvePath (and any tests) are updated to
handle the new exception or Optional result and perform response construction
(including reading error.html and setting HttpResponseBuilder status) at the
call site.
- Line 33: The code in FileResolver that computes path from requestPath always
strips the first character for non-root paths, corrupting input like
"index.html"; change the logic in the method that contains the String path = ...
line to only remove a leading '/' when one is present (e.g., if
(requestPath.equals("/")) use "index.html", else if
(requestPath.startsWith("/")) use requestPath.substring(1), else use requestPath
as-is) so the code handles both "/index.html" and "index.html" correctly.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 77-81: The success branch in handle(...) sets the internal
statusCode but does not update the HttpResponseBuilder, causing stale response
status; when file.isFile() succeeds (the branch that calls
Files.readAllBytes(...) and response.setBody(...)), also call
response.setStatusCode(SC_OK) so the HttpResponseBuilder reflects the 200 status
(and keep the internal statusCode in sync or remove duplication if appropriate).
- Around line 85-87: StaticFileHandler constructs the 404 errorFile using the
hardcoded WEB_ROOT which can differ from the configured rootDir/root used
elsewhere; change the 404 lookup to build the File from the same configured root
(use the existing rootDir or root variable used earlier in this class instead of
WEB_ROOT) so pageNotFound.html is resolved from the configured document root;
update the reference in the error handling branch where errorFile is created and
read to use that configured root variable and keep the same
Files.readAllBytes(response.setBody(...)) flow.
---
Nitpick comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 64-94: handleGetRequest and handle duplicate the
sanitize/resolve/read/not-found logic; extract that common behavior into a
single private helper (e.g., private Result resolveAndReadStatic(String
resolvedPath) or serveStaticFile) inside StaticFileHandler and have both
handleGetRequest and handle call it. The helper should: compute canonical root
via ConfigLoader.get().server().rootDir(), canonicalize the requested path,
return a result containing status code and body bytes, enforce the root
containment check (403), read file bytes for existing files (200), and for
missing files return 404 with pageNotFound.html fallback if present; then update
handleGetRequest and handle to use the helper’s returned status/body and remove
the duplicated file-handling code.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/example/FileResolver.javasrc/main/java/org/example/StaticFileHandler.javasrc/main/java/org/example/http/CachingFilter.javasrc/test/java/org/example/http/HttpCachingHeadersTest.java
Comment on lines
+27
to
+44
| HttpResponseBuilder response = new HttpResponseBuilder(); | ||
| File errorFile = new File("src/main/resources/error.html"); | ||
|
|
||
| if (requestPath == null || requestPath.isEmpty()) { | ||
| requestPath = "/"; | ||
| } | ||
| String path = requestPath.equals("/") ? "index.html" : requestPath.substring(1); | ||
| String rootDir = ConfigLoader.get().server().rootDir(); | ||
|
|
||
| if (path.contains("..")) { | ||
| try { | ||
| response.setBody(Files.readAllBytes(errorFile.toPath())); | ||
| } catch (IOException e) { | ||
| response.setBody("Forbidden".getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
| response.setStatusCode(HttpResponseBuilder.SC_FORBIDDEN); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
resolvePath mixes concerns and signals errors via null.
On Line 27-28, response/error-page handling is created inside a path resolver, but that response object is never returned. On Line 43, returning null as the error signal makes downstream handling fragile. Keep this method strictly about resolution and use an explicit error contract (e.g., exception or dedicated result type) instead of a nullable path string.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/FileResolver.java` around lines 27 - 44, The
resolvePath method in FileResolver mixes HTTP response construction
(HttpResponseBuilder/error.html) with path resolution and signals errors by
returning null; remove any creation or modification of HttpResponseBuilder and
file-based error handling from resolvePath and instead make it return a strict
resolution result (e.g., non-null String path, Optional<String>, or throw a
dedicated checked/unchecked exception such as PathResolutionException) when a
forbidden or invalid path is detected (e.g., path.contains("..")). Update
resolvePath to only compute and validate the filesystem path using
ConfigLoader.get().server().rootDir() and index logic (index.html), and replace
the null return with the chosen explicit error mechanism; ensure all callers of
resolvePath (and any tests) are updated to handle the new exception or Optional
result and perform response construction (including reading error.html and
setting HttpResponseBuilder status) at the call site.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 87-88: The current StaticFileHandler code checks
errorFile.exists() before reading the 404 page, which can still allow a
directory to be read and cause Files.readAllBytes(...) to throw; update the
check to ensure the target is a regular file (e.g., use errorFile.isFile() or
errorFile.exists() && errorFile.isFile()) before calling Files.readAllBytes, and
if the check fails fall back to a safe default body or leave the 404 response
empty so a directory won't turn a 404 into a 500; references: errorFile,
Files.readAllBytes, response.setBody.
- Around line 77-82: The handler mutates instance fields fileBytes and
statusCode inside StaticFileHandler.handle(...), causing cross-request state;
change these to method-local variables and only write to the provided
HttpResponseBuilder (response.setBody(...) and response.setStatusCode(...)) so
no instance state is modified — locate the block that reads
Files.readAllBytes(file.toPath()) and replace assignments to the fields
fileBytes and statusCode with local vars (e.g., byte[] fileBytesLocal and int
statusCodeLocal) or remove them entirely, keeping only response updates to
ensure the handler remains reusable and thread-safe.
- Around line 77-91: In StaticFileHandler.handle(...), the response Content-Type
is never set for the file/not-found branches; determine the MIME type (e.g. via
Files.probeContentType(file.toPath()) or a similar MIME utility) and call
response.setHeader("Content-Type", mime) before setting the body for the
successful file branch (where file.isFile()) and for the error branches (when
serving errorFile or the plain "404 Not Found" bytes); fall back to
"application/octet-stream" for unknown types and use "text/html" for
pageNotFound.html and "text/plain" for the plain 404 string so strict clients
receive correct MIME headers.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/FileResolver.javasrc/main/java/org/example/StaticFileHandler.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/FileResolver.java
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/example/StaticFileHandler.java (1)
82-82:⚠️ Potential issue | 🟠 MajorRemove shared mutable state write from request-scoped
handle(...).Line 82 writes
statusCode = SC_OKeven though this method already usesHttpResponseBuilder. This reintroduces cross-request shared state risk in a reused handler instance.🔧 Proposed fix
if (file.isFile()) { byte[] fileBytes = Files.readAllBytes(file.toPath()); response.setBody(fileBytes); response.setStatusCode(SC_OK); response.setContentTypeFromFilename(file.getName()); - statusCode = SC_OK; }🤖 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 82, The handler method handle(...) in StaticFileHandler is mutating the shared field statusCode (assignment statusCode = SC_OK) during request processing; remove that write and rely solely on the local HttpResponseBuilder (or a request-scoped variable) to set the response status to avoid cross-request shared mutable state. Locate the assignment to statusCode in the handle(...) method and delete it (or replace usages with responseBuilder.status(SC_OK) / a local int localStatus), ensuring any later code reads status from the request-local response builder or local variable instead of the shared statusCode field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 97-99: The file has an extra closing brace that prematurely closes
the StaticFileHandler class inside the handle(...) method; remove the unmatched
brace so that handle(...) ends correctly and sendGetRequest(...) remains defined
inside the StaticFileHandler class (ensure the method closing brace corresponds
to handle(...) and that the class-level closing brace remains after
sendGetRequest(...)).
---
Duplicate comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Line 82: The handler method handle(...) in StaticFileHandler is mutating the
shared field statusCode (assignment statusCode = SC_OK) during request
processing; remove that write and rely solely on the local HttpResponseBuilder
(or a request-scoped variable) to set the response status to avoid cross-request
shared mutable state. Locate the assignment to statusCode in the handle(...)
method and delete it (or replace usages with responseBuilder.status(SC_OK) / a
local int localStatus), ensuring any later code reads status from the
request-local response builder or local variable instead of the shared
statusCode field.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> # Conflicts: # src/main/java/org/example/StaticFileHandler.java
JohanHiths
force-pushed
the
refactor-file-resolving
branch
from
96620e4 to
737ded2
Compare
…istent path resolution in test `HttpRequest` instances
JohanHiths
force-pushed
the
refactor-file-resolving
branch
2 times, most recently
from
3b579ea to
f4b6798
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/java/org/example/filter/LocaleStatsFilterTest.java (1)
15-18:⚠️ Potential issue | 🟡 MinorMissing
ConfigLoader.loadOnce()in test setup.
FileResolver.resolvePath()internally callsConfigLoader.get(), which throwsIllegalStateExceptionif config hasn't been loaded. This test class is missing the config initialization that other test files have.Tests may pass due to execution order (if another test loads config first), but running this test class in isolation will fail.
🛠️ Proposed fix
+import org.example.config.ConfigLoader; +import java.nio.file.Paths; + class LocaleStatsFilterTest { `@BeforeEach` void resetStats() { + ConfigLoader.loadOnce(Paths.get("src/test/resources/test-config.yml")); LocaleStatsFilter.resetStatsForTests(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/filter/LocaleStatsFilterTest.java` around lines 15 - 18, The test setup in LocaleStatsFilterTest is missing a call to initialize configuration, causing FileResolver.resolvePath (which uses ConfigLoader.get()) to throw when run in isolation; update the test setup (e.g., in the existing resetStats() method or a new `@BeforeAll`) to invoke ConfigLoader.loadOnce() before tests run and then call LocaleStatsFilter.resetStatsForTests(); ensure ConfigLoader.loadOnce() is called once before any FileResolver.resolvePath invocations to prevent IllegalStateException.src/main/java/org/example/ConnectionHandler.java (1)
33-37:⚠️ Potential issue | 🟠 Major
resolvedPathand served file root can diverge whenwebRootis set.With
ConnectionHandler(Socket client, String webRoot), files are served fromwebRoot, butresolvedPathis still computed from config root. This makes request metadata inconsistent with what is actually served (notably for caching/filter logic).Suggested direction
-String resolvedPath = FileResolver.resolvePath(parser.getUri()); +String resolvedPath = (webRoot != null) + ? FileResolver.resolvePath(parser.getUri(), webRoot) + : FileResolver.resolvePath(parser.getUri());Also prefer a single path source-of-truth for both request metadata and static file serving.
Also applies to: 67-68, 74-75
🤖 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 33 - 37, The constructor ConnectionHandler(Socket client, String webRoot) currently sets webRoot but leaves resolvedPath computed from the global/config root, causing metadata mismatch; update the code so the same source-of-truth path (the webRoot passed into the constructor) is used when computing resolvedPath and anywhere static files are served or filters are built (e.g., in buildFilters(), any method that references resolvedPath, and the request handling logic that reads files), making sure resolvedPath is initialized from this.webRoot rather than the config root and propagated to the caching/filter logic so both served content and request metadata match.
♻️ Duplicate comments (1)
src/main/java/org/example/StaticFileHandler.java (1)
77-83:⚠️ Potential issue | 🟠 MajorRemove the leftover shared-state write in
handle(...).Line 82 mutates the instance field
statusCodeeven though this method already writes directly toresponse. That keeps cross-request mutable state in a path that should be request-local.🔧 Proposed fix
if (file.isFile()) { byte[] fileBytes = Files.readAllBytes(file.toPath()); response.setBody(fileBytes); response.setStatusCode(SC_OK); response.setContentTypeFromFilename(file.getName()); - statusCode = SC_OK; }🤖 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 77 - 83, The method handle(...) in StaticFileHandler writes the HTTP response directly to the provided response object but also mutates the instance field statusCode (the line setting statusCode = SC_OK), leaving shared mutable state across requests; remove that mutation and rely solely on response.setStatusCode(...) so handle(...) remains request-local—search for method handle(...) and the field statusCode in StaticFileHandler and delete the assignment to statusCode while keeping response.setStatusCode(SC_OK) and response.setBody(...) intact.
🧹 Nitpick comments (6)
src/test/java/org/example/server/ConfigurableFilterPipelineTest.java (2)
9-9: Unused import.
BeforeAllis imported but not used in this class.♻️ Remove unused import
-import org.junit.jupiter.api.BeforeAll;🤖 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` at line 9, Remove the unused JUnit import: delete the import statement for BeforeAll (import org.junit.jupiter.api.BeforeAll;) from ConfigurableFilterPipelineTest.java since the class does not reference the BeforeAll annotation or method; ensure no other code depends on BeforeAll before committing.
304-307: RedundantFileResolver.resolvePath()call and potential path mismatch.The helper calls
resolvePathtwice, discarding the first result. Additionally, theresolvedPathis always computed for"/whatever"regardless of thepathparameter passed tonewRequest().If tests rely on
request.getResolvedPath()matching the actual path, this could cause subtle bugs.♻️ Proposed fix
private static HttpRequest newRequest(String path) { - FileResolver.resolvePath("/whatever"); - return new HttpRequest("GET", path, "HTTP/1.1", Map.of(), "", FileResolver.resolvePath("/whatever")); + String resolvedPath = FileResolver.resolvePath(path); + return new HttpRequest("GET", path, "HTTP/1.1", Map.of(), "", resolvedPath); }🤖 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 304 - 307, The helper newRequest currently calls FileResolver.resolvePath("/whatever") twice and always resolves "/whatever" instead of the supplied path, which is redundant and can cause mismatched resolved paths; change newRequest(String path) to compute a single resolvedPath = FileResolver.resolvePath(path) (remove the unused first call) and pass that resolvedPath into the new HttpRequest(...) constructor so request.getResolvedPath() reflects the provided path (refer to newRequest, FileResolver.resolvePath, and the HttpRequest constructor).src/test/java/org/example/filter/LocaleStatsFilterTest.java (1)
22-29: RedundantFileResolver.resolvePath()call.Same pattern as other files: line 22 calls
resolvePathbut discards the result, then line 29 calls it again. Store the result in a variable and reuse it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/filter/LocaleStatsFilterTest.java` around lines 22 - 29, The test makes two calls to FileResolver.resolvePath() and discards the first; instead call FileResolver.resolvePath("/whatever") once, assign it to a local variable (e.g., resolvedPath) and pass that variable into the HttpRequest constructor (replace the duplicate call), updating any references in LocaleStatsFilterTest to use resolvedPath.src/test/java/org/example/filter/CompressionFilterTest.java (1)
47-56: RedundantFileResolver.resolvePath()calls throughout test class.The pattern of calling
resolvePathand discarding the result, then calling it again for theHttpRequestconstructor, appears throughout this file (lines 47/55, 82/89, 110/112, 151/153, 174/176, 197/199, 219/224, 248/253, 271/274, 293/299/310).Consider extracting a helper method similar to
ConfigurableFilterPipelineTest.newRequest()or store the result in a variable.♻️ Example fix for one test
`@Test` void testGzipCompressionWhenClientSupportsIt() throws Exception { Map<String, String> headers = new HashMap<>(); headers.put("Accept-Encoding", "gzip, deflate"); - FileResolver.resolvePath("/whatever"); + String resolvedPath = FileResolver.resolvePath("/whatever"); HttpRequest request = new HttpRequest( "GET", "/", "HTTP/1.1", headers, null, - FileResolver.resolvePath("/whatever") + resolvedPath );Or create a helper method:
private HttpRequest createRequest(String path, Map<String, String> headers) { String resolvedPath = FileResolver.resolvePath(path); return new HttpRequest("GET", path, "HTTP/1.1", headers, null, resolvedPath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/filter/CompressionFilterTest.java` around lines 47 - 56, Tests repeatedly call FileResolver.resolvePath(path) and discard the result before calling it again for the HttpRequest constructor; consolidate by storing the resolved path in a local variable or adding a helper like createRequest(String path, Map<String,String> headers) that calls FileResolver.resolvePath(path) once and constructs the HttpRequest("GET", path, "HTTP/1.1", headers, null, resolvedPath); update tests to use the helper or the local resolvedPath variable and remove the redundant resolvePath() calls (see FileResolver.resolvePath, HttpRequest, and consider mirroring ConfigurableFilterPipelineTest.newRequest).src/test/java/org/example/filter/LocaleFilterWithCookieTest.java (1)
27-35: RedundantFileResolver.resolvePath()call.Line 28 calls
FileResolver.resolvePath("/whatever")but discards the result. The same call is made again on line 35 to pass toHttpRequest. Remove the unused call.This pattern repeats in other tests in this file (lines 47, 66, 85) and is inconsistently applied (missing in
testCookieWithWhitespaceOnly).♻️ Proposed fix
`@Test` void testDefaultLocaleWhenNoHeaderOrCookie() { - - FileResolver.resolvePath("/whatever"); + String resolvedPath = FileResolver.resolvePath("/whatever"); HttpRequest request = new HttpRequest( "GET", "/", "HTTP/1.1", Map.of(), null, - FileResolver.resolvePath("/whatever") + resolvedPath );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/filter/LocaleFilterWithCookieTest.java` around lines 27 - 35, Remove the redundant FileResolver.resolvePath("/whatever") call that discards its result and instead pass a single resolvePath result into the HttpRequest constructor; update the other occurrences in LocaleFilterWithCookieTest (the repeated calls at the same pattern and the inconsistent case in testCookieWithWhitespaceOnly) so each test calls FileResolver.resolvePath(...) only once and uses that returned value as the final argument to the HttpRequest(...) call, ensuring consistency across tests.src/main/java/org/example/StaticFileHandler.java (1)
64-95: Consolidate to one file-serving code path.
handle(...)now duplicates behavior that already exists inhandleGetRequest(...)/sendGetRequest(...). Keeping both implementations invites divergence (root resolution, headers, and error-body behavior) and weakens the refactor goal.♻️ Refactor direction
- private void handleGetRequest(String uri) throws IOException { - ... - } + // Extract shared file resolution/serving logic into one private method + // used by both call paths, or migrate sendGetRequest(...) to use handle(...).🤖 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 64 - 95, The handle(...) method in StaticFileHandler duplicates file-serving logic already implemented in handleGetRequest(...) / sendGetRequest(...), risking divergence in root resolution, headers, and error-body behavior; refactor handle(...) to delegate to the existing handlers by calling handleGetRequest(request, response, chain) or sendGetRequest(...) as appropriate (preserving any necessary pre-checks like path canonicalization and security check that uses root resolution), remove the duplicated file read/response code (the block that reads fileBytes, sets status/content-type, and the 404 fallback with pageNotFound.html), and ensure any shared state (e.g., statusCode) is updated consistently by the delegated methods so there is a single source of truth for file serving behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 67-68: Check whether FileResolver.resolvePath(parser.getUri())
returned null and, if so, stop further request processing and return an
appropriate error/forbidden response; specifically, after the call that assigns
resolvedPath replace continuing logic with a null-check that rejects the request
(e.g., send 403/404 or close the connection) instead of falling back to the
unsanitized parser.getUri(). Apply the same null-check and early-return behavior
for the other occurrence that uses resolvedPath (the block around lines
referencing resolvedPath at 93-95) so no file-serving logic runs with a null
resolvedPath.
---
Outside diff comments:
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 33-37: The constructor ConnectionHandler(Socket client, String
webRoot) currently sets webRoot but leaves resolvedPath computed from the
global/config root, causing metadata mismatch; update the code so the same
source-of-truth path (the webRoot passed into the constructor) is used when
computing resolvedPath and anywhere static files are served or filters are built
(e.g., in buildFilters(), any method that references resolvedPath, and the
request handling logic that reads files), making sure resolvedPath is
initialized from this.webRoot rather than the config root and propagated to the
caching/filter logic so both served content and request metadata match.
In `@src/test/java/org/example/filter/LocaleStatsFilterTest.java`:
- Around line 15-18: The test setup in LocaleStatsFilterTest is missing a call
to initialize configuration, causing FileResolver.resolvePath (which uses
ConfigLoader.get()) to throw when run in isolation; update the test setup (e.g.,
in the existing resetStats() method or a new `@BeforeAll`) to invoke
ConfigLoader.loadOnce() before tests run and then call
LocaleStatsFilter.resetStatsForTests(); ensure ConfigLoader.loadOnce() is called
once before any FileResolver.resolvePath invocations to prevent
IllegalStateException.
---
Duplicate comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 77-83: The method handle(...) in StaticFileHandler writes the HTTP
response directly to the provided response object but also mutates the instance
field statusCode (the line setting statusCode = SC_OK), leaving shared mutable
state across requests; remove that mutation and rely solely on
response.setStatusCode(...) so handle(...) remains request-local—search for
method handle(...) and the field statusCode in StaticFileHandler and delete the
assignment to statusCode while keeping response.setStatusCode(SC_OK) and
response.setBody(...) intact.
---
Nitpick comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 64-95: The handle(...) method in StaticFileHandler duplicates
file-serving logic already implemented in handleGetRequest(...) /
sendGetRequest(...), risking divergence in root resolution, headers, and
error-body behavior; refactor handle(...) to delegate to the existing handlers
by calling handleGetRequest(request, response, chain) or sendGetRequest(...) as
appropriate (preserving any necessary pre-checks like path canonicalization and
security check that uses root resolution), remove the duplicated file
read/response code (the block that reads fileBytes, sets status/content-type,
and the 404 fallback with pageNotFound.html), and ensure any shared state (e.g.,
statusCode) is updated consistently by the delegated methods so there is a
single source of truth for file serving behavior.
In `@src/test/java/org/example/filter/CompressionFilterTest.java`:
- Around line 47-56: Tests repeatedly call FileResolver.resolvePath(path) and
discard the result before calling it again for the HttpRequest constructor;
consolidate by storing the resolved path in a local variable or adding a helper
like createRequest(String path, Map<String,String> headers) that calls
FileResolver.resolvePath(path) once and constructs the HttpRequest("GET", path,
"HTTP/1.1", headers, null, resolvedPath); update tests to use the helper or the
local resolvedPath variable and remove the redundant resolvePath() calls (see
FileResolver.resolvePath, HttpRequest, and consider mirroring
ConfigurableFilterPipelineTest.newRequest).
In `@src/test/java/org/example/filter/LocaleFilterWithCookieTest.java`:
- Around line 27-35: Remove the redundant FileResolver.resolvePath("/whatever")
call that discards its result and instead pass a single resolvePath result into
the HttpRequest constructor; update the other occurrences in
LocaleFilterWithCookieTest (the repeated calls at the same pattern and the
inconsistent case in testCookieWithWhitespaceOnly) so each test calls
FileResolver.resolvePath(...) only once and uses that returned value as the
final argument to the HttpRequest(...) call, ensuring consistency across tests.
In `@src/test/java/org/example/filter/LocaleStatsFilterTest.java`:
- Around line 22-29: The test makes two calls to FileResolver.resolvePath() and
discards the first; instead call FileResolver.resolvePath("/whatever") once,
assign it to a local variable (e.g., resolvedPath) and pass that variable into
the HttpRequest constructor (replace the duplicate call), updating any
references in LocaleStatsFilterTest to use resolvedPath.
In `@src/test/java/org/example/server/ConfigurableFilterPipelineTest.java`:
- Line 9: Remove the unused JUnit import: delete the import statement for
BeforeAll (import org.junit.jupiter.api.BeforeAll;) from
ConfigurableFilterPipelineTest.java since the class does not reference the
BeforeAll annotation or method; ensure no other code depends on BeforeAll before
committing.
- Around line 304-307: The helper newRequest currently calls
FileResolver.resolvePath("/whatever") twice and always resolves "/whatever"
instead of the supplied path, which is redundant and can cause mismatched
resolved paths; change newRequest(String path) to compute a single resolvedPath
= FileResolver.resolvePath(path) (remove the unused first call) and pass that
resolvedPath into the new HttpRequest(...) constructor so
request.getResolvedPath() reflects the provided path (refer to newRequest,
FileResolver.resolvePath, and the HttpRequest constructor).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/org/example/ConnectionHandler.javasrc/main/java/org/example/StaticFileHandler.javasrc/main/java/org/example/http/HttpResponseBuilder.javasrc/test/java/org/example/filter/CompressionFilterTest.javasrc/test/java/org/example/filter/LocaleFilterTest.javasrc/test/java/org/example/filter/LocaleFilterWithCookieTest.javasrc/test/java/org/example/filter/LocaleStatsFilterTest.javasrc/test/java/org/example/filter/RequestTimeOutFilterTest.javasrc/test/java/org/example/server/ConfigurableFilterPipelineTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/org/example/filter/LocaleFilterTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/http/HttpResponseBuilder.java
Comment on lines
+67
to
+68
| String resolvedPath = FileResolver.resolvePath(parser.getUri()); | ||
|
|
There was a problem hiding this comment.
Block request processing when resolvedPath is null.
FileResolver.resolvePath(...) can return null for forbidden paths, but execution continues and the unsanitized URI is still used for file serving. This can bypass the intended traversal guard.
Suggested fix
String resolvedPath = FileResolver.resolvePath(parser.getUri());
+if (resolvedPath == null) {
+ HttpResponseBuilder forbidden = new HttpResponseBuilder();
+ forbidden.setStatusCode(HttpResponseBuilder.SC_FORBIDDEN);
+ client.getOutputStream().write(forbidden.build());
+ client.getOutputStream().flush();
+ return;
+}Also applies to: 93-95
🤖 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 67 - 68, Check
whether FileResolver.resolvePath(parser.getUri()) returned null and, if so, stop
further request processing and return an appropriate error/forbidden response;
specifically, after the call that assigns resolvedPath replace continuing logic
with a null-check that rejects the request (e.g., send 403/404 or close the
connection) instead of falling back to the unsanitized parser.getUri(). Apply
the same null-check and early-return behavior for the other occurrence that uses
resolvedPath (the block around lines referencing resolvedPath at 93-95) so no
file-serving logic runs with a null resolvedPath.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/test/java/org/example/filter/LocaleStatsFilterTest.java (1)
15-18:⚠️ Potential issue | 🟡 MinorMissing
ConfigLoader.loadOnce()initialization.Like
LocaleFilterTest, this test file usesFileResolver.resolvePath()without initializingConfigLoaderfirst. Add a@BeforeAllsetup to load the test configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/filter/LocaleStatsFilterTest.java` around lines 15 - 18, Add a `@BeforeAll` setup to LocaleStatsFilterTest that calls ConfigLoader.loadOnce() before any tests run; currently resetStats() only resets LocaleStatsFilter via LocaleStatsFilter.resetStatsForTests() but the tests call FileResolver.resolvePath() and need ConfigLoader initialized first, so add a static method annotated with `@BeforeAll` that invokes ConfigLoader.loadOnce() to ensure configuration is loaded for the tests.src/main/java/org/example/StaticFileHandler.java (2)
142-154:⚠️ Potential issue | 🔴 CriticalCritical:
sendGetRequestmethod is malformed and incomplete.Multiple issues:
- Missing import for
OutputStreamhandleGetRequest(uri)called with wrong signature (expectsHttpRequest, HttpResponseBuilder)statusCodeandfileBytesare undefined variables- Missing closing brace - the method doesn't end before line 151's
@OverrideThis method appears to be leftover from an old implementation and should either be removed or completely rewritten.
🐛 Proposed fix: Remove broken method
- public void sendGetRequest(OutputStream outputStream, String uri) throws IOException { - handleGetRequest(uri); - HttpResponseBuilder response = new HttpResponseBuilder(); - response.setStatusCode(statusCode); - // Use MimeTypeDetector instead of hardcoded text/html - response.setContentTypeFromFilename(uri); - response.setBody(fileBytes); - outputStream.write(response.build()); - outputStream.flush(); `@Override` public void handle(HttpRequest request, HttpResponseBuilder response) {🤖 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 142 - 154, The sendGetRequest method is broken and should be removed: delete the entire sendGetRequest(OutputStream, String) method (including any malformed braces) so only the correct handle(HttpRequest, HttpResponseBuilder) / handleGetRequest(HttpRequest, HttpResponseBuilder) flow remains; also remove any now-unused imports (e.g., java.io.OutputStream) and references to undefined symbols statusCode and fileBytes introduced by that method to keep the class compile-clean.
9-15:⚠️ Potential issue | 🟡 MinorDuplicate static import.
HttpResponseBuilder.*is imported twice (lines 9 and 15).-import static org.example.http.HttpResponseBuilder.*; - import java.io.File; import java.io.IOException; import java.nio.file.Files; - -import static org.example.http.HttpResponseBuilder.*; +import static org.example.http.HttpResponseBuilder.*;🤖 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 9 - 15, Remove the duplicate static import of HttpResponseBuilder.* in the StaticFileHandler class: keep a single "import static org.example.http.HttpResponseBuilder.*;" and delete the redundant one so there is only one static import for HttpResponseBuilder in StaticFileHandler.java; this will eliminate the duplicate import warning while preserving access to the HttpResponseBuilder static members used in the class.
♻️ Duplicate comments (2)
src/main/java/org/example/ConnectionHandler.java (1)
67-77:⚠️ Potential issue | 🔴 CriticalCritical:
resolvedPathnull check still missing.
FileResolver.resolvePath()returnsnullfor path traversal attempts (when path contains ".."), but the code proceeds to construct and process the request regardless. The 403 response built insideFileResolveris never sent to the client.🐛 Proposed fix
String resolvedPath = FileResolver.resolvePath(parser.getUri()); + if (resolvedPath == null) { + HttpResponseBuilder forbidden = new HttpResponseBuilder(); + forbidden.setStatusCode(HttpResponseBuilder.SC_FORBIDDEN); + forbidden.setBody("403 Forbidden"); + client.getOutputStream().write(forbidden.build()); + client.getOutputStream().flush(); + return; + } HttpRequest request = new HttpRequest(,
🤖 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 67 - 77, After calling FileResolver.resolvePath(parser.getUri()) in ConnectionHandler, add a null check for resolvedPath and, if null, immediately send the 403 response to the client and close the connection instead of constructing an HttpRequest; do not proceed to new HttpRequest(...). Use the 403 response built by FileResolver (or construct an equivalent HTTP 403 response) and send it via the existing response-sending method used elsewhere in ConnectionHandler, then return early so no further request processing occurs.src/main/java/org/example/StaticFileHandler.java (1)
119-125:⚠️ Potential issue | 🔴 CriticalUndefined variable
statusCodeused.Line 124 assigns to
statusCodewhich is neither a field nor a local variable. This was likely part of the old implementation but was removed.🐛 Proposed fix
if (file.isFile()) { byte[] fileBytes = Files.readAllBytes(file.toPath()); response.setBody(fileBytes); response.setStatusCode(SC_OK); response.setContentTypeFromFilename(file.getName()); - statusCode = SC_OK; }🤖 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 119 - 125, The assignment to undefined variable statusCode in the file-serving branch must be removed or replaced; update the block handling file.isFile() so it does not reference statusCode (it's not declared) and rely on response.setStatusCode(SC_OK) instead — remove the line "statusCode = SC_OK" (or declare and use a proper local/field if you truly need a separate status tracking variable) and ensure response.setBody(...), response.setStatusCode(SC_OK) and response.setContentTypeFromFilename(...) remain intact.
🧹 Nitpick comments (6)
src/test/java/org/example/server/ConfigurableFilterPipelineTest.java (1)
304-307: Remove redundantFileResolver.resolvePathcall on line 305.Line 305 calls
FileResolver.resolvePath("/whatever")but discards the result. The same call is made again on line 306 where the result is actually used. This is leftover code that should be removed.♻️ Proposed fix
private static HttpRequest newRequest(String path) { - FileResolver.resolvePath("/whatever"); return new HttpRequest("GET", path, "HTTP/1.1", Map.of(), "", FileResolver.resolvePath("/whatever")); }🤖 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 304 - 307, In newRequest, remove the redundant call to FileResolver.resolvePath("/whatever") that discards its return value; keep only the single call used as the fourth argument in the HttpRequest constructor within the newRequest method so FileResolver.resolvePath is invoked exactly once when constructing the HttpRequest.src/main/java/org/example/http/HttpResponseBuilder.java (1)
112-114:addHeader()duplicates existingsetHeader()method.Both methods have identical implementations (
this.headers.put(key, value)). This creates redundancy and potential confusion about which method to use.Consider removing
addHeader()and usingsetHeader()consistently, or differentiate their behavior (e.g.,addHeadercould append to multi-value headers whilesetHeaderoverwrites).♻️ Proposed fix: Remove duplicate method
- - public void addHeader(String key, String value){ - this.headers.put(key, value); - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/http/HttpResponseBuilder.java` around lines 112 - 114, The addHeader() method in HttpResponseBuilder duplicates setHeader() (both call this.headers.put(key, value)); remove addHeader() to avoid redundancy and update any callers to use setHeader() instead, or if multi-value support is desired, change addHeader() to append to the existing entry using the headers field (e.g., concatenate or use a list) while leaving setHeader() to overwrite — adjust usages of addHeader() accordingly and run tests to ensure no regressions.src/test/java/org/example/filter/LocaleFilterTest.java (1)
22-30: RedundantFileResolver.resolvePath()call.Line 22 calls
FileResolver.resolvePath("/whatever")but discards the result. Only the call at line 29 is used. This pattern repeats in all 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/filter/LocaleFilterTest.java` around lines 22 - 30, In LocaleFilterTest remove the redundant standalone calls to FileResolver.resolvePath("/whatever") that discard their return value (the call at the top of each test) and keep only the call used as the HttpRequest constructor argument; update each test method so FileResolver.resolvePath is invoked once when building the HttpRequest (used in the HttpRequest(...) call) and delete the extraneous invocation(s) to eliminate the no-op side-effect.src/test/java/org/example/filter/CompressionFilterTest.java (2)
9-9: Unused importBeforeEach.
BeforeEachis imported but no@BeforeEachannotated method exists in this test class.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/filter/CompressionFilterTest.java` at line 9, The import org.junit.jupiter.api.BeforeEach in CompressionFilterTest is unused; remove the BeforeEach import from the test class to eliminate the unused import warning (or if setup logic is needed, add an `@BeforeEach-annotated` method named e.g. setUp() instead and implement setup there), but preferred fix is to delete the unused BeforeEach import.
47-56: RedundantFileResolver.resolvePath()calls.The standalone call at line 47 discards its result; only the second call at line 55 is actually used. This pattern appears throughout the file.
♻️ Proposed fix
- FileResolver.resolvePath("/whatever"); - HttpRequest request = new HttpRequest( "GET", "/", "HTTP/1.1", headers, null, FileResolver.resolvePath("/whatever") );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/filter/CompressionFilterTest.java` around lines 47 - 56, The test contains redundant FileResolver.resolvePath() invocations (e.g., a standalone call before constructing the HttpRequest) that discard their result; in CompressionFilterTest replace the redundant calls by calling FileResolver.resolvePath(...) once, assign its result to a local variable (e.g., resolvedPath) and pass that variable into the HttpRequest constructor and any other places that currently call resolvePath again; remove the unused standalone calls throughout the file to eliminate duplication and potential side-effect confusion.src/test/java/org/example/filter/LocaleFilterWithCookieTest.java (1)
27-36: RedundantFileResolver.resolvePath()calls in tests.Same pattern as other test files: line 28 discards the result while line 35 uses it. Consider removing the standalone calls throughout 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/filter/LocaleFilterWithCookieTest.java` around lines 27 - 36, Remove the redundant standalone FileResolver.resolvePath() call used only for side effects in LocaleFilterWithCookieTest; instead, use the resolved path only where it is needed when constructing the HttpRequest. Update the test by deleting the extra FileResolver.resolvePath("/whatever") invocation (the one not assigned or passed) so the only call to FileResolver.resolvePath is the argument passed into the HttpRequest constructor, leaving the HttpRequest creation and surrounding assertions (in tests referencing LocaleFilterWithCookieTest and its use of FileResolver.resolvePath) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 57-65: Remove the orphaned git-conflict/branch marker block inside
StaticFileHandler that references "refactor-file-resolving" and the incomplete
code using WEB_ROOT, fileBytes, statusCode and SC_FORBIDDEN; delete the entire
stray block (lines containing the branch name and the File root/file
canonicalization snippet) so only the valid file-resolution logic remains, and
ensure no remaining references to WEB_ROOT or statusCode are left dangling in
StaticFileHandler after removal.
In `@src/test/java/org/example/filter/LocaleFilterTest.java`:
- Around line 15-18: The test setup in LocaleFilterTest is missing
initialization of the configuration loader; update the `@BeforeEach` setup method
to call ConfigLoader.loadOnce() before invoking
LocaleStatsFilter.resetStatsForTests() so FileResolver.resolvePath() (which uses
ConfigLoader.get().server().rootDir()) has a properly initialized ConfigLoader;
ensure you reference ConfigLoader.loadOnce() and
LocaleStatsFilter.resetStatsForTests() in the change.
---
Outside diff comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 142-154: The sendGetRequest method is broken and should be
removed: delete the entire sendGetRequest(OutputStream, String) method
(including any malformed braces) so only the correct handle(HttpRequest,
HttpResponseBuilder) / handleGetRequest(HttpRequest, HttpResponseBuilder) flow
remains; also remove any now-unused imports (e.g., java.io.OutputStream) and
references to undefined symbols statusCode and fileBytes introduced by that
method to keep the class compile-clean.
- Around line 9-15: Remove the duplicate static import of HttpResponseBuilder.*
in the StaticFileHandler class: keep a single "import static
org.example.http.HttpResponseBuilder.*;" and delete the redundant one so there
is only one static import for HttpResponseBuilder in StaticFileHandler.java;
this will eliminate the duplicate import warning while preserving access to the
HttpResponseBuilder static members used in the class.
In `@src/test/java/org/example/filter/LocaleStatsFilterTest.java`:
- Around line 15-18: Add a `@BeforeAll` setup to LocaleStatsFilterTest that calls
ConfigLoader.loadOnce() before any tests run; currently resetStats() only resets
LocaleStatsFilter via LocaleStatsFilter.resetStatsForTests() but the tests call
FileResolver.resolvePath() and need ConfigLoader initialized first, so add a
static method annotated with `@BeforeAll` that invokes ConfigLoader.loadOnce() to
ensure configuration is loaded for the tests.
---
Duplicate comments:
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 67-77: After calling FileResolver.resolvePath(parser.getUri()) in
ConnectionHandler, add a null check for resolvedPath and, if null, immediately
send the 403 response to the client and close the connection instead of
constructing an HttpRequest; do not proceed to new HttpRequest(...). Use the 403
response built by FileResolver (or construct an equivalent HTTP 403 response)
and send it via the existing response-sending method used elsewhere in
ConnectionHandler, then return early so no further request processing occurs.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 119-125: The assignment to undefined variable statusCode in the
file-serving branch must be removed or replaced; update the block handling
file.isFile() so it does not reference statusCode (it's not declared) and rely
on response.setStatusCode(SC_OK) instead — remove the line "statusCode = SC_OK"
(or declare and use a proper local/field if you truly need a separate status
tracking variable) and ensure response.setBody(...),
response.setStatusCode(SC_OK) and response.setContentTypeFromFilename(...)
remain intact.
---
Nitpick comments:
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 112-114: The addHeader() method in HttpResponseBuilder duplicates
setHeader() (both call this.headers.put(key, value)); remove addHeader() to
avoid redundancy and update any callers to use setHeader() instead, or if
multi-value support is desired, change addHeader() to append to the existing
entry using the headers field (e.g., concatenate or use a list) while leaving
setHeader() to overwrite — adjust usages of addHeader() accordingly and run
tests to ensure no regressions.
In `@src/test/java/org/example/filter/CompressionFilterTest.java`:
- Line 9: The import org.junit.jupiter.api.BeforeEach in CompressionFilterTest
is unused; remove the BeforeEach import from the test class to eliminate the
unused import warning (or if setup logic is needed, add an `@BeforeEach-annotated`
method named e.g. setUp() instead and implement setup there), but preferred fix
is to delete the unused BeforeEach import.
- Around line 47-56: The test contains redundant FileResolver.resolvePath()
invocations (e.g., a standalone call before constructing the HttpRequest) that
discard their result; in CompressionFilterTest replace the redundant calls by
calling FileResolver.resolvePath(...) once, assign its result to a local
variable (e.g., resolvedPath) and pass that variable into the HttpRequest
constructor and any other places that currently call resolvePath again; remove
the unused standalone calls throughout the file to eliminate duplication and
potential side-effect confusion.
In `@src/test/java/org/example/filter/LocaleFilterTest.java`:
- Around line 22-30: In LocaleFilterTest remove the redundant standalone calls
to FileResolver.resolvePath("/whatever") that discard their return value (the
call at the top of each test) and keep only the call used as the HttpRequest
constructor argument; update each test method so FileResolver.resolvePath is
invoked once when building the HttpRequest (used in the HttpRequest(...) call)
and delete the extraneous invocation(s) to eliminate the no-op side-effect.
In `@src/test/java/org/example/filter/LocaleFilterWithCookieTest.java`:
- Around line 27-36: Remove the redundant standalone FileResolver.resolvePath()
call used only for side effects in LocaleFilterWithCookieTest; instead, use the
resolved path only where it is needed when constructing the HttpRequest. Update
the test by deleting the extra FileResolver.resolvePath("/whatever") invocation
(the one not assigned or passed) so the only call to FileResolver.resolvePath is
the argument passed into the HttpRequest constructor, leaving the HttpRequest
creation and surrounding assertions (in tests referencing
LocaleFilterWithCookieTest and its use of FileResolver.resolvePath) unchanged.
In `@src/test/java/org/example/server/ConfigurableFilterPipelineTest.java`:
- Around line 304-307: In newRequest, remove the redundant call to
FileResolver.resolvePath("/whatever") that discards its return value; keep only
the single call used as the fourth argument in the HttpRequest constructor
within the newRequest method so FileResolver.resolvePath is invoked exactly once
when constructing the HttpRequest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9492dd1-fcef-47fe-945f-d41fe66f5897
📒 Files selected for processing (10)
src/main/java/org/example/ConnectionHandler.javasrc/main/java/org/example/StaticFileHandler.javasrc/main/java/org/example/http/HttpResponseBuilder.javasrc/main/java/org/example/httpparser/HttpRequest.javasrc/test/java/org/example/filter/CompressionFilterTest.javasrc/test/java/org/example/filter/LocaleFilterTest.javasrc/test/java/org/example/filter/LocaleFilterWithCookieTest.javasrc/test/java/org/example/filter/LocaleStatsFilterTest.javasrc/test/java/org/example/filter/RequestTimeOutFilterTest.javasrc/test/java/org/example/server/ConfigurableFilterPipelineTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/httpparser/HttpRequest.java
Comment on lines
+15
to
+18
| @BeforeEach | ||
| void resetStats() { | ||
| LocaleStatsFilter.resetStatsForTests(); | ||
| } |
There was a problem hiding this comment.
Missing ConfigLoader.loadOnce() initialization.
Other filter tests (e.g., RequestTimeOutFilterTest, LocaleFilterWithCookieTest) call ConfigLoader.loadOnce() in setup. FileResolver.resolvePath() reads from ConfigLoader.get().server().rootDir(), which may fail or return unexpected results without proper initialization.
🛠️ Proposed fix
+ `@BeforeAll`
+ static void initConfig() {
+ ConfigLoader.loadOnce(Paths.get("src/test/resources/test-config.yml"));
+ }
+
`@BeforeEach`
void resetStats() {
LocaleStatsFilter.resetStatsForTests();
}📝 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
| @BeforeEach | |
| void resetStats() { | |
| LocaleStatsFilter.resetStatsForTests(); | |
| } | |
| `@BeforeAll` | |
| static void initConfig() { | |
| ConfigLoader.loadOnce(Paths.get("src/test/resources/test-config.yml")); | |
| } | |
| `@BeforeEach` | |
| void resetStats() { | |
| LocaleStatsFilter.resetStatsForTests(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/example/filter/LocaleFilterTest.java` around lines 15 - 18,
The test setup in LocaleFilterTest is missing initialization of the
configuration loader; update the `@BeforeEach` setup method to call
ConfigLoader.loadOnce() before invoking LocaleStatsFilter.resetStatsForTests()
so FileResolver.resolvePath() (which uses ConfigLoader.get().server().rootDir())
has a properly initialized ConfigLoader; ensure you reference
ConfigLoader.loadOnce() and LocaleStatsFilter.resetStatsForTests() in the
change.
…tion and update locale statistics validation logic, ensuring proper lowercase comparison. # Conflicts: # src/test/java/org/example/filter/CompressionFilterTest.java # src/test/java/org/example/filter/LocaleFilterTest.java # src/test/java/org/example/filter/LocaleStatsFilterTest.java
JohanHiths
force-pushed
the
refactor-file-resolving
branch
from
70c44b8 to
12b48b3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolving duplication in CachingFilter, so CachingFilter and StaticFileHandler have correct responsibilities and don't interfere with each other! No duplicate logic
Summary by CodeRabbit
New Features
Bug Fixes
Tests