Add ETag support to StaticFileHandler with caching for static files - #108
Conversation
- Implemented ETag generation using SHA-256 for stronger caching. - Added `Cache-Control` header with a short max-age for development convenience. - Improved handling of conditional requests with `If-None-Match` to return 304 responses. - Included tests for ETag and caching behavior validation.
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds SHA-256-based strong ETag computation and case-insensitive If-None-Match handling (including "*" and multiple values) to static file responses, returning 304 Not Modified when appropriate and sending Cache-Control: public, max-age=5 on 200 and 304 responses. Includes unit tests for ETag and conditional 304 behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as StaticFileHandler
participant FS as FileSystem
Client->>Handler: GET /static/file\n(optional If-None-Match header)
Handler->>FS: read file bytes
FS-->>Handler: file bytes
Handler->>Handler: compute SHA-256 ETag (opaque quoted hex)
Handler->>Handler: parse If-None-Match (case-insensitive, handle multiple values & "*")
alt If-None-Match matches ETag or "*"
Handler-->>Client: 304 Not Modified\nheaders: ETag, Cache-Control: public, max-age=5
else
Handler-->>Client: 200 OK\nheaders: Content-Type, ETag, Cache-Control: public, max-age=5\nbody: file bytes
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 (4)
src/test/java/org/juv25d/handler/StaticFileHandlerTest.java (1)
57-77: Missing test case:If-None-Matchwith weak ETag prefix (W/"...")The existing test covers the exact round-trip (
ETagvalue echoed back verbatim), which always passes. The RFC-compliance issue identified inetagMatches— whereIf-None-Match: W/"<hash>"should also yield 304 — has no coverage. Add a third assertion (or a separate test) that sends the ETag with aW/prefix and still expects 304.🧪 Suggested additional test
`@Test` void shouldReturn304WhenIfNoneMatchMatchesWeakEtag() { HttpRequest first = createRequest("GET", "/index.html"); HttpResponse firstResponse = StaticFileHandler.handle(first); String strongEtag = firstResponse.headers().get("ETag"); // Simulate a browser that replays as a weak ETag String weakEtag = "W/" + strongEtag; Map<String, String> headers = new HashMap<>(); headers.put("If-None-Match", weakEtag); HttpRequest second = createRequest("GET", "/index.html", headers); HttpResponse secondResponse = StaticFileHandler.handle(second); assertThat(secondResponse.statusCode()).isEqualTo(304); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/handler/StaticFileHandlerTest.java` around lines 57 - 77, Add a test that verifies weak ETag matching by creating a new test method (e.g., shouldReturn304WhenIfNoneMatchMatchesWeakEtag) which calls StaticFileHandler.handle for the first GET to obtain the strong ETag, constructs a weak ETag by prefixing "W/" to that value, sends a second GET with If-None-Match set to the weak ETag, and asserts the response is 304 (Not Modified), has an empty body, and preserves the ETag and Cache-Control headers; this ensures the etagMatches behavior in StaticFileHandler correctly treats a W/"..." If-None-Match as a match.src/main/java/org/juv25d/handler/StaticFileHandler.java (3)
88-94: Consider adding aLast-Modifiedheader for complete HTTP caching semanticsRFC 7232 states that "the preferred behavior for an origin server is to send both a strong entity-tag and a Last-Modified value in successful responses to a retrieval request." Browsers and CDNs can use
Last-Modifiedas a fallback validator (viaIf-Modified-Since) when ETags are unavailable. For classpath resources, the JAR manifest orURL.openConnection().getLastModified()can serve as a source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 88 - 94, Add a Last-Modified header when building responses in StaticFileHandler: after computing mimeType, etag and populating headers (the Map<String,String> headers in the response construction), obtain the resource's last-modified timestamp (e.g., via resourceUrl.openConnection().getLastModified() or from the JAR manifest when classpath resources are used), format it to an HTTP-date string, and put it into headers with key "Last-Modified" before returning the HttpResponse(200, "OK", headers, fileContent); ensure the value is omitted only if no reliable last-modified timestamp is available.
65-86: File content is always fully loaded, even when a 304 will be returnedThe full resource is read into memory at line 66 before the
If-None-Matchcheck at line 79. For a 304 response, the entirefileContentbyte array is allocated and discarded. This is fine for the small classpath resources targeted here, but if the handler is ever extended to serve larger assets, it becomes an unnecessary I/O and allocation cost.Consider caching ETags in a static
Map<String, String>keyed by resource path so conditional requests can short-circuit before the file read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 65 - 86, Introduce a static ConcurrentHashMap<String,String> etagCache in StaticFileHandler and use it to avoid reading the full resource when possible: first fetch the If-None-Match header via getHeaderIgnoreCase(request.headers(), "If-None-Match") and check etagMatches(ifNoneMatch, cachedEtag) using etagCache.get(resourcePath); if it matches, return the 304 with ETag and Cache-Control without calling loadResource or allocate fileContent; only call loadResource(resourcePath) and computeStrongEtag(fileContent) when there is no cached ETag (or when caching miss) and then store the computed ETag into etagCache.put(resourcePath, etag); ensure the cache is a ConcurrentHashMap to be thread-safe and keep existing behavior (headers, MAX_AGE_SECONDS, logging) otherwise.
189-198:toHexis replaceable withHexFormat(Java 17+)This project targets Java 25, so
java.util.HexFormat.of().formatHex(byte[])is available. The customtoHexmethod and its usage incomputeStrongEtagcan be replaced.♻️ Proposed refactor
- private static String computeStrongEtag(byte[] content) { - try { - MessageDigest digest = MessageDigest.getInstance("SHA-256"); - byte[] hash = digest.digest(content); - return "\"" + toHex(hash) + "\""; - }catch (NoSuchAlgorithmException e) { - return "\"" + content.length + "\""; - } - } - - private static String toHex(byte[] bytes) { - char[] hex = "0123456789abcdef".toCharArray(); - char[] out = new char[bytes.length * 2]; - for (int i = 0; i < bytes.length; i++) { - int v = bytes[i] & 0xFF; - out[i * 2] = hex[v >>> 4]; - out[i * 2 + 1] = hex[v & 0x0F]; - } - return new String(out); - } + private static final HexFormat HEX = HexFormat.of(); + + private static String computeStrongEtag(byte[] content) { + try { + MessageDigest digest = MessageDigest.getInstance("SHA-256"); + byte[] hash = digest.digest(content); + return "\"" + HEX.formatHex(hash) + "\""; + } catch (NoSuchAlgorithmException e) { + throw new AssertionError("SHA-256 not available", e); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 189 - 198, Replace the custom toHex implementation with java.util.HexFormat: remove the toHex(byte[]) method in StaticFileHandler and change computeStrongEtag to call HexFormat.of().formatHex(digest) (or a class-level static HexFormat HEX = HexFormat.of() and use HEX.formatHex(digest)) so the code uses the JDK's hex formatter (keeps lowercase output), and add the necessary import for java.util.HexFormat.
🤖 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/juv25d/handler/StaticFileHandler.java`:
- Around line 184-186: In StaticFileHandler's catch block for
NoSuchAlgorithmException (the block that currently returns "\"" + content.length
+ "\""), do not use content.length as the ETag fallback because it leads to
collisions; instead either re-throw as an AssertionError (e.g., throw new
AssertionError(e)) since SHA-256 is required, or generate a collision-resistant
fallback ETag (e.g., use a UUID or SecureRandom/ System.nanoTime-based token
combined with content length) and return that; update the catch in the method
that computes the ETag so it references content and uses the chosen approach.
- Around line 200-216: The etagMatches method uses a strong exact-string
compare; change it to perform the RFC7232 weak-comparison: normalize each token
from ifNoneMatchHeader (split parts), trim and remove an optional weak prefix
"W/" and surrounding quotes, do the same normalization for currentEtag, then
compare opaque-tag strings character-by-character; keep the existing "*"
handling and null/blank guards and ensure you still return true on any matching
part in etagMatches(String ifNoneMatchHeader, String currentEtag).
---
Nitpick comments:
In `@src/main/java/org/juv25d/handler/StaticFileHandler.java`:
- Around line 88-94: Add a Last-Modified header when building responses in
StaticFileHandler: after computing mimeType, etag and populating headers (the
Map<String,String> headers in the response construction), obtain the resource's
last-modified timestamp (e.g., via
resourceUrl.openConnection().getLastModified() or from the JAR manifest when
classpath resources are used), format it to an HTTP-date string, and put it into
headers with key "Last-Modified" before returning the HttpResponse(200, "OK",
headers, fileContent); ensure the value is omitted only if no reliable
last-modified timestamp is available.
- Around line 65-86: Introduce a static ConcurrentHashMap<String,String>
etagCache in StaticFileHandler and use it to avoid reading the full resource
when possible: first fetch the If-None-Match header via
getHeaderIgnoreCase(request.headers(), "If-None-Match") and check
etagMatches(ifNoneMatch, cachedEtag) using etagCache.get(resourcePath); if it
matches, return the 304 with ETag and Cache-Control without calling loadResource
or allocate fileContent; only call loadResource(resourcePath) and
computeStrongEtag(fileContent) when there is no cached ETag (or when caching
miss) and then store the computed ETag into etagCache.put(resourcePath, etag);
ensure the cache is a ConcurrentHashMap to be thread-safe and keep existing
behavior (headers, MAX_AGE_SECONDS, logging) otherwise.
- Around line 189-198: Replace the custom toHex implementation with
java.util.HexFormat: remove the toHex(byte[]) method in StaticFileHandler and
change computeStrongEtag to call HexFormat.of().formatHex(digest) (or a
class-level static HexFormat HEX = HexFormat.of() and use HEX.formatHex(digest))
so the code uses the JDK's hex formatter (keeps lowercase output), and add the
necessary import for java.util.HexFormat.
In `@src/test/java/org/juv25d/handler/StaticFileHandlerTest.java`:
- Around line 57-77: Add a test that verifies weak ETag matching by creating a
new test method (e.g., shouldReturn304WhenIfNoneMatchMatchesWeakEtag) which
calls StaticFileHandler.handle for the first GET to obtain the strong ETag,
constructs a weak ETag by prefixing "W/" to that value, sends a second GET with
If-None-Match set to the weak ETag, and asserts the response is 304 (Not
Modified), has an empty body, and preserves the ETag and Cache-Control headers;
this ensures the etagMatches behavior in StaticFileHandler correctly treats a
W/"..." If-None-Match as a match.
jesperlarsson1910
left a comment
jesperlarsson1910
left a comment
There was a problem hiding this comment.
Looks like and overall solid implimentation, some small rabbit feedback but should be a fast fix.
Would it be smart to add some kind of internal cache for the ETags and maybe the content? I think that right now we read the entire file on every request even when it hasn't changed.
If it something to implement it could also be done in a seperate sub-issue/pr as an enhancement and keep this pr as is since it looks to be fully functional and I don't think the performance impact is that big right now.
Enhance ETag comparison with support for weak tags and improve error handling for missing SHA-256.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/org/juv25d/handler/StaticFileHandler.java (3)
76-91: ETag logic is correct; consider caching computed ETags to avoid per-request SHA-256.Every request reloads the file bytes from the classpath and recomputes the SHA-256 hash. With
max-age=5, cache misses are frequent, so the hash is recomputed on every un-cached request. For a development server with small static files this is fine, but if this ever gets used under higher load, a simpleMap<String, String>path-to-ETag cache (lazily populated, invalidated on startup or file change) would eliminate the redundant work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 76 - 91, Add a lightweight path->ETag cache in StaticFileHandler to avoid recomputing SHA-256 on every request: introduce a Map<String,String> (e.g., etagCache) at class scope, populate it lazily in the code paths that call computeStrongEtag(resourceBytes) (use resourcePath as the key), return the cached ETag when present, and update the cache when a new ETag is computed; ensure cache is cleared on startup or when files change (or document that it is only valid for process lifetime) and keep existing logic using computeStrongEtag, getHeaderIgnoreCase, etagMatches, and MAX_AGE_SECONDS unchanged.
205-221:etagMatches— weak-comparison viaopaqueTagcorrectly resolves the previous RFC 7232 issue.One minor edge case:
value.split(",")is a naive split that would misparse an ETag containing a literal comma (e.g.,"ab,cd"would split into"abandcd"). Per RFC 7230, commas are valid inside opaque-tag content. This doesn't affect this server in practice because the generated ETags are 64-character lowercase hex strings, but it's worth noting for general robustness. A proper fix would require scanning quoted strings to find delimiters outside quotes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 205 - 221, The etagMatches method currently uses value.split(",") which will incorrectly split ETag values that contain commas inside quoted opaque-tags; update etag parsing in etagMatches (and keep using opaqueTag(currentEtag)) to scan the header string and split only on commas that are not inside a quoted string (handle leading/trailing whitespace, quoted strings, and backslash-escaped quotes per RFC 7230), producing each raw member token to pass to opaqueTag for comparison; implement the scanner in etagMatches (or a small helper used by etagMatches) so quoted commas are respected and each extracted part is trimmed before calling opaqueTag.
189-198: ReplacetoHexwithHexFormat.of().formatHex().
java.util.HexFormatwas introduced in Java 17 and is immutable and thread-safe. The project targets Java 25, so this standard API is available. ThetoHexmethod can be eliminated entirely.♻️ Proposed refactor
+import java.util.HexFormat;In
computeStrongEtag:private static String computeStrongEtag(byte[] content) { try { MessageDigest digest = MessageDigest.getInstance("SHA-256"); byte[] hash = digest.digest(content); - return "\"" + toHex(hash) + "\""; + return "\"" + HexFormat.of().formatHex(hash) + "\""; } catch (NoSuchAlgorithmException e) { throw new AssertionError("SHA-256 not available", e); } }Then remove
toHexentirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/handler/StaticFileHandler.java` around lines 189 - 198, Replace the custom toHex implementation in StaticFileHandler with the JDK HexFormat utility: remove the toHex(byte[] bytes) method and update any callers (notably computeStrongEtag in StaticFileHandler) to use java.util.HexFormat.of().formatHex(bytes) instead; ensure imports are adjusted and tests/uses of computeStrongEtag still pass after removing the old method.
🤖 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/juv25d/handler/StaticFileHandler.java`:
- Around line 200-203: opaqueTag currently calls etag.trim() without a null
check which can NPE if etag is ever null; update opaqueTag(String etag) to
return null immediately when etag is null (or otherwise handle null safely) and
keep the W/ stripping behavior for non-null values, and then update
etagMatches(...) where opaqueTag(...) is used so comparisons guard against a
null opaqueTag result (e.g., treat null as non-matching or compare safely) —
refer to opaqueTag, etagMatches, and callers that use
computeStrongEtag/currentEtag/part to locate and fix the checks.
---
Nitpick comments:
In `@src/main/java/org/juv25d/handler/StaticFileHandler.java`:
- Around line 76-91: Add a lightweight path->ETag cache in StaticFileHandler to
avoid recomputing SHA-256 on every request: introduce a Map<String,String>
(e.g., etagCache) at class scope, populate it lazily in the code paths that call
computeStrongEtag(resourceBytes) (use resourcePath as the key), return the
cached ETag when present, and update the cache when a new ETag is computed;
ensure cache is cleared on startup or when files change (or document that it is
only valid for process lifetime) and keep existing logic using
computeStrongEtag, getHeaderIgnoreCase, etagMatches, and MAX_AGE_SECONDS
unchanged.
- Around line 205-221: The etagMatches method currently uses value.split(",")
which will incorrectly split ETag values that contain commas inside quoted
opaque-tags; update etag parsing in etagMatches (and keep using
opaqueTag(currentEtag)) to scan the header string and split only on commas that
are not inside a quoted string (handle leading/trailing whitespace, quoted
strings, and backslash-escaped quotes per RFC 7230), producing each raw member
token to pass to opaqueTag for comparison; implement the scanner in etagMatches
(or a small helper used by etagMatches) so quoted commas are respected and each
extracted part is trimmed before calling opaqueTag.
- Around line 189-198: Replace the custom toHex implementation in
StaticFileHandler with the JDK HexFormat utility: remove the toHex(byte[] bytes)
method and update any callers (notably computeStrongEtag in StaticFileHandler)
to use java.util.HexFormat.of().formatHex(bytes) instead; ensure imports are
adjusted and tests/uses of computeStrongEtag still pass after removing the old
method.
Improve ETag handling by adding null checks for opaqueTag parsing
addee1
left a comment
addee1
left a comment
There was a problem hiding this comment.
Nice update 👍
This is a solid addition with the ETag support and 304 handling. The flow is easy to follow, and the helper methods keep the code clean and simple. Good to see the correct headers on both 200 and 304 responses, and nice test coverage as well. This clearly improves the static file handling.
Well done 🙂
DennSel
left a comment
DennSel
left a comment
There was a problem hiding this comment.
I stand by the improvements from jesperlarrson1910, but they can be handled on their own if/when needed.
5 participants
Add http caching for static files
Cache-Controlheader with a short max-age for development convenience.If-None-Matchto return 304 responses.Summary by CodeRabbit
New Features
Tests