[Rust][Bounty] Issues with client code generation.
seunlanlege opened this issue · 24 comments
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
reqwestcrate 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
👍 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.
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
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.
@wing328 any luck that this is picked up soon?
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
It seems that params structures as well as their fields generated by #6230 are private and hence are not usable :(
Change the api for methods from:
Please use the option --remove-operation-id-prefix
Remove prefix of operationId, e.g. config_getId => getId
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.
@imp do you mind showing the error message you got when using the *Params structures?
@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).
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.
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 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?
@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.
@HenningHolmDE the fix has been merged into master. Please pull the latest to give it another try.
-
The generated return type returns a
ResponseContentwhere it should be an enum to account for multiple success types. -
The generated Error type should also not use the
ResponseContenttype 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),
}I believe all tasks have been completed. Thanks @seunlanlege again for sponsoring these fixes/enhancements 🙏
However the return values are still not typed sometimes. Running the latest version of the generator on the Open food facts api spec( here: https://openfoodfacts.github.io/openfoodfacts-server/reference/api.html#tag/Read-Requests) results in the following return type as an example:
#[derive(Clone, Debug, PartialEq, Default, Serialize, Deserialize)]
pub struct SearchForProducts {
#[serde(rename = "count", skip_serializing_if = "Option::is_none")]
pub count: Option<serde_json::Value>,
#[serde(rename = "page", skip_serializing_if = "Option::is_none")]
pub page: Option<serde_json::Value>,
#[serde(rename = "page_count", skip_serializing_if = "Option::is_none")]
pub page_count: Option<serde_json::Value>,
#[serde(rename = "page_size", skip_serializing_if = "Option::is_none")]
pub page_size: Option<serde_json::Value>,
#[serde(rename = "products", skip_serializing_if = "Option::is_none")]
pub products: Option<serde_json::Value>,
#[serde(rename = "skip", skip_serializing_if = "Option::is_none")]
pub skip: Option<serde_json::Value>,
}
impl SearchForProducts {
pub fn new() -> SearchForProducts {
SearchForProducts {
count: None,
page: None,
page_count: None,
page_size: None,
products: None,
skip: None,
}
}
}I just checkt that for the petstore example everything works fine, so normally it should work.