openapi-generator
openapi-generator copied to clipboard
OpenAPITools
[BUG][Rust][client][reqwest] Wrong representation of binary body (octet-stream) in request/response
Bug Report Checklist
- [X] Have you provided a full/minimal spec to reproduce the issue?
- [X] Have you validated the input using an OpenAPI validator (example)?
- [X] Have you tested with the latest master to confirm the issue still exists?
- [X] Have you searched for related issues/PRs?
- [X] 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
reqwestlibrary
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(orInto<Body>) if it's expected as a body of the request - return
reqwest::Responseif output type is binary (could be wrapper though, which containsStream+headersfor example)
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))
}
}
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
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 ?
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
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 ?
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
I think doing this modification will break the current multipart implementation as it uses the file method with the PathBuf input type
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