Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust client code generation is unhygenic #512

Closed
euank opened this issue Jul 9, 2018 · 2 comments
Closed

Rust client code generation is unhygenic #512

euank opened this issue Jul 9, 2018 · 2 comments

Comments

@euank
Copy link
Contributor

euank commented Jul 9, 2018

Description

The rust client's code generation can have variable name conflicts with function's definition and the generated parameter names. For example, if a parameter to a function using oauth2 auth is called "token", the below code will override its value:

{{#isOAuth}}
if let Some(ref token) = configuration.oauth_access_token {
let auth = hyper::header::Authorization(

Suggest a fix/enhancement

There are a couple possible options here.

One option would be to reserve all variable names used in the body of such functions and ave the java code remap the parameter names.

Another could be to prefix all those variables with something like "openapi_local" and assume no one will name parameters starting with that string.

We could also rebind all the parameters immediately to known names which won't conflict, and then allow shadowing throughout the rest of the function. This is also fairly easy, and I think might be the best short term solution.

In the long term, I think this can be improved by having a helper non-generated method to make the request, and then have each generated method simply be a call to that helper with the correct parameters, allowing the generated method's scope to not polute the main logic of handling a request.

@wing328
Copy link
Member

wing328 commented Jul 9, 2018

Another could be to prefix all those variables with something like "openapi_local" and assume no one will name parameters starting with that string.

We use this approach for other generators (e.g. C#, Ruby client generators) by prefixing "local_var_" or "localVar", e.g.

C#: https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/csharp/OpenAPIClientNetStandard/src/Org.OpenAPITools/Api/PetApi.cs#L508

Ruby: https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/ruby/lib/petstore/api/pet_api.rb#L44

For Rust, I suggest we follow the similar approach to avoid conflicts with variable naming.

May I know if you have time to contribute the enhancement? I can show you some good starting points if you want.

@euank
Copy link
Contributor Author

euank commented Jul 9, 2018

I've started working on writing a generic helper that is called by the generated methods since I think that's an aesthetically nicer solution. I do expect I'll be able to get it working and contribute it, but if I can't I can easily enough prefix all the local variables and PR that instead.

euank added a commit to euank/openapi-generator that referenced this issue Jul 23, 2018
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.
wing328 pushed a commit that referenced this issue Jul 23, 2018
* [Rust] Move request logic into standalone file

This reduces the number of variables which are used in the generated
operations, thus fixing #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
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this issue Feb 27, 2019
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants