Skip to content

rust: better support for binary body parameters #9021

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

rust: better support for binary body parameters #9021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions ...openapi-generator/src/main/java/org/openapitools/codegen/languages/RustClientCodegen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@
package org.openapitools.codegen.languages;

import com.google.common.base.Strings;
import io.swagger.v3.oas.models.Operation;
import io.swagger.v3.oas.models.media.ArraySchema;
import io.swagger.v3.oas.models.media.Schema;
import io.swagger.v3.oas.models.media.StringSchema;
import io.swagger.v3.oas.models.servers.Server;
import io.swagger.v3.parser.util.SchemaTypeUtil;
import org.openapitools.codegen.*;
import org.openapitools.codegen.meta.features.*;
Expand Down Expand Up @@ -503,6 +505,38 @@ public String toOperationId(String operationId) {
return StringUtils.underscore(sanitizedOperationId);
}

@Override
public CodegenOperation fromOperation(String path, String httpMethod, Operation operation, List<Server> servers) {
CodegenOperation op = super.fromOperation(path, httpMethod, operation, servers);

// If this endpoint has a single binary body parameter, we want
// to accept a body argument with the trait bound Into<Body>.
// This requires a type parameter in the function definition,
// in advance of actual parameter, and is thus far easier to decide
// here than in the template.
if (REQWEST_LIBRARY.equals(getLibrary()) && op.bodyParams != null && op.bodyParams.size() == 1) {
CodegenParameter cp = op.bodyParams.get(0);

if (cp.isBinary) {
cp.baseType = cp.dataType = "B";
op.vendorExtensions.put("x-rust-type-parameter", "B: Into<reqwest::Body>");

// A copy of the body parameter will appear somewhere in the
// allParams list. We need to doctor that one as well.
int seen = 0;
for (CodegenParameter ap : op.allParams) {
if (ap.isBinary && ap.isBodyParam) {
ap.baseType = ap.dataType = "B";
seen++;
}
}
assert seen == 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use assert in test code only?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about simply using isBinary in the mustache template instead of creating x-rust-type-parameter ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use assert in 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 isBinary in the mustache template instead of creating x-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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jayvdb jayvdb Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wing328 which exception should be raised here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just a RuntimeException

}
}

return op;
}

@Override
public Map<String, Object> postProcessOperationsWithModels(Map<String, Object> objs, List<Object> allModels) {
@SuppressWarnings("unchecked")
Expand Down
93 changes: 86 additions & 7 deletions modules/openapi-generator/src/main/resources/rust/reqwest/api.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,65 @@ pub {{#supportAsync}}async {{/supportAsync}}fn {{{operationId}}}(configuration:

{{/vendorExtensions.x-group-parameters}}
{{^vendorExtensions.x-group-parameters}}
pub {{#supportAsync}}async {{/supportAsync}}fn {{{operationId}}}(configuration: &configuration::Configuration, {{#allParams}}{{{paramName}}}: {{^required}}Option<{{/required}}{{#required}}{{#isNullable}}Option<{{/isNullable}}{{/required}}{{#isString}}{{#isArray}}Vec<{{/isArray}}&str{{#isArray}}>{{/isArray}}{{/isString}}{{#isUuid}}{{#isArray}}Vec<{{/isArray}}&str{{#isArray}}>{{/isArray}}{{/isUuid}}{{^isString}}{{^isUuid}}{{^isPrimitiveType}}{{^isContainer}}{{#isBodyParam}}crate::models::{{/isBodyParam}}{{/isContainer}}{{/isPrimitiveType}}{{{dataType}}}{{/isUuid}}{{/isString}}{{^required}}>{{/required}}{{#required}}{{#isNullable}}>{{/isNullable}}{{/required}}{{^-last}}, {{/-last}}{{/allParams}}) -> Result<{{#supportMultipleResponses}}ResponseContent<{{{operationIdCamelCase}}}Success>{{/supportMultipleResponses}}{{^supportMultipleResponses}}{{^returnType}}(){{/returnType}}{{#returnType}}{{{returnType}}}{{/returnType}}{{/supportMultipleResponses}}, Error<{{{operationIdCamelCase}}}Error>> {
pub {{#supportAsync}}async {{/supportAsync}}fn {{{operationId}}}{{!
}}{{#vendorExtensions.x-rust-type-parameter}}{{!
}}<{{{vendorExtensions.x-rust-type-parameter}}}>{{!
}}{{/vendorExtensions.x-rust-type-parameter}}{{!
}}(
configuration: &configuration::Configuration,
{{!
}}{{#allParams}} {{{paramName}}}: {{!
!
! If this is an optional parameter, or a required parameter that
! is nullable, wrap the type in an Option<T>:
!
}}{{^required}}Option<{{/required}}{{!
}}{{#required}}{{!
}}{{#isNullable}}Option<{{/isNullable}}{{!
}}{{/required}}{{!

!
! Represent a string as a string slice (&str).
! TODO: Array should be a slice, not a Vector; i.e., &[&str].
!
}}{{#isString}}{{!
}}{{#isArray}}Vec<{{/isArray}}{{!
}}&str{{!
}}{{#isArray}}>{{/isArray}}{{!
}}{{/isString}}{{!

!
! Represent a UUID as a string slice (&str).
! TODO: Array should be a slice, not a Vector; i.e., &[&str].
!
}}{{#isUuid}}{{!
}}{{#isArray}}Vec<{{/isArray}}{{!
}}&str{{!
}}{{#isArray}}>{{/isArray}}{{!
}}{{/isUuid}}{{!

}}{{^isString}}{{^isUuid}}{{!
}}{{^isPrimitiveType}}{{^isContainer}}{{#isBodyParam}}{{!
!
! Complex body parameters need a prefix to refer to the
! struct we defined in the models module:
!
}}crate::models::{{!
}}{{/isBodyParam}}{{/isContainer}}{{/isPrimitiveType}}{{!
}}{{{dataType}}}{{!
}}{{/isUuid}}{{/isString}}{{!

!
! Close out the Option<T> wrapper:
!
}}{{^required}}>{{/required}}{{!
}}{{#required}}{{!
}}{{#isNullable}}>{{/isNullable}}{{!
}}{{/required}}{{!
}},
{{!
}}{{/allParams}}{{!
}}) -> Result<{{#supportMultipleResponses}}ResponseContent<{{{operationIdCamelCase}}}Success>{{/supportMultipleResponses}}{{^supportMultipleResponses}}{{^returnType}}(){{/returnType}}{{#returnType}}{{{returnType}}}{{/returnType}}{{/supportMultipleResponses}}, Error<{{{operationIdCamelCase}}}Error>> {
{{/vendorExtensions.x-group-parameters}}

let local_var_client = &configuration.client;
Expand Down Expand Up @@ -271,12 +329,33 @@ pub {{#supportAsync}}async {{/supportAsync}}fn {{{operationId}}}(configuration:
local_var_req_builder = local_var_req_builder.form(&local_var_form_params);
{{/hasFormParams}}
{{/isMultipart}}
{{#hasBodyParam}}
{{#bodyParams}}
local_var_req_builder = local_var_req_builder.json(&{{{paramName}}});
{{/bodyParams}}
{{/hasBodyParam}}

{{!
!
! Body parameter:
!
!}}{{#hasBodyParam}}{{!
}}{{#bodyParams}}{{!
!
! If the body parameter is binary, we pass it to the reqwest body()
! method. This requires the body parameter argument to this function
! to be generic, and match the Into<reqwest::Body> bound, allowing
! a stream or a byte buffer to be used:
!
!}}{{#isBinary}}{{!
}} local_var_req_builder = local_var_req_builder.body({{{paramName}}});
{{!
}}{{/isBinary}}{{!
!
! Otherwise, we assume the parameter has the Serializable trait, and
! pass it to the reqwest json() method:
!
}}{{^isBinary}}{{!
}} local_var_req_builder = local_var_req_builder.json(&{{{paramName}}});
{{!
}}{{/isBinary}}{{!
}}{{/bodyParams}}{{!
}}{{/hasBodyParam}}{{!
}}
let local_var_req = local_var_req_builder.build()?;
let {{^supportAsync}}mut {{/supportAsync}}local_var_resp = local_var_client.execute(local_var_req){{#supportAsync}}.await{{/supportAsync}}?;

Expand Down
45 changes: 37 additions & 8 deletions samples/client/petstore/rust/reqwest/petstore/src/apis/pet_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,10 @@ pub enum UploadFileError {
}


pub fn add_pet(configuration: &configuration::Configuration, body: crate::models::Pet) -> Result<(), Error<AddPetError>> {
pub fn add_pet(
configuration: &configuration::Configuration,
body: crate::models::Pet,
) -> Result<(), Error<AddPetError>> {

let local_var_client = &configuration.client;

Expand Down Expand Up @@ -112,7 +115,11 @@ pub fn add_pet(configuration: &configuration::Configuration, body: crate::models
}
}

pub fn delete_pet(configuration: &configuration::Configuration, pet_id: i64, api_key: Option<&str>) -> Result<(), Error<DeletePetError>> {
pub fn delete_pet(
configuration: &configuration::Configuration,
pet_id: i64,
api_key: Option<&str>,
) -> Result<(), Error<DeletePetError>> {

let local_var_client = &configuration.client;

Expand Down Expand Up @@ -145,7 +152,10 @@ pub fn delete_pet(configuration: &configuration::Configuration, pet_id: i64, api
}

/// Multiple status values can be provided with comma separated strings
pub fn find_pets_by_status(configuration: &configuration::Configuration, status: Vec<String>) -> Result<Vec<crate::models::Pet>, Error<FindPetsByStatusError>> {
pub fn find_pets_by_status(
configuration: &configuration::Configuration,
status: Vec<String>,
) -> Result<Vec<crate::models::Pet>, Error<FindPetsByStatusError>> {

let local_var_client = &configuration.client;

Expand Down Expand Up @@ -176,7 +186,10 @@ pub fn find_pets_by_status(configuration: &configuration::Configuration, status:
}

/// Multiple tags can be provided with comma separated strings. Use tag1, tag2, tag3 for testing.
pub fn find_pets_by_tags(configuration: &configuration::Configuration, tags: Vec<String>) -> Result<Vec<crate::models::Pet>, Error<FindPetsByTagsError>> {
pub fn find_pets_by_tags(
configuration: &configuration::Configuration,
tags: Vec<String>,
) -> Result<Vec<crate::models::Pet>, Error<FindPetsByTagsError>> {

let local_var_client = &configuration.client;

Expand Down Expand Up @@ -207,7 +220,10 @@ pub fn find_pets_by_tags(configuration: &configuration::Configuration, tags: Vec
}

/// Returns a single pet
pub fn get_pet_by_id(configuration: &configuration::Configuration, pet_id: i64) -> Result<crate::models::Pet, Error<GetPetByIdError>> {
pub fn get_pet_by_id(
configuration: &configuration::Configuration,
pet_id: i64,
) -> Result<crate::models::Pet, Error<GetPetByIdError>> {

let local_var_client = &configuration.client;

Expand Down Expand Up @@ -241,7 +257,10 @@ pub fn get_pet_by_id(configuration: &configuration::Configuration, pet_id: i64)
}
}

pub fn update_pet(configuration: &configuration::Configuration, body: crate::models::Pet) -> Result<(), Error<UpdatePetError>> {
pub fn update_pet(
configuration: &configuration::Configuration,
body: crate::models::Pet,
) -> Result<(), Error<UpdatePetError>> {

let local_var_client = &configuration.client;

Expand Down Expand Up @@ -271,7 +290,12 @@ pub fn update_pet(configuration: &configuration::Configuration, body: crate::mod
}
}

pub fn update_pet_with_form(configuration: &configuration::Configuration, pet_id: i64, name: Option<&str>, status: Option<&str>) -> Result<(), Error<UpdatePetWithFormError>> {
pub fn update_pet_with_form(
configuration: &configuration::Configuration,
pet_id: i64,
name: Option<&str>,
status: Option<&str>,
) -> Result<(), Error<UpdatePetWithFormError>> {

let local_var_client = &configuration.client;

Expand Down Expand Up @@ -308,7 +332,12 @@ pub fn update_pet_with_form(configuration: &configuration::Configuration, pet_id
}
}

pub fn upload_file(configuration: &configuration::Configuration, pet_id: i64, additional_metadata: Option<&str>, file: Option<std::path::PathBuf>) -> Result<crate::models::ApiResponse, Error<UploadFileError>> {
pub fn upload_file(
configuration: &configuration::Configuration,
pet_id: i64,
additional_metadata: Option<&str>,
file: Option<std::path::PathBuf>,
) -> Result<crate::models::ApiResponse, Error<UploadFileError>> {

let local_var_client = &configuration.client;

Expand Down
19 changes: 15 additions & 4 deletions samples/client/petstore/rust/reqwest/petstore/src/apis/store_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ pub enum PlaceOrderError {


/// For valid response try integer IDs with value < 1000. Anything above 1000 or nonintegers will generate API errors
pub fn delete_order(configuration: &configuration::Configuration, order_id: &str) -> Result<(), Error<DeleteOrderError>> {
pub fn delete_order(
configuration: &configuration::Configuration,
order_id: &str,
) -> Result<(), Error<DeleteOrderError>> {

let local_var_client = &configuration.client;

Expand All @@ -77,7 +80,9 @@ pub fn delete_order(configuration: &configuration::Configuration, order_id: &str
}

/// Returns a map of status codes to quantities
pub fn get_inventory(configuration: &configuration::Configuration, ) -> Result<::std::collections::HashMap<String, i32>, Error<GetInventoryError>> {
pub fn get_inventory(
configuration: &configuration::Configuration,
) -> Result<::std::collections::HashMap<String, i32>, Error<GetInventoryError>> {

let local_var_client = &configuration.client;

Expand Down Expand Up @@ -112,7 +117,10 @@ pub fn get_inventory(configuration: &configuration::Configuration, ) -> Result<:
}

/// For valid response try integer IDs with value <= 5 or > 10. Other values will generated exceptions
pub fn get_order_by_id(configuration: &configuration::Configuration, order_id: i64) -> Result<crate::models::Order, Error<GetOrderByIdError>> {
pub fn get_order_by_id(
configuration: &configuration::Configuration,
order_id: i64,
) -> Result<crate::models::Order, Error<GetOrderByIdError>> {

let local_var_client = &configuration.client;

Expand All @@ -138,7 +146,10 @@ pub fn get_order_by_id(configuration: &configuration::Configuration, order_id: i
}
}

pub fn place_order(configuration: &configuration::Configuration, body: crate::models::Order) -> Result<crate::models::Order, Error<PlaceOrderError>> {
pub fn place_order(
configuration: &configuration::Configuration,
body: crate::models::Order,
) -> Result<crate::models::Order, Error<PlaceOrderError>> {

let local_var_client = &configuration.client;

Expand Down
Loading