[BUG][Rust][client][reqwest] Wrong representation of binary body (octet-stream) in request/response #18117

DDtKey · 2024-03-15T14:59:31Z

Bug Report Checklist

Have you provided a full/minimal spec to reproduce the issue?
Have you validated the input using an OpenAPI validator (example)?
Have you tested with the latest master to confirm the issue still exists?
Have you searched for related issues/PRs?
What's the actual output vs expected output?
[Optional] Sponsorship to speed up the bug fix or feature request (example)

Description

Currently any binaries in generate client for rust (with reqwest library at least) uses std::path::PathBuf.
And several issues here:

it's supposed to use FS, but it's not necessary
it doesn't support streaming
and more important: it just doesn't work

E.g for request it generates the following code:

local_var_req_builder.json(&body);   // assumes it's json content

And for response it's even worse:

let local_var_content = local_var_resp.text().await?; // reads all content to `String`
// ... omitted 
serde_json::from_str(&local_var_content).map_err(Error::from) // it tries to de-serialize `PathBuf` from `String`
// moreover, headers not accessible, but it can be useful to check them before starting to consume the body

openapi-generator version

7.4.0/7.3.0 and likely earlier versions

OpenAPI declaration file content or url

openapi: 3.0.1
info:
  title: My title
  description: My description
  version: 1.0.0
paths:
  /upload:
    post:
      requestBody:
        required: true
        content:
          application/octet-stream:
            schema:
              type: string
              format: binary
      responses:
        '200':
          description: 'OK'
  /download:
    get:
      responses:
        '200':
          description: 'OK'
          content:
            application/octet-stream:
              schema:
                type: string
                format: binary
          headers: 
            Content-Length: 
              schema: 
                type: integer
            Content-Range:
              required: true
              schema:
                type: string

Generation Details

No special configs, just generate -i ./test.yaml -g rust --package-name stream-test -o ./out --library reqwest

Steps to reproduce

Take provided openapi schema
generate a client for rust using reqwest library

Related issues/PRs

Suggest a fix

First thought could be to use reqwest::Body - but it doesn't cover streaming of response (at least for reqwest 0.11.x). Only Response itself can be converted with bytes_stream. As well as into bytes.

But it's possible to wrap a Stream to reqwest::Body.

So I think it would be correct to go with this way:

expect reqwest::Body(or Into<Body>) if it's expected as a body of the request
return reqwest::Response if output type is binary (could be wrapper though, which contains Stream + headers for example)

The text was updated successfully, but these errors were encountered:

Gaelik-git · 2024-05-15T14:39:04Z

What do you think about first making a MVP that would work with Vec<u8> ?

We could have the generated code looking like for the download ?

pub async fn download_get(configuration: &configuration::Configuration, ) -> Result<Vec<u8>, Error<DownloadGetError>> {
    let local_var_configuration = configuration;

    let local_var_client = &local_var_configuration.client;

    let local_var_uri_str = format!("{}/download", local_var_configuration.base_path);
    let mut local_var_req_builder = local_var_client.request(reqwest::Method::GET, local_var_uri_str.as_str());

    if let Some(ref local_var_user_agent) = local_var_configuration.user_agent {
        local_var_req_builder = local_var_req_builder.header(reqwest::header::USER_AGENT, local_var_user_agent.clone());
    }

    let local_var_req = local_var_req_builder.build()?;
    let local_var_resp = local_var_client.execute(local_var_req).await?;

    let local_var_status = local_var_resp.status();

    if !local_var_status.is_client_error() && !local_var_status.is_server_error() {
        let bytes = local_var_resp.bytes().await?;
        Ok(bytes.into_iter().collect())
    } else {
        let local_var_content = local_var_resp.text().await?;
        let local_var_entity: Option<DownloadGetError> = serde_json::from_str(&local_var_content).ok();
        let local_var_error = ResponseContent { status: local_var_status, content: local_var_content, entity: local_var_entity };
        Err(Error::ResponseError(local_var_error))
    }
}

DDtKey · 2024-05-15T15:16:06Z

That's definitely much better than current implementation - it will work at least. So it could be a first iteration, but doesn't solve the issue.

It worth mentioning, that it generates not really secure client - .bytes().await?; and .text().await?; may cause OOM if stream is large enough.

Also, starting with Vec<u8> will tie us to this for a while, and in order to support streaming we will need to either provide a configuration flag (to preserve compatibility) or claim as a braking change and release only with the next major version of openapi-generator

Gaelik-git · 2024-05-15T15:45:11Z

Wouldn't the text() return an error if the content of the stream is not valid utf-8 ?

Should we try to keep the same types for reqwest and hyper ? I think neither support this now

Also we have to make sure it also work when supportAsync is disabled ?

DDtKey · 2024-05-15T16:01:17Z

Wouldn't the text() return an error if the content of the stream is not valid utf-8 ?

Yes, but that could be just a valid utf-8 file(e.g CSV) or even compromised data. I'm just saying both methods (bytes & text) could cause OOM if they're called automatically.

Should we try to keep the same types for reqwest and hyper ? I think neither support this now

That's a good question, I do not think we have to, because these clients are in general operate with different types.

Also we have to make sure it also work when supportAsync is disabled ?

Yes, that's also fair point. And here we don't have many options, that's why returning reqwest::Response might be a good solution (user can decide what to use, Vec, Stream or copy_to). And, in general, similar approach is applicable for hyper.

I just find this more flexible, however it can be confusing why Response is returned after handling its possible error codes and etc. So probably some custom wrapper still makes sense

Gaelik-git · 2024-05-16T07:14:10Z

I get why we would like to return reqwest::Response, I tried it and it looks ok as it gives the user the ability to do what they want.
I not very familiar with the project though, I had to change the typeMapping for the type file and binary to reqwest::Response

typeMapping.put("file", "reqwest::Response");
typeMapping.put("binary", "reqwest::Response");

The thing is that now this type is also expected when you want to send file or binary data

This is the produced code

pub async fn upload_post(configuration: &configuration::Configuration, body: reqwest::Response) -> Result<(), Error<UploadPostError>> {
    let local_var_configuration = configuration;

    let local_var_client = &local_var_configuration.client;

    let local_var_uri_str = format!("{}/upload", local_var_configuration.base_path);
    let mut local_var_req_builder = local_var_client.request(reqwest::Method::POST, local_var_uri_str.as_str());

    if let Some(ref local_var_user_agent) = local_var_configuration.user_agent {
        local_var_req_builder = local_var_req_builder.header(reqwest::header::USER_AGENT, local_var_user_agent.clone());
    }
    local_var_req_builder = local_var_req_builder.body(body);

    let local_var_req = local_var_req_builder.build()?;
    let local_var_resp = local_var_client.execute(local_var_req).await?;

    let local_var_status = local_var_resp.status();

    if !local_var_status.is_client_error() && !local_var_status.is_server_error() {
        Ok(())
    } else {
        let local_var_content = local_var_resp.text().await?;
        let local_var_entity: Option<UploadPostError> = serde_json::from_str(&local_var_content).ok();
        let local_var_error = ResponseContent { status: local_var_status, content: local_var_content, entity: local_var_entity };
        Err(Error::ResponseError(local_var_error))
    }
}

Response implements Into<Body> so it compiles but I am not sure how would a user create a response from an existing file on FS for example.

It probably means we need a custom type inside the crate (or an existing one), what do you think ?

DDtKey · 2024-05-16T11:35:20Z

The thing is that now this type is also expected when you want to send file or binary data

Yes, this one is tricky, for client side we need to have another type here, and I believe it should be Body. We could hardcode this in mustache by condition isFile

In fact, I'd prefer to return something like Body for responses too, but reqwest doesn't provide anything for that on Body level - I mean body can't be directly converted into stream.

But I think it's pretty good middle ground to go with Body for requests and Response for returning values

Gaelik-git · 2024-05-16T14:27:39Z

I think doing this modification will break the current multipart implementation as it uses the file method with the PathBuf input type

DDtKey · 2024-05-16T15:55:05Z

Yes, we also need to distinguish application/octet-stream from multipart/form-data

Example in the issue is about application/octet-stream

And AFAIK there is already a check for that: {{#isMultipart}}/{{^isMultipart}}:
https://github.com/OpenAPITools/openapi-generator/blob/d7290b3b7b1d227a0010444d27daa616dbd3e364/modules/openapi-generator/src/main/resources/rust/reqwest/api.mustache#L245C8-L293

DDtKey added the Issue: Bug label Mar 15, 2024

Gaelik-git mentioned this issue May 16, 2024

(wip) Binary Type Support #18682

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG][Rust][client][reqwest] Wrong representation of binary body (octet-stream) in request/response #18117

[BUG][Rust][client][reqwest] Wrong representation of binary body (octet-stream) in request/response #18117

DDtKey commented Mar 15, 2024 •

edited

Loading

Gaelik-git commented May 15, 2024 •

edited

Loading

DDtKey commented May 15, 2024

Gaelik-git commented May 15, 2024

DDtKey commented May 15, 2024 •

edited

Loading

Gaelik-git commented May 16, 2024

DDtKey commented May 16, 2024 •

edited

Loading

Gaelik-git commented May 16, 2024

DDtKey commented May 16, 2024 •

edited

Loading

[BUG][Rust][client][reqwest] Wrong representation of binary body (octet-stream) in request/response #18117

[BUG][Rust][client][reqwest] Wrong representation of binary body (octet-stream) in request/response #18117

Comments

DDtKey commented Mar 15, 2024 • edited Loading

Bug Report Checklist

Description

openapi-generator version

OpenAPI declaration file content or url

Generation Details

Steps to reproduce

Related issues/PRs

Suggest a fix

Gaelik-git commented May 15, 2024 • edited Loading

DDtKey commented May 15, 2024

Gaelik-git commented May 15, 2024

DDtKey commented May 15, 2024 • edited Loading

Gaelik-git commented May 16, 2024

DDtKey commented May 16, 2024 • edited Loading

Gaelik-git commented May 16, 2024

DDtKey commented May 16, 2024 • edited Loading

DDtKey commented Mar 15, 2024 •

edited

Loading

Gaelik-git commented May 15, 2024 •

edited

Loading

DDtKey commented May 15, 2024 •

edited

Loading

DDtKey commented May 16, 2024 •

edited

Loading

DDtKey commented May 16, 2024 •

edited

Loading