rust: better support for binary body parameters #9021
Conversation
The Rust-based reqwest client does not correctly handle a binary body parameter today. It creates a PathBuf parameter and attempts to pass that to the RequestBuilder json() method, which sends the path itself as the body of the request. Instead, it would help to parameterise operations that accept a binary body so that they may accept any object that implements the Into<Body> trait. This would allow the caller to pass a binary body in several forms, such as a slice of bytes or a Stream that reads from some other source.
|
While the sample application does not currently have any binary body parameters, I have one in another application I am working on. This portion of the API definition document: ... becomes the following generated routine ... pub async fn worker_task_upload_chunk<B: Into<reqwest::Body>>(
configuration: &configuration::Configuration,
task: &str,
body: B,
) -> Result<crate::models::UploadedChunk, Error<WorkerTaskUploadChunkError>> {
let local_var_client = &configuration.client;
let local_var_uri_str = format!("{}/v1/worker/task/{task}/chunk", configuration.base_path, task=crate::apis::urlencode(task));
let mut local_var_req_builder = local_var_client.post(local_var_uri_str.as_str());
if let Some(ref local_var_user_agent) = 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();
let local_var_content = local_var_resp.text().await?;
if !local_var_status.is_client_error() && !local_var_status.is_server_error() {
serde_json::from_str(&local_var_content).map_err(Error::from)
} else {
let local_var_entity: Option<WorkerTaskUploadChunkError> = 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))
}
} |
| seen++; | ||
| } | ||
| } | ||
| assert seen == 1; |
There was a problem hiding this comment.
Should we use assert in test code only?
There was a problem hiding this comment.
What about simply using isBinary in the mustache template instead of creating x-rust-type-parameter ?
There was a problem hiding this comment.
Should we use
assertin test code only?
I put the assert here because it should be an invariant. The generated code won't be correct (something is seriously wrong) if there is more than one binary body parameter in the allParams list, when there was only one in the bodyParams list. It seems like it would be better to crash rather than to generate incorrect code if that is ever true, as it presumably represents a bug in the code.
What about simply using
isBinaryin the mustache template instead of creatingx-rust-type-parameter?
I'm not sure how to do that with the limited functionality available in the templates. We need three things to be true:
- that it's binary
- that it's a body parameter
- that it's the only body parameter
In addition to these three conditions, we need to make that determination outside of the {{#allParams}} loop in the template where we emit function arguments. I could not see how to use the isBinary property of a specific index in the bodyParams list as a predicate in the appropriate part of the template, or how to use the length of the list as a predicate.
I also expect that as the Rust code generator becomes more complete we will need to do more of this. That is, add more trait bounds and generic parameters as certain other types of arguments are added; e.g., today for strings we generate code like:
async fn endpoint(
arg1: &str,
arg2: &str,
)But it would be more common to accept something like an AsRef<str>; e.g.,
async fn endpoint<S1: AsRef<str>, S2: AsRef<str>>(
arg1: S1,
arg2: S2,
)This means you can pass either a &str or a String or, I believe, something that implements Display.
What are your thoughts?
There was a problem hiding this comment.
I put the assert here because it should be an invariant. The generated code won't be correct (something is seriously wrong)
Don't think we should use assert in non-test code. We usually use condition to perform a check and throw an exception instead.
There was a problem hiding this comment.
@wing328 which exception should be raised here?
|
Thanks Joshua for your work on implementing a more generic approach to uploading bytes, this specific feature is quite useful for a project we are building at www.smartlayers.io |
|
@bcourtine this seems relevant to the |
|
I believe that this PR should enhance https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/resources/3_0/rust/petstore.yaml to include an endpoint which triggers this new code, and then the samples will generate the new code. |
|
What's the status here? I need a |
The Rust-based reqwest client does not correctly handle a binary body
parameter today. It creates a
PathBufparameter and attempts to passthat to the
RequestBuilderjson()method, which sends the path itselfas the body of the request.
Instead, it would help to parameterise operations that accept a binary
body so that they may accept any object that implements the
Into<Body>trait. This would allow the caller to pass a binary body in several
forms, such as a slice of bytes or a
Streamthat reads from some othersource.
Note possible related issue: #5218
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.For Windows users, please run the script in Git BASH.
master,5.1.x,6.0.x