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

Kotlin support of Square Bracket on variable name causing an issue with query parameters' kotlin variable names with square brackets as syntax error #31

Closed
yudhir opened this issue Jun 1, 2019 · 8 comments · Fixed by #32
Labels
bug Something isn't working

Comments

@yudhir
Copy link

yudhir commented Jun 1, 2019

@JvmSuppressWildcards
interface JobPostingApi {
  

  /** 
   * Retrieves the collection of JobPosting resources.
   * The endpoint is owned by defaultname service owner
   * @param datePosted[before]  (optional)
   * @param datePosted[strictlyBefore]  (optional)
   * @param datePosted[after]  (optional)
   * @param datePosted[strictlyAfter]  (optional)
   * @param page The collection page number (optional)
   * @param itemsPerPage The number of items per page (optional)
   */
  @Headers(
    "X-Operation-ID: getJobPostingCollection"
  )
  @GET("/api/job_postings")
  fun getJobPostingCollection(
    @retrofit2.http.Query("datePosted[before]") datePosted[before]: String?, <===ERROR
    @retrofit2.http.Query("datePosted[strictly_before]") datePosted[strictlyBefore]: String?, <===ERROR
    @retrofit2.http.Query("datePosted[after]") datePosted[after]: String?,<===ERROR
    @retrofit2.http.Query("datePosted[strictly_after]") datePosted[strictlyAfter]: String?,<===ERROR
    @retrofit2.http.Query("page") page: Int?,<===CORRECT
    @retrofit2.http.Query("itemsPerPage") itemsPerPage: Int?
  ): Single<List<JobPosting>>

A type annotation is required on the value parameter.

Another Example

/** 
   * Retrieves the collection of City resources.
   * The endpoint is owned by defaultname service owner
   * @param id  (optional)
   * @param id[]  (optional)
   * @param name  (optional)
   * @param name[]  (optional)
   * @param page The collection page number (optional)
   * @param itemsPerPage The number of items per page (optional)
   */
  @Headers(
    "X-Operation-ID: getCityCollection"
  )
  @GET("/api/cities")
  fun getCityCollection(
    @retrofit2.http.Query("id") id: Int?,
    @retrofit2.http.Query("id[]") id[]: List<Int>?,
    @retrofit2.http.Query("name") name: String?,
    @retrofit2.http.Query("name[]") name[]: List<String>?,
    @retrofit2.http.Query("page") page: Int?,
    @retrofit2.http.Query("itemsPerPage") itemsPerPage: Int?
  ): Single<List<City>>
@cortinico
Copy link
Collaborator

@yudhir Thanks for the report. Can you also post the spec that you used to generate the code you posted?

@yudhir
Copy link
Author

yudhir commented Jun 1, 2019

"\/api\/job_postings": {
      "get": {
        "tags": [
          "JobPosting"
        ],
        "operationId": "getJobPostingCollection",
        "produces": [
          "application\/ld+json",
          "application\/json",
          "text\/html",
          "application\/hal+json",
          "multipart\/form-data",
          "application\/vnd.api+json"
        ],
        "summary": "Retrieves the collection of JobPosting resources.",
        "responses": {
          "200": {
            "description": "JobPosting collection response",
            "schema": {
              "type": "array",
              "items": {
                "$ref": "#\/definitions\/JobPosting"
              }
            }
          }
        },
        "parameters": [
          {
            "name": "datePosted[before]",
            "in": "query",
            "required": false,
            "type": "string"
          },
          {
            "name": "datePosted[strictly_before]",
            "in": "query",
            "required": false,
            "type": "string"
          },
          {
            "name": "datePosted[after]",
            "in": "query",
            "required": false,
            "type": "string"
          },
          {
            "name": "datePosted[strictly_after]",
            "in": "query",
            "required": false,
            "type": "string"
          },
          {
            "name": "page",
            "in": "query",
            "required": false,
            "description": "The collection page number",
            "type": "integer"
          },
          {
            "name": "itemsPerPage",
            "in": "query",
            "required": false,
            "description": "The number of items per page",
            "type": "integer"
          }
        ]
      },

Basically, this parameter with datePosted[before], square brackets, the variable names could use dateposted_before_ and so on .

No other errors.

@cortinico
Copy link
Collaborator

Yup we could do that. The problem I envision is that those two properties will then clash:

          {
            "name": "datePosted[after]",
            "in": "query",
            "required": false,
            "type": "string"
          },
          {
            "name": "datePosted_after_",
            "in": "query",
            "required": false,
            "type": "string"
          },

The underscore approach sounds reasonable, although the parameter name won't look great :/
Maybe @macisamuele has some ideas on how to solve this?

@yudhir
Copy link
Author

yudhir commented Jun 1, 2019

It generates

data class CommentMinusnewsarticle (
        @Json(name = "commentbody") @field:Json(name = "commentbody") var commentbody: String? = null
)

for category-newsarticle to solve a similar problem with "-" as variable name

@macisamuele
Copy link
Collaborator

@cortinico : I think that we might tweak https://github.com/Yelp/swagger-gradle-codegen/blob/master/plugin/src/main/java/com/yelp/codegen/utils/KotlinLangUtils.kt#L160 or https://github.com/Yelp/swagger-gradle-codegen/blob/master/plugin/src/main/java/com/yelp/codegen/KotlinGenerator.kt#L147 to achieve the desired objective.

NOTE: Modifying the sanitisation method would solve the edce case presented on this issue but it might open-up ways to have parameter name clashing (ie. id[] and id might end up on having the same "kotlin" name)

@yudhir : In the issue description you listed you cases where the generated code "mis-behave". Could you please post the specs of the second example as well? My feeling is that in the second case the specs are weird as you could have multiple name parameters via arrays specs with multi collection format.

@yudhir
Copy link
Author

yudhir commented Jun 2, 2019

"\/api\/cities": {
      "get": {
        "tags": [
          "City"
        ],
        "operationId": "getCityCollection",
        "produces": [
          "application\/ld+json",
          "application\/json",
          "text\/html",
          "application\/hal+json",
          "multipart\/form-data",
          "application\/vnd.api+json"
        ],
        "summary": "Retrieves the collection of City resources.",
        "responses": {
          "200": {
            "description": "City collection response",
            "schema": {
              "type": "array",
              "items": {
                "$ref": "#\/definitions\/City"
              }
            }
          }
        },
        "parameters": [
          {
            "name": "id",
            "in": "query",
            "required": false,
            "type": "integer"
          },
          {
            "name": "id[]",
            "in": "query",
            "required": false,
            "type": "array",
            "items": {
              "type": "integer"
            },
            "collectionFormat": "multi"
          },
          {
            "name": "name",
            "in": "query",
            "required": false,
            "type": "string"
          },
          {
            "name": "name[]",
            "in": "query",
            "required": false,
            "type": "array",
            "items": {
              "type": "string"
            },
            "collectionFormat": "multi"
          },
          {
            "name": "page",
            "in": "query",
            "required": false,
            "description": "The collection page number",
            "type": "integer"
          },
          {
            "name": "itemsPerPage",
            "in": "query",
            "required": false,
            "description": "The number of items per page",
            "type": "integer"
          }
        ]
      },

@macisamuele
Copy link
Collaborator

@yudhir as mentioned in the comment above we might have name clashing for parameter names like id and id[] as they would be sanitised to the same "kotlin" name.

By checking the last snippet of the specs I see an anti-pattern with the id or name parameters.
You might simplify the specs by using something like

...
paths:
  /api/cities:
    get:
      parameters:
      - name: id
        in: query
        required: false
        type: array
        items:
          type: integer
        collectionFormat: multi
...

The definition of the id parameter in this case would allow the following requests to be properly validated:

  • curl -X GET <web_host>/api/cities
  • curl -X GET <web_host>/api/cities?id=1
  • curl -X GET <web_host>/api/cities?id=1&id=2

Defining id as single id and id[] as the list of the ids would (and this is a personal opinion) make the HTTP interface harder to be understood and should give you not real advantage.

In the next days we, me or @cortinico, might be trying to put some effort on the sanitisation process. If you bandwidth and you could publish a PR doing that it would be really appreciated.

@yudhir
Copy link
Author

yudhir commented Jun 2, 2019

Names with square brackets and other reserved keywords must be read.

Swaggergencli does this

/**
   * Retrieves the collection of City resources.
   * 

   * @param id  (optional)

   * @param id2  (optional)

   * @param name  (optional)

   * @param name2  (optional)

   * @param page The collection page number (optional)

   * @param itemsPerPage The number of items per page (optional)

   * @return Call&lt;List&lt;City&gt;&gt;

   */
  
  
  
    
  @GET("api/cities")
  Observable<List<City>> getCityCollection(
    @retrofit2.http.Path("id") Integer id, @retrofit2.http.Path("id[]") List<Integer> id2, @retrofit2.http.Path("name") String name, @retrofit2.http.Path("name[]") List<String> name2, @retrofit2.http.Path("page") Integer page, @retrofit2.http.Path("itemsPerPage") Integer itemsPerPage
  );
``

cortinico added a commit that referenced this issue Jun 4, 2019
The generator is broken for operations with parameters that have square
brackets in the name. Apparently the version of Swagger Codegen that
we're using is not excluding `[` and `]` when doing the camelization of
the parameter name.

Fixes #31
@cortinico cortinico added the bug Something isn't working label Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants