[Rust][Bounty] Issues with client code generation.

[seunlanlege]

Description

The generated client code for rust has a few problems:

• It uses the deprecated futures v0.1, when it should use the newer futures v0.3
• The return types of the api methods aren't strongly typed:

Box<dyn Future<Item = ::std::collections::HashMap<String, serde_json::Value>, Error = Error<serde_json::Value>>>

The models are properly generated but the return type of the methods don't reference the generated models at all.

• Some models aren't generated at all when defined as such in the schema:

openapi-generator version

openapi-generator-cli 5.0.0-SNAPSHOT
  commit : 46216cd
  built  : 2020-04-28T13:03:54Z
  source : https://github.com/openapitools/openapi-generator
  docs   : https://openapi-generator.tech/

OpenAPI declaration file content or url

https://api.slack.com/specs/openapi/v2/slack_web.json

Command line used for generation

java -jar ~/Downloads/openapi.jar generate -g rust -i https://api.slack.com/specs/openapi/v2/slack_web.json -o rust

Steps to reproduce

run the command above

Related issues/PRs

Suggest a fix/enhancement

• The generated client code should use the reqwest crate with async/await.
• Change the api for methods from:

pub trait UsersApi {
    fn users_conversations(&self, params: UsersConversationsParams) -> Result<::std::collections::HashMap<String, serde_json::Value>, Error>;
    fn users_delete_photo(&self, params: UsersDeletePhotoParams) -> Result<::std::collections::HashMap<String, serde_json::Value>, Error>;
    fn users_get_presence(&self, params: UsersGetPresenceParams) -> Result<::std::collections::HashMap<String, serde_json::Value>, Error>;
    fn users_identity(&self, params: UsersIdentityParams) -> Result<serde_json::Value, Error>;
    fn users_info(&self, params: UsersInfoParams) -> Result<::std::collections::HashMap<String, serde_json::Value>, Error>;
    fn users_list(&self, params: UsersListParams) -> Result<::std::collections::HashMap<String, serde_json::Value>, Error>;
    fn users_lookup_by_email(&self, params: UsersLookupByEmailParams) -> Result<::std::collections::HashMap<String, serde_json::Value>, Error>;
    fn users_profile_get(&self, params: UsersProfileGetParams) -> Result<::std::collections::HashMap<String, serde_json::Value>, Error>;
    fn users_profile_set(&self, params: UsersProfileSetParams) -> Result<::std::collections::HashMap<String, serde_json::Value>, Error>;
    fn users_set_active(&self, params: UsersSetActiveParams) -> Result<::std::collections::HashMap<String, serde_json::Value>, Error>;
    fn users_set_photo(&self, params: UsersSetPhotoParams) -> Result<::std::collections::HashMap<String, serde_json::Value>, Error>;
    fn users_set_presence(&self, params: UsersSetPresenceParams) -> Result<::std::collections::HashMap<String, serde_json::Value>, Error>;
}

to:

pub async fn conversations(config: &Configuration, params: UsersConversationsParams) -> Result;
pub async fn delete_photo(config: &Configuration, params: UsersDeletePhotoParams) -> Result;
pub async fn get_presence(config: &Configuration, params: UsersGetPresenceParams) -> Result;
pub async fn identity(config: &Configuration, params: UsersIdentityParams) -> Result;
pub async fn info(config: &Configuration, params: UsersInfoParams) -> Result;
pub async fn list(config: &Configuration, params: UsersListParams) -> Result;
pub async fn lookup_by_email(config: &Configuration, params: UsersLookupByEmailParams) -> Result;
pub async fn profile_get(config: &configuration, params: UsersProfileGetParams) -> Result;
pub async fn profile_set(config: &Configuration, params: UsersProfileSetParams) -> Result;
pub async fn set_active(config: &Configuration, params: UsersSetActiveParams) -> Result;
pub async fn set_photo(config: &Configuration, params: UsersSetPhotoParams) -> Result;
pub async fn set_presence(config: &Configuration, params: UsersSetPresenceParams) -> Result;

• Error and sucess return types should be strongly typed.
• Instead of making each param an argument on the method, the generated client code should aggregate the params into a struct that can be passed to the api method. e.g

Instead of:

fn users_list(&self, cursor: Option<&str>, token: Option<&str>, limit: Option<i32>, include_locale: Option<bool>)

do:

#[derive(Default)]
struct UsersListParams {
    cursor: Option<&str>,
    token: Option<&str>,
    limit: Option<i32>,
    include_locale: Option<bool>,
}

fn users_list(&self, params: UsersListParams)

I'm willing to sponsor work on this issue to the tune of $500.
cc @wing328

[auto-labeler]

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

[wing328]

The generated client code should use the reqwest crate with async/await.

You can use reqwest by providing the library option:

	library
	    library template (sub-template) to use. (Default: hyper)
	        hyper - HTTP client: Hyper.
	        reqwest - HTTP client: Reqwest.

In CLI, you need to add --library reqwest

[wing328]

Instead of making each param an argument on the method, the generated client code should aggregate the params into a struct that can be passed to the api method. e.g

Some other generators (e.g. PHP, Java (jersey2)) already support x-group-parameters to group method parameters into a single object.

Basically we need to do the same for Rust client generator.

[seunlanlege]

@wing328 any luck that this is picked up soon?

[wing328]

@seunlanlege can you please PM me via Slack?

https://join.slack.com/t/openapi-generator/shared_invite/enQtNzAyNDMyOTU0OTE1LTY5ZDBiNDI5NzI5ZjQ1Y2E5OWVjMjZkYzY1ZGM2MWQ4YWFjMzcyNDY5MGI4NjQxNDBiMTlmZTc5NjY2ZTQ5MGM

[wing328]

Instead of making each param an argument on the method, the generated client code should aggregate the params into a struct that can be passed to the api method. e.g

Done via #6230

[imp]

It seems that params structures as well as their fields generated by #6230 are private and hence are not usable :(

[wing328]

@imp that's fixed by #6381. Please give the latest master a try.

[wing328]

Change the api for methods from:

Please use the option --remove-operation-id-prefix

Remove prefix of operationId, e.g. config_getId => getId

[imp]

Still no dice. The *Params structures are public now, however they are located in the private module and are not re-exported from there alongside the trait and API client. So still no access to them from outside of the generated crate.

[wing328]

@imp do you mind showing the error message you got when using the *Params structures?

[imp]

@wing328 Here we go:

    Checking storage v1.0.0 (/xxx/out/rust-client)
error[E0432]: unresolved import `storage::apis::ListParams`
 --> examples/disk.rs:7:5
  |
7 | use storage::apis::ListParams;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^ no `ListParams` in `apis`

error[E0603]: module `disks_api` is private
  --> examples/disk.rs:8:20
   |
8  | use storage::apis::disks_api::ListParams;
   |                    ^^^^^^^^^ private module
   |
note: the module `disks_api` is defined here
  --> /xxx/out/rust-client/src/apis/mod.rs:35:1
   |
35 | mod disks_api;
   | ^^^^^^^^^^^^^^

So the ListParams is not re-exported from storage::apis and hence cannot be used from there (line 7). And it also cannot be used from disks_api, where it is defined, since the disks_api module is private (line 8).

[wing328]

@imp thanks for the info. I'll take another look tomorrow.

[wing328]

@imp I've filed #6453 to fix the issue. My local tests look good. Please have a look when you've time. Thanks.

[imp]

Confirm it works now.

Although now all the *Params structures gained a (potentially) long prefix (the trait name is now added to all the structure names). Was that done due to there risk of the name conflict between different APIs? If so, perhaps it might be possible to keep the structures with simple names and rename them on use? Something like

[apis/mod.rs]

pub use self::disks_api::ListParams as DisksApiListParams;

instead of current

pub use self::disks_api::{ DisksApiListParams };

That way the consumer has the choice of importing these structures from apis.rs with longer names as well as pulling them from the inner module with shorter names if there in fact would be no name conflicts.

Just a thought.

[wing328]

That way the consumer has the choice of importing these structures from apis.rs with longer names as well as pulling them from the inner module with shorter names if there in fact would be no name conflicts.

Sounds good. I'll file a PR

[wing328]

@imp I've filed #6470 to implement your suggestion.

[HenningHolmDE]

@wing328 First of all: Thanks for great work!
I'm currently playing around with the new features in order to migrate my project to async/await once 5.0.0 is released. The useSingleRequestParameter addition is really nice!

But when activating the option on the current snapshot (), the code won't compile. I am afraid #6470 broke the re-exports in api_mod.mustache as the Params postfix got lost. Locally changing the template seems to work:

diff --git a/templates/reqwest/api_mod.mustache b/templates/reqwest/api_mod.mustache
index 5e98f19..46f6471 100644
--- a/templates/reqwest/api_mod.mustache
+++ b/templates/reqwest/api_mod.mustache
@@ -67,7 +67,7 @@ pub use self::{{{classFilename}}}::{ {{{operationId}}} };
 {{#vendorExtensions.x-group-parameters}}
 {{#allParams}}
 {{#-first}}
-pub use self::{{{classFilename}}}::{{{operationIdCamelCase}}} as {{{classname}}}{{{operationIdCamelCase}}};
+pub use self::{{{classFilename}}}::{{{operationIdCamelCase}}}Params as {{{classname}}}{{{operationIdCamelCase}}}Params;
 {{/-first}}
 {{/allParams}}
 {{/vendorExtensions.x-group-parameters}}

Or am I wrong here?

[wing328]

@HenningHolmDE thanks for reporting the issue and suggesting the fix. Sorry for the regression.

I've filed #6586 to fix it and add a test to catch the issue moving forward.

[wing328]

@HenningHolmDE the fix has been merged into master. Please pull the latest to give it another try.

[HenningHolmDE]

@wing328 I now could build my project without having to change the template and the generated code looks fine, so the missing Params issue is fixed with #6586. Thanks for fixing this so quickly!

[seunlanlege]

1. The generated return type returns a ResponseContent where it should be an enum to account for multiple success types.

2. The generated Error type should also not use the ResponseContent type but the generated error enum directly.

a la

pub enum Error<T> {
    Reqwest(reqwest::Error),
    Serde(serde_json::Error),
    Io(std::io::Error),
    ResponseError(T),
}

[wing328]

I believe all tasks have been completed. Thanks @seunlanlege again for sponsoring these fixes/enhancements 🙏