Open
Conversation
…re validering och beroendeinjektion i StaticFileHandler, samt uppdaterat hantering i CacheFilter
|
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. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a thread-safe LRU FileCache and URI-based CacheFilter, refactors StaticFileHandler for configurable roots, streaming and cache paths, expands HttpResponseBuilder, introduces filter pipeline (Filter, FilterChain, IpFilter, LocaleFilter, FilterChainImpl), request/config infrastructure (HttpRequest, AppConfig, ConfigLoader), MIME detector, many tests, Dockerfile and docs updates. Changes
Sequence DiagramsequenceDiagram
participant Client
participant StaticFileHandler
participant CacheFilter
participant FileCache
participant FileProvider as Disk
Client->>StaticFileHandler: GET /path
activate StaticFileHandler
alt fileSize < threshold
StaticFileHandler->>CacheFilter: getOrFetch(uri, provider)
activate CacheFilter
CacheFilter->>FileCache: get(uri)
activate FileCache
FileCache-->>CacheFilter: bytes or null
deactivate FileCache
alt cache hit
CacheFilter-->>StaticFileHandler: bytes
else cache miss
CacheFilter->>FileProvider: fetch(uri)
activate FileProvider
FileProvider-->>CacheFilter: bytes
deactivate FileProvider
CacheFilter->>FileCache: put(uri, bytes)
activate FileCache
FileCache-->>CacheFilter: done
deactivate FileCache
CacheFilter-->>StaticFileHandler: bytes
end
deactivate CacheFilter
else stream large file
StaticFileHandler->>FileProvider: open & stream chunks
activate FileProvider
FileProvider-->>StaticFileHandler: byte chunks
deactivate FileProvider
end
StaticFileHandler->>Client: send headers + body/stream
deactivate StaticFileHandler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 6
🧹 Nitpick comments (10)
src/main/java/org/example/FileCache.java (2)
14-14: Consider extracting aFileCacheinterface for dependency injection.The current concrete class makes it difficult to swap implementations (e.g., for testing or alternative backends). Based on learnings,
FileCacheshould be refactored to an interface to allow for different cache implementations (e.g., in-memory, Redis) and should be dependency-injected intoStaticFileHandler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/FileCache.java` at line 14, Extract a FileCache interface and refactor the concrete class into an implementation (e.g., FileCacheImpl) so the cache can be swapped via DI: create a public interface named FileCache exposing the methods currently used by the class (e.g., get, put, invalidate or whatever methods exist on the concrete FileCache), rename the existing class to FileCacheImpl (or keep it implementing FileCache) and implement the interface, then modify StaticFileHandler to accept a FileCache in its constructor (or via setter) and use the interface type instead of the concrete class so tests or alternate backends (in-memory, Redis) can be injected.
101-109:CacheEntry.timestampis stored but never read.The
timestampfield is set in the constructor but is not used anywhere in the cache logic (no TTL expiration, no reporting). Consider removing it to avoid confusion, or implementing TTL-based expiration if that was the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/FileCache.java` around lines 101 - 109, The CacheEntry.timestamp field is assigned but never used; either remove that field and its assignment in the CacheEntry constructor, or implement TTL-based expiration by using CacheEntry.timestamp: add a TTL constant (e.g., CACHE_TTL_MS) and update the cache access methods (e.g., the get(...) and any existing put(...) or cleanup methods) to check System.currentTimeMillis() - entry.timestamp > CACHE_TTL_MS, evict expired entries before returning values, and consider a periodic cleanup or lazy removal on access; reference the CacheEntry class and its constructor, and the cache get/put methods to locate where to apply the change.src/test/java/org/example/FileCacheTest.java (1)
7-117: Consider adding tests for LRU eviction behavior.The test suite covers core CRUD operations well but does not exercise the eviction logic (
MAX_ENTRIESorMAX_CACHE_SIZElimits). An eviction test would have caught thecurrentSizedrift bug inFileCache.removeEldestEntry. For example, insert entries that exceed the 50 MB limit and assert that older entries are evicted andgetCurrentSizeInBytes()remains bounded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/FileCacheTest.java` around lines 7 - 117, Add a new unit test that exercises FileCache's LRU eviction by inserting enough entries to exceed MAX_CACHE_SIZE (and/or MAX_ENTRIES) and asserting older keys are evicted and getCurrentSizeInBytes() stays <= MAX_CACHE_SIZE; specifically target the eviction path exercised by FileCache.removeEldestEntry and verify both contains(key) and get(key) for evicted vs retained keys and that getCurrentSizeInBytes() does not drift upward after multiple evictions to catch the bug in currentSize accounting.src/main/java/org/example/http/HttpResponseBuilder.java (2)
46-57: Potential duplicateContent-Length/Connectionheaders if set in the headers map.
buildHeaders()unconditionally appendsContent-LengthandConnection: closeafter iteratingheaders. If a caller also includes"Content-Length"or"Connection"in the headers map (viasetHeaders), the response will contain duplicate headers. Current callers inStaticFileHandleronly setContent-Type, so this isn't broken today, but the API is fragile.Consider either filtering those keys from
headersbefore iteration, or documenting the contract that callers must not set them.🤖 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 46 - 57, The buildHeaders() method always appends "Content-Length" and "Connection: close" after iterating the headers map, causing duplicate headers if callers put those keys into the headers map; update buildHeaders() to filter out/ignore "Content-Length" and "Connection" (case-insensitively) when iterating headers (or alternatively remove them from the headers map earlier, e.g., in setHeaders), so only the canonical values appended by buildHeaders() appear; ensure you reference the headers map and REASON_PHRASES/statusCode logic in buildHeaders(), and adjust or document behavior used by callers like StaticFileHandler to avoid duplicates.
60-68: Local variablecontentLengthshadows the instance field.The
int contentLengthdeclared at line 62 shadowsthis.contentLength(long). This works correctly becausebuild()computes its own value, but it's confusing and could lead to subtle bugs if someone later tries to unify the two code paths.🤖 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 60 - 68, The method build() currently declares a local int contentLength that shadows the instance field contentLength; remove the local variable and assign the computed length directly to the instance field (this.contentLength) so both code paths update the same field. In build(), when body is empty and bytebody != null setBody(new String(bytebody, StandardCharsets.UTF_8)) and then assign this.contentLength = bytebody.length; otherwise compute this.contentLength = body.getBytes(StandardCharsets.UTF_8).length (promote to long if needed) so the class-level contentLength consistently reflects the computed value; adjust any downstream code expecting a long accordingly.src/test/java/org/example/StaticFileHandlerTest.java (2)
206-244: Caching/non-caching tests only verify both calls succeed — they don't verify cache behavior.
testSmallFileUsesCachingandtestLargeFileNotCachedboth assert that two consecutive calls return200 OK, which would pass regardless of whether caching is active. To actually verify cache hit/miss behavior, consider injecting a mock or spyCacheFilterand asserting the expected interactions (e.g.,fetchcalled once for cached, twice for non-cached), or at minimum delete the file between calls to confirm the second request is served from cache.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/StaticFileHandlerTest.java` around lines 206 - 244, Update the two tests to actually verify cache behavior rather than just 200 responses: in testSmallFileUsesCaching, replace or wrap the real CacheFilter with a mock/spy (or inject a test double) and assert that the cache's fetch/store methods (e.g., CacheFilter.fetch or CacheFilter.get/put) are called only once across the two handler.sendGetRequest calls for "cached.html"; alternatively, after the first successful request delete the underlying file and assert the second request still returns 200 to prove the response came from cache. In testLargeFileNotCached, similarly use a mock/spy CacheFilter and assert that fetch/get is invoked for every handler.sendGetRequest call (i.e., called twice), or keep the file and verify that removing the file before the second request causes the second request to fail if not cached (to prove no caching). Ensure the tests reference handler.sendGetRequest and the CacheFilter methods used by your server to locate instrumentation points.
148-171: Memory-usage test is inherently flaky.
Runtime.totalMemory() - freeMemory()is a coarse snapshot that is heavily influenced by GC timing, other tests, and theByteArrayOutputStreamitself (which buffers the entire 15 MB response in-heap). The 50 MB threshold is generous, but this test can still intermittently fail or pass vacuously depending on JVM state. Consider either removing it or replacing it with a deterministic approach (e.g., a countingOutputStreamthat tracks bytes written without buffering the full payload).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/StaticFileHandlerTest.java` around lines 148 - 171, The test testStreamingFilesDoNotExceedMemoryThreshold is flaky because it measures JVM heap via Runtime and uses ByteArrayOutputStream which buffers the whole payload; replace the memory-based assertion with a deterministic check: use a non-buffering counting OutputStream (implement a simple OutputStream that increments a long counter on write) instead of ByteArrayOutputStream, call handler.sendGetRequest(output, "memory_test.bin"), assert the counter equals the expected file length and remove the Runtime memory measurements and the 50MB assertion; reference the test method name testStreamingFilesDoNotExceedMemoryThreshold and handler.sendGetRequest when making the change.src/main/java/org/example/StaticFileHandler.java (2)
122-131: Lambda ignores itspathparameter, capturing the outerfileinstead.The
FileProvider.fetch(String uri)lambda at line 124 receives apathargument but ignores it, using the capturedfilevariable. This makes theFileProviderinterface misleading — it suggests the provider resolves the URI, but the actual resolution happened earlier. If the cache is ever shared across contexts, the wrong file could be served.♻️ Suggested clarification
byte[] fileBytes = cacheFilter.getOrFetch(uri, - path -> Files.readAllBytes(file.toPath()) + ignored -> Files.readAllBytes(file.toPath()) );Or better, have the provider actually use its argument by passing the resolved path:
byte[] fileBytes = cacheFilter.getOrFetch(uri, - path -> Files.readAllBytes(file.toPath()) + u -> Files.readAllBytes(new File(webRoot, u).toPath()) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/StaticFileHandler.java` around lines 122 - 131, The lambda passed to cacheFilter.getOrFetch in sendSmallFile currently ignores its parameter and closes over the outer file, causing the provider to always read the same File; change the provider so it actually uses its argument (the FileProvider.fetch(String path) parameter) to read bytes (e.g., use Files.readAllBytes(Paths.get(path)) or new File(path).toPath()) and ensure the key you pass (uri) matches the path semantics expected by getOrFetch so the cached entry corresponds to the provided path; update sendSmallFile, cacheFilter.getOrFetch call, and the lambda to reference the path parameter instead of the captured file.
97-120: Consider usingFiles.probeContentType()or a map-based lookup instead of an if-else chain.The manual if-else chain for content types works but is verbose and hard to extend. A static
Map<String, String>keyed by extension would be more maintainable.🤖 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 97 - 120, Replace the long if-else in StaticFileHandler.getContentType with a static Map<String,String> (e.g. EXTENSION_CONTENT_TYPE) that maps extensions like "html","css","js","png", etc. to their MIME types; initialize the map in a static block or as an immutable map, then in getContentType extract the file extension from the uri, look it up in EXTENSION_CONTENT_TYPE and return the mapped value if present; if not found, call Files.probeContentType(path) as a fallback (catching IOExceptions) and finally default to "application/octet-stream".src/main/java/org/example/CacheFilter.java (1)
7-7:FileCacheis hardcoded; consider injecting it.
CacheFilterinstantiates its ownFileCacheinternally, making it impossible to share or swap the cache implementation. SinceCacheFilteris already injected intoStaticFileHandler, acceptingFileCacheas a constructor parameter would complete the DI chain. Based on learnings, FileCache should be dependency-injected and ideally be an interface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/CacheFilter.java` at line 7, CacheFilter currently constructs a concrete FileCache instance (private final FileCache cache = new FileCache()), preventing DI and substitution; change CacheFilter to accept a FileCache (preferably an interface type) via its constructor and store it as a final field instead of new-ing it. Update the StaticFileHandler constructor where CacheFilter is injected (or where CacheFilter is created) to pass the shared FileCache implementation (or an implementation bound in your DI setup). Ensure FileCache is represented as an interface (e.g., FileCache -> FileCacheInterface) or an abstract type so different implementations can be provided.
🤖 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/CacheFilter.java`:
- Around line 13-16: In CacheFilter (used by sendSmallFile) avoid the TOCTOU by
removing the separate contains(uri) call and instead call cache.get(uri) once,
treating a non-null return as a cache hit: call cache.get(uri), if the returned
value is non-null call logCacheHit(uri) and return that value; do not call
contains(uri) or rely on it, since FileCache.put()/LRU eviction can remove the
entry between contains() and get().
In `@src/main/java/org/example/FileCache.java`:
- Around line 22-29: The LRU eviction currently never decrements the
FileCache.currentSize when the anonymous LinkedHashMap.removeEldestEntry evicts
an entry; update the override in the FileCache() constructor to retrieve the
evicted entry via eldest.getValue(), compute its size (e.g., entry.data.length
or entry.getSize()), subtract that amount from currentSize before returning
true, and ensure you null-check eldest.getValue() to avoid NPE; keep the
original eviction condition (cache.size() > MAX_ENTRIES || currentSize >
MAX_CACHE_SIZE) but perform the currentSize adjustment just prior to returning
true so the cache size accounting stays correct.
- Around line 42-50: The get(String key) method in FileCache currently acquires
lock.readLock() but calling cache.get(key) on an access-ordered LinkedHashMap
mutates its ordering; change the method to use lock.writeLock() (acquire/release
the write lock instead of the read lock) around the lookup to prevent concurrent
structural modifications of cache (the LinkedHashMap) and ensure safe access to
CacheEntry and entry.data; keep the null check (Objects.requireNonNull(key)) and
the existing try/finally pattern but swap readLock() calls to writeLock() calls
on the lock field.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 40-63: The current sendGetRequest sends headers via
sendHttpHeaders before reading the body, which allows a 200+Content-Length to
reach the client even if reading the file later fails; change the small-file
path (file.length() < FILE_SIZE_THRESHOLD) to read the file bytes into memory
first (e.g., use the same logic currently in sendSmallFile but perform the read
in sendGetRequest), then call sendHttpHeaders and write the bytes so headers
reflect a successfully-read body, and for the large-file path wrap the
sendLargeFile call in a try-catch inside sendGetRequest (or ensure sendLargeFile
itself catches IOExceptions) to log the error and avoid swallowing exceptions
that would produce silent truncation; reference sendGetRequest, sendHttpHeaders,
sendSmallFile, sendLargeFile and FILE_SIZE_THRESHOLD when making the changes.
- Line 47: The File is being constructed with a URI that may start with a
leading slash so new File(webRoot, uri) becomes an absolute path and bypasses
the webRoot; update resolveTargetFile()/sendGetRequest() flow to strip any
leading '/' (e.g., normalize uri = uri.replaceFirst("^/+", "")) before
constructing the File (the line "File file = new File(webRoot, uri)") and then
proceed to verifyFileIsWithinWebRoot(file) so legitimate requests like
"/index.html" resolve under webRoot; ensure tests are updated to include URIs
with leading slashes to cover this case.
In `@src/test/java/org/example/StaticFileHandlerTest.java`:
- Around line 103-105: Move the AssertJ failure message configuration before the
evaluation call so the custom message is used: in StaticFileHandlerTest (the
assertion around separatorIndex from findHeaderBodySeparator(outputBytes)),
chain .withFailMessage("Kunde inte hitta HTTP-header separator") immediately
after assertThat(separatorIndex) and before .isGreaterThanOrEqualTo(0) so the
message is applied on failure rather than after the assertion has already
executed.
---
Nitpick comments:
In `@src/main/java/org/example/CacheFilter.java`:
- Line 7: CacheFilter currently constructs a concrete FileCache instance
(private final FileCache cache = new FileCache()), preventing DI and
substitution; change CacheFilter to accept a FileCache (preferably an interface
type) via its constructor and store it as a final field instead of new-ing it.
Update the StaticFileHandler constructor where CacheFilter is injected (or where
CacheFilter is created) to pass the shared FileCache implementation (or an
implementation bound in your DI setup). Ensure FileCache is represented as an
interface (e.g., FileCache -> FileCacheInterface) or an abstract type so
different implementations can be provided.
In `@src/main/java/org/example/FileCache.java`:
- Line 14: Extract a FileCache interface and refactor the concrete class into an
implementation (e.g., FileCacheImpl) so the cache can be swapped via DI: create
a public interface named FileCache exposing the methods currently used by the
class (e.g., get, put, invalidate or whatever methods exist on the concrete
FileCache), rename the existing class to FileCacheImpl (or keep it implementing
FileCache) and implement the interface, then modify StaticFileHandler to accept
a FileCache in its constructor (or via setter) and use the interface type
instead of the concrete class so tests or alternate backends (in-memory, Redis)
can be injected.
- Around line 101-109: The CacheEntry.timestamp field is assigned but never
used; either remove that field and its assignment in the CacheEntry constructor,
or implement TTL-based expiration by using CacheEntry.timestamp: add a TTL
constant (e.g., CACHE_TTL_MS) and update the cache access methods (e.g., the
get(...) and any existing put(...) or cleanup methods) to check
System.currentTimeMillis() - entry.timestamp > CACHE_TTL_MS, evict expired
entries before returning values, and consider a periodic cleanup or lazy removal
on access; reference the CacheEntry class and its constructor, and the cache
get/put methods to locate where to apply the change.
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 46-57: The buildHeaders() method always appends "Content-Length"
and "Connection: close" after iterating the headers map, causing duplicate
headers if callers put those keys into the headers map; update buildHeaders() to
filter out/ignore "Content-Length" and "Connection" (case-insensitively) when
iterating headers (or alternatively remove them from the headers map earlier,
e.g., in setHeaders), so only the canonical values appended by buildHeaders()
appear; ensure you reference the headers map and REASON_PHRASES/statusCode logic
in buildHeaders(), and adjust or document behavior used by callers like
StaticFileHandler to avoid duplicates.
- Around line 60-68: The method build() currently declares a local int
contentLength that shadows the instance field contentLength; remove the local
variable and assign the computed length directly to the instance field
(this.contentLength) so both code paths update the same field. In build(), when
body is empty and bytebody != null setBody(new String(bytebody,
StandardCharsets.UTF_8)) and then assign this.contentLength = bytebody.length;
otherwise compute this.contentLength =
body.getBytes(StandardCharsets.UTF_8).length (promote to long if needed) so the
class-level contentLength consistently reflects the computed value; adjust any
downstream code expecting a long accordingly.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 122-131: The lambda passed to cacheFilter.getOrFetch in
sendSmallFile currently ignores its parameter and closes over the outer file,
causing the provider to always read the same File; change the provider so it
actually uses its argument (the FileProvider.fetch(String path) parameter) to
read bytes (e.g., use Files.readAllBytes(Paths.get(path)) or new
File(path).toPath()) and ensure the key you pass (uri) matches the path
semantics expected by getOrFetch so the cached entry corresponds to the provided
path; update sendSmallFile, cacheFilter.getOrFetch call, and the lambda to
reference the path parameter instead of the captured file.
- Around line 97-120: Replace the long if-else in
StaticFileHandler.getContentType with a static Map<String,String> (e.g.
EXTENSION_CONTENT_TYPE) that maps extensions like "html","css","js","png", etc.
to their MIME types; initialize the map in a static block or as an immutable
map, then in getContentType extract the file extension from the uri, look it up
in EXTENSION_CONTENT_TYPE and return the mapped value if present; if not found,
call Files.probeContentType(path) as a fallback (catching IOExceptions) and
finally default to "application/octet-stream".
In `@src/test/java/org/example/FileCacheTest.java`:
- Around line 7-117: Add a new unit test that exercises FileCache's LRU eviction
by inserting enough entries to exceed MAX_CACHE_SIZE (and/or MAX_ENTRIES) and
asserting older keys are evicted and getCurrentSizeInBytes() stays <=
MAX_CACHE_SIZE; specifically target the eviction path exercised by
FileCache.removeEldestEntry and verify both contains(key) and get(key) for
evicted vs retained keys and that getCurrentSizeInBytes() does not drift upward
after multiple evictions to catch the bug in currentSize accounting.
In `@src/test/java/org/example/StaticFileHandlerTest.java`:
- Around line 206-244: Update the two tests to actually verify cache behavior
rather than just 200 responses: in testSmallFileUsesCaching, replace or wrap the
real CacheFilter with a mock/spy (or inject a test double) and assert that the
cache's fetch/store methods (e.g., CacheFilter.fetch or CacheFilter.get/put) are
called only once across the two handler.sendGetRequest calls for "cached.html";
alternatively, after the first successful request delete the underlying file and
assert the second request still returns 200 to prove the response came from
cache. In testLargeFileNotCached, similarly use a mock/spy CacheFilter and
assert that fetch/get is invoked for every handler.sendGetRequest call (i.e.,
called twice), or keep the file and verify that removing the file before the
second request causes the second request to fail if not cached (to prove no
caching). Ensure the tests reference handler.sendGetRequest and the CacheFilter
methods used by your server to locate instrumentation points.
- Around line 148-171: The test testStreamingFilesDoNotExceedMemoryThreshold is
flaky because it measures JVM heap via Runtime and uses ByteArrayOutputStream
which buffers the whole payload; replace the memory-based assertion with a
deterministic check: use a non-buffering counting OutputStream (implement a
simple OutputStream that increments a long counter on write) instead of
ByteArrayOutputStream, call handler.sendGetRequest(output, "memory_test.bin"),
assert the counter equals the expected file length and remove the Runtime memory
measurements and the 50MB assertion; reference the test method name
testStreamingFilesDoNotExceedMemoryThreshold and handler.sendGetRequest when
making the change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/org/example/CacheFilter.javasrc/main/java/org/example/FileCache.javasrc/main/java/org/example/StaticFileHandler.javasrc/main/java/org/example/http/HttpResponseBuilder.javasrc/test/java/org/example/FileCacheTest.javasrc/test/java/org/example/StaticFileHandlerTest.java
* Added basic YAML config-file. * Added class ConfigLoader with static classes for encapsulation * Added static metod loadOnce and step one of static method load * Added static method createMapperFor that checks for YAML or JSON-files before creating an ObjectMapper object. * implement ConfigLoader refs #13 * Added AppConfig.java record for config after coderabbit feedback * Updated ConfigLoader to use AppConfig record and jackson 3 * Added tests for ConfigLoader and reset cached method in ConfigLoader to ensure test isolation with static cache * Removed unused dependency. Minor readability tweaks in AppConfig. * Added check for illegal port numbers to withDefaultsApplied-method. * Added test for illegal port numbers.
* Add 404 error handling to `StaticFileHandler` with fallback page * Add test coverage for `StaticFileHandler` and improve constructor flexibility - Introduced a new constructor in `StaticFileHandler` to support custom web root paths, improving testability. - Added `StaticFileHandlerTest` to validate static file serving and error handling logic. * Add test for 404 handling in `StaticFileHandler` - Added a test to ensure nonexistent files return a 404 response. - Utilized a temporary directory and fallback file for improved test isolation. * Verify `Content-Type` header in `StaticFileHandlerTest` after running Pittest, mutations survived where setHeaders could be removed without test failure. * Remove unused `.btn` styles from `pageNotFound.html` * Improve 404 handling in `StaticFileHandler`: add fallback to plain text response if `pageNotFound.html` is missing, and fix typo in `pageNotFound.html`, after comments from CodeRabbit.
* Implements configuration loading and ensures that ConfigLoader is invoked during application startup (App.main). * Minor formating.
* initial commit, added interfaces Filter and FilterChain * Added HttpRequest class, groups together all information about a request that the server needs and easier to handle by future filters * added interfaces Filter and FilterChain with Java Servlet Filter architecture. * added FilterChainImpl * Corrected imports from JDKS HttpRequest, to projects HttpRequest class * Changed, params for FilterChain * Updated HttpRequest with attributes, * Revert "Updated HttpRequest with attributes," This reverts commit 0fd490e.
* Added MIME detector class and test class * Added logic for Mime detector class * Added Unit tests * Added logic in HttpResponseBuilder and tests to try it out * Solves duplicate header issue * Removed a file for another issue * Changed hashmap to Treemap per code rabbits suggestion * Corrected logic error that was failing tests as per P2P review * Added more reason phrases and unit testing, also applied code rabbits suggestions! * Added changes to Responsebuilder to make merging easier * Changed back to earlier commit to hande byte corruption new PR * Added StaticFileHandler from main * Added staticFileHandler with binary-safe writing * Fix: Normalize Content-Type charset to uppercase UTF-8 Changed 'charset=utf-8' to 'charset=UTF-8' in StaticFileHandlerTest to match MimeTypeDetector output and align with HTTP RFC standard. Uppercase UTF-8 is the correct format per RFC 2616/7231.
* Update Dockerfile Dockerfiles now copies www folder aswell * Added building and copying of dependency jar files * Fix dependency path in Dockerfile and update classpath in ENTRYPOINT configuration. * Fixed typo in Entrypoint configuration * Expose port 8080 in Dockerfile and changed appuser to come before ENTRYPOINT configuration. * Adjust Dockerfile paths for classes and dependencies, update `COPY` targets accordingly.
* Added comprehensive README.MD * Added formatting recommendations, clearer info
* Prevent path traversal and sanitize URI in StaticFileHandler. * Add test for path traversal protection and support 403 responses. * Add tests for URI sanitization and handling of encoded/special URLs. * Add test for null byte injection prevention in StaticFileHandler * Improve StaticFileHandler path traversal handling and test coverage * Improve URI sanitization and add test for 404 response handling in StaticFileHandler
* Resolve port: CLI > config > default * Wire port resolution to AppConfig/ConfigLoader and update docs/tests * Update PortConfigurationGuide.md * Update PortConfigurationGuide.md * Remove ServerPortResolver; use ConfigLoader for port * Update PortConfigurationGuide.md * Update PortConfigurationGuide.md * may be done
* refactor: add predefined HTTP status code constants to HttpResponseBuilder * refactor: use status code constants in StaticFileHandler * test: refactor StaticFileHandlerTest to use status code constants * test: refactor HttpResponseBuilderTest to use status code constants and explicit assertations
* Fix path in Dockerfile for `www` directory copy operation * Correct relative path for `www` directory in Dockerfile copy operation
* Add IpFilter and corresponding test skeleton Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Extend IpFilter with blocklist mode and add unit tests Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Enhance IpFilter to return 403 for blocked IPs and add corresponding test case Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Extend IpFilter with allowlist mode and add corresponding unit tests Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Refactor IpFilter to support both allowlist and blocklist modes, update logic, and add unit tests for allowlist mode Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Handle missing client IP in IpFilter, return 400 response, and add corresponding test case Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Refactor tests in `IpFilterTest` to use `assertAll` for improved assertion grouping and readability Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Refactor `IpFilter` to use `HttpResponse` instead of `HttpResponseBuilder` and update tests accordingly Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Add unit tests for edge cases in `IpFilter` allowlist and blocklist modes Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Refactor `IpFilter` and tests to use `HttpResponseBuilder` instead of `HttpResponse` Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Handle empty client IP in `IpFilter`, return 400 response, and add corresponding test case Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Add comments to `IpFilter` empty methods, clarifying no action is required Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Fix typos in comments within `IpFilterTest` Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Add Javadoc comments to `IpFilter` and `IpFilterTest` for improved clarity and documentation * Refactor `IpFilter` to use thread-safe collections and normalize IP addresses * Make `mode` field in `IpFilter` volatile to ensure thread safety * Ensure UTF-8 encoding for response string in `IpFilterTest` and add attribute management to `HttpRequest` * Ensure UTF-8 encoding for response string in `IpFilterTest` and add attribute management to `HttpRequest` Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Integrate IP filtering into `ConnectionHandler` and update configuration to support filter settings Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Refactor IP filter check in `ConnectionHandler` to use `Boolean.TRUE.equals` for improved null safety Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * Validate null inputs for allowed/blocked IPs in `IpFilter`, enhance test coverage, and fix typographical error in comments Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> * refactor: extract applyFilters() method using FilterChainImpl Co-authored-by: Andreas Kaiberger <andreas.kaiberger@gmail.com> * refactor: cache filter list at construction time Co-authored-by: Andreas Kaiberger <andreas.kaiberger@gmail.com> * refactor: cache filter list at construction time Co-authored-by: Andreas Kaiberger <andreas.kaiberger@gmail.com> * test: verify GPG signing * Replace hardcoded status codes in `IpFilter` and `ConnectionHandler` with constants from `HttpResponseBuilder` for better maintainability Co-authored-by: Rickard Ankar <rickard.ankar@iths.se> --------- Co-authored-by: Rickard Ankar <rickard.ankar@iths.se>
* Re-commit LocaleFilter + tests to clean branch for PR * Update LocaleFilter to handle quality weights and improve javadoc * Fix: rename test method to reflect actual headers scenario * Fix: ensure resolveLocale never returns empty string; strip quality weights
…re validering och beroendeinjektion i StaticFileHandler, samt uppdaterat hantering i CacheFilter
…soptimerad hantering av små filer, förbättrad URI-säkerhet, samt lagt till omfattande tester
…ache-hantering i FileCache med evictions och samtidighet, uppdaterad StaticFileHandler med bättre säkerhet, logik för streaming av stora filer, samt förbättrade tester
|
Smart användning av read/write-locks, bra jobbat! |
AntonAhlqvist
approved these changes
Feb 26, 2026
Martin-E-Karlsson
approved these changes
Feb 26, 2026
Martin-E-Karlsson
left a comment
Martin-E-Karlsson
left a comment
There was a problem hiding this comment.
Riktigt bra jobbat, det blev verkligen ett otroligt stort job du tog på dig! Toppen att du tog dig igenom alla merge issues dessutom! All funktionalitet verkar vara på plats och testerna av bade FelacheTest och de tillagda StaticFileHandler ser ut att vara täckande, väl implementerade och fungerar vid körning efter temporär merge med main.
Jag har framför allt nitpicks om magic numbers, språkanvänding och dokumentation. Vi har så vitt inte jag vet bestämt om vi ska hålla oss till Engelska eller inte i koden. Men jag tror att det är bra om exception meddelanden och annan kommunikation mot användaren är på engelska. Nitpicks:
HttpResponseBuilder:
- Saknar Docstring
CacheFilter - 10-11: Svenska exception meddelanden
- Saknar Docstrings
Filecache: - Docstrings och kommentarer skrivna på Svenska
- 25: Initialisera Capacity och Load Factor som variable?
- 34: Kvarglömd minneskommentar
- 48, 59, 60: Svenska exception meddelanden
- 67, 68, 107: Borde 1024 värdena lagras i en KILOBYTE_SIZE variable?
StaticFileHandler: - Docstrings saknas på samtliga metoder
StaticFileHandlerTest: - 169-170: Docstring på svenska
- 111, 113, 132, 134, 153, 155: Värden som troligtvis borde lagras i beskrivande variable.
Som sagt jag har bara nitpicks, all funktionalitet ser bra ut!
EdvinSandgren
approved these changes
Feb 26, 2026
EdvinSandgren
left a comment
EdvinSandgren
left a comment
There was a problem hiding this comment.
Tycker att det ser mycket bra ut! Bra jobbat.
|
tycker också det ser bra ut, snyggt jobbat! |
Kathify
approved these changes
Feb 28, 2026
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
14 participants
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.
Summary by CodeRabbit
New Features
Documentation
Tests
Assets