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

[HenningHolmDE]

Is your feature request related to a problem? Please describe.

Before #6481 has been merged this morning, my test code for using the 5.0.0 SNAPSHOT with supportAsync: true and useSingleRequestParameter: true looked something like this:

    let servers = servers_api::list_servers(&configuration, params)
        .await
        .map_err(|err| format!("API call to list_servers failed: {:?}", err))?
        .servers;

For the same behaviour, I now have to write:

    let success = servers_api::list_servers(&configuration, params)
        .await
        .map_err(|err| format!("API call to list_servers failed: {:?}", err))?
        .entity
        .ok_or("API call to list_servers failed: Response could not be parsed.".to_string())?;
    let servers = match success {
        servers_api::ListServersSuccess::Status200(response) => Ok(response.servers),
        _ => Err("API call to list_servers failed: Unexpected success status code.".to_string()),
    }?;

In my oppinion, the code has become unnecessarily complex here.

Describe the solution you'd like

I am not sure if I overlooked a way for a simpler approach, but it would be nice to only deal with one layer of error handling when using the API.

Describe alternatives you've considered

As this is only related to template code, I can of course change this back to the previous behaviour using a customized templates, but I would like to stick to the default templates as far as possible.

Additional context

I am now using SNAPSHOT version 5.0.0-20200613.071037-272.

Thanks for looking into this! If I can give a hand finding a solution, I would be glad to do so.

[bcourtine]

Hi @HenningHolmDE ,

I am the culprit for this PR, but I agree.
In the first place, I did not want to implement it because code is now more complex, but it was a feature request to handle multiple 2xx schemas (which is very rare).

After discussion with @wing328 we will propose the feature as optionnal, adding a CLI option.

[HenningHolmDE]

Hi @bcourtine ,
I meant no offence. This is an iterative process and I am sure, after some fiddling around, we will find a solution that makes everyone happy. :)

After giving this some thought: Even when hadnling multiple 2xx, why do we need UnknownList and UnknownValue? (These are actually errors, aren't they?) Otherwise we could make the enum layer optional for when multiple 2xx are specified for that return type.

[bcourtine]

I let this default values to avoid an empty enum for methods that only return a success return code without body.
UnknownList and UnknownValue also allow to get an unexpected object (if returned object don't match the API description).

It can probably be improved. I will look at it later.

[HenningHolmDE]

What downside would an empty enum entry for "disembodied results" have?

pub enum GetSomethingSuccess {
    Status200(crate::models::GetSomethingResponse),
    Status201,
}

As UnknownList and UnknownValue (isn't every list also a value?) have to be handled separately in the code using the API anyways (if required), maybe something like Error::UnexpectedValue(serde_json::Value) could be a feasible tradeoff.

[bcourtine]

I agree an unexpected value error would be a good tradeoff 👍

But the empty enum could occur.
In OpenAPI, by convention, the "default response" is considered as an error.
But some APIs use "default response" to describe the success return. In this case, there can be no 2xx return code, leading to an empty enum if there is no default value.

[bcourtine]

The PR should fix the problem.

[HenningHolmDE]

@bcourtine I tested a local build of fae63e3 against my code and both variants (supportMultipleReturns: true and supportMultipleReturns: false) seem to work fine. (With the latter winning with respect to simplicity for my use case.)

Well done! 👍

Thinking about the name of the config option: As the OpenAPI Specifications names them "responses" (Schema: Responses Object), maybe supportMultipleResponses would be more fitting?

[bcourtine]

Good idea: just renamed the param to supportMultipleResponses.