28 file format compression filter - #82
Conversation
…format-compression-filter
…format-compression-filter
|
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 a new CompressionFilter that conditionally gzip-compresses HTTP responses based on Accept-Encoding, response size, and content type; adds tests covering compression behaviors and adds header/body accessors to HttpResponseBuilder. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Filter as CompressionFilter
participant Chain as FilterChain
participant Resp as HttpResponseBuilder
Client->>Filter: HTTP request (may include Accept-Encoding)
Filter->>Chain: doFilter(request, responseBuilder)
Chain-->>Resp: populate response (status, headers, body)
Chain-->>Filter: return control
Filter->>Resp: inspect headers/body
alt should compress
Filter->>Resp: gzip-compress body
Filter->>Resp: set Content-Encoding: gzip\nmerge Vary: Accept-Encoding
end
Filter->>Client: final response (possibly gzipped)
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: 5
🧹 Nitpick comments (4)
src/main/java/org/example/filter/CompressionFilter.java (2)
104-123:mergeHeadersis unnecessary — use the already-publicsetHeaderAPI
HttpResponseBuilderalready exposessetHeader(String, String)as a public method (seeHttpResponseBuilder.javalines 85–87). The entiremergeHeadersmethod plus the reflection read of theheadersfield can be eliminated. Simply callresponse.setHeader("Content-Encoding", "gzip")andresponse.setHeader("Vary", "Accept-Encoding")directly — no reflection, no intermediateHashMap, no encapsulation breach.Additionally, the intermediate
HashMapis case-sensitive, which creates an ordering hazard when the existingTreeMap(CASE_INSENSITIVE_ORDER)entries are copied in: if there are mixed-case duplicates, the final order inside theHashMapis unspecified beforesetHeadersnormalises them again in its newTreeMap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/CompressionFilter.java` around lines 104 - 123, Remove the private mergeHeaders method and its reflective access to HttpResponseBuilder.headers; instead, call the public API on the existing HttpResponseBuilder instance to set headers directly (e.g. use response.setHeader("Content-Encoding", "gzip") and response.setHeader("Vary", "Accept-Encoding")). Replace any callers of mergeHeaders(...) with direct setHeader calls so no intermediate HashMap or reflection is used and header case/ordering issues are avoided.
70-101: ReplaceSystem.out/err.printlnwith a proper loggerScattered
System.out.println/System.err.printlncalls should be replaced by a standard logging framework (e.g.,java.util.logging.Logger, SLF4J, or whatever the project already uses) so that log levels, output destinations, and formatting can be controlled without code changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/CompressionFilter.java` around lines 70 - 101, Replace all System.out.println/System.err.println calls in CompressionFilter with the project's logging framework: add a private static final Logger (e.g., LOGGER) in CompressionFilter and use LOGGER.info/debug for informational messages like "Client accepts gzip compression", "Body too small...", "Skipping compression...", "Compressed ...", and "Added Content-Encoding: gzip header", and use LOGGER.error for the exception case in the catch block around gzipCompress; ensure you include the exception (e) in the error log call to capture stacktrace and preserve the same message content and variable values when converting each print to a log statement.src/test/java/org/example/filter/CompressionFilterTest.java (2)
19-51: Missing assertions on response headers after compressionThe
testGzipCompressionWhenClientSupportsIttest (and the JSON/charset variants) only verifies the body size and decompressed content. It never asserts thatContent-Encoding: gzipandVary: Accept-Encodingwere actually added to the response headers. A filter that compressed the body but forgot to setContent-Encodingwould silently pass all current tests yet produce broken responses in production.Consider adding (once public
getHeaderis available onHttpResponseBuilder, or via the same reflection helper already present in the test):// assert headers set correctly String contentEncoding = getHeaderFromResponse(response, "Content-Encoding"); assertEquals("gzip", contentEncoding, "Content-Encoding header must be set"); String vary = getHeaderFromResponse(response, "Vary"); assertNotNull(vary); assertTrue(vary.contains("Accept-Encoding"), "Vary header must include Accept-Encoding");🤖 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 19 - 51, The testGzipCompressionWhenClientSupportsIt currently only checks body size and decompression; update it to also assert that the response headers were updated by CompressionFilter: use the existing reflection helper or the public getHeaderFromResponse to read headers from the HttpResponseBuilder and assert that "Content-Encoding" equals "gzip" and that the "Vary" header is present and contains "Accept-Encoding" (use the same helper already used elsewhere in tests to fetch headers so the assertions target the actual headers set by CompressionFilter).
1-14: Consider upgrading tojunit-jupiter6.0.3The test file uses junit-jupiter 6.0.2 (released January 6, 2026). The latest GA release is JUnit 6.0.3 (February 15, 2026), which includes several bug fixes. While 6.0.2 is fully functional, bumping to 6.0.3 is a trivial change and picks up those fixes for free.
🤖 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 1 - 14, Update the JUnit Jupiter dependency used by tests to 6.0.3: change the junit-jupiter artifact version (org.junit.jupiter:junit-jupiter or junit-jupiter-engine as applicable) in your build configuration (pom.xml or build.gradle) from 6.0.2 to 6.0.3, then rebuild and run tests (e.g., mvn test or ./gradlew test) to ensure CompressionFilterTest and other tests still pass; the test class to validate is CompressionFilterTest in package org.example.filter.
🤖 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/filter/CompressionFilter.java`:
- Around line 110-116: The reflection reads of private fields must be replaced
by public accessors: add methods to HttpResponseBuilder such as getHeader(String
name) that returns headers.get(name) (respecting the existing case-insensitive
map) and getBodyBytes() that returns bytebody if non-null or
body.getBytes(StandardCharsets.UTF_8) otherwise; then update mergeHeaders,
getResponseContentType, and getResponseBody to call
HttpResponseBuilder.getHeader(...) and getBodyBytes() instead of using
getDeclaredField/setAccessible and remove the reflective code and suppression
annotations.
- Around line 87-89: The log line in CompressionFilter that computes the
compression percentage can overflow because it multiplies compressed.length (an
int) by 100 as a 32-bit int; change the calculation to perform the
multiplication in long to avoid overflow: cast one operand (e.g.,
compressed.length or originalBody.length) to long before multiplying and ensure
the division uses long arithmetic, then format/convert the result back for
printing so the percentage is correct; locate the System.out.println call in
CompressionFilter that references originalBody.length and compressed.length and
update the arithmetic to use long.
- Around line 85-102: The compression block in compressIfNeeded must (1) skip
compression if the response already has a Content-Encoding header (check
response.getHeaders()/containsKey("Content-Encoding") before calling
gzipCompress), (2) ensure Content-Length is not preserved after changing the
body by removing any existing "Content-Length" header from the merged headers so
HttpResponseBuilder.build() can recompute it, and (3) append "Accept-Encoding"
to an existing Vary header instead of overwriting it (if response headers
contain "Vary", set it to existingValue + ", Accept-Encoding", otherwise add
"Vary: Accept-Encoding"); update the call sites around gzipCompress,
response.setBody, mergeHeaders and response.setHeaders accordingly.
In `@src/test/java/org/example/filter/CompressionFilterTest.java`:
- Around line 45-46: The tests use String.getBytes() without a charset which
relies on the platform default; update all occurrences in CompressionFilterTest
(e.g., the assertions comparing compressedBody.length to largeBody.getBytes(),
and the other two occurrences around lines 171 and 193) to call
getBytes(StandardCharsets.UTF_8) so the byte-length comparisons match the
filter's UTF-8 encoding; ensure you import java.nio.charset.StandardCharsets if
not present.
- Around line 98-110: The decompressGzip method leaks resources because
GZIPInputStream (and the ByteArrayInputStream/ByteArrayOutputStream) are not
closed; rewrite decompressGzip to use try-with-resources to create the
GZIPInputStream (and optionally wrap ByteArrayInputStream/ByteArrayOutputStream)
so all streams are automatically closed, while preserving the existing read loop
and returning the UTF-8 string; locate the method by name decompressGzip and
update the instantiation of
GZIPInputStream/ByteArrayInputStream/ByteArrayOutputStream accordingly.
---
Nitpick comments:
In `@src/main/java/org/example/filter/CompressionFilter.java`:
- Around line 104-123: Remove the private mergeHeaders method and its reflective
access to HttpResponseBuilder.headers; instead, call the public API on the
existing HttpResponseBuilder instance to set headers directly (e.g. use
response.setHeader("Content-Encoding", "gzip") and response.setHeader("Vary",
"Accept-Encoding")). Replace any callers of mergeHeaders(...) with direct
setHeader calls so no intermediate HashMap or reflection is used and header
case/ordering issues are avoided.
- Around line 70-101: Replace all System.out.println/System.err.println calls in
CompressionFilter with the project's logging framework: add a private static
final Logger (e.g., LOGGER) in CompressionFilter and use LOGGER.info/debug for
informational messages like "Client accepts gzip compression", "Body too
small...", "Skipping compression...", "Compressed ...", and "Added
Content-Encoding: gzip header", and use LOGGER.error for the exception case in
the catch block around gzipCompress; ensure you include the exception (e) in the
error log call to capture stacktrace and preserve the same message content and
variable values when converting each print to a log statement.
In `@src/test/java/org/example/filter/CompressionFilterTest.java`:
- Around line 19-51: The testGzipCompressionWhenClientSupportsIt currently only
checks body size and decompression; update it to also assert that the response
headers were updated by CompressionFilter: use the existing reflection helper or
the public getHeaderFromResponse to read headers from the HttpResponseBuilder
and assert that "Content-Encoding" equals "gzip" and that the "Vary" header is
present and contains "Accept-Encoding" (use the same helper already used
elsewhere in tests to fetch headers so the assertions target the actual headers
set by CompressionFilter).
- Around line 1-14: Update the JUnit Jupiter dependency used by tests to 6.0.3:
change the junit-jupiter artifact version (org.junit.jupiter:junit-jupiter or
junit-jupiter-engine as applicable) in your build configuration (pom.xml or
build.gradle) from 6.0.2 to 6.0.3, then rebuild and run tests (e.g., mvn test or
./gradlew test) to ensure CompressionFilterTest and other tests still pass; the
test class to validate is CompressionFilterTest in package org.example.filter.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/filter/CompressionFilter.javasrc/test/java/org/example/filter/CompressionFilterTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/example/filter/CompressionFilter.java (1)
109-187:⚠️ Potential issue | 🟠 MajorReflection on private
HttpResponseBuilderfields is still unresolved.Four methods (
hasContentEncoding,getResponseHeader,mergeHeaders,getResponseContentType) all independently callgetDeclaredField("headers") + setAccessible(true). In addition to the fragility and layer-violation concerns raised previously, there is a concrete failure risk inmergeHeaders: if reflection throws at line 156, thecatchis silently swallowed,mergednever receives the original headers, and the subsequentresponse.setHeaders(merged)call wipes all existing headers (includingContent-Type) from the response — leaving onlyContent-Encoding: gzipandVary: Accept-Encoding. This would produce a completely broken response.The fix recommended in the prior review remains valid: add
getHeader(String)andgetBodyBytes()public accessors toHttpResponseBuilder, then remove all reflection from this filter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/CompressionFilter.java` around lines 109 - 187, The filter currently uses fragile reflection on HttpResponseBuilder.headers in hasContentEncoding, getResponseHeader, mergeHeaders, and getResponseContentType which can fail silently and cause mergeHeaders to wipe existing headers; add public accessor methods on HttpResponseBuilder: String getHeader(String name), byte[] getBodyBytes() (or InputStream/ByteBuffer as appropriate), and Map<String,String> getHeaders() (or a safe copy), then remove all reflection calls in CompressionFilter and replace them with calls to getHeader/getHeaders/getBodyBytes; ensure mergeHeaders uses the provided getHeaders() copy, preserves existing non-Content-Length headers, and does not swallow exceptions silently (propagate or log) so a reflection/config error cannot erase response headers.
🤖 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/filter/CompressionFilter.java`:
- Around line 105-106: The catch block in CompressionFilter that swallows
IOException must be changed to either log the exception or rethrow it so
failures in gzipCompress are visible; update the catch in the try/catch around
gzipCompress (and the subsequent setBody/setHeaders flow) to call the class
logger (e.g., logger.error or LOG.error) with a clear message and the exception,
or rethrow a ServletException/IOException from doFilter/doFilterInternal so
callers can handle it; ensure the change references the gzipCompress call and
the setBody/setHeaders paths in CompressionFilter so compression failures are
not silently ignored.
---
Duplicate comments:
In `@src/main/java/org/example/filter/CompressionFilter.java`:
- Around line 109-187: The filter currently uses fragile reflection on
HttpResponseBuilder.headers in hasContentEncoding, getResponseHeader,
mergeHeaders, and getResponseContentType which can fail silently and cause
mergeHeaders to wipe existing headers; add public accessor methods on
HttpResponseBuilder: String getHeader(String name), byte[] getBodyBytes() (or
InputStream/ByteBuffer as appropriate), and Map<String,String> getHeaders() (or
a safe copy), then remove all reflection calls in CompressionFilter and replace
them with calls to getHeader/getHeaders/getBodyBytes; ensure mergeHeaders uses
the provided getHeaders() copy, preserves existing non-Content-Length headers,
and does not swallow exceptions silently (propagate or log) so a
reflection/config error cannot erase response headers.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/filter/CompressionFilter.javasrc/test/java/org/example/filter/CompressionFilterTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/example/filter/CompressionFilterTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/example/http/HttpResponseBuilder.java (1)
90-93: Return a defensive copy ofbytebodyto avoid external mutation.Exposing the internal array allows callers to alter the response content after it’s been set, which can corrupt the response or desync
Content-Length. A defensive copy avoids this without changing behavior for callers that only read.Proposed change
public byte[] getBodyBytes() { - if (bytebody != null) return bytebody; + if (bytebody != null) return bytebody.clone(); return body.getBytes(StandardCharsets.UTF_8); }🤖 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 90 - 93, In HttpResponseBuilder.fix the getBodyBytes method so it returns a defensive copy of the internal bytebody array instead of exposing the array directly: when bytebody is non-null, return Arrays.copyOf(bytebody, bytebody.length) (or equivalent) to prevent external mutation; otherwise continue to return body.getBytes(StandardCharsets.UTF_8) as before; ensure you reference the bytebody field and getBodyBytes method so Content-Length and internal state remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 90-93: In HttpResponseBuilder.fix the getBodyBytes method so it
returns a defensive copy of the internal bytebody array instead of exposing the
array directly: when bytebody is non-null, return Arrays.copyOf(bytebody,
bytebody.length) (or equivalent) to prevent external mutation; otherwise
continue to return body.getBytes(StandardCharsets.UTF_8) as before; ensure you
reference the bytebody field and getBodyBytes method so Content-Length and
internal state remain consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/filter/CompressionFilter.javasrc/main/java/org/example/http/HttpResponseBuilder.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/filter/CompressionFilter.java
|
Bra jobbat! Koden ser korrekt, ren och rimligt faktorerad ut. Testerna täcker nog med cases i mina ögon och ser ut att vara korrekt implementerade ut. Tyvärr går det inte att testa om korrekt response ges under körning med ett Insomnia request för än FilterChainImpl är fullt implementerad. Ska vi vänta med merge tills att CompressionFilter går att testa via Insomnia? |
AnnaZiafar
left a comment
AnnaZiafar
left a comment
There was a problem hiding this comment.
Tycker att koden är väldigt välskriven. Snyggt att du undviker magic numbers och du har en väldigt välgenomtänkt filtrering. Vi måste ju lägga alla dessa filter någonstans sedan men det kanske är bättre att göra ett separat issue, för just nu har vi inte implementerat filterklasserna någonstans i koden. Tycker iaf att denna är redo för merge!
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/example/http/HttpResponseBuilder.java (1)
94-97: Optional: eliminate the duplicated body-resolution logic inbuild().
getBodyBytes()and the body-resolution block inbuild()(lines 112–118) are identical in semantics.build()could delegate togetBodyBytes()to keep the logic in one place.♻️ Proposed refactor — delegate body resolution in
build()togetBodyBytes()public byte[] build() { - byte[] contentBody; - int contentLength; - - if (bytebody != null) { - contentBody = bytebody; - contentLength = bytebody.length; - } else { - contentBody = body.getBytes(StandardCharsets.UTF_8); - contentLength = contentBody.length; - } + byte[] contentBody = getBodyBytes(); + int contentLength = contentBody.length;🤖 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 94 - 97, The body byte-resolution logic is duplicated between getBodyBytes() and the body handling in build(); update build() to delegate to getBodyBytes() instead of reimplementing the same logic. Locate the build() method's body-resolution block and replace its logic with a call to getBodyBytes(), ensuring any local variables or null checks used after resolution still reference the returned byte[] from getBodyBytes(); keep getBodyBytes() as the single source of truth for converting bytebody/body to UTF-8 bytes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 94-97: The body byte-resolution logic is duplicated between
getBodyBytes() and the body handling in build(); update build() to delegate to
getBodyBytes() instead of reimplementing the same logic. Locate the build()
method's body-resolution block and replace its logic with a call to
getBodyBytes(), ensuring any local variables or null checks used after
resolution still reference the returned byte[] from getBodyBytes(); keep
getBodyBytes() as the single source of truth for converting bytebody/body to
UTF-8 bytes.
MartinStenhagen
left a comment
MartinStenhagen
left a comment
There was a problem hiding this comment.
Bra jobbat Johan! Jag instämmer med er andra att det är en bra implementation. Tycker även testerna ser bra ut och är tillräckliga för merge. Om du vill vara extra robust i testerna kan du lägga till ett par edge case tester för:
-
Verifiera att filtret inte försöker komprimera igen om response redan har Content-Encoding (t.ex. gzip).
-
Case-insensitive hantering av Accept-Encoding: Säkerställ att headern upptäcks oavsett casing (accept-encoding, ACCEPT-ENCODING, etc).
-
Korrekt hantering av befintlig Vary: Om Vary redan finns (t.ex. Vary: Origin), bör Accept-Encoding appendas utan att dupliceras.
Jag är på Annas linje att koden är redo att mergas redan nu, även om det inte går att göra integrationstester med Insomina ännu. Filtret är relativt isolerat och lätt att ta bort ur filterkedjan om något skulle krångla efter merge. @Martin-E-Karlsson det kanske kan bli en egen issue att göra integrationstester med insomina när FilterChainImpl är klar?
|
@MartinStenhagen |
Martin-E-Karlsson
left a comment
Martin-E-Karlsson
left a comment
There was a problem hiding this comment.
Jag citerar helt enkelt mig själv från tidigare kommentar:
"Bra jobbat! Koden ser korrekt, ren och rimligt faktorerad ut. Testerna täcker nog med cases i mina ögon och ser ut att vara korrekt implementerade ut."
Jag prövade precis att köra app och tester på branchen med alla nuvarande ändringar i main mergeade. All verkar fungera så jag ger grön ljus!
4 participants
Summary by CodeRabbit