Skip to content
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

Gzip middleware for client #2673

Merged
merged 4 commits into from Jul 2, 2019
Merged

Conversation

@balatbn
Copy link

@balatbn balatbn commented Jun 26, 2019

Uses compress from fs2

private val supportedCompressions =
Seq(ContentCoding.gzip.coding, ContentCoding.deflate.coding).mkString(", ")

def apply[F[_]: Sync](bufferSize: Int = 32 * 1024)(client: Client[F]): Client[F] =

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a Sync constraint necessary?

Loading

Copy link
Author

@balatbn balatbn Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fs2.compress.gunip requires an F that is RaiseThrowable. Since RaiseThrowable is from fs2, I added Sync here. Can we use any other type instead?

Loading

Copy link
Member

@rossabaker rossabaker Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ApplicativeError?

Loading

Copy link
Author

@balatbn balatbn Jun 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried changing it to ApplicativeError. Now, I'm getting missing Bracket error for new Client creation. I'll update PR by changing this to Bracket

Loading

Copy link
Author

@balatbn balatbn Jun 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated PR

Loading

Copy link
Member

@rossabaker rossabaker left a comment

Thanks, this looks like a good start.

Loading

response.headers.get(`Content-Encoding`) match {
case Some(header)
if header.contentCoding == ContentCoding.gzip || header.contentCoding == ContentCoding.`x-gzip` =>
response.body.through(fs2.compress.gunzip(bufferSize))
Copy link
Member

@rossabaker rossabaker Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this remove that ContentCoding as well? Similar on the deflate case.

Loading

Copy link
Author

@balatbn balatbn Jun 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it is better not to touch this header as client code can use this to confirm whether response was actually compressed. I can remove this if this is not required.

Loading

Copy link
Member

@rossabaker rossabaker Jun 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better that we do, because the Content-Coding no longer reflects reality. This is important in case the client is used in a proxy. We should also be sure Content-Length is gone. I can't remember offhand whether withBodyStream takes care of this or not.

Loading

Copy link
Author

@balatbn balatbn Jun 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed code to remove both the headers

Loading

Copy link
Member

@rossabaker rossabaker left a comment

Looks good to me once the headers are cleaned up.

Loading

response.headers.get(`Content-Encoding`) match {
case Some(header)
if header.contentCoding == ContentCoding.gzip || header.contentCoding == ContentCoding.`x-gzip` =>
response.body.through(fs2.compress.gunzip(bufferSize))
Copy link
Member

@rossabaker rossabaker Jun 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better that we do, because the Content-Coding no longer reflects reality. This is important in case the client is used in a proxy. We should also be sure Content-Length is gone. I can't remember offhand whether withBodyStream takes care of this or not.

Loading

@ChristopherDavenport ChristopherDavenport merged commit de2dfcb into http4s:master Jul 2, 2019
1 check passed
Loading
rossabaker added a commit to rossabaker/http4s that referenced this issue Jul 3, 2019
rossabaker added a commit that referenced this issue Jul 3, 2019
Backport #2673: Gzip middleware for client
@balatbn balatbn deleted the clientgzip branch Jul 3, 2019
desbo added a commit to desbo/http4s that referenced this issue Feb 11, 2021
When the gzip middleware decompresses a response, these headers are no
longer correct, so it seems best to remove them. This is crucial when
the client is used as a proxy (or, in my case, as an STTP backend) to
prevent downstream consumers attempting to decompress the response
again.

This change was previously introduced here:
http4s#2673 (comment) but was
removed (possibly unintentionally?) in this PR: http4s#3476
desbo added a commit to desbo/http4s that referenced this issue Feb 11, 2021
When the gzip middleware decompresses a response, these headers are no
longer correct, so it seems best to remove them. This is crucial when
the client is used as a proxy (or, in my case, as an STTP backend) to
prevent downstream consumers attempting to decompress the response
again.

This change was previously introduced here:
http4s#2673 (comment) but was
removed (possibly unintentionally?) in this PR: http4s#3476
desbo added a commit to desbo/http4s that referenced this issue Feb 11, 2021
When the gzip middleware decompresses a response, these headers are no
longer correct, so it seems best to remove them. This is crucial when
the client is used as a proxy (or, in my case, as an STTP backend) to
prevent downstream consumers attempting to decompress the response
again.

This change was previously introduced here:
http4s#2673 (comment) but was
removed (possibly unintentionally?) in this PR: http4s#3476
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants