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

Fix Compression #1478

Merged
merged 7 commits into from
Oct 8, 2021
Merged

Fix Compression #1478

merged 7 commits into from
Oct 8, 2021

Conversation

johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented Aug 30, 2021

The Problem

The compression option is badly broken.

This is from the documents:

| `compression`        | Boolean         | Whether to compress requests. Gzip compression is used. Defaults to `false`. Responses are automatically inflated if they are compressed. If a custom transport object is used, it must handle the request compression and response inflation.

In fact, much of this is not working as per spec.

  1. Whether to compress requests. --> NOT WORKING The option does not currently compress requests, it merely sets the Accept-Encoding header which only affects responses.
  2. Responses are automatically inflated if they are compressed. --> NOT WORKING. Responses are currently not inflated unless the compression option is explicitly set. This means that if compression is not set, any compressed responses will be unreadable (i.e. cause an error.)

The Fix

  1. compression option should compress outbound requests by setting the Content-Encoding header.
  2. Add code to each of the HTTP adapters to gzip the request body. (If already gzipped, ensure the Content-Encoding header is set.)
  3. Decompress compressed responses, whether or not compression is set.

- Decompress compressed responses, whether or not use_compression is set.
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@johnnyshields
Copy link
Contributor Author

@picandocodigo can you please review this?

@johnnyshields
Copy link
Contributor Author

At TableCheck we are using this in production by the way (with Faraday)

@picandocodigo
Copy link
Member

Thanks for this contribution @johnnyshields! I'll review and merge it as soon as possible 👍

@picandocodigo
Copy link
Member

jenkins test this please

@picandocodigo picandocodigo merged commit d5f08c0 into elastic:main Oct 8, 2021
@picandocodigo
Copy link
Member

Thank you very much for this contribution @johnnyshields!

an-empty-string pushed a commit to ActionNetwork/elasticsearch-ruby that referenced this pull request Mar 8, 2023
- "compression" option should compress outbound requests.
- Decompress compressed responses, whether or not use_compression is set.

Properly set header on gzipped request body
Perform compression inside HTTP adapters
Explicitly require zlib library
Content-Encoding is set in the HTTP adapters, so we don't need it in the apply_headers method
Add specs for curb, faraday, and manticore
Update curb_spec.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants