Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds HttpCachingHeaders for assembling HTTP caching headers, a CachingFilter that enforces path safety, computes ETags from file last-modified, handles If-None-Match/If-Modified-Since with 304/403/404 logic, exposes response headers via HttpResponseBuilder.getHeaders(), and unit tests verifying header behavior and defensive copying. Changes
sequenceDiagram
participant Client
participant CachingFilter
participant FileSystem
participant FilterChain
participant HttpCachingHeaders
participant ResponseBuilder
Client->>CachingFilter: GET /www/path (with optional If-None-Match/If-Modified-Since)
CachingFilter->>FileSystem: resolve path & stat file
alt path traversal detected
CachingFilter->>ResponseBuilder: setStatus(403)
ResponseBuilder-->>Client: 403
else file missing
CachingFilter->>ResponseBuilder: setStatus(404)
ResponseBuilder-->>Client: 404
else file exists
CachingFilter->>FileSystem: read lastModified
CachingFilter->>CachingFilter: generate ETag (from lastModified)
CachingFilter->>CachingFilter: compare incoming headers to ETag/Last-Modified
alt resource unchanged
CachingFilter->>HttpCachingHeaders: set ETag, Last-Modified, Cache-Control
HttpCachingHeaders->>ResponseBuilder: applyTo(response.getHeaders())
ResponseBuilder->>Client: 304 (no body)
else resource fresh
CachingFilter->>FilterChain: chain.doFilter(request, response)
FilterChain->>ResponseBuilder: produce body
CachingFilter->>HttpCachingHeaders: set ETag, Last-Modified, Cache-Control
HttpCachingHeaders->>ResponseBuilder: applyTo(response.getHeaders())
ResponseBuilder->>Client: 200 with body
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (5)
src/test/java/org/example/http/HttpCachingHeadersTest.java (2)
30-38: Test method naming and misleading variable name.Two issues:
CacheControlReturnsCorrectValuestarts with an uppercase letter — Java convention (and JUnit best practice) is to start method names with a lowercase letter.- The variable on line 35 is named
etagValuebut it holds aCache-Controlvalue. Same issue on line 47. Rename tocacheControlValuefor clarity.🤖 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 30 - 38, Rename the test method CacheControlReturnsCorrectValue to start with a lowercase letter (e.g., cacheControlReturnsCorrectValue) and rename the local variable etagValue to cacheControlValue in HttpCachingHeadersTest where you retrieve getHeaders().get("Cache-Control") (and the second occurrence around line 47) so the variable name accurately reflects it contains the Cache-Control header value; keep assertions the same but reference cacheControlValue instead of etagValue.
12-85: Missing test coverage forsetLastModifiedandsetNoCache.Neither method is exercised by any test.
setLastModifiedin particular involves date formatting logic that would benefit from a test verifying the RFC 1123 output.Would you like me to generate test cases for these methods?
🤖 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 12 - 85, Add unit tests that exercise HttpCachingHeaders.setLastModified(...) and HttpCachingHeaders.setNoCache(): for setLastModified create a known Instant/Date and call setLastModified, then fetch the header via getHeaders() and assert the "Last-Modified" value matches the RFC 1123 formatted string for that instant (use a fixed timezone/Instant to make the expected string deterministic); for setNoCache call it, verify getHeaders().get("Cache-Control") equals "no-cache, no-store, must-revalidate" (or the exact string implemented) and that applyTo(targetMap) copies the same header into the provided map; ensure tests use the public APIs setLastModified, setNoCache, getHeaders, and applyTo so behavior is validated without inspecting internals.src/main/java/org/example/http/HttpCachingHeaders.java (3)
38-40:setNoCacheonly setsCache-Control; consider also settingPragma: no-cache.For HTTP/1.0 proxy compatibility, no-cache responses typically include
Pragma: no-cachealongside theCache-Controldirective. This is a minor gap but worth noting if older intermediaries are in scope.🤖 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 38 - 40, The setNoCache method in HttpCachingHeaders currently only sets Cache-Control; update it to also set the legacy Pragma header to "no-cache" for HTTP/1.0 proxy compatibility by adding a call to setPragma("no-cache") (or setHeader("Pragma", "no-cache")) alongside setCacheControl within the setNoCache method so both Cache-Control and Pragma are emitted.
52-54: Naming:setDefaultCacheControlStaticis ambiguous.The
Staticsuffix could be misread as referring to astaticJava method. A name likesetDefaultCacheControlForStaticResourcesorsetCacheControlPublicwould be clearer.🤖 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 52 - 54, Rename the ambiguous method HttpCachingHeaders.setDefaultCacheControlStatic to a clearer name such as setDefaultCacheControlForStaticResources or setCacheControlPublic and update all call sites; ensure the method continues to call setCacheControl("public, max-age=3600") and preserve visibility and behavior, and run compile/tests to update any references to the old method name.
22-24: Consider null-guardingsetHeaderparameters.Passing
nullfornameorvaluewill silently store nulls in the map, which could causeNullPointerExceptiondownstream (e.g., when writing headers to an HTTP response). A simpleObjects.requireNonNullcheck would make failures obvious at the point of the mistake.🤖 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 22 - 24, The setHeader method in HttpCachingHeaders currently allows null name or value to be inserted into the headers map; add null guards at the start of setHeader (e.g., Objects.requireNonNull for both name and value) so a clear NPE is thrown immediately and avoids storing nulls in the headers map; update any imports if necessary and keep method behavior otherwise 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/http/HttpCachingHeaders.java`:
- Around line 26-28: addETagHeader currently sets the ETag header verbatim which
breaks RFC 7232; update addETagHeader(String etag) to normalize and quote ETag
values before calling setHeader(ETAG, ...): trim input, detect and preserve a
weak prefix "W/" if present, ensure the actual tag is wrapped in double quotes
only if not already quoted, and pass the normalized value to setHeader;
alternatively, if you intend callers to supply quoted values, update the
method's Javadoc to state that explicitly rather than leaving it implicit.
In `@src/test/java/org/example/http/HttpCachingHeadersTest.java`:
- Around line 65-84: The test uses the wrong key casing when mutating the map so
it inserts a new key instead of attempting to overwrite the original; in the
test method getHeaders_shouldReturnDefensiveCopy change the mutation to use the
exact original header key "ETag" (i.e., replace returnedMap.put("Etag",
"hacked") with returnedMap.put("ETag", "hacked")) and update the corresponding
assertion to assertThat(returnedMap.get("ETag")).isEqualTo("hacked"); keep the
final assertion that cachingHeaders.getHeaders().get("ETag") isEqualTo("123" )
to verify the defensive copy.
---
Nitpick comments:
In `@src/main/java/org/example/http/HttpCachingHeaders.java`:
- Around line 38-40: The setNoCache method in HttpCachingHeaders currently only
sets Cache-Control; update it to also set the legacy Pragma header to "no-cache"
for HTTP/1.0 proxy compatibility by adding a call to setPragma("no-cache") (or
setHeader("Pragma", "no-cache")) alongside setCacheControl within the setNoCache
method so both Cache-Control and Pragma are emitted.
- Around line 52-54: Rename the ambiguous method
HttpCachingHeaders.setDefaultCacheControlStatic to a clearer name such as
setDefaultCacheControlForStaticResources or setCacheControlPublic and update all
call sites; ensure the method continues to call setCacheControl("public,
max-age=3600") and preserve visibility and behavior, and run compile/tests to
update any references to the old method name.
- Around line 22-24: The setHeader method in HttpCachingHeaders currently allows
null name or value to be inserted into the headers map; add null guards at the
start of setHeader (e.g., Objects.requireNonNull for both name and value) so a
clear NPE is thrown immediately and avoids storing nulls in the headers map;
update any imports if necessary and keep method behavior otherwise unchanged.
In `@src/test/java/org/example/http/HttpCachingHeadersTest.java`:
- Around line 30-38: Rename the test method CacheControlReturnsCorrectValue to
start with a lowercase letter (e.g., cacheControlReturnsCorrectValue) and rename
the local variable etagValue to cacheControlValue in HttpCachingHeadersTest
where you retrieve getHeaders().get("Cache-Control") (and the second occurrence
around line 47) so the variable name accurately reflects it contains the
Cache-Control header value; keep assertions the same but reference
cacheControlValue instead of etagValue.
- Around line 12-85: Add unit tests that exercise
HttpCachingHeaders.setLastModified(...) and HttpCachingHeaders.setNoCache(): for
setLastModified create a known Instant/Date and call setLastModified, then fetch
the header via getHeaders() and assert the "Last-Modified" value matches the RFC
1123 formatted string for that instant (use a fixed timezone/Instant to make the
expected string deterministic); for setNoCache call it, verify
getHeaders().get("Cache-Control") equals "no-cache, no-store, must-revalidate"
(or the exact string implemented) and that applyTo(targetMap) copies the same
header into the provided map; ensure tests use the public APIs setLastModified,
setNoCache, getHeaders, and applyTo so behavior is validated without inspecting
internals.
Boppler12
left a comment
Boppler12
left a comment
There was a problem hiding this comment.
Enligt coderobbit så har du ganske lite covrange på Docstring Coverage eftersom coderabbit säger att man vill ha runt 80 % covrage och denna kod har bara ca15%.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/example/http/HttpCachingHeaders.java (1)
40-42: Consider null-guarding public entry points.
setHeaderis the root entry point for all mutations. Passing anullname silently stores anullkey in the map; passing anullvalue throughaddETagHeaderproduces the string"null"as the ETag. A lightweight guard here protects all callers without requiring per-method checks.✏️ Proposed defensive guard
public void setHeader(String name, String value) { + if (name == null || name.isBlank()) { + throw new IllegalArgumentException("Header name must not be null or blank"); + } headers.put(name, 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/HttpCachingHeaders.java` around lines 40 - 42, The public mutator setHeader currently accepts null name or value and lets a null key/value into the headers map (causing unexpected behavior like the string "null" in addETagHeader); add a lightweight null-guard at the top of setHeader that validates name and value (e.g., throw IllegalArgumentException or NullPointerException with a clear message) before calling headers.put(name, value) so all callers (including addETagHeader) are protected from nulls.
🤖 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/HttpCachingHeaders.java`:
- Around line 75-77: In HttpCachingHeaders, update the setNoCache method to stop
including the redundant "must-revalidate" directive when calling
setCacheControl: replace the current value "no-store, no-cache, must-revalidate"
with a minimal and unambiguous header such as "no-store, no-cache" (or just
"no-store" if that matches intended behavior) in the setNoCache()
implementation; ensure any callers/tests that assert the previous combined
directive are updated to expect the new compact value so behavior and assertions
remain consistent.
- Around line 44-52: The Javadoc for addETagHeader in HttpCachingHeaders is
misleading and contains a typo; update the `@param` description to say the method
expects the raw, unquoted ETag token (e.g., abc) and that the method will add
the surrounding double quotes per RFC 7232, and fix "quouted" to "quoted" so
callers won’t pre-quote and produce a malformed ETag.
---
Nitpick comments:
In `@src/main/java/org/example/http/HttpCachingHeaders.java`:
- Around line 40-42: The public mutator setHeader currently accepts null name or
value and lets a null key/value into the headers map (causing unexpected
behavior like the string "null" in addETagHeader); add a lightweight null-guard
at the top of setHeader that validates name and value (e.g., throw
IllegalArgumentException or NullPointerException with a clear message) before
calling headers.put(name, value) so all callers (including addETagHeader) are
protected from nulls.
…on made test fail. to handle various ETag formats; improve Javadocs and correct typos in `HttpCachingHeaders`
…lify `setNoCache` behavior.
|
Would it be possible to implement this functionality using a filter that checks incoming and outgoing request? Should this also handle the incoming requests where the client doesn't want the file if it hasn't been modified since a set date and time that will be in the incoming request header? And then return the right status code or the file. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/org/example/http/CachingFilter.java (1)
69-72: ETag based solely onlastModifiedtimestamp provides no benefit overLast-Modified.As noted in the PR discussion, the ETag should be derived from the file's content (e.g., a hash) so it changes when the content changes, even if the timestamp is unreliable or unchanged. Using
file.lastModified()makes ETag and Last-Modified carry identical information.A simple content-based approach:
♻️ Content-hash ETag
+import java.security.MessageDigest; +import java.nio.file.Files; +import java.util.HexFormat; ... private String generateEtag(File file) { - return String.valueOf(file.lastModified()); + try { + byte[] content = Files.readAllBytes(file.toPath()); + byte[] hash = MessageDigest.getInstance("SHA-256").digest(content); + return "\"" + HexFormat.of().formatHex(hash) + "\""; + } catch (Exception e) { + // Fallback: timestamp-based weak ETag + return "W/\"" + file.lastModified() + "\""; + } }Also note that per HTTP spec, ETag values should be quoted strings (e.g.,
"abc123"). The current value is unquoted.🤖 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 69 - 72, The generateEtag(File file) implementation uses file.lastModified() which duplicates Last-Modified; replace it with a content-based hash by reading the file stream and computing a strong digest (e.g., SHA-256) using MessageDigest, convert the digest to a hex string, and return it as a quoted ETag (e.g., "\"" + hex + "\""); update generateEtag(File file) to handle IOExceptions (propagate or translate to ServletException/RuntimeException as appropriate) and stream the file (not load entire file into memory) so large files are handled efficiently.
🤖 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 30-32: The code in CachingFilter uses the raw request.getPath() to
construct new File("www", path), which allows path traversal; change this to
canonicalize and validate the resolved file is inside the web root before
serving: construct the File (using the same new File("www", path) or
equivalent), call getCanonicalFile() on both the web root File and the resolved
File, compare that the resolved canonical path startsWith the web root canonical
path, and if not reject the request (e.g., return 403); ensure you handle the
checked IOException from getCanonicalFile() by adding a try-catch around the
canonicalization in the method containing this code or propagate the exception
in the method signature.
- Line 65: Replace the call that overwrites all response headers in
CachingFilter (response.setHeaders(cachingHeaders.getHeaders())) with a merge
using the existing applyTo API: call
cachingHeaders.applyTo(response.getHeaders()) so caching headers are merged into
the response's existing header map instead of discarding headers like
Content-Type and Content-Length; update the code path around chain.doFilter to
invoke applyTo on the response header map rather than setHeaders or getHeaders
from cachingHeaders.
- Around line 41-56: The current block crashes and miscompares headers: first
guard against null/empty headers.get("If-Modified-Since") and
headers.get("If-None-Match") before parsing/using them; parse the client's
If-Modified-Since with DateTimeFormatter.RFC_1123_DATE_TIME into an Instant
inside a try/catch to handle DateTimeParseException; obtain the file's actual
last-modified Instant (e.g., via
Files.getLastModifiedTime(file.toPath()).toInstant()) and compare that
fileLastModified to the parsed client Instant (if fileLastModified is not after
clientInstant then set response.setStatusCode(304)); keep the ETag logic using
generateEtag(file) and short-circuit to 304 when ifNoneMatch.equals(ETag);
ensure no parsing is attempted when modifiedSince is null and handle parse
failures by skipping the If-Modified-Since check.
---
Nitpick comments:
In `@src/main/java/org/example/http/CachingFilter.java`:
- Around line 69-72: The generateEtag(File file) implementation uses
file.lastModified() which duplicates Last-Modified; replace it with a
content-based hash by reading the file stream and computing a strong digest
(e.g., SHA-256) using MessageDigest, convert the digest to a hex string, and
return it as a quoted ETag (e.g., "\"" + hex + "\""); update generateEtag(File
file) to handle IOExceptions (propagate or translate to
ServletException/RuntimeException as appropriate) and stream the file (not load
entire file into memory) so large files are handled efficiently.
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 (1)
src/main/java/org/example/http/HttpResponseBuilder.java (1)
29-34:⚠️ Potential issue | 🟡 Minor
REASON_PHRASESis missing the304entry, newly relevant now thatCachingFilteruses it.
build()falls back to"OK"for any unknown status code, so a 304 conditional response currently producesHTTP/1.1 304 OKrather thanHTTP/1.1 304 Not Modified. While most clients rely on the numeric code, this is technically incorrect HTTP and can confuse intermediary caches or debugging tools.🐛 Proposed fix
private static final Map<Integer, String> REASON_PHRASES = Map.of( 200, "OK", 201, "Created", + 304, "Not Modified", 400, "Bad Request", 404, "Not Found", 500, "Internal Server Error");🤖 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 29 - 34, REASON_PHRASES map in HttpResponseBuilder is missing the 304 entry causing build() to fall back to "OK"; add an entry mapping 304 to "Not Modified" in the REASON_PHRASES constant so build() emits "HTTP/1.1 304 Not Modified" (this change addresses behavior observed by CachingFilter when it produces 304 responses).
🧹 Nitpick comments (2)
src/main/java/org/example/http/HttpResponseBuilder.java (1)
51-53:getHeaders()returns a mutable reference to the internal map.
setHeaders()defensively copies its input, butgetHeaders()returns the liveheadersfield. Any caller can directlyclear(),remove(), orput()entries without going through the builder's API. The intent of exposing a mutable map forapplyTo()merging is clear here, but documenting this contract (or returning the map only from an internal/package-private method) would prevent accidental misuse by other callers.🤖 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 51 - 53, getHeaders() currently returns the live mutable headers map allowing external callers to modify internal state; change it to either return an unmodifiable copy (Collections.unmodifiableMap(headers)) or make getHeaders package-private/internal and add a clearly named internal method (e.g., getMutableHeadersForApply or headersForMerge) used only by applyTo() to expose the live map, and update setHeaders to remain as-is; also add a short Javadoc to document the chosen contract so callers know whether the returned map is safe to mutate.src/main/java/org/example/http/CachingFilter.java (1)
79-82: ETag should be a content hash, not the last-modified timestamp.
generateEtagreturnsString.valueOf(file.lastModified()), which is the same value already exposed in theLast-Modifiedheader. This makes the ETag branch redundant — it provides no stronger cache validation. As noted in the PR review discussion, the ETag should be derived by hashing the file content so it reflects actual content changes.♻️ Proposed refactor
private String generateEtag(File file) { - return String.valueOf(file.lastModified()); + try { + byte[] content = java.nio.file.Files.readAllBytes(file.toPath()); + java.security.MessageDigest md = java.security.MessageDigest.getInstance("MD5"); + byte[] digest = md.digest(content); + return java.util.HexFormat.of().formatHex(digest); + } catch (Exception e) { + // Fallback to last-modified if hashing fails + return String.valueOf(file.lastModified()); + } }Note: for large files, streaming the hash via
DigestInputStreamavoids loading the full content into memory.🤖 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 79 - 82, generateEtag currently returns file.lastModified(), which duplicates Last-Modified and makes ETag pointless; change the generateEtag(File file) implementation to compute a content-based hash (e.g., SHA-256) by streaming the file through a MessageDigest/DigestInputStream and then return a hex (or base64) string representation (optionally wrapped/quoted for HTTP ETag semantics); ensure you handle IOExceptions and close streams properly and avoid loading the whole file into memory for large files.
🤖 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 58-65: The code in CachingFilter that calls
DateTimeFormatter.RFC_1123_DATE_TIME.parse(modifiedSince) can throw
DateTimeParseException for malformed If-Modified-Since headers; wrap the
parse+Instant.from call in a try-catch that catches DateTimeParseException and
on catch simply ignore the header (treat the precondition as false) and continue
normal processing, only performing the fileLastModified check and
response.setStatusCode(304) when parsing succeeds; optionally log the parse
failure at debug/trace level but do not propagate the exception.
- Around line 48-56: In HttpCachingHeaders.addETagHeader() the code builds a
quoted formattedEtag but mistakenly calls setHeader(ETAG, etag) with the
unquoted value; update the call in addETagHeader to use formattedEtag (i.e.,
setHeader(ETAG, formattedEtag)) so the ETag header is emitted as a quoted string
per RFC 7232 and ensure any downstream comparisons still use
generateEtag()/If-None-Match logic as-is.
---
Outside diff comments:
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 29-34: REASON_PHRASES map in HttpResponseBuilder is missing the
304 entry causing build() to fall back to "OK"; add an entry mapping 304 to "Not
Modified" in the REASON_PHRASES constant so build() emits "HTTP/1.1 304 Not
Modified" (this change addresses behavior observed by CachingFilter when it
produces 304 responses).
---
Duplicate comments:
In `@src/main/java/org/example/http/CachingFilter.java`:
- Around line 35-37: The current check uses string prefix comparison
(file.getPath().startsWith(wwwRoot.getPath())) which allows partial path
traversal; replace it with a Path-based component comparison: obtain
canonical/real Paths for both file and wwwRoot (e.g.,
file.getCanonicalFile().toPath() and wwwRoot.getCanonicalFile().toPath() or use
toRealPath()), normalize them, and use Path.startsWith(rootPath) to verify
containment before returning 403; update the check in CachingFilter accordingly
(use filePath.startsWith(rootPath) instead of the string startsWith).
---
Nitpick comments:
In `@src/main/java/org/example/http/CachingFilter.java`:
- Around line 79-82: generateEtag currently returns file.lastModified(), which
duplicates Last-Modified and makes ETag pointless; change the generateEtag(File
file) implementation to compute a content-based hash (e.g., SHA-256) by
streaming the file through a MessageDigest/DigestInputStream and then return a
hex (or base64) string representation (optionally wrapped/quoted for HTTP ETag
semantics); ensure you handle IOExceptions and close streams properly and avoid
loading the whole file into memory for large files.
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 51-53: getHeaders() currently returns the live mutable headers map
allowing external callers to modify internal state; change it to either return
an unmodifiable copy (Collections.unmodifiableMap(headers)) or make getHeaders
package-private/internal and add a clearly named internal method (e.g.,
getMutableHeadersForApply or headersForMerge) used only by applyTo() to expose
the live map, and update setHeaders to remain as-is; also add a short Javadoc to
document the chosen contract so callers know whether the returned map is safe to
mutate.
…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
JohanHiths
force-pushed
the
http-caching-headers
branch
from
2159192 to
af7fcdb
Compare
…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.
JohanHiths
force-pushed
the
http-caching-headers
branch
from
4fccd32 to
5552e85
Compare
Removed the getStatusCode method from HttpResponseBuilder.
| if(!file.exists()){ | ||
| response.setStatusCode(404); |
There was a problem hiding this comment.
Glöm inte att ändra 404 för SC_NOT_FOUND :) (de nya reason phrases på builder)
MartinStenhagen
left a comment
MartinStenhagen
left a comment
There was a problem hiding this comment.
Tycker att du har en bra grund här.
Ett par förslag på ändringar:
-
HttpCachingHeaders instansieras men används inte för att applicera headers.
Just nu sätts ETag, Last-Modified och Cache-Control manuellt. Eftersom helper-klassen redan finns i PR:n är det enklast att använda den konsekvent: cachingHeaders.getHeaders().forEach(response::addHeader); och ta bort de manuella response.addHeader(...)-raderna. Då undviks dubbel logik och defaults/format hålls på ett ställe. -
Använd HttpCachingHeaders även vid SC_NOT_MODIFIED.
Nu returnerar 304-grenarna utan ETag/Last-Modified/Cache-Control. Bygg HttpCachingHeaders per request och applicera headers både innan return vid 304 och efter chain.doFilter för 200-svar. -
Filtret bör inte göra egen filupplösning/404 eftersom StaticFileHandler redan hanterar detta. Nuvarande caching fungerar dock i grunden, så det känns rimligt att få in funktionaliteten nu och bryta ut den strukturella förbättringen (ta bort duplicerad logik och säkerställa konsekvent beteende) till en separat issue, t.ex. “Refactor static file resolving to avoid duplication in CachingFilter”.
…ndling, and ensure consistent ETag and caching headers in responses. At the request of Martin Stenhagen and gvaguirres # Conflicts: # src/main/java/org/example/http/CachingFilter.java
JohanHiths
force-pushed
the
http-caching-headers
branch
from
9c145b5 to
d121ec8
Compare
|
Bra feedback! Tror jag har gjort alla ändringar som behövs just nu, om jag missat något får du gärna göra en ny kommentar om du vill. Annars gör jag resten i nästa issue! |
There was a problem hiding this comment.
Bra ändringar och toppen att du skapat en ny issue. Ett par saker som kvarstår:
- Vid 304 appliceras cachingHeaders.getHeaders() innan cachingHeaders har fyllts med några värden. addETagHeader, setLastModified och setDefaultCacheControlStatic() anropas först senare, efter 304-checkarna. Det innebär att 304-svaren i praktiken skickas utan ETag, Last-Modified och Cache-Control.
Förslag:
Flyttta dessa rader så att de ligger direkt efter att eTag och lastModified har beräknats, och innan 304-checkarna.
cachingHeaders.addETagHeader(eTag);
cachingHeaders.setLastModified(Instant.ofEpochMilli(file.lastModified()));
cachingHeaders.setDefaultCacheControlStatic();
Så resultatet blir:
String ifNoneMatch = headers.get("If-None-Match");
// Flytta hit
cachingHeaders.addETagHeader(eTag);
cachingHeaders.setLastModified(lastModified);
cachingHeaders.setDefaultCacheControlStatic();
if (ifNoneMatch != null && ifNoneMatch.equals(eTag)) { ... }
-
Testerna går inte längre att köra. CachingFilter använder nu ConfigLoader.get() och förutsätter att init() har körts. Testerna skapar idag filtret och anropar doFilter() direkt utan att ladda config eller anropa init(). För att kod och tester ska vara i synk behöver testerna uppdateras så att de följer den nya livscykeln för filtret.
-
Ändra till package org.example.filter; för att det ska stämma överens med strukturen i projektet.
- Renamed packages for clarity (`http` to `filter`). - Added `ResolveFileHandler` class placeholder. - Updated `CachingFilter` for better caching header handling (moved and optimized logic). - Added `ConfigLoader` setup in `CachingFilterTest`. Requested by Martin Stenhagen
MartinStenhagen
left a comment
MartinStenhagen
left a comment
There was a problem hiding this comment.
Din senaste commit löser det jag hade synpunkter på. Bra jobbat.
5 participants
Issue #35
Summary by CodeRabbit
New Features
Tests