-
Notifications
You must be signed in to change notification settings - Fork 102
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
support compress request body #403
base: master
Are you sure you want to change the base?
Conversation
d7f708a
to
f85194a
Compare
src/main.rs
Outdated
if args.compress >= 1 && request.headers().get(CONTENT_ENCODING).is_none() { | ||
let mut compressed = false; | ||
if let Some(body) = request.body_mut() { | ||
if let Ok(body_bytes) = body.buffer() { |
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.
Using buffer()
might consume a lot of memory for extremely large files
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.
The into_reader() method of reqwest#Body is pub(crate). We cannot access it. Currently, it is not possible to achieve our goal.
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.
We'd have to wrap the file after opening it and before turning it into a Body
. Currently we open the file here:
Line 444 in f75d092
request_builder.body(File::open(file_name)?).header( |
But it's also OK to leave this out for now.
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.
@zuisong Can you add a TODO comment about compressing the file body without buffering it?
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.
Happy Chinese New Year! 🥳🥳🥳
I'm currently on vacation, but I will update the code as soon as possible regarding your comments later.
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.
No rush at all. Enjoy your vacation!
e64e14f
to
1ec0267
Compare
@@ -779,6 +780,43 @@ fn successful_digest_auth() { | |||
.stdout(contains("HTTP/1.1 200 OK")); | |||
} | |||
|
|||
#[cfg(feature = "online-tests")] |
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.
Can you move the tests into a separate file under tests/cases
?
/// The Content-Encoding header is set to deflate. | ||
/// | ||
/// Compression is skipped if it appears that compression ratio is negative. | ||
/// Compression can be forced by repeating this option. |
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.
Should we also mention that compression cannot be forced if the Content-Encoding
request header is present?
get_command() | ||
.arg(format!("{}/deflate", server.base_url())) | ||
.args([ | ||
&format!("key={}", "1".repeat(1000)), | ||
"-x", | ||
"-x", | ||
"-f", | ||
"--pretty=none", | ||
]) | ||
.assert() | ||
.stdout(indoc::formatdoc! {r#" | ||
HTTP/1.1 200 OK | ||
Date: N/A | ||
Content-Type: text/plain | ||
Content-Length: 1004 | ||
|
||
key={c} | ||
"#, c = "1".repeat(1000),}); | ||
|
||
get_command() | ||
.arg(format!("{}/deflate", server.base_url())) | ||
.args([ | ||
&format!("key={}", "1".repeat(1000)), | ||
"-x", | ||
"-x", | ||
"-f", | ||
"--pretty=none", | ||
]) | ||
.assert() | ||
.stdout(indoc::formatdoc! {r#" | ||
HTTP/1.1 200 OK | ||
Date: N/A | ||
Content-Type: text/plain | ||
Content-Length: 1004 | ||
|
||
key={c} | ||
"#, c = "1".repeat(1000),}); |
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.
Is the duplicate command invocation necessary here?
} | ||
|
||
#[test] | ||
fn compress_request_body() { |
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.
Would it be possible to break this into multiple test cases?
implements https://httpie.io/docs/cli/compressed-request-body