[Java][Retrofit] fix wrong order of path params

[giuliopulina]

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: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming langauge.

Fixes #6391: retrofit2 is crashing when @query params are defined before @path param in the api.
With this change, params are correctly reordered (@path param are always defined before @query param).

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

@bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet

[wing328]

@giuliopulina May I know why the test cases for findPetsByTags are removed? Is it throwing errors when run locally?

[giuliopulina]

@wing328 Yes, I confirm that mvn integration-test was throwing error.

In fact, if I try to call the service: http:https://petstore.swagger.io/v2/pet/findByTags?tags=friendly

I receive this error:

  <apiResponse>
      <message>something bad happened</message>
      <type>unknown</type>
  </apiResponse>

And also, if I look in swagger UI, I see that this endpoint is deprecated:

[wing328]

The CIs (Travis, CircleCI) are running a local copy of the Petstore server as the public petstore server contains invalid pet data from time to time (since anyone can update the record).

Please add those tests back and the CI tests should pass.

[giuliopulina]

done 👍

[wing328]

@giuliopulina I did some tests locally and the result is good. Thanks for the PR.

[florent37]

thanks a lot for your work :)

[mmallis87]

I have a related question regarding operations path params specifically: does anyone think that we should enforce path params order based on how they appear in on the URL path instead of relying on the order in the parameters array?

For instance when the URL path is "/accounts/{accountId}/users/{userId}" and

"parameters": [
          {
            "name": "userId",
            "in": "path",
            "description": "The external user number (int) or user ID Guid.",
            "required": true,
            "type": "string"
          },
          {
            "name": "accountId",
            "in": "path",
            "description": "The external account number (int) or account ID Guid.",
            "required": true,
            "type": "string"
          }
]

the generated method will have a signature that looks like public void myMethod(String userId, String accountId);. I think it makes more sense to have public void myMethod(String accountId, String userId);. Curious to see what you guys think!

[giuliopulina]

I have a related question regarding operations path params specifically: does anyone think that we should enforce path params order based on how they appear in on the URL path instead of relying on the order in the parameters array?

For instance when the URL path is "/accounts/{accountId}/users/{userId}" and

"parameters": [
          {
            "name": "userId",
            "in": "path",
            "description": "The external user number (int) or user ID Guid.",
            "required": true,
            "type": "string"
          },
          {
            "name": "accountId",
            "in": "path",
            "description": "The external account number (int) or account ID Guid.",
            "required": true,
            "type": "string"
          }
]

the generated method will have a signature that looks like public void myMethod(String userId, String accountId);. I think it makes more sense to have public void myMethod(String accountId, String userId);. Curious to see what you guys think!

Hi @mmallis87 for what my opinion may be worth, I think it can make sense: however, I think this behavior should be aligned between all the different generators, not just for the Retrofit one, so it should be an higher level design decision.

[mmallis87]

@giuliopulina I agree that this type of change should be aligned across all generators since it's not specific to Java or Retrofit.