Sending non-String HTTP request bodies no longer supported?

[indygreg]

#297 refactored various HTTP request APIs.

New generic APIs for sending HTTP requests in lib.rs (e.g. pub async fn execute(&self, request: http::Request<String>) -> Result<http::Response<hyper::Body>> now mandate the use of http::Request<String>, effectively preventing the sending of binary bodies. In addition, building the request can transparently coerce binary bodies into String.

I am currently relying on Octocrab for uploading release assets and this change in behavior caused binary assets (represented as Vec<u8> to get encoded as their string/debug representation. See astral-sh/python-build-standalone#172.

Would it be possible to make execute() generic or for there to be a separate API to allow issuing of http::Request<Vec<u8>>? Essentially some way to issue HTTP requests with non-String bodies.

[XAMPPRocky]

Thank you for your issue! It was definitely not intentional to regress this functionality, and it should be allowed.

As for what's the best way to do this I'm not sure as I've haven't looked at the code in awhile. I think I'd probably say that it should be generic by the parameter, and it should accept anything that can be in a http::Request

Cc @L1ghtman2k since you wrote a lot of the new code, do you have any thoughts on how best to resolve this?

[L1ghtman2k]

Hey, thanks for mentioning this, I wasn't aware of this use case.

I explained my reasoning for the change here: https://github.com/XAMPPRocky/octocrab/pull/297/files#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R1214-R1218:

    // Since Octocrab doesn't require streamable bodies(aka, file upload) because it is serde::Serialize),
    // we can just use String body, since it is both http_body::Body(required by Hyper::Client), and Clone(required by BoxService).

    // In case octocrab needs to support cases where body is strictly streamable, it should use something like reqwest::Body,
    // since it differentiates between retryable bodies, and streams(aka, it implements try_clone(), which is needed for middlewares like retry).

I was under the impression that Octocrab didn't support streamable requests to begin with, since it required serde::Serlialize. Now that I look back, I see that is only the case for functions like _put, post, get, etc, but not for the actual execute method.

I think to resolve this, as per my comment above, we will need to do some work that was previously done by reqwest::Body to differentiate between retriable, and Streaming requests: https://docs.rs/reqwest/latest/src/reqwest/async_impl/body.rs.html#24-36

[indygreg]

My use case of Octocrab doesn't currently require streamable bodies: I just construct a body from a Vec<u8>.

I'll note that I'm using Octocrab for both its typed API interfaces as well as the generic put/get/execute interface to send requests to APIs not currently implemented in Octocrab. Reinventing auth/retry/etc in execute() isn't super difficult and if you say you don't want to support non-String bodies I'll adapt. But since I'm already using a GitHub client crate in my project, I feel like I shouldn't need to reinvent this wheel.

[L1ghtman2k]

I would personally like to see Vec<u8>, or some other struct other than String implemented here.

I just run into a few issues trying to implement Vec<u8>, and hyper::Body during the refactor, which lead me to use String.

The problem for Vec<u8> is that the HttpBody trait (required for hyper Client) doesn't implement it(while it does implement string), so here we would already have to create a custom wrapper type, and do:

struct myVecWrapper(Vec<u8>);
impl HttpBody for myVecWrapper {

(I personally didn't want to go this route as it introduces a custom type, and I imagine there would be all sorts of confusion)

Then I looked into implementing http::Body. The problem here was that I couldn't use it in retry methods because it is a stream.

I think we would need to either re-implement the way reqwest::Body handles this, or see if we can re-use their approach directly

[L1ghtman2k]

A few ways to go about this is to work with http::Body, but make retry middleware work by buffering the requests, as long as they are not very large... I will try to look into this upcoming weekend.

It seems like guys at tonic have run into this same issue, so I will link this for future reference: hyperium/tonic#733

[XAMPPRocky]

(I personally didn't want to go this route as it introduces a custom type, and I imagine there would be all sorts of confusion)

I think we could add a custom type to address this, but I think we should make it non-public, and have the parameter be a generic conversion, that way we can support non-streaming bodies today, and add streaming bodies later. Something along the following, which would allow people to just pass Vec<u8> without even knowing we have a wrapper struct for it.

trait IntoHttpBody {
     type Body: HttpBody;
     fn into_http_body() -> Self::Body;
}

struct MyVecWrapper(Vec<u8>);
impl HttpBody for MyVecWrapper;
impl IntoHttpBody for Vec<u8> {
     type Body = MyVecWrapper;
     fn into_http_body() -> Self::Body { ... }
}

pub fn execute(&self, impl IntoHttpBody) -> Result<()>;

I think we would need to either re-implement the way reqwest::Body handles this, or see if we can re-use their approach directly

This is also acceptable to me, it would also be great if we could spin this functionality out into a separate crate, so that it can be used in crates without needing to copy the code.

[L1ghtman2k]

I will try the above approach sometime this weekend, though I am leaning toward skipping the retry, and letting the client handle it until we can get a proper layer retry layer implemented(meanwhile relying on http::Body. Something along the lines of: https://github.com/linkerd/linkerd2-proxy/blob/3933feb587c27f5f8c5c76669d9d99b44b0a60b2/linkerd/app/outbound/src/http/retry.rs#L12
(mentioned in tonic issue)

Edit: Got somewhat busy, and couldn't look at it yet...