Move decompression to earlier phase in receivePipeline.#5360
Move decompression to earlier phase in receivePipeline.#5360ascheja wants to merge 2 commits intoktorio:mainfrom
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:
WalkthroughRefactors the compression plugin to decode request bodies in a new internal ContentDecoding pipeline phase and changes the decode receiver to ContentDecoding.Context; adds ContentDecoding implementation, updates plugin interception, and adds tests plus a test dependency to validate request decompression scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@ktor-server/ktor-server-plugins/ktor-server-compression/jvm/src/io/ktor/server/plugins/compression/ContentDecoding.kt`:
- Around line 1-3: Update the copyright header in ContentDecoding.kt to match
the rest of the PR (use "2014-2026" as in CompressionTest.kt) so the file header
reads the same as other new files; locate the header comment at the top of
ContentDecoding.kt and replace "2014-2024" with "2014-2026".
- Around line 23-32: The plugin is creating a new
PipelinePhase("ContentDecoding") on every install which defeats the pipeline's
reference-equality dedupe; change ContentDecoding.install to use a single shared
phase instance instead of constructing a new one each time (e.g., add a
companion-object/singleton Phase or reuse a predefined
ApplicationReceivePipeline phase) and then call
pipeline.receivePipeline.insertPhaseBefore(ApplicationReceivePipeline.Transform,
Phase) and pipeline.receivePipeline.intercept(Phase) { ... } so duplicate
installs (e.g., on nested routes) will not add multiple ContentDecoding phases.
🧹 Nitpick comments (2)
ktor-server/ktor-server-plugins/ktor-server-compression/jvm/src/io/ktor/server/plugins/compression/ContentDecoding.kt (1)
7-10: Use star imports forio.ktor.*packages.Lines 8 and 10 use specific imports from
io.ktor.*packages. As per coding guidelines, star imports should be used for these.Proposed fix
import io.ktor.server.application.* -import io.ktor.server.request.ApplicationReceivePipeline +import io.ktor.server.request.* import io.ktor.util.pipeline.* -import io.ktor.utils.io.ByteReadChannel +import io.ktor.utils.io.*As per coding guidelines: "Use star imports for
io.ktor.*packages".ktor-server/ktor-server-plugins/ktor-server-compression/jvm/test/io/ktor/server/plugins/compression/CompressionTest.kt (1)
7-37: Use star imports forio.ktor.*packages.Multiple specific imports from
io.ktor.*packages should be star imports per project conventions. For example:Proposed fix (partial)
-import io.ktor.client.request.forms.MultiPartFormDataContent -import io.ktor.client.request.forms.formData -import io.ktor.client.request.header -import io.ktor.client.request.post -import io.ktor.client.request.setBody -import io.ktor.client.statement.bodyAsBytes -import io.ktor.client.statement.bodyAsText -import io.ktor.http.ContentType -import io.ktor.http.HttpHeaders -import io.ktor.http.content.PartData -import io.ktor.http.content.forEachPart -import io.ktor.serialization.kotlinx.json.json -import io.ktor.server.application.Application -import io.ktor.server.application.install -import io.ktor.server.plugins.contentnegotiation.ContentNegotiation -import io.ktor.server.request.receive -import io.ktor.server.request.receiveMultipart -import io.ktor.server.response.respond -import io.ktor.server.response.respondText -import io.ktor.server.routing.post -import io.ktor.server.routing.routing -import io.ktor.server.testing.testApplication -import io.ktor.utils.io.ByteChannel -import io.ktor.utils.io.toByteArray +import io.ktor.client.request.forms.* +import io.ktor.client.request.* +import io.ktor.client.statement.* +import io.ktor.http.* +import io.ktor.http.content.* +import io.ktor.serialization.kotlinx.json.* +import io.ktor.server.application.* +import io.ktor.server.plugins.contentnegotiation.* +import io.ktor.server.request.* +import io.ktor.server.response.* +import io.ktor.server.routing.* +import io.ktor.server.testing.* +import io.ktor.utils.io.*As per coding guidelines: "Use star imports for
io.ktor.*packages".
...lugins/ktor-server-compression/jvm/src/io/ktor/server/plugins/compression/ContentDecoding.kt
Show resolved
Hide resolved
...lugins/ktor-server-compression/jvm/src/io/ktor/server/plugins/compression/ContentDecoding.kt
Show resolved
Hide resolved
31a6c87 to
06c7d95
Compare
bjhham
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
We'll need to assess whether or not anyone might be relying on the old behavior here to consider including this in a minor release. It's possible some people may want to store the raw compressed bytes, but unlikely - anyway, I'll get back to you on this.
|
Looks like this introduces a couple failures in |
|
Sorry, overlooked that test class as it was in a completely different subproject. Moved my tests over to it and adjusted the assertions for the two failing tests (both received as |
69bd3d6 to
8babd74
Compare
|
Could you please run |
8babd74 to
90878e0
Compare
|
Did that, but it looks like the linter check is still failing? |
90878e0 to
45c01c6
Compare
Fixes #KTOR-6683.
If request body decompression is enabled the received body should equal the uncompressed bytes. Fixes #KTOR-6683.
45c01c6 to
4bb153c
Compare
|
Figured it out - the problem was a line with more than 120 characters, but |
Subsystem
Server, compression
Motivation
See KTOR-6683.
Currently content decoding only works in cases where the application directly receives a
ByteReadChannel. When combining it with any default transformations (ByteArray,Parameters,MultiPartData, ...) the content decoding would silently not work because the Compression'stransformBodywould already get the converted subject from the automatically installed default transformations. The same principle applies to theContentNegotationplugin if it is installed before theCompressionplugin, although in that case the transformation would likely fail.Solution
Simply moves the content decoding into it's own phase right before the default
Transformphase.