[Rust][Client][Reqwest] Better http error handling #6481

bcourtine · 2020-05-29T07:24:53Z

When the server response is not OK (code >= 300), return error is not a generic "Reqwest::Error" but a custom "ResponseError", containing the status, the raw body (String), and a deserialization as "serde_json::Value" when it is possible.

Related issue: #6178

PR checklist

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: 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

…ort async with reqwest 0.10+).

seunlanlege · 2020-05-29T10:00:34Z

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

+        let mut resp = client.execute(req)?;
+        if resp.status().is_success() {
+            Ok(resp.json()?)
+        } else {
+            let status = resp.status();
+            let content = resp.text()?;
+            let entity: Option<serde_json::Value> = serde_json::from_str(&content).ok();
+            let error = crate::apis::ResponseErrorContent { status, content, entity };
+            Err(Error::ResponseError(error))
+        }


hey 🙋‍♂️ great attempt on this issue, but this isn't complete yet.

The openapi spec allows for defining a schema for every possible response status: eg

"responses": { "200": {}, "202": {}, "400": {}, "default": {} }

so instead of just checking for a success status, you'll need to match over the response status.

and deserialize based on the defined schema for that status as well as the fallback(default) schema.

I imagine this means that the return type of methods would become an enum,

pub enum OperationIdResponse { Status200(Status200Schema), Status202(Status202Schema), Status400(Status400Schema), Unspecified(DefaultSchema) } // .. snip match response.status().as_u16() { 200 => Ok(OperationIdResponse::Status200(response.json().await?)) 202 => Ok(OperationIdResponse::Status202(response.json().await?)) 400 => Ok(OperationIdResponse::Status400(response.json().await?)) _ => Ok(OperationIdResponse::Unspecified(response.json().await?)) } // .. snip

Hi @seunlanlege,

Thanks for your comment. I know this is just a first attempt which is incomplete.

I had the same idea of an enum as response, but I don't like it. I think we should not return Ok when the return status is not ok…

I am working on something like:

#[derive(Debug, Clone)] pub struct ResponseErrorContent<T> { pub status: reqwest::StatusCode, pub content: String, pub entity: Option<T>, } #[derive(Debug)] pub enum Error<T> { Reqwest(reqwest::Error), Serde(serde_json::Error), Io(std::io::Error), ResponseError(ResponseErrorContent<T>), }

where T could be an enum of error types corresponding to error codes.

also bear in mind that there could be more than one success response in the 2xx range, So success types would potentially be an enum

With an enum, multiple success codes can be handled, but this case is not frequent. So I don't know if it is worth complicating the generated code for this.

multiple success codes can be handled, but this case is not frequent.

unfortunately it's actually very common 😅

jira/sendgrid's openapi specifications have multiple 2xx success schemas

…de_json::Value` in generated code).

bcourtine · 2020-05-29T12:26:33Z

@seunlanlege Still not complete, but the last version should be much closer from what you want.

bcourtine · 2020-05-30T07:55:40Z

Feature seems almost complete, with some limitations:

deserialization will match the error structure. So if 4XX and 5XX errors have the same structure, a 500 response may result in a Status400 enum value.
for some APIs (Slack), default response is the error case. For others (petstore), it is the ok response. I let the default response in the error enum, but it depends…

seunlanlege · 2020-06-01T19:29:41Z

🙋 @bcourtine need a hand with this?

bcourtine · 2020-06-02T07:21:50Z

raising_hand @bcourtine need a hand with this?

Hi @seunlanlege,

Thanks for the proposition.

I think current code is the best compromise between features and complexity of generated code. Here are my thoughts about a track I did not implement (matching the return code to force the error enum value):

let error_entity = match response.status().as_u16() {
    400 => OperationErrors::Status400(response.json().await?)
    401 => OperationErrors::Status401(response.json().await?)
    402 => OperationErrors::Status402(response.json().await?)
    // etc.
}

I think this code is more complicated and useless, since serde can choose automatically a value from the response body structure. If exact return code is needed by client code, it is not lost since the ResponseErrorContent struct contains it.

As a user, when I generate such code, I correct generated code to group responses that have the same structure (with for example Status4XX instead of Status400 + Status401 + …). Such modification is much easier if there is not an explicit code match in the operation.

I may miss some quick improvements. If you find simple ways to improve the code, feel free to propose suggestions/evolutions.

If not, I didn't have plans to work more on this issue, and am waiting for the merge approval (/cc @wing328 ).

seunlanlege · 2020-06-02T11:48:54Z

hey @bcourtine so I tested this with the slack api and this PR doesn't actually address the original issue. Which was, the return types of the methods should be strongly-typed.

Here's a method signature from one of slack's apis.

/// method signature shouldn't return a hashmap, it should be strongly typed with the provided schema in the spec
 pub async fn delete_photo(configuration: &configuration::Configuration, params: DeletePhotoParams) -> Result<::std::collections::HashMap<String, serde_json::Value>, Error<DeletePhotoErrors>>;

/// struct for typed errors of method `delete_photo`
#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(untagged)]
pub enum DeletePhotoErrors {
    /// this also shouldn't be hashmap, this should be strongly typed as well, there's a schema in the spec for it
    DefaultResponse(::std::collections::HashMap<String, serde_json::Value>),
    /// this is unnecessary
    UnknownList(Vec<serde_json::Value>),
    /// as well as this.
    UnknownValue(serde_json::Value),
}

Also on the topic of enum success types, this is taken from jira's openapi specification, as you can see there's a definition for 200 and 201 responses.

if you are unable to continue work on this that's fine please let @wing328 know so he can assiggn it to someone else.

wing328 · 2020-06-03T14:07:04Z

About the HashMap response type (in the default response), I've performed some tests with other generators as well and at this stage, I think it's a bug with the default codegen so I'll take a look and hopefully file a fix by the end of this week.

wing328 · 2020-06-05T09:35:34Z

The issue with slack_web.json has to do with Swagger Parser incorrectly parsing the inline schema when additionalProperties is set to false.

As a workaround I've removed it here: https://gist.github.com/wing328/61225b4ad2d838447340a70494e09333

Can you please this modified spec a try to see if the output looks good to you?

I did a test and no longer see the Map/Dict response and the inline model is created as a model correctly.

seunlanlege · 2020-06-05T14:45:31Z

Hey @wing328 I've tested this and i noticed that the DefaultResponse variant of the error contains the success return data.

/// struct for typed errors of method `close`
#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(untagged)]
pub enum CloseErrors {
    DefaultResponse(crate::models::ImCloseSchema),
    UnknownList(Vec<serde_json::Value>),
    UnknownValue(serde_json::Value),
}
// for the method:
pub async fn close(configuration: &configuration::Configuration, params: CloseParams) -> Result<crate::models::ImCloseSchema, Error<CloseErrors>>;

bcourtine · 2020-06-07T17:02:28Z

Hi @seunlanlege,

Some APIs use default response to describe API "success", others use it as a "global error catch".

Since both usages exist, it is not possible to guess if default response must be added in the error enum or not. So it is always added. If you don't need it (because default describes a success instead of an error), you can to remove it.

seunlanlege · 2020-06-08T12:07:31Z

Since both usages exist, it is not possible to guess if default response must be added in the error enum or not. So it is always added. If you don't need it (because default describes a success instead of an error), you can to remove it.

what if we added a CLI argument that tells the generator how to handle the default case? as an error or success.

bcourtine · 2020-06-09T06:58:52Z

Possible, but I don't know if CLI should be used for such micro-tunning of code generation.
What do you think about it, @wing328 ?

…or structs/enums).

…better-error-typing

wing328 · 2020-06-11T10:56:44Z

I've filed #6625 to fix the default response.

When testing with this PR, I got the following changes (partial):

@@ -248,7 +248,7 @@ pub enum ChannelsMarkError {
 #[derive(Debug, Clone, Serialize, Deserialize)]
 #[serde(untagged)]
 pub enum ChannelsRenameError {
-    DefaultResponse(crate::models::ChannelsRenameSchema),
+    DefaultResponse(crate::models::ChannelsRenameErrorSchema),
     UnknownList(Vec<serde_json::Value>),
     UnknownValue(serde_json::Value),
 }
@@ -257,7 +257,7 @@ pub enum ChannelsRenameError {
 #[derive(Debug, Clone, Serialize, Deserialize)]
 #[serde(untagged)]
 pub enum ChannelsRepliesError {
-    DefaultResponse(crate::models::ChannelsRepliesSchema),
+    DefaultResponse(crate::models::ChannelsRepliesErrorSchema),
     UnknownList(Vec<serde_json::Value>),
     UnknownValue(serde_json::Value),
 }
@@ -266,7 +266,7 @@ pub enum ChannelsRepliesError {
 #[derive(Debug, Clone, Serialize, Deserialize)]
 #[serde(untagged)]
 pub enum ChannelsSetPurposeError {
-    DefaultResponse(crate::models::ChannelsSetPurposeSchema),
+    DefaultResponse(crate::models::ChannelsSetPurposeErrorSchema),
     UnknownList(Vec<serde_json::Value>),
     UnknownValue(serde_json::Value),
 }

I think this confirms the default response is now correctly using the error models defined

wing328 · 2020-06-12T07:52:42Z

@bcourtine #6625 has been merged into master. Can you please merge the latest master into this branch and regenerate the samples? Thank you.

UPDATE: Done via a72be48

…rator into 5609-rust-client-better-error-typing

seunlanlege · 2020-06-12T14:50:43Z

just works™

wing328 · 2020-06-12T14:52:38Z

@seunlanlege thanks for the confirmation. I'll merge once all the tests pass.

wing328 · 2020-06-13T04:28:48Z

Travis CI tests passed: https://travis-ci.org/github/OpenAPITools/openapi-generator/builds/697621225?utm_source=github_status&utm_medium=notification but status not sent back to Github.

bcourtine added 2 commits May 29, 2020 08:42

Stronger typing for http errors with Rust client. (OpenAPITools#5609).

9e3565e

Merge branch 'master' into 5609-rust-client-better-error-typing (supp…

948d653

…ort async with reqwest 0.10+).

seunlanlege suggested changes May 29, 2020

View reviewed changes

bcourtine added 2 commits May 29, 2020 13:56

Error structure can be parametrized (but is still hardcoded with `ser…

ed3e8fb

…de_json::Value` in generated code).

Each API method has is own enum of functionnal errors.

3ad86b3

bcourtine added 2 commits May 29, 2020 14:46

Fix the missing "Debug" derivation for API error enums.

755fc69

Generate models for error deserialization.

ff3d510

wing328 added Client: Rust Enhancement: General labels Jun 3, 2020

wing328 added this to the 5.0.0 milestone Jun 3, 2020

bcourtine added 3 commits June 9, 2020 10:04

Handle several 2xx success models.

250d0ed

Expose new API objects, required to use the API (params, success, err…

a890621

…or structs/enums).

Merge remote-tracking branch 'upstream/master' into 5609-rust-client-…

733fd9d

…better-error-typing

Merge branch 'master' of https://github.com/openapitools/openapi-gene…

a72be48

…rator into 5609-rust-client-better-error-typing

wing328 merged commit 8e2bf99 into OpenAPITools:master Jun 13, 2020

HenningHolmDE mentioned this pull request Jun 13, 2020

[REQ][Rust Client] New Success types lead to exessive code when using the API #6650

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rust][Client][Reqwest] Better http error handling #6481

[Rust][Client][Reqwest] Better http error handling #6481

bcourtine commented May 29, 2020 •

edited by wing328

Loading

seunlanlege May 29, 2020 •

edited

Loading

bcourtine May 29, 2020

bcourtine May 29, 2020

seunlanlege May 29, 2020

bcourtine May 29, 2020

seunlanlege May 31, 2020 •

edited

Loading

bcourtine commented May 29, 2020

bcourtine commented May 30, 2020

seunlanlege commented Jun 1, 2020

bcourtine commented Jun 2, 2020

seunlanlege commented Jun 2, 2020

wing328 commented Jun 3, 2020

wing328 commented Jun 5, 2020 •

edited

Loading

seunlanlege commented Jun 5, 2020

bcourtine commented Jun 7, 2020

seunlanlege commented Jun 8, 2020

bcourtine commented Jun 9, 2020

wing328 commented Jun 11, 2020

wing328 commented Jun 12, 2020 •

edited

Loading

seunlanlege commented Jun 12, 2020

wing328 commented Jun 12, 2020

wing328 commented Jun 13, 2020

[Rust][Client][Reqwest] Better http error handling #6481

[Rust][Client][Reqwest] Better http error handling #6481

Conversation

bcourtine commented May 29, 2020 • edited by wing328 Loading

PR checklist

seunlanlege May 29, 2020 • edited Loading

Choose a reason for hiding this comment

bcourtine May 29, 2020

Choose a reason for hiding this comment

bcourtine May 29, 2020

Choose a reason for hiding this comment

seunlanlege May 29, 2020

Choose a reason for hiding this comment

bcourtine May 29, 2020

Choose a reason for hiding this comment

seunlanlege May 31, 2020 • edited Loading

Choose a reason for hiding this comment

bcourtine commented May 29, 2020

bcourtine commented May 30, 2020

seunlanlege commented Jun 1, 2020

bcourtine commented Jun 2, 2020

seunlanlege commented Jun 2, 2020

wing328 commented Jun 3, 2020

wing328 commented Jun 5, 2020 • edited Loading

seunlanlege commented Jun 5, 2020

bcourtine commented Jun 7, 2020

seunlanlege commented Jun 8, 2020

bcourtine commented Jun 9, 2020

wing328 commented Jun 11, 2020

wing328 commented Jun 12, 2020 • edited Loading

seunlanlege commented Jun 12, 2020

wing328 commented Jun 12, 2020

wing328 commented Jun 13, 2020

bcourtine commented May 29, 2020 •

edited by wing328

Loading

seunlanlege May 29, 2020 •

edited

Loading

seunlanlege May 31, 2020 •

edited

Loading

wing328 commented Jun 5, 2020 •

edited

Loading

wing328 commented Jun 12, 2020 •

edited

Loading