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

feat: avoid clobbering original body for next read #45

Merged
merged 1 commit into from
Mar 12, 2022

Conversation

StevenACoffman
Copy link
Contributor

Hello! If you read the request body the way you do, subsequent layers (like a logging middleware) will find it empty. This copy's the original body and reads that, preserving the original for subsequent reads.

Signed-off-by: Steve Coffman [email protected]

@StevenACoffman StevenACoffman requested a review from moul as a code owner August 24, 2021 01:22
@trafico-bot trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label Aug 24, 2021
@ioito
Copy link

ioito commented Aug 31, 2021

/close #48

@djale1k
Copy link
Contributor

djale1k commented Aug 31, 2021

Regarding the issue #48 If we call this in a long process and more req at time the performance boost I made it's good maybe in this case it's better to keep your copy of req before sending it to GetCurlCommand @moul @StevenACoffman @ioito

@StevenACoffman
Copy link
Contributor Author

@Dzalevski Presumably people are making HTTP Requests to do something useful, and this library is used for logging or debugging, so #31 broke existing usages of this library, which I discovered when I updated.

If both safely reading the request and better-performing-but-unsafely reading the request use cases were desirable, there could be an additional function, but the documentation should warn people using the unsafe one.

So this would have GetCurlCommand and GetCurlCommandUnsafe or GetCurlCommandNoClobber

@trafico-bot trafico-bot bot added ✅ Approved Pull Request has been approved and can be merged and removed 🔍 Ready for Review Pull Request is not reviewed yet labels Mar 12, 2022
@moul moul changed the title Avoid clobbering original body for next read feat: avoid clobbering original body for next read Mar 12, 2022
@moul moul merged commit ee0b0d5 into moul:master Mar 12, 2022
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved Pull Request has been approved and can be merged labels Mar 12, 2022
@github-actions
Copy link

🎉 This PR is included in version 2.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@dominik-lekse
Copy link

To clarify: This PR and therefore release 2.3.0 restores the behavior of GetCurlCommand before the merge of PR #31?

I agree with the argument of @StevenACoffman, that #31 introduced a breaking change by emptying the request body. To provide an insight on a use case, I am using http2curl to log HTTP requests as part of an integration test suite. After the upgrade with the breaking change, I spent an unreasonable amount of time to investigate why the service under test did not receive any request body.

Therefore, :thumbs: up for the proposal by @StevenACoffman to add the performance-optimized version as a new function. I throw GetCurlCommandFast in the arena of possible names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ Merged Pull Request has been merged successfully released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants