Set negative value to second argument of esp_http_client_open method if request header has Transfer-Encoding: chunked

[ysak-y]

What

Set nevative value to request_content_len property if can't unwrap content_len somehow.

Why

Ref: #309

I want to implement streaming http post that sets Transfer-Encoding: chunked to the header and doesn't set Content-Length. But this library sets Content-Length: 0 if I don't set Content-Length to the header and got 400 error (Please read above issue description).

As I investigate, I can avoid this behavior and can get what I'm looking for if set negative value to the second argument of esp_http_client_open method.
#309 (comment)

So I want to set negative value if can't fetch correct value from content_len.

How

Set request_content_len type to i64 because I want to set negative value to it, and set request_content_len property to -1 (Some negative value) if can't unwrap content_len.

[ysak-y]

I will test it more and ask someone to review it later.

[ivmarkov]

I don't get it. Do you want to implement chunked encoding yourself? Because the built-in ESP IDF HTTP client does not support it.

[ivmarkov]

Actually I stand corrected. Chunked encoding DOES seems to be supported, after all.

[Vollbrecht]

yeah we just need to make sure to then not set the content-length header ourself. Also we might need to check if the chunked support is only for the latest master / 5.1 or also included inside the 4.4.6 client so @ysak-y if you would check if it would work on both versions

[ivmarkov]

@ysak-y if you plan to finish this, you need to address the CI build errors.

[ysak-y]

Thank you for your feedbacks. I'll try to implement it soon. Please just a while.

@ysak-y if you plan to finish this, you need to address the CI build errors.

Yes, I know. I'll try to pass the CI later.

[ysak-y]

UPDATE

I investigated esp-idf-svc more, and I implemented it as follows finally.

a86d5f7

Convert type of request_content_len property from u64 to i32 to accept negative value. In the first place, I think this property should be i32 type because it will be cast to i32 finally. It is described in #311 .

0da0e59

Implement is_chunked_streaming_request method to check whether header has Transfer-Encoding: chunked in it. Then pass -1 if header has it, otherwise, self.request_content_len value that is an existing behavior.

I thought is_chunked_streaming_request should check Content-Length header at first, but it seems we can ignore it if Transfer-Encoding: chunked is in its header. So is_chunked_streaming_request checks only Transfer-Encoding header.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding#directives

Example

I implement example code as follows (This is only HTTP part, not full example), and then got 200 response, and server accepted Hello World!! value!!. It works on ESP-IDF v5.1 and v4.4.6 also.

    let payload = b"Hello";
    let payload2 = b" World!!";

    let headers = vec![
        ("Content-Type", "plain/text"),
        ("Transfer-Encoding", "chunked"),
    ];
    let mut request = clinet.post(URL, &headers).unwrap();

    // Write payload
    request
        .write(format!("{:X}", payload.len()).as_bytes())
        .unwrap();
    request.write(b"\r\n").unwrap();
    request.write(payload).unwrap();
    request.write(b"\r\n").unwrap();

    // Write payload2
    request
        .write(format!("{:X}", payload2.len()).as_bytes())
        .unwrap();
    request.write(b"\r\n").unwrap();
    request.write(payload2).unwrap();
    request.write(b"\r\n").unwrap();

    // Write ending
    request.write(b"0\r\n\r\n").unwrap();

    request.flush().unwrap();

    let response = request.submit().unwrap();

Please refer following documents if you don't know how to write chunked data with streaming post.
https://datatracker.ietf.org/doc/html/rfc9112#name-chunked-transfer-coding

[ysak-y]

I suggested there instead of i64 to do Option. That might or might not be a good idea. Please think about that, and try to come - in the other PR - with a complete solution implementing chunked encoding for POST/PUT requests.
#311 (comment)

I think i32 would be better because esp_http_client_open expects i32 type, and request_content_len is only used for it. What do you think? @ivmarkov

[ivmarkov]

I suggested there instead of i64 to do Option. That might or might not be a good idea. Please think about that, and try to come - in the other PR - with a complete solution implementing chunked encoding for POST/PUT requests.
#311 (comment)

I think i32 would be better because esp_http_client_open expects i32 type, and request_content_len is only used for it. What do you think? @ivmarkov

I don't like it very much as i32 is "too wide". What does it even mean if the user passes -2 as a value?
Also the fact that ESP IDF expects i32 does not mean that our Rust wrapper should expect i32.

UPDATE: Let me look at the whole patch first.

[ysak-y]

Also the fact that ESP IDF expects i32 does not mean that our Rust wrapper should expect i32.

Ah, make sense. I agree with your opinion. I'll consider better way.

[ysak-y]

@ivmarkov I modify request_content_len type to u64 that is same as current and cast it to i32 expressly only when pass it to esp_http_client_open method. Does it make sense to you? If so, I'll align commits of this PR (e.g. We don't need a86d5f7 anymore).
adeaca5

[Vollbrecht]

Don't forget to handle the case when transfer-encoding chunked is set alongside with Content-Length. While the HTTP spec discourage it, to have both set, the client driver wouldn't probably complain. So we should cover that by either emitting a warning to the user or forbid it

[ysak-y]

While the HTTP spec discourage it, to have both set, the client driver wouldn't probably complain.

Ah... nice catch. My idea is just emit a warning or throw error. But, it may be server's responsibility. Actually, in NodeJS, built-in http server throws error when request has both (Content-Length and Transfer-Encoding) in its headers, but it wouldn't throw error even if request has both when sending http request (https://nodejs.org/api/http.html).

I think basically client doesn't validate header values because there are some specification differences between server. We can notice mistakes if we get 400 BAD REQUEST error from server.

What do you think?

[ivmarkov]

Assuming you do the small changes suggested below, I think can merge.

What I actually STILL don't like in this approach is that the chunked encoding is left to the user. I get it that the ESP IDF client does not do it by default, but this is still annoying. For one, the ESP IDF server automatically decodes/encodes in chunked encoding, which is asymmetric to their client. I think also the ESP IDF client automatically decodes chunked encoding, so it is weird that it cannot encode it, no?

[ivmarkov]

I think you don't need this method at all. This post says, that as long as you pass -1 to esp_http_client_open, the client would assume chunked encoding.

[ivmarkov]

Please just change the type of EspHttpConnection::request_content_len to i64, as you originally started doing. Sorry for asking you to do other stuff - I did not realize, that request_content_len is a private field which is not visible to the user.

[ysak-y]

Okay, change type to i64.
a7d084c

[ivmarkov]

Assuming you'll change request_content_len to i64, as suggested below, just do here unwrap_or(-1)

[ivmarkov]

Remove the if. Assuming is_chunked_streaming_request is removed, and then unwrapping content-len just sets request_content_len to -1 if content_len is None, you'll no longer need it.

[ysak-y]

As you commented, change type of request_content_len to i64 and just pass -1 if can't unwrap content_len variable. Then remove is_chunked_streaming_request because it is no longer needed. Does it make sense to you? @ivmarkov
If it seems okay, I'll rebase commits to merge soon.

a7d084c

I think also the ESP IDF client automatically decodes chunked encoding, so it is weird that it cannot encode it, no?

I agree with it. But as you say, original ESP-IDF doesn't implement it now. I think esp-idf-svc shouldn't implement original encoding method because it would be complicated if ESP-IDF implements it later. esp-idf-svc should be just the wrapper library of ESP-IDF.

[Vollbrecht]

esp-idf-svc should be just the wrapper library of ESP-IDF.

This maybe your first impression, but we often have an opinionated implementation over the original raw idf api to make it first and foremost a good rust api. In fact the goal is to have our api stable compared to idf versions like between 4.4 and 5.1 and don't leak the original api out. Though in reality its not always so simple, so good compromises will win in the end.

[ivmarkov]

@ysak-y @Vollbrecht The crate now supports automatic chunked encoding (user does not have to do it herself, unless that's explicitly requested using the new Configuration::raw_request_body parameter).

Also I had to fix a buglet from this PR whereas if no Content-Length was explicitly set by the user, the client was switched to POST + chunked encoding even when the user was doing an innocent GET and forgot to put a Content-Length: 0 header.