-
Notifications
You must be signed in to change notification settings - Fork 789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add request decompression to server GZip middleware #7119
base: series/0.23
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,10 +36,11 @@ object GZip extends GZipPlatform { | |
isZippable: Response[G] => Boolean = defaultIsZippable[G](_: Response[G]), | ||
): Http[F, G] = | ||
Kleisli { (req: Request[G]) => | ||
req.headers.get[`Accept-Encoding`] match { | ||
val unzippedRequest = unzipOrPass(req, bufferSize) | ||
unzippedRequest.headers.get[`Accept-Encoding`] match { | ||
case Some(acceptEncoding) if satisfiedByGzip(acceptEncoding) => | ||
http(req).map(zipOrPass(_, bufferSize, level, isZippable)) | ||
case _ => http(req) | ||
http(unzippedRequest).map(zipOrPass(_, bufferSize, level, isZippable)) | ||
case _ => http(unzippedRequest) | ||
} | ||
} | ||
|
||
|
@@ -51,6 +52,11 @@ object GZip extends GZipPlatform { | |
(contentType.get.mediaType == MediaType.application.`octet-stream`)) | ||
} | ||
|
||
private def isZipped[F[_]](req: Request[F]): Boolean = | ||
req.headers.get[`Content-Encoding`].map(_.contentCoding).exists { coding => | ||
coding === ContentCoding.gzip || coding === ContentCoding.`x-gzip` | ||
} | ||
|
||
private def satisfiedByGzip(acceptEncoding: `Accept-Encoding`) = | ||
acceptEncoding.satisfiedBy(ContentCoding.gzip) || acceptEncoding.satisfiedBy( | ||
ContentCoding.`x-gzip` | ||
|
@@ -67,6 +73,15 @@ object GZip extends GZipPlatform { | |
case resp => resp // Don't touch it, Content-Encoding already set | ||
} | ||
|
||
private def unzipOrPass[F[_]: Compression]( | ||
request: Request[F], | ||
bufferSize: Int, | ||
): Request[F] = | ||
request match { | ||
case req if isZipped(req) => unzipRequest(bufferSize, req) | ||
case req => req | ||
} | ||
|
||
private def zipResponse[F[_]: Compression]( | ||
bufferSize: Int, | ||
level: DeflateParams.Level, | ||
|
@@ -89,4 +104,16 @@ object GZip extends GZipPlatform { | |
.putHeaders(`Content-Encoding`(ContentCoding.gzip)) | ||
.pipeBodyThrough(compressPipe) | ||
} | ||
|
||
private def unzipRequest[F[_]: Compression]( | ||
bufferSize: Int, | ||
req: Request[F], | ||
): Request[F] = { | ||
val decompressPipe = Compression[F].gunzip(bufferSize = bufferSize).andThenF(_.content) | ||
logger.trace("GZip middleware decoding content").unsafeRunSync() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love running this effect like this, though I see that it is status quo. Perhaps in 1.x we should move the logging out to a separate wrapper method. Something like, object Gzip extends GzipPlatform {
def withLogging[F[_]: Functor, G[_]: Compression: Sync](...):` Http[F, G] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of asking for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see. Makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like out of scope of this PR, doesn't it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing existing code is out-of-scope. Avoiding writing new code with anti-pattterns (established as they may be) is in-scope :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it! I'll change it a little There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change seems to have some repercussions. Biggest one being breaking compatibility but also when working on it I had an issue with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What's the error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I change the signature of
But |
||
req | ||
.removeHeader[`Content-Length`] | ||
.removeHeader[`Content-Encoding`] | ||
.pipeBodyThrough(decompressPipe) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the
.handleErrorWith
in #5546 (comment) is a good idea to throwMalformedMessageBodyFailure
for streams that aren't valid gzip streams, but maybe it could be more specific (and I should have added a test for it).