Swagger-codegen: [Android] Retrofit CodeGen : @Path must be before @Query

[florent37]

Description

With SwaggerCodeGen 2.2.2 (.jar) we have an issue with retrofit,
a query has @Path and @Query params, and when it's generated, some @Query are before the @Path one. But Retrofit crash in this case

sample :

@GET("/users/{name}/images/")
Call<User> getUserImage(@Path("name") String name, @Query("page") int page)

will works

but

@GET("/users/{name}/images/")
Call<User> getUserImage(@Query("page") int page, @Path("name") String name)

will crash

Swagger-codegen version

SwaggerCodeGen 2.2.2 (.jar)

Command line used for generation

generate -i MY_FILE -o OUTPUT_DIR -l java --library=retrofit2 -c specs/myConfigfile.json

[wing328]

@florent37 thanks for reporting the issue. Can you try the latest mater or 2.2.3 (latest stable release) to see if it's still an issue?

Snapshot version of the latest master can be found in https://github.com/swagger-api/swagger-codegen#compatibility

[florent37]

same issue,

@Headers({
    "Content-Type:application/json" 
  })
  @PUT("dossiers_epargne_patrimoniale/{idDossier}/projets/{numeroProjet}")
  Call<RetourMiseAJourProjetEntretien> mettreAJourProjet(
    @retrofit2.http.Path("idDossier") String idDossier, @retrofit2.http.Path("numeroProjet") String numeroProjet, @retrofit2.http.Body Projet body, @retrofit2.http.Query("etape_interruption") String etapeInterruption
  );

with the descriptor :

"/dossiers_epargne_patrimoniale/{idDossier}/projets": {
            "put": {
                "tags": ["DossierEpargnePatrimoniale"],
                "summary": "opération permettant de mettre à jour (remplace) l'ensemble des projets d'un entretien",
                "description": "mettre à jour une liste de projets + indique l'étape d'interruption à insérer dans le dossier de vente",
                "operationId": "mettreAJourListeProjets",
                "consumes": ["application/json"],
                "produces": ["application/json"],
                "parameters": [{
                    "name": "idDossier",
                    "in": "path",
                    "description": "id dossier de vente",
                    "required": true,
                    "type": "string"
                }, {
                    "in": "body",
                    "name": "body",
                    "description": "Contient les données nécessaires à la mise à jour des projets d'un Dossier Entretien Patrimonial",
                    "required": true,
                    "schema": {
                        "type": "array",
                        "items": {
                            "$ref": "#/definitions/Projet"
                        }
                    }
                }, {
                    "name": "etape_interruption",
                    "in": "query",
                    "description": "code de l'étape d'interruption",
                    "required": true,
                    "type": "string"
                }],
                "responses": {
                    "200": {
                        "description": "succès",
                        "schema": {
                            "$ref": "#/definitions/RetourMiseAJourProjetEntretien"
                        }
                    },
                    "403": {
                        "description": "erreur fonctionnelle"
                    },
                    "500": {
                        "description": "erreur technique"
                    }
                }
            }
        },

[wing328]

Can you please share the spec? What about reordering the parameters in the spec to have path parameters first if that's not the current order?

[florent37]

:/ sorry my client does not allow me to share the entiere spec

[wing328]

@florent37 then please download the spec to your local drive and rearrange the parameters by putting path parameters first to see if that works around the issue.

[florent37]

we don't want to change the order manually because it's a 3rd party provider that generates it with the website, and put it in a server

[florent37]

the spec change very week, and it's a jenkins job that download it, then regenerates the client,
so change it locally may be difficult and could create error

[florent37]

when you process the "parameters" map, can you order the fields if the client is retrofit ?

[wing328]

Yes, we can do it in the postProcessOperations (https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/JavaClientCodegen.java#L302 shows how we did it for Java feign API client)

Would you have time to contribute the enhancement?

[florent37]

I do not have time for now, sorry

[wing328]

@florent37 that's ok. I've marked it as "need community contribution" to see if anyone from the community has cycle to contribute the enhancement.

For the time being, please use other Java API clients (e.g. feign, okhttp-gson, etc), which should not have similar issue.

[giuliopulina]

@florent37 I can help, but I would need more information:

1) which are the rules for parameter declaration order in Retrofit? Is there anything else except @Query and @Path param rule you mentioned? I couldn't find documentation about this
2) Could you please confirm that this is happening only when some query parameters are marked as required?

[wing328]

@giuliopulina thanks for offering help on this.

1. I believe @path parameters must go first

2. My understanding is that the issue has to do with order of the parameters, not related to "required" or "optional".

cc @florent37

[giuliopulina]

@wing328

1) I was wondering if there are any other rules (for retrofit annotations like @Body, @Field, @Header..), in order to not implement a partial fix just for this particular use case, but to implement the correct ordering for all cases
2) I was asking because I noticed that, in this section of code, params are reordered (if 'sortParamsByRequiredFlag' is enabled): as Path params are mandatory by default, if query params are optional everything should work (but that doesn't mean that we shouldn't fix the issue) :P

https://github.com/swagger-api/swagger-codegen/blob/c66a0aaa07695276fc3fa6a24bb42d2176d0f5cc/modules/swagger-codegen/src/main/java/io/swagger/codegen/DefaultCodegen.java#L2246-L2253

[wing328]

1. Yup, very good question and I agree we better fix everything related to the order of the parameters now instead of waiting for another users to report an issue to us.

2. Right. The default is to put all the required parameters (including path parameters) in front.

[giuliopulina]

so it seems that the only two conditions related to ordering of parameters are the following ones:

https://github.com/square/retrofit/blob/522592bb11fc8aa1ee491c3e7a1fe55675c372cd/retrofit/src/main/java/retrofit2/ServiceMethod.java#L366-L368

https://github.com/square/retrofit/blob/522592bb11fc8aa1ee491c3e7a1fe55675c372cd/retrofit/src/main/java/retrofit2/ServiceMethod.java#L386-L388

But as codegen doesn't use @Url annotation, it is safe to just reorder the parameter to put Path ones before Query ones.

I will do a PR in the next days.

[wing328]

@florent37 the fix by @giuliopulina has been merged into master.

I wonder if you can give it a try by pulling the latest master or using the SNAPSHOT version of 2.3.0 as mentioned in the project's README

[florent37]

thanks a lot :+1: