[Rust] Client library choice between hyper and reqwest #1258

bcourtine · 2018-10-17T13:37:35Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Port of PR swagger-api/swagger-codegen#8804

Propose an alternative library (reqwest) to generate a Rust client. This library is much easier to use than hyper (especially to deal with https services).

wing328 · 2018-10-17T14:08:54Z

@bcourtine thanks for the PR. I'd a quick look and overall it looks good.

Can you please add samples/client/petstore/rust-reqwest to the following line in pom.xml so that Travis will test the auto-generated code for Rust (reqwest) client?

openapi-generator/pom.xml

Line 1027 in aac88a5

<module>samples/client/petstore/rust</module>

bjgill

Thanks for the PR. Before I give this a more in-depth look, I've got a couple of high-level questions/comments.

bjgill · 2018-10-17T14:29:22Z

bin/rust-petstore.sh

@@ -27,6 +27,6 @@ fi

 # if you've executed sbt assembly previously it will use that instead.
 export JAVA_OPTS="${JAVA_OPTS} -XX:MaxPermSize=256M -Xmx1024M -DloggerPath=conf/log4j.properties"
-ags="generate -t modules/openapi-generator/src/main/resources/rust -i modules/openapi-generator/src/test/resources/2_0/petstore.yaml -g rust -o samples/client/petstore/rust -DpackageName=petstore_client $@"
+ags="generate -t modules/openapi-generator/src/main/resources/rust -i modules/openapi-generator/src/test/resources/2_0/petstore.yaml -g rust -o samples/client/petstore/rust -DpackageName=petstore_client --library=hyper $@"


I don't think this is back-compatible. As such, I would suggest creating this against the branch for the next major release - 4.0.x, I believe. See https://github.com/OpenAPITools/openapi-generator/wiki/Git-Branches

This "library" modification is optional. If no library is chosen for generation, Hyper remains the default library. I may be wrong, but I think it is backward compatible.

If hyper is the default, the change should be backward-compatible.

If reqwest is the default, the change should target 3.4.x branch as it's a breaking change with fallbacks.

From RustClientCodegen.java :

CliOption libraryOption = new CliOption(CodegenConstants.LIBRARY, "library template (sub-template) to use"); libraryOption.setEnum(supportedLibraries); // set hyper as the default libraryOption.setDefault(HYPER_LIBRARY); cliOptions.add(libraryOption); setLibrary(HYPER_LIBRARY);

bjgill · 2018-10-17T14:32:41Z

modules/openapi-generator/src/main/resources/rust/Cargo.mustache

 tokio-core = "*"
+{{/hyper}}


My highest-level question - why make the decision at codegen time rather than at compile time? You could presumably achieve the same effect with Rust features. I'm not saying you're wrong - merely curious as to why you chose as you did.

I don't have a good reason for that. I am a beginner in Rust development and learning.

I don't know what purpose this dev lib deserves. Since I could generate my project without it in the Cargo file, I dropped it, simply to keep the file as tiny as possible.

If you think it is a mistake, I can put it back.

I probably misunderstood your question (I thought it was about tokio-core lib only).

For the whole library choice, the main reason is that since I'm learning Rust, I don't know how to use it yet.

Another reason whould be that hyper and reqwest templates are quite different: API main methods currently don't have the same return type (Result vs Box<Future<Result>>>. So I presume that generating a client managing both libs with features would have much more complex code (but since I don't know features, I may be wrong).

That seems reasonable. I could well imagine that trying to have both in the same crate could cause problems if they ever had incompatible dependencies.

…ution.

bcourtine · 2018-10-18T06:16:44Z

Shippable failed tests seems unrelated to current PR. It fails in "ElixirPetstoreClientTests" which is not impacted by this branch commits.

bjgill · 2018-10-18T15:57:14Z

Thanks for your prompt answers.

It looks if you're still busy making code changes. Please let me know when you've finished and I'll take a closer look.

bcourtine · 2018-10-19T08:45:55Z

You can review it. It works pretty well (I used the reqwest generator with success on several projects).

There are still some bugs, but I can't find an easy way to correct them, and won't have enough time to work on it soon. For now, I correct theses remaining problems post generation when they occur.

FYI, here are the problems I spotted that are not corrected yet:

If a query parameter has type Vec<i32>, generated code does not compile:

        let query_string = {
            let mut query = ::url::form_urlencoded::Serializer::new(String::new());
            query.append_pair("vec_i32_param", &vec_i32_param.join(",").to_string());
            query.finish()
        };

It must be corrected by something like:

        let query_string = {
            let mut query = ::url::form_urlencoded::Serializer::new(String::new());
            let vec_str_param: Vec<String> = vec_i32_param.iter().map(|p| p.to_string()).collect();
            query.append_pair("vec_i32_param", &vec_str_param.join(",").to_string());
            query.finish()
        };

Header management has several problems (marked as TODO in RustClientCodegen.java) .

With reqwest, I did not find a simple way to create a custom raw header (like with hyper), and had to introduce a structure for each header (cf. configuration.rs file). All possibles headers are collected in java postProcessOperationsWithModels method. Theses headers are used without transformation. So, corresponding generated code does not respect Rust case convention. Worse, if a header key contains a char like '-', generated code does not compile.

bcourtine · 2018-10-19T08:56:18Z

Another thought about headers. In Cargo.toml, for reqwest generation, I would like to remove explicit hyper dependency to rely on transitive dependency management.

I had to reintroduce the dependency because of headers. Implementing reqwest::header::Header requires to use directly hyper::error::Error. It is the only required code where reqwest does not obfuscate hyper usage.

wing328 · 2018-10-19T09:14:26Z

        let vec_str_param: Vec<String> = vec_i32_param.iter().map(|p| p.to_string()).collect();

What about using {{#isInteger}} and/or {{#isLong}} tag to add the above line for integer and/or long?

bcourtine · 2018-10-19T09:25:44Z

Are theses booleans valuated for a list of integers/longs?

I prefer waiting for @bjgill opinion:

This problems is not specific to reqwest and also exists on hyper generated code.
There can be a better Rust syntax to achieve the same goal.

wing328 · 2018-10-19T09:33:37Z

Feel free to wait for @bjgill option. I was just throwing ideas...

If it's a list, you will need to do the following instead:

{{#isListContainer}}
{{#isInteger}}
...
{{/isInteger}}
{{/isListContainer}}

Please use -DdebugOperations or -DdebugModels to get a full list of mustache tags.

bcourtine · 2018-10-19T10:19:27Z

@wing328 Thanks for the ideas: your proposal sounds good. I just want @bjgill opinion on the best Rust syntax to achieve correction (there may be a more effective way than transforming each integer into a string before concatenating…).

bjgill · 2018-10-19T13:31:36Z

I don't think any of the Rust generators handle query parameters very cleanly. For lists, you proposal seems sensible. I wonder whether we want to special-case integers, though. Will we not want to do the same thing for all non-string types?

Indeed, I believe that String::to_string is a no-op. Thus, could you always include .iter().map(|p| p.to_string()).collect() if {{#isListContainer}}?

bcourtine · 2018-10-19T14:38:29Z

Sounds a good idea. I do that and let you review the whole PR ;)

bjgill

Looks broadly sensible, thanks. I've reviewed the sample as being easier to do than reviewing the mustache templates themselves.

I may have commented on some of the preexisting code that you haven't changed (e.g. the models, maybe?). Feel free to dismiss any such comments - I'm certainly not expecting you to fix up oddities in previous contributions.

I would recommend looking at the warnings produced by cargo check and cargo clippy - I suspect they won't all be worth fixing, but some may be of interest.

bjgill · 2018-10-19T13:33:52Z

bin/rust-reqwest-petstore.sh

+export JAVA_OPTS="${JAVA_OPTS} -XX:MaxPermSize=256M -Xmx1024M -DloggerPath=conf/log4j.properties"
+ags="generate -t modules/openapi-generator/src/main/resources/rust -i modules/openapi-generator/src/test/resources/2_0/petstore.yaml -g rust -o samples/client/petstore/rust-reqwest -DpackageName=petstore_client --library=reqwest $@"
+
+java ${JAVA_OPTS} -jar ${executable} ${ags}


Why have the rust-reqwest sample generation in a separate script? Could you not extend the rust sample generation to do both, reducing the risk that someone will do one and forget to do the other?

I don't have a particular reason. I will merge both scripts.

bjgill · 2018-10-19T13:34:38Z

...es/openapi-generator/src/main/java/org/openapitools/codegen/languages/RustClientCodegen.java

@@ -141,6 +133,15 @@ public RustClientCodegen() {
        cliOptions.add(new CliOption(CodegenConstants.HIDE_GENERATION_TIMESTAMP, CodegenConstants.HIDE_GENERATION_TIMESTAMP_DESC)
                .defaultValue(Boolean.TRUE.toString()));

+        supportedLibraries.put(HYPER_LIBRARY, "HTTP client: Hyper.");
+        supportedLibraries.put(REQWEST_LIBRARY, "HTTP client: Reqwest");


Minor nit - inconsistent full stops?

bjgill · 2018-10-19T13:37:09Z

...es/openapi-generator/src/main/java/org/openapitools/codegen/languages/RustClientCodegen.java

        supportingFiles.add(new SupportingFile("model_mod.mustache", modelFolder, "mod.rs"));
-        supportingFiles.add(new SupportingFile("lib.rs", "src", "lib.rs"));
+        supportingFiles.add(new SupportingFile("lib.rs.mustache", "src", "lib.rs"));


Minor nit: the convention seems to be x.rs => x.mustache

...es/openapi-generator/src/main/java/org/openapitools/codegen/languages/RustClientCodegen.java

bjgill · 2018-10-19T13:39:58Z

...es/openapi-generator/src/main/java/org/openapitools/codegen/languages/RustClientCodegen.java

+                }
+            }
+
+            // TODO Manage Rust var codestyle (snake case) and compile problems (headers with special chars, like '-').


Could you use StringUtils.underscore to get the header name into a form that will always compile?

It is possible, but complicated. It requires to introduce an object to maintain both native and underscored versions. Native version is required for the reqwest::header::Header implementation (header name).

bjgill · 2018-10-19T15:24:33Z

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

+        Ok(())
+    }
+
+    fn upload_file(&self, pet_id: i64, additional_metadata: &str, file: ::models::File) -> Result<::models::ApiResponse, Error> {


Should we be doing something with additional_metadata and file?

File management is a remaining TODO forked from hyper code.

bjgill · 2018-10-19T15:28:06Z

samples/client/petstore/rust-reqwest/src/models/api_response.rs

+/* 
+ * OpenAPI Petstore
+ *
+ * This is a sample server Petstore server. For this sample, you can use the api key `special-key` to test the authorization filters.


Is this correct?

I will check. I think I did not modify models. They come from the historical hyper code.

bjgill · 2018-10-19T15:28:28Z

samples/client/petstore/rust-reqwest/src/models/api_response.rs

+ * Generated by: https://openapi-generator.tech
+ */
+
+/// ApiResponse : Describes the result of uploading an image resource


What is this doc comment attached to?

bjgill · 2018-10-19T15:29:24Z

samples/client/petstore/rust-reqwest/src/models/api_response.rs

+  pub fn with_code(mut self, code: i32) -> ApiResponse {
+    self.code = Some(code);
+    self
+  }


What is the purpose of all these getters and setters? Why not have the fields public?

Copied from original code. Don't know the reason...

But I imagine the purpose is to get a "fluent API", with method calls chained: model.with_code("code").with_name("name").... It is a common pattern in other languages like java to keep fields private and to provide this access methods.

EDIT: I note this for later, but I won't correct it in this branch: it would be a breaking change in generated API since models are shared between hyper and reqwest versions.

I note for later, but I won't correct it in this branch: it would be a breaking change in generated API since models are shared between hyper and reqwest versions.

bjgill · 2018-10-19T15:29:35Z

samples/client/petstore/rust-reqwest/src/models/api_response.rs

+      code: None,
+      _type: None,
+      message: None
+    }


Can you not implement default instead?

After a closer look, it is not as simple. When there is a required var in a model, default constructor new has required vars as parameters. So, Default trait cannot be implemented in this case.

Good point - https://github.com/nrc/derive-new might be what you want, then.

bjgill · 2018-10-19T15:37:00Z

modules/openapi-generator/src/main/resources/rust/reqwest/api.mustache

@@ -56,7 +56,7 @@ impl {{classname}} for {{classname}}Client {
        let query_string = {
            let mut query = ::url::form_urlencoded::Serializer::new(String::new());
 {{#queryParams}}
-            query.append_pair("{{baseName}}", &{{paramName}}{{#isListContainer}}.join(","){{/isListContainer}}.to_string());
+            query.append_pair("{{baseName}}", &{{paramName}}{{#isListContainer}}.iter().map(|p| p.to_string()).collect::<Vec<String>>().join(","){{/isListContainer}}.to_string());


Nice.

For bonus points, you could use into_iter instead of iter. As I understand it, iter produces references to p, and to_string will therefore clone p. into_iter would give you an owned p, and thus to_string would not clone..

bcourtine · 2018-10-21T09:42:10Z

Thanks for the complete review. I will correct this. But since I am quite busy, corrections may not be available before some days.

bcourtine · 2018-10-21T18:33:56Z

I just looked at reqwest 0.9 release notes and 😭 :

The RequestBuilder has simple methods to set headers using strings, which can work in most places.

Most custom header code added in configuration template can be dropped with this feature.

…st transition feature).

bcourtine · 2018-10-22T12:06:17Z

@bjgill Most of the points you spotted have been corrected. You can review this new version.

I did not correct model problems, because it would be a breaking change in generated API (since models are used by both hyper and reqwest versions), especially replacing accessors by public fields.

bjgill · 2018-10-22T16:23:02Z

Looks good, thanks. I think all my outstanding comments are things that are present in the hyper generator already and therefore should not block this PR.

bcourtine · 2018-10-22T17:22:33Z

@bjgill Cool, thanks. I stop changing code for now and it let you (or @wing328 ) merge the PR.

wing328 · 2018-10-23T09:04:52Z

bin/rust-petstore.sh

 java ${JAVA_OPTS} -jar ${executable} ${ags}
+
+# Generate client using reqwest lib.
+ags="generate -t modules/openapi-generator/src/main/resources/rust -i modules/openapi-generator/src/test/resources/2_0/petstore.yaml -g rust -o samples/client/petstore/rust-reqwest -DpackageName=petstore_client --library=reqwest $@"


Usually, we create another script (e.g. bin/rust-petstore-reqwest.sh) for a different HTTP library. (You will find a lot of ./bin/java-petstore-{library}.sh scripts for different HTTP libraries supported by the Java client generator)

I suggest we do the same for Rust reqwest instead of putting both into one singel script.

This reverts commit 970f996.

bcourtine · 2018-10-23T12:40:32Z

@wing328 Done.

Poke @bjgill who suggested merging theses scripts ;)

bjgill · 2018-10-23T12:42:17Z

I'm happy for this to be merged (modulo CI) - sorry for suggesting something that you then had to undo.

bcourtine · 2018-10-23T13:30:46Z

@bjgill I was just trolling. Merging samples scripts (and undoing it) was trivial.

bjgill · 2018-10-24T17:01:03Z

@wing328 - anything else? Otherwise, I'll merge this tomorrow.

wing328 · 2018-10-26T04:22:04Z

@bjgill nothing from me. Please go ahead with the merge.

wing328 · 2018-10-26T16:48:38Z

Thank you @bcourtine again for the contribution and @bjgill for the review.

I've sent out a tweet (https://twitter.com/oas_generator/status/1055863197687664640) to help promote the enhancement.

Have a nice weekend.

wing328 · 2018-10-31T15:57:04Z

@bcourtine thanks for the PR, which is included in the v3.3.2 release: https://twitter.com/oas_generator/status/1057649626101112832

…1258) * Port of PR swagger-api/swagger-codegen#8804. * Correction of conflict with PR OpenAPITools#528 (missing template file). * Add rust-reqwest samples to Circle CI tests. * Add integration test pom.xml file with launcher to trigger cargo execution. * Deduplicate Maven project name. * Fix "api_key" header for Petstore. * Better API key management. * Fix query param for lists of objects other than strings (numbers, etc.). * Update to reqwest 0.9, and refactor of header management (using reqwest transition feature). * Merge scripts generating rust-hyper and rust-reqwest samples. * Consistent full stops. * Use raw variables in all Rust mustache templates. * Replace production build in CI with a quick simple check. * Update samples. * Finish Reqwest 0.9 migration (removing "hyper 0.11" transition feature). * Configuration implements Default trait. * API template reorganized: HashMap is not required anymore. * Revert "Merge scripts generating rust-hyper and rust-reqwest samples." This reverts commit 970f996. * Remove deprecated "-XX:MaxPermSize" java arg.

bcourtine added 2 commits October 17, 2018 15:25

Port of PR swagger-api/swagger-codegen#8804.

ef0a2a2

Correction of conflict with PR OpenAPITools#528 (missing template file).

aac88a5

Add rust-reqwest samples to Circle CI tests.

a347327

bjgill reviewed Oct 17, 2018

View reviewed changes

bcourtine added 3 commits October 17, 2018 17:25

Add integration test pom.xml file with launcher to trigger cargo exec…

7b4dfc2

…ution.

Deduplicate Maven project name.

e7c99c5

Fix "api_key" header for Petstore.

3c7c52a

Better API key management.

0ef969d

wing328 added Client: Rust Enhancement: New generator Enhancement: Feature and removed Enhancement: New generator labels Oct 19, 2018

wing328 added this to the 3.3.2 milestone Oct 19, 2018

Fix query param for lists of objects other than strings (numbers, etc.).

a1c865c

bjgill reviewed Oct 19, 2018

View reviewed changes

Update to reqwest 0.9, and refactor of header management (using reqwe…

e5f514a

…st transition feature).

bcourtine added 7 commits October 22, 2018 10:23

Merge scripts generating rust-hyper and rust-reqwest samples.

970f996

Consistent full stops.

556231e

Use raw variables in all Rust mustache templates.

457ff19

Replace production build in CI with a quick simple check.

9327601

Update samples.

0b1f506

Finish Reqwest 0.9 migration (removing "hyper 0.11" transition feature).

0eff82b

Configuration implements Default trait.

595a4fc

API template reorganized: HashMap is not required anymore.

4208034

wing328 reviewed Oct 23, 2018

View reviewed changes

bcourtine added 2 commits October 23, 2018 14:36

Revert "Merge scripts generating rust-hyper and rust-reqwest samples."

e13b3e1

This reverts commit 970f996.

Remove deprecated "-XX:MaxPermSize" java arg.

605f950

wing328 approved these changes Oct 26, 2018

View reviewed changes

bjgill merged commit f4eb969 into OpenAPITools:master Oct 26, 2018

dbldippy mentioned this pull request Jan 10, 2019

[Rust] client codegen ignores all fields that are of type "formData" #525

Closed

[Rust] Client library choice between hyper and reqwest #1258

[Rust] Client library choice between hyper and reqwest #1258

Conversation

bcourtine commented Oct 17, 2018

PR checklist

Description of the PR

wing328 commented Oct 17, 2018

bjgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcourtine commented Oct 18, 2018

bjgill commented Oct 18, 2018

bcourtine commented Oct 19, 2018

bcourtine commented Oct 19, 2018

wing328 commented Oct 19, 2018

bcourtine commented Oct 19, 2018

wing328 commented Oct 19, 2018

bcourtine commented Oct 19, 2018

bjgill commented Oct 19, 2018

bcourtine commented Oct 19, 2018

bjgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcourtine Oct 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcourtine Oct 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcourtine commented Oct 21, 2018

bcourtine commented Oct 21, 2018

bcourtine commented Oct 22, 2018

bjgill commented Oct 22, 2018

bcourtine commented Oct 22, 2018

Choose a reason for hiding this comment

bcourtine commented Oct 23, 2018

bjgill commented Oct 23, 2018

bcourtine commented Oct 23, 2018

bjgill commented Oct 24, 2018

wing328 commented Oct 26, 2018

wing328 commented Oct 26, 2018

wing328 commented Oct 31, 2018

bcourtine Oct 21, 2018 •

edited

Loading

bcourtine Oct 21, 2018 •

edited

Loading