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

Set content-length when decompressing #192

Merged

Conversation

tanguilp
Copy link
Contributor

When decompressing, the content-length is left unchanged. As a consequence, it's no longer matches the body's length, and could confuse other steps or users.

This PR automatically sets content-length after decompression.

It could be discussed if it's worth adding it if it wasn't there in the first place. I'd argue it cannot hurt, and proxies routinely do this. Not only it doesn't hurt, but it can help a client know when the response has been fully recieved (should Req be used in a reverse-proxy one day) especially when using multiplexing (Connection: keep-alive).

Not sure if we should update the documentation to notice users about these changes.

@tanguilp
Copy link
Contributor Author

Asked about Tesla here: elixir-tesla/tesla#598

test/req/steps_test.exs Outdated Show resolved Hide resolved
test/req/steps_test.exs Outdated Show resolved Hide resolved
@wojtekmach
Copy link
Owner

Thank you! Could you rebase on main and mention this behaviour in docs?

@tanguilp tanguilp force-pushed the set-content-length-when-decompressing branch from 495821f to 54a8795 Compare June 23, 2023 12:54
lib/req/steps.ex Outdated
Comment on lines 820 to 865
This step updates the following headers to reflect the changes:
- `content-legnth` is set to the length of the decompressed body
- `content-length` is set to the length of the decompressed body
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll have to add that content-encoding is removed, but this will be done in another PR. Hence the list with one item for now

@tanguilp
Copy link
Contributor Author

I've rebased and updated the doc. Thanks for the review!

@tanguilp tanguilp force-pushed the set-content-length-when-decompressing branch from 0ce7e1e to ca5e882 Compare August 24, 2023 16:38
@tanguilp tanguilp marked this pull request as ready for review August 24, 2023 16:41
@tanguilp
Copy link
Contributor Author

Reopening after fixing conflicts (it was out-of-sync).

In the current state, this PR is a prerequisite for #190 but let me know if you wanna merge in a different order (or merge at all).

@wojtekmach wojtekmach merged commit 709b424 into wojtekmach:main Aug 24, 2023
2 checks passed
@wojtekmach
Copy link
Owner

Thank you!

@yordis
Copy link

yordis commented Sep 10, 2023

@tanguilp @wojtekmach do you mind code reviewing elixir-tesla/tesla#606 🙏🏻 🙏🏻 🙏🏻 🙏🏻 🙏🏻 🙏🏻 🙏🏻

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