[Rust] add new Rust client generator

[wing328]

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 for non-breaking changes and 3.0.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Add Rust client generator based on the Petstore samples provided by @farcaller (#6092)

[farcaller]

In the generated code for ApiResponse model, Type field is renamed to Type_ (should be underscorecased too)

[farcaller]

🤦‍♂️ just removed all the comments accidentally.

[farcaller]

Must be mapped to Vec<u8>, String can only hold a vaild utf-16 byte sequence.

[farcaller]

Not needed in rust, we can import everything by absolute paths, also saves the reserved namespace from pollution.

[farcaller]

The equivalent is generated in client.rs

[wing328]

Shall we name it as api_client.rs instead to make consistent with other generators?

[farcaller]

SGTM.

[farcaller]

Should be mod.rs

[farcaller]

I think language: rust is all you need for rust testing here.

[farcaller]

This doesn't exist?

[farcaller]

this generates references to e.g. pet_api_api, not pet_api

[farcaller]

Both mod entries should be outside of {{operations}} loop or the generated code gets multiple mod declarations.

[farcaller]

This generates multiple identical entries, is it missing {{#-last}} / {{/-last}}?

[farcaller]

Should be /// here and everywhere the api docs are expected.

Is description guaranteed to be a single line?

[wing328]

@farcaller thanks for the review note. I'll work on those shortly.

[farcaller]

Must be outside of the loop (single trait with multiple fns within.

[farcaller]

Item = () specifies the return type (() is the return type of none). It should be resolved to the appropriate model.

[farcaller]

ditto on impl outside of the loop

[farcaller]

`{{datatype}} ends up being an empty string here?

[farcaller]

Ditto on Item = ...

[farcaller]

How swagger deals with different types that map to a single downstream type?

e.g. in a model, a string field is String, but in argument, a string is usually a &str.

[wing328]

We can use vendor extensions to annotate that. Do you have a mapping for all different primitive types?

[farcaller]

I think String -> &str is the only special case, actually.

[wing328]

Fixed via 9ff1cc4. Please pull the latest to take a look

[farcaller]

{{path}} resolves to strings like /user/{username}, in case it has args, they must be substituted by {} and added to macro arguments list.

[wing328]

so it becomes /user/{}, right? and how to add it to the macro arguments list?

[farcaller]

Yes. It would look like format!("{}{}", configuration.base_path, path) assuming that path was defined in the function argument.

[farcaller]

I mean.. format!("{}/user/{}", configuration.base_path, user) in this particular case.

[farcaller]

Should be {{classFilename}}

[farcaller]

datatype for an array is Vec<TYPE>, not []TYPE

[farcaller]

Also, the TYPE, if it's another model, isn't available in this scope and should be referenced as super::MODEL_NAME (from the parent file)

[farcaller]

I assume there's no sense to have an optional array as it's same as empty array in swagger semantics? Then arrays shouldn't be wrapped in Option<>.

Also: see next one

[farcaller]

If the array can truly be optional, then init it with None (a value of Option)

[farcaller]

Some(...) only applies to Option<> (optional) fields. If the field isn't optional, assign as self.{{name}} = {{name}}

[wing328]

I've pushed af9537e to incorporate your suggested fixes but I didn't have a chance to verify the Rust generator's output yet.

Please continue to comment on the auto-generated code so that we can move this forward. Thanks!

[wing328]

In the generated code for ApiResponse model, Type field is renamed to Type_ (should be underscorecased too)

Fixed via e22a48f

[farcaller]

Here and everywhere where the comment isn't an apidoc comment it must remain // or it's a compilation failure (i.e. /// is used only before methods / functions / fields)

[farcaller]

format string must start with verbatim {} for the base_path: "{}{{path}}".

Actually, to make it less work, you can skip rewriting the path, e.g. if the path is /user/{userid} render it like this:

format!("{}/user/{userid}", configuration.base_path, userid=userid)

[farcaller]

Actually, the way code is structured, we have one use per imported mod:

pub use self::{{classFilename}}::{{classname}}. We don't import the operations.

[farcaller]

if returntype references a model, it should be prefixed with super::

[farcaller]

Notice, for arrays of models, that's Vec<super::Pet>, not e.g. super::Vec<Pet>.

[wing328]

OK. I'll need more time to handle this.

[wing328]

Done via c8ff9a9

[farcaller]

I'm looking at this method:

  /store/inventory:
    get:
      tags:
        - store
      summary: Returns pet inventories by status
      description: Returns a map of status codes to quantities
      operationId: getInventory
      produces:
        - application/json
      parameters: []
      responses:
        '200':
          description: successful operation
          schema:
            type: object
            additionalProperties:
              type: integer
              format: int32
      security:
        - api_key: []

And I'm confused of the return type. How is it expected to look in json?

[wing328]

And I'm confused of the return type. How is it expected to look in json?

{
  "cat": 10,
  "dog", 2,
  "rabbit", 32
}

Ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#model-with-mapdictionary-properties

[wing328]

More fixes via c8ff9a9

For "///", can you point out in the template where it should be used? Is it some sort of method/field documentation?

[farcaller]

Ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#model-with-mapdictionary-properties

Ok, so for these the mapped type would be ::std::collections::HashMap<String, T> where T is the target type.

[farcaller]

/// here.

[farcaller]

/// here.

[farcaller]

/// here.

[farcaller]

body leaks in here, it shouldn't (it's handled down in line 83)

[wing328]

So this line should only contain the path parameters and query parameters?

[farcaller]

For clarification, format!(...) builds an absolute URL here, starting with the scheme and down to query arguments. We get the scheme and base path from configuration.base_path, then the path is hard-coded static into the string, then the query arguments follow.

With format!("{}somepath/{option}/whatever?{}", A, B, option=C), the first {} is resolved to A, the second (which is in the ver end of the string) is resolved to B, and {option} is resolved to C via named argument.

[farcaller]

So yes, answering your question, only path parameters and query parameters (if any) will end up in here.

[farcaller]

The naming is getting mixed up, e.g.

format!("{}/pet/{petId}", configuration.base_path, pet_id=pet_id, api_key=api_key))

here petId comes from the original field name, but the assignment is done to pet_id.

[farcaller]

also, on the previous one: there's api_key, and it's not mapped to the URI path. Where's that coming from?

[farcaller]

Also here. I noticed that there are more arguments that I didn't expect to see, I guess those are query arguments?

They need to be processed like this:

// if query params are empty ...
let mut req = hyper::Request::new(
    hyper::Method::{{httpMethod}},
    format!("{}{{path}}", configuration.base_path{{#allParams}}, {{paramName}}={{paramName}}{{/allParams}}));
// else
let query = url::form_urlencoded::Serializer::new(String::new())
    {{#allParams}}.append_pair("{{paramName}}", {{paramName}}){{/allParams}} // only for query params
    .finish();
let mut req = hyper::Request::new(
    hyper::Method::{{httpMethod}},
    format!("{}{{path}}?{}", configuration.base_path, query{{#allParams}}, {{paramName}}={{paramName}}{{/allParams}}));

[wing328]

We need to deal with other type of parameters as well: header, query, form.

Please have a look at Go API client API templates or other templates on how we handle all these parameters using a hash/dictionary:

https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/go/api.mustache#L41-L47

https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/go/api.mustache#L103-L186

For API key, it's defined in security defintions and it's handled in the {{authMethod}} block: https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/go/api.mustache#L188-L203

Is there a way we can put the hyper::Request logic in api_client.rs instead while the API layer (e.g. pet_api.rs, user_api.rs) is only responsible for putting different parameters (e.g. header, form, etc) into a hash/dictionary? Later if there's demand for using another HTTP lib in Rust, we can simply make api_client.rs swappable.

[farcaller]

Ok, so, the code would look like:

        let configuration: &configuration::Configuration<C> = self.configuration.borrow();

        let method = hyper::Method::{{httpMethod}};
        
        // { if empty query args }
        let uri = format!("{}{{path}}", configuration.base_path{{#allParams}}, {{paramName}}={{paramName}}{{/allParams}}));
        // { else }
        let query = url::form_urlencoded::Serializer::new(String::new())
            {{#queryParams}}.append_pair("{{paramName}}", {{paramName}}){{/queryParams}} // only for query params
            .finish();
        let uri = format!("{}{{path}}{}", configuration.base_path, query{{#allParams}}, {{paramName}}={{paramName}}{{/allParams}}));
        // { end }
        
        let mut req = hyper::Request::new(method, uri);
        
        // { if header args }
        let mut headers = req.headers_mut();
        {{#headerParams}}
        headers.set_raw("{{paramName}}", "{{paramValue}}");
        {{/headerParams}}
        // { end }

        // { if body args }
        let serialized = serde_json::to_string(body).unwrap();
        req.headers_mut().set(hyper::header::ContentType::json());
        req.headers_mut().set(hyper::header::ContentLength(serialized.len() as u64));
        req.set_body(serialized);
        // { end }

        // send request
        Box::new(
            configuration.client.request(req).and_then(|res| { res.body().concat2() })
            .map_err(|e| Error::from(e))
            // { if no response expected }
            .and_then(|_| futures::future::ok(()))
            // { else }
            .and_then(|body| {
                let parsed: Result<{{returnType}}, _> = serde_json::from_slice(&body);
                parsed.map_err(|e| Error::from(e))
            }).map_err(|e| Error::from(e))
            // { end}
        )

This covers path, query, header params. Trying to figure where do form params land and there's also the "file" thing that requires multipart upload.

As on extracting the logic, I think it's better to have version 1 with code explicit. There are a few options on how to extract the logic, but I'm not sure which one would be more effective in terms of calls/memory allocation and still researching that.

[wing328]

Fixed docstring and added support for dictionary/hash via 73ceea0

[farcaller]

Ok, so, the code would look like:

        let configuration: &configuration::Configuration<C> = self.configuration.borrow();

        let method = hyper::Method::{{httpMethod}};
        
        // { if empty query args }
        let uri = format!("{}{{path}}", configuration.base_path{{#allParams}}, {{paramName}}={{paramName}}{{/allParams}}));
        // { else }
        let query = url::form_urlencoded::Serializer::new(String::new())
            {{#queryParams}}.append_pair("{{paramName}}", {{paramName}}){{/queryParams}} // only for query params
            .finish();
        let uri = format!("{}{{path}}{}", configuration.base_path, query{{#allParams}}, {{paramName}}={{paramName}}{{/allParams}}));
        // { end }
        
        let mut req = hyper::Request::new(method, uri);
        
        // { if header args }
        let mut headers = req.headers_mut();
        {{#headerParams}}
        headers.set_raw("{{paramName}}", "{{paramValue}}");
        {{/headerParams}}
        // { end }

        // { if body args }
        let serialized = serde_json::to_string(body).unwrap();
        req.headers_mut().set(hyper::header::ContentType::json());
        req.headers_mut().set(hyper::header::ContentLength(serialized.len() as u64));
        req.set_body(serialized);
        // { end }

        // send request
        Box::new(
            configuration.client.request(req).and_then(|res| { res.body().concat2() })
            .map_err(|e| Error::from(e))
            // { if no response expected }
            .and_then(|_| futures::future::ok(()))
            // { else }
            .and_then(|body| {
                let parsed: Result<{{returnType}}, _> = serde_json::from_slice(&body);
                parsed.map_err(|e| Error::from(e))
            }).map_err(|e| Error::from(e))
            // { end}
        )

This covers path, query, header params. Trying to figure where do form params land and there's also the "file" thing that requires multipart upload.

As on extracting the logic, I think it's better to have version 1 with code explicit. There are a few options on how to extract the logic, but I'm not sure which one would be more effective in terms of calls/memory allocation and still researching that.

[farcaller]

Fixed docstring and added support for dictionary/hash via 73ceea0

It's getting rendered as e.g.

super::::std::collections::HashMap<String, i32>

notice that there shouldn't be super:: in this case (the i32 is a primitive type), but if it was a pet, it'd be ::std::collections::HashMap<String, super::Pet>.

[wing328]

it'd be ::std::collections::HashMap<String, super::Pet>

Should be fixed.

As on extracting the logic, I think it's better to have version 1 with code explicit. There are a few options on how to extract the logic

I'm not saying version 1 is bad. My suggestion is based on experience working on other generators. I think it's just a matter of time we need to refactor common logics into ApiClient class to accommodate more feature requests.

but I'm not sure which one would be more effective in terms of calls/memory allocation and still researching that.

We can work on the performance as a day 2 requirement. First and foremost, we'll need to release a functional Rust API client to collect feedbacks from the community.

[wing328]

TODO: add logic to check optional/required ({{#required}}) parameters (query, form, header, body)

[wing328]

For template changes/fixes, please feel free to submit a PR directly to rust_generator branch to avoid the overhead in back-and-forth communications.

[farcaller]

I'm not saying version 1 is bad. My suggestion is based on experience working on other generators. I think it's just a matter of time we need to refactor common logics into ApiClient class to accommodate more feature requests.

Ack.

For template changes/fixes, please feel free to submit a PR directly to rust_generator branch to avoid the overhead in back-and-forth communications.

#6114

After this PR, there are only a handful of issues left.

[farcaller]

in here, if it's an array, it's rendered as e.g. Option<Vec<Tag>>, super:: is missing from the child struct name.

[farcaller]

Same as above, if it's an array, super:: needs to be added to the child struct name.

Also, super:: is generally missing if it's a non-primitive type.

Can we rewrite literally any use of a model into an absolute path? E.g. for model T, it should be referenced as ::models::{{classnameFile}}::{{classname}}. Then we don't need to deal with super anywhere.

[wing328]

Let's go with ::models::{{classnameFile}}::{{classname}} instead.

[farcaller]

Ack. Is there any simple way to substitute that in java code?

[wing328]

Yup, just pushed 7f9ced9. Please take a look when you've time.

[farcaller]

Finished dev [unoptimized + debuginfo] target(s) in 1.19 secs

it finally builds! Not sure if it's functional, though.

[farcaller]

Sorry, I've got a few busy days. Will get to tests later this week.

[wing328]

@farcaller no problem. Please take your time.

[wing328]

@farcaller what about releasing an alpha/beta version so that Rust developers can test it out?

[BenjaminGill-Metaswitch]

Sorry - I'm about to throw a spanner in the works.

We (@Metaswitch) have been working on an implementation of Rust client/server generation in swagger-codegen for nearly a year (for use in a micro-service framework). It works well enough that we're immanently going to be using it in production.

We're currently working on open-sourcing our implementation, which should occur within the next couple of weeks.

I've only had a cursory glance at this implementation - the main difference seems to be that we have a single Api trait that the client uses and the server implements.

[quodlibetor]

Drive-by review because I was excited to see this getting done. Feel free to ignore.

[quodlibetor]

seems a bit off

[wing328]

Yup, it should be replaced by "Rust API client ..."

[quodlibetor]

FWIW I don't see a lot of Impl suffixes in Rust. I believe it would be more idiomatic to make this something like {{{classname}}}Connection or rename the trait to {{{classname}}}Client.

[wing328]

@quodlibetor do you mind submitting a PR with the suggested change (together with the fix to replace "Go" with "Rust")?

[quodlibetor]

Also seems a bit off.

[wing328]

Good catch. I'll do a grep for "Go" before release to make sure we don't have any Go-related code/doc left over in the Rust generator and templates.

[quodlibetor]

Sure thing, I ought to be able to do it tomorrow. Is the standard to regenerate the petshop code as part of rename PRs?

[wing328]

Yes, we usually run the shell script (./bin/) or windows batch file (.\bin\windows) to regenerate the Petstore samples so that CI (travis, circleci, etc) can verify the change (there's no CI setup for Rust Petstore sample yet)

[quodlibetor]

Cool then. I'll put up the PR and you can decide if it fits.

[wing328]

Sorry - I'm about to throw a spanner in the works.

@BenjaminGill-Metaswitch we look forward to the Rust client and server generators by @Metaswitch

Haskell also starts with a client and server generator and now there're suggestions to create another Haskell client generator to meet different requirements.

We'll keep this Rust client generator and later see if we can combine both into one via CLI options.

[quodlibetor]

Created #6247 , a PR against this branch.

[wing328]

Created #6247 , a PR against this branch.

@quodlibetor PR merged into master. Thanks for the contribution.

In your opinion, do you think the Rust generator is ready for alpha/beta release?

[farcaller]

I ran a few tests against the petstore and it worked as expected for me, apart from the whole thing where I forgot to add getters to api

[wing328]

I ran a few tests against the petstore and it worked as expected for me, apart from the whole thing where I forgot to add getters to api

@farcaller Nice. I'll release it later today by merging it into master and create a separate ticket (issue) to track future enhancements.

Thanks for the great work!

[farcaller]

#6249 fixes getters.

[wing328]

Btw, are you guys aware of any official/popular Rust style guide? We'll update https://github.com/swagger-api/swagger-codegen/blob/master/CONTRIBUTING.md#style-guide accordingly.

The ones I found by doing a google search ("rust style guide") do not seem to be popular nor official.

[farcaller]

I believe this is where the official style is being developed: https://github.com/rust-lang-nursery/fmt-rfcs, but it's not finalized yet.

[wing328]

PR merged into master.

Updated README to add @farcaller as the Rust template creator.

[wing328]

Tweet to promote the new generator: https://twitter.com/wing328/status/894127231034720256

Future enhancements: #6250

[BenjaminGill-Metaswitch]

(As a follow on from #6105 (comment) above, we've just released our alternative implementation of Rust codegen - see #5905 (comment))