[Rust][Client] Multiple returns becomes optional (fixes #6650). #6673

bcourtine · 2020-06-15T14:26:39Z

By default, rust client (using Reqwest) returns a simple return type. Handling multiple returns (several 2xx codes is optional).

PR checklist

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. 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/config/java*. For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @frol (2017/07) @farcaller (2017/08) @bjgill (2017/12) @richardwhiuk (2019/07) @paladinzh (2020/05) @wing328

seunlanlege · 2020-06-16T09:43:39Z

samples/client/petstore/rust/reqwest/petstore/src/apis/pet_api.rs

-            let entity: Option<UploadFileSuccess> = serde_json::from_str(&content).ok();
-            let result = ResponseContent { status, content, entity };
-            Ok(result)
+            serde_json::from_str(&content).map_err(Error::from)


missing Ok() also i tried testing locally, didn't work for me:

java -jar ./modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g rust -i slack_web_modified.json -o slack --skip-validate-spec --remove-operation-id-prefix --library reqwest --additional-properties useSingleRequestParameter=true,supportAsync=true,supportMultipleReturns=true

Hi,

I generated a slack client from official Github JSON with the same parameters as yours:

java -jar ./modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g rust -i https://raw.githubusercontent.com/slackapi/slack-api-specs/master/web-api/slack_web_openapi_v2.json -o /tmp/slack --skip-validate-spec --remove-operation-id-prefix --library reqwest --additional-properties useSingleRequestParameter=true,supportAsync=true,supportMultipleReturns=true

The client compiles and works fine: can you provide some information to reproduce the problem?

PS: in issue #6650, @HenningHolmDE tested the commit with and without the new param and didn't notice a problem.

In the code serde_json::from_str(&content).map_err(Error::from) you quoted, Ok() is not missing, since this line produces the expected return type Result<Entity, Error>.

This line is equivalent to Ok(serde_json::from_str(&content)?)

supportMultipleReturns=false

this was the only thing that worked for me

Do you have an example OAS to reproduce your problem?

I have a compiling client (with Rust 1.44.0 and both values for supportMultipleReturns).

@bcourtine I think @seunlanlege is using https://gist.githubusercontent.com/wing328/61225b4ad2d838447340a70494e09333/raw/b42fa7cf01df2a9269b910d19393a96081a246ff/slack_web_modified.json

Hm, using that spec file also works for me (rust stable as well as nightly):

java -jar ./modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g rust -i https://gist.githubusercontent.com/wing328/61225b4ad2d838447340a70494e09333/raw/b42fa7cf01df2a9269b910d19393a96081a246ff/slack_web_modified.json -o slack --skip-validate-spec --remove-operation-id-prefix --library reqwest --additional-properties useSingleRequestParameter=true,supportAsync=true,supportMultipleReturns=true cd slack cargo check > Finished dev [unoptimized + debuginfo] target(s) in 0.14s

Or is the problem related to function instead of compilation?

…ses".

seunlanlege · 2020-06-17T12:28:14Z

I think the last thing here is to remove the unknown value/list variants from the error type. The api specification defines all possible response schemas. So those variants are redundant.

HenningHolmDE · 2020-06-17T12:59:02Z

I think the last thing here is to remove the unknown value/list variants from the error type. The api specification defines all possible response schemas. So those variants are redundant.

Wouldn't it be useful to be able to access the data if the response type is unexpected? (This is of course an error, but we are talking error handling here.)
As for the UnknownList, this could already be covered by UnknownValue as serde_json::value::Value can represent this as Array(Vec<Value>)?

(Just my two cents as I was mentioned erlier and got the notfication for the comment.)

wing328 · 2020-06-17T13:30:56Z

Wouldn't it be useful to be able to access the data if the response type is unexpected?

That's what we're doing at the moment in other clients to include the response payload in the error response object (e.g. ApiException class) so the developers can find out more clues on why the error occurs from the response payload.

seunlanlege · 2020-06-17T16:06:39Z

Wouldn't it be useful to be able to access the data if the response type is unexpected?

yeah, I understand your reasoning.

@bcourtine let's limit it to just Unknown(serde_json::Value) for now then like @HenningHolmDE pointed out.

seunlanlege · 2020-06-17T16:12:59Z

samples/client/petstore/rust/reqwest/fileResponseTest/src/apis/default_api.rs

-            Ok(result)
+            serde_json::from_str(&content).map_err(Error::from)
        } else {
            let entity: Option<FileresponsetestError> = serde_json::from_str(&content).ok();


in light of this, I don't think it's possible for serde_json::from_str() to fail because you've already covered serde_json::Value

ResponseContent should become:

struct ResponseContent { entity: T, status: StatusCode, }

As you pointed out, the call to serde_json::from_str() won't fail due to unexpected Types as we have covered serde_json::Value - but it might fail when content does not contain valid JSON.

hmm, then we should use the response.json() method.

I'm afraid this will not help, as in case of invalid JSON reqwest::Response.json() will also fail:

pub async fn json<T: DeserializeOwned>(self) -> Result<T>
Try to deserialize the response body as JSON.
[...]
Errors
This method fails whenever the response body is not in JSON format or it cannot be properly deserialized to target type T. For more details please see serde_json::from_reader.

yes but then we already have an error for reqwest defined.

#[derive(Debug)] pub enum Error<T> { Reqwest(reqwest::Error), Serde(serde_json::Error), Io(std::io::Error), ResponseError(ResponseContent<T>), }

so code becomes

let status = resp.status(); if status.is_success() { // would recommend using derive_more::From for the error type Ok(response.json().await.map_err(Error::Reqwest)?) } else { let entity: CloseError = response.json().await.map_err(Error::Reqwest)?; let error = ResponseContent { status, entity }; Err(Error::ResponseError(error)) }

hopefully, @bcourtine's comment clarifies things @HenningHolmDE

to clarify further:

What I'm advocating for is removing the content field in ResponseContent

because:

since the response content is malformed JSON, the only thing useful to do with it is: log to stdout/err

since the response content is malformed JSON, the only thing useful to do with it is: log to stdout/err

It may or may not be malformed. We've seen issues in which a different type of object was returned (e.g. expecting array of Dog but get a single Dog, expecting a Cat but get a Dog due to invalid spec, expecting JSON but got XML as XML is the default when accept is not specified in the request header in that particular case) so my recommendation is still to include it in the Error response object to make developer's life easier in troubleshooting what's wrong.

I was under the impression that logging the reqwest Error had the response text, but after testing i just confirmed it doesn't so this is fine.

include it in the Error response object to make developer's life easier in troubleshooting what's wrong.

I think that's the last thing, we can finally get this merged? 🎊 🥳

I think that's the last thing, we can finally get this merged? 🎊 🥳

No objections from my side. 👍

👍 👍 👍

wing328 · 2020-06-18T15:26:23Z

 |packageVersion|Rust package version.| |1.0.0|
 |supportAsync|If set, generate async function call instead. This option is for 'reqwest' library only| |true|
+|supportMultipleResponses|If set, return type wraps an enum of all possible 2xx schemas. This option is for 'reqwest' library only| |false|
 |useSingleRequestParameter|Setting this property to true will generate functions with a single argument containing all API endpoint parameters instead of one argument per parameter.| |false|
 
 ## IMPORT MAPPING
Perform git status
On branch pull/6673
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   docs/generators/rust.md

@bcourtine Please update the doc as well when you've time.

seunlanlege · 2020-06-18T21:02:30Z

Any chance we can get this merged today?

wing328 · 2020-06-19T06:03:25Z

Just pushed ae1a050 to resolve the CI failure. Will merge if all tests pass.

fixes OpenAPITools#6650. Rust client: multiple returns becomes optional.

fae63e3

seunlanlege reviewed Jun 16, 2020

View reviewed changes

Rename new param "supportMultipleReturns" into "supportMultipleRespon…

74f033e

…ses".

seunlanlege suggested changes Jun 17, 2020

View reviewed changes

Remove redundant UnknownList enum option.

67ef2ce

wing328 added Client: Rust Enhancement: Feature labels Jun 18, 2020

wing328 added this to the 5.0.0 milestone Jun 18, 2020

update doc

ae1a050

wing328 merged commit 93bd857 into OpenAPITools:master Jun 19, 2020

Uh oh!

[Rust][Client] Multiple returns becomes optional (fixes #6650). #6673

[Rust][Client] Multiple returns becomes optional (fixes #6650). #6673

Uh oh!

Conversation

bcourtine commented Jun 15, 2020

PR checklist

Uh oh!

seunlanlege Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcourtine Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seunlanlege commented Jun 17, 2020

Uh oh!

HenningHolmDE commented Jun 17, 2020

Uh oh!

wing328 commented Jun 17, 2020

Uh oh!

seunlanlege commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seunlanlege Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wing328 commented Jun 18, 2020

Uh oh!

seunlanlege commented Jun 18, 2020

Uh oh!

wing328 commented Jun 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

seunlanlege Jun 16, 2020 •

edited

Loading

bcourtine Jun 16, 2020 •

edited

Loading

seunlanlege commented Jun 17, 2020 •

edited

Loading

seunlanlege Jun 17, 2020 •

edited

Loading