[Rust] Split out request logic, implement form parameters #528

euank · 2018-07-10T17:51:36Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change.
Copied the technical committee

Description of the PR

This PR contains two changes which I needed to make for a particular swagger client to work.

The first moves most request logic into a standalone file / method. This makes it much erasier to avoid variable conflicts since the Request.execute method doesn't have the generated params in its scope, and as such doesn't have to worry about conflicting with them.

I intend for .execute to behave identically to what we previously generated per operation.

I did, however, also implement form data while I was at it. That is included as a second separate commit, and I can file it separately if that's preferred.

Fixes #512, #525

euank · 2018-07-10T18:27:06Z

CI failure is a network or mvn flake it looks like

bjgill

I really like what you've done - it makes the mustache templates a lot more readable. I might need to move rust-server over to use this pattern as well.

bjgill · 2018-07-19T13:08:55Z

modules/openapi-generator/src/main/resources/rust/request.rs

+use serde;
+use serde_json;
+
+pub struct ApiKey {


Should these be pub(crate)?

Yup, updated, thanks!

bjgill · 2018-07-19T13:12:33Z

samples/client/petstore/rust/src/apis/swagger_request.rs

@@ -0,0 +1,214 @@
+use std::borrow::Cow;


Is it necessary to have multiple copies of this file?

Oops, this was a rebase mess-up to have both here; this one's gone now.

bjgill · 2018-07-19T13:25:08Z

modules/openapi-generator/src/main/resources/rust/request.rs

+    pub fn returns_nothing(mut self, val: bool) -> Self {
+        self.no_return_type = val;
+        self
+    }


We only ever call this with val=true. Do we need to have val?

bjgill · 2018-07-19T13:28:54Z

modules/openapi-generator/src/main/resources/rust/request.rs

+                    parsed.map_err(|e| Error::from(e))
+                })
+        )
+    }


I've not reviewed this in detail - I assume you've just copied the logic across.

More or less.

I did add minor error handling around uri parsing that was an unwrap before and restructured it a bit to make it read cleaner, but it's meant to be just a straight copy+paste.

Other than forms. I did add form parameters since those weren't present at all before.

This reduces the number of variables which are used in the generated operations, thus fixing OpenAPITools#512. This also fixed a TODO related to URI parsing errors. Other than that, it is meant to be functionally identical.

Up until now, they just weren't there at all

wing328 · 2018-07-23T15:11:16Z

@euank thanks for the PR, which has been merged into master.

@bjgill thanks for reviewing the change.

* Port of PR swagger-api/swagger-codegen#8804. * Correction of conflict with PR #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.

…ls#528) * [Rust] Move request logic into standalone file This reduces the number of variables which are used in the generated operations, thus fixing OpenAPITools#512. This also fixed a TODO related to URI parsing errors. Other than that, it is meant to be functionally identical. * [Rust] Add support for non-file form params Up until now, they just weren't there at all * [Rust] Use more rustic terms in example

…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.

euank force-pushed the rust-split-out-request-making branch from 92c8dd4 to ea41745 Compare July 10, 2018 19:29

wing328 added the Client: Rust label Jul 12, 2018

wing328 modified the milestone: 3.1.1 Jul 12, 2018

wing328 added the Enhancement: General label Jul 19, 2018

wing328 added this to the 3.1.2 milestone Jul 19, 2018

bjgill reviewed Jul 19, 2018

View reviewed changes

euank added 3 commits July 22, 2018 22:54

[Rust] Move request logic into standalone file

248cfcf

This reduces the number of variables which are used in the generated operations, thus fixing OpenAPITools#512. This also fixed a TODO related to URI parsing errors. Other than that, it is meant to be functionally identical.

[Rust] Add support for non-file form params

6a31ef5

Up until now, they just weren't there at all

[Rust] Use more rustic terms in example

546f510

euank force-pushed the rust-split-out-request-making branch from ea445b6 to 546f510 Compare July 23, 2018 05:55

bjgill approved these changes Jul 23, 2018

View reviewed changes

wing328 merged commit 2e6bec7 into OpenAPITools:master Jul 23, 2018

euank deleted the rust-split-out-request-making branch July 23, 2018 15:22

euank restored the rust-split-out-request-making branch July 23, 2018 15:22

bcourtine added a commit to bcourtine/openapi-generator that referenced this pull request Oct 17, 2018

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

aac88a5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rust] Split out request logic, implement form parameters #528

[Rust] Split out request logic, implement form parameters #528

euank commented Jul 10, 2018

euank commented Jul 10, 2018

bjgill left a comment

bjgill Jul 19, 2018

euank Jul 23, 2018

bjgill Jul 19, 2018

euank Jul 23, 2018

bjgill Jul 19, 2018

bjgill Jul 19, 2018

euank Jul 23, 2018

wing328 commented Jul 23, 2018

[Rust] Split out request logic, implement form parameters #528

[Rust] Split out request logic, implement form parameters #528

Conversation

euank commented Jul 10, 2018

PR checklist

Description of the PR

euank commented Jul 10, 2018

bjgill left a comment

Choose a reason for hiding this comment

bjgill Jul 19, 2018

Choose a reason for hiding this comment

euank Jul 23, 2018

Choose a reason for hiding this comment

bjgill Jul 19, 2018

Choose a reason for hiding this comment

euank Jul 23, 2018

Choose a reason for hiding this comment

bjgill Jul 19, 2018

Choose a reason for hiding this comment

bjgill Jul 19, 2018

Choose a reason for hiding this comment

euank Jul 23, 2018

Choose a reason for hiding this comment

wing328 commented Jul 23, 2018