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

[all] Fix inconsistent parameter names when ensuring uniqueness. #640

Merged
merged 2 commits into from
Jul 25, 2018

Conversation

Jauler
Copy link
Contributor

@Jauler Jauler commented Jul 24, 2018

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

If parameter renaming takes place while ensuring unique names in DefaultCodegen.java fromOperation method, parameter lists queryParams, pathParams, headerParams, cookieParams are not updated accordingly which causes wrong parameters to be selected when using {{#pathParams}}, {{#queryParams}} etc. in mustache templates.

@jmini
Copy link
Member

jmini commented Jul 25, 2018

Thank you a lot for this PR.

Can you provide a small test case or example spec to reproduce the issue?

@Jauler
Copy link
Contributor Author

Jauler commented Jul 25, 2018

I was using spec from https://github.com/kubernetes/kubernetes/blob/release-1.10/api/openapi-spec/swagger.json which is rather big...

The important part is:

......
    "parameters": [
     {
      "uniqueItems": true,
      "type": "string",
      "description": "path to the resource",
      "name": "path",
      "in": "path",
      "required": true
     },
     {
      "uniqueItems": true,
      "type": "string",
      "description": "Path is the URL path to use for the current proxy request to pod.",
      "name": "path",
      "in": "query"
     }
    ]
......

Notice parameter named "path" in both, query params and in path params, which caused the generation of new name for allParams list which was not propogated to pathParams and queryParams lists.

P.S. If You believe that this is not enough to demonstrate the issue, later I could try to prepare a small test case for that.

@jmini
Copy link
Member

jmini commented Jul 25, 2018

Thank you for the explanations.
I just added a unit test for your fix 34cf828.
If the CI is green, I will merge it...


That said, the rename logic has a lot of bugs... For example with 3 params named myparam, they get renamed myparam, myparam2 and myparam2...
This can be solved in a follow up PR.

@jmini jmini merged commit 44d419c into OpenAPITools:master Jul 25, 2018
@jmini jmini added this to the 3.2.0 milestone Jul 25, 2018
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants