Add --preserve-encoding option to keep response body encoding #294

trueb2 · 2022-11-29T23:22:31Z

Changes

Add a --download-encoding flag to choose an Accept-Encoding header other than "identity" during a download request. During the download, the "decompress" function that reads the response and writes it to the output will passthrough data in whatever Content-Encoding was received if --download-encoding was specified.

Motivation

When downloading a large file, I want to be able to transfer it compressed and leave it compressed. For example, stream download a 500MiB CSV file in the gzip encoding, saving it to a data.csv.gz file.

jwtrueb@jmbp xh % cargo run --release -- get http://localhost:3000/download/csv/asdf -d --download-encoding gzip
HTTP/1.1 200 OK
Content-Encoding: gzip
Content-Type: application/csv
Date: Tue, 29 Nov 2022 23:17:12 GMT
Transfer-Encoding: chunked
Vary: accept-encoding

Downloading to "asdf"
Done. 147B in 0.00177s (80.92KiB/s)
jwtrueb@jmbp xh % gzcat asdf
ts,user_id,device_id,source,pressure,temperature,humidity
2022-11-28T20:13:12.704030+00:00,2,5,1,99999.0,25.3,16.4
2022-11-28T20:14:02.446438+00:00,2,5,1,99699.0,25.1,16.2

blyxxyz · 2022-11-30T09:41:15Z

One reason to use Accept-Encoding: identity is to be able to resume downloads (with --continue). We'd have to figure out how these features interact. Maybe that should be an error message.

This option seems too specific. Maybe we should allow overriding the encoding via a normal header argument and add a --preserve-encoding/--no-decode/--verbatim-body/whatever option? Then you'd be able to run xh -d --preserve-encoding :3000/download/csv/asdf accept-encoding:gzip.

We have to coordinate this with HTTPie, that's where we got the current behavior. Are you interested in making an issue?

trueb2 · 2022-11-30T16:17:13Z

One reason to use Accept-Encoding: identity is to be able to resume downloads (with --continue). We'd have to figure out how these features interact. Maybe that should be an error message.

This option seems too specific. Maybe we should allow overriding the encoding via a normal header argument and add a --preserve-encoding/--no-decode/--verbatim-body/whatever option? Then you'd be able to run xh -d --preserve-encoding :3000/download/csv/asdf accept-encoding:gzip.

We have to coordinate this with HTTPie, that's where we got the current behavior. Are you interested in making an issue?

Sure whatever works. I couldn't figure out how to get HTTPie to do what I wanted as easily as xh. Looks like it wouldn't be too bad to switch this to a --preseve-encoding and a manual header. That still seems to be in the spirit of the CLI.

I don't really care to use --continue for downloads, so I did not consider how that interacts with anything other than identity. I think an error message makes sense if both --continue and --preserve-encoding are used.

I could just implement that instead of making an issue. Is it a problem to have features in xh that aren't in HTTPie?

ducaale · 2022-11-30T22:04:55Z

Is it a problem to have features in xh that aren't in HTTPie?

We are open to adding features not supported in HTTPie. But we don't want to end up in a situation where the same feature is implemented differently in xh and HTTPie. Many xh users are coming from HTTPie, and some of them might even rely on HTTPie docs while using xh.

Let's either open a new issue or maybe leave a comment on httpie/cli#178, and then we can continue with --preserve-encoding/--no-decode/--verbatim-body/something else.

trueb2 · 2022-12-05T22:32:24Z

@ducaale That is quite the old issue you dug up! I see it is related to #130

I think we could comment on the xh approach in the httpie issue, then implement an approach. It seems like this isn't a very hot feature request, and I don't want it to end up as another issue that goes 9 years with an implementation as easy as this. What is your preferred implementation/flag?

ducaale · 2022-12-13T18:33:53Z

We can go with the proposed implementation in #294 (comment) and use --preserve-encoding (cURL uses --raw but that option is already used in HTTPie/xh).

Note that we currently don't allow the user to set accept-encoding in download mode (see #141). We can roll back that behavior and error when --continue is used with a custom accept-encoding.

tests/cli.rs

src/cli.rs

ducaale · 2023-01-04T12:37:33Z

src/main.rs

@@ -501,6 +509,7 @@ fn run(args: Cli) -> Result<i32> {
        printer.print_request_body(&mut request)?;
    }

+    let preserve_encoding = args.preserve_encoding;


Nit: can you move this line to after L503 and replace all instances of args.preserve_encoding with preserve_encoding?

let response_charset = args.response_charset; let response_mime = args.response_mime.as_deref(); let preserve_encoding = args.preserve_encoding;

ducaale · 2023-01-04T13:10:11Z

src/main.rs

+            } else {
+                request_builder =
+                    request_builder.header(ACCEPT_ENCODING, HeaderValue::from_static("identity"));
+            }


Can you remove the else branching so that ACCEPT_ENCODING is set to a default value when args.resume && encoding != HeaderValue::from_static("identity") is false?

Suggested change

} else {

request_builder =

request_builder.header(ACCEPT_ENCODING, HeaderValue::from_static("identity"));

}

}

request_builder =

request_builder.header(ACCEPT_ENCODING, HeaderValue::from_static("identity"));

This doesn't matter that much since it will be overridden later with header values provided by the user. However, reducing possible branches helps with any potential debugging needed in the future.

ducaale · 2023-01-04T13:31:21Z

src/printer.rs

+        let (may_decode, compression_type) = if !preserve_encoding {
+            let compression_type = get_compression_type(response.headers());
+            (true, compression_type)
+        } else {
+            (false, None)
+        };


Is it possible to use the same approach you took in download.rs?

Suggested change

let (may_decode, compression_type) = if !preserve_encoding {

let compression_type = get_compression_type(response.headers());

(true, compression_type)

} else {

(false, None)

};

let compression_type = if !preserve_encoding {

get_compression_type(response.headers())

} else {

None

};

I don't think that it is possible since the logic below assumes that the content has been decompressed before colorizing, formatting, or decoding. We wouldn't want that logic to run on potentially encoded data. The other branches call code like decode_stream to interpret UTF8 or other format types.

Fair enough. Although we have some logic for binary detection (see decode_blob and BinaryGuard), they might not cover all cases.

At the moment, plain text is treated as binary if preserve_encoding is true. Can you handle that case?

// TODO: rename preserve_encoding to skip_decompression let (mut body, is_binary) = if preserve_encoding { let compression_type = get_compression_type(response.headers()); let body = decompress(response, None); (body, compression_type.is_some()) } else { let compression_type = get_compression_type(response.headers()); let body = decompress(response, compression_type); (body, false) };

Also, the term decode is a bit overloaded in this module. Can we rename this function's preserve_encoding parameter to skip_decompression?

ducaale · 2023-01-04T14:02:34Z

src/cli.rs

+    /// During download, keep the raw encoding of the body. Intended for use with download or
+    /// to debug an encoded response body.
+    ///
+    /// For example, set Accept-Encoding: gzip and use --preserve-encoding to skip decompression.
+    #[clap(long)]
+    pub preserve_encoding: bool,
+


Let's shorten the first line to fit within ~80 chars. Any extended help can go in subsequent lines.

(to see the difference, try cargo run -- --help and cargo run -- help)

/// Skip decompressing the response body /// /// longer help

Also, should we move this option to after response_mime to improve discoverability?

Add request and keep encoding for download body flag

bcbb38a

trueb2 changed the title ~~Add request and keep encoding for download body flag~~ Add flag to request and keep encoding for a download body Nov 29, 2022

trueb2 added 2 commits December 23, 2022 12:46

Update download flag to keep raw encoding to preserve_encoding

f025ddf

Update formatting and test cases

dffabdc

ducaale requested changes Dec 23, 2022

View reviewed changes

tests/cli.rs Outdated Show resolved Hide resolved

tests/cli.rs Outdated Show resolved Hide resolved

src/cli.rs Outdated Show resolved Hide resolved

trueb2 added 3 commits January 3, 2023 12:46

Update tests for --preserve-encoding flag

eafb676

Support --preserve-encoding without --download

f6c7961

Merge remote-tracking branch 'upstream/master'

966b985

trueb2 changed the title ~~Add flag to request and keep encoding for a download body~~ Add --preserve-encoding option to keep response body encoding Jan 3, 2023

ducaale reviewed Jan 4, 2023

View reviewed changes

zuisong mentioned this pull request Jan 16, 2024

when add --download flag , xh does not requests the compressed response body #346

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --preserve-encoding option to keep response body encoding #294

Add --preserve-encoding option to keep response body encoding #294

trueb2 commented Nov 29, 2022

blyxxyz commented Nov 30, 2022

trueb2 commented Nov 30, 2022

ducaale commented Nov 30, 2022 •

edited

Loading

trueb2 commented Dec 5, 2022

ducaale commented Dec 13, 2022 •

edited

Loading

ducaale Jan 4, 2023 •

edited

Loading

ducaale Jan 4, 2023

ducaale Jan 4, 2023

trueb2 Jan 5, 2023

ducaale Jan 6, 2023

ducaale Jan 4, 2023

Add --preserve-encoding option to keep response body encoding #294

Are you sure you want to change the base?

Add --preserve-encoding option to keep response body encoding #294

Conversation

trueb2 commented Nov 29, 2022

Changes

Motivation

blyxxyz commented Nov 30, 2022

trueb2 commented Nov 30, 2022

ducaale commented Nov 30, 2022 • edited Loading

trueb2 commented Dec 5, 2022

ducaale commented Dec 13, 2022 • edited Loading

ducaale Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

ducaale Jan 4, 2023

Choose a reason for hiding this comment

ducaale Jan 4, 2023

Choose a reason for hiding this comment

trueb2 Jan 5, 2023

Choose a reason for hiding this comment

ducaale Jan 6, 2023

Choose a reason for hiding this comment

ducaale Jan 4, 2023

Choose a reason for hiding this comment

ducaale commented Nov 30, 2022 •

edited

Loading

ducaale commented Dec 13, 2022 •

edited

Loading

ducaale Jan 4, 2023 •

edited

Loading