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

[Go] Parse integer array query parameters. Fix typos #11105

Merged
merged 1 commit into from
Jan 4, 2022

Conversation

aliakseiz
Copy link
Contributor

*Affects the Go-server only.

Limited support of integer array parameters in a query

Adds a code generation to parse query parameter fields of int32 array and int64 array types. Assumes that style: form and explode: false are used as serialization method.

openapi: "3.0.0"
info:
  description: Example API
  version: 1.0.0
  title: "Example API"

servers:
  - url: http://localhost:8080/api/v1
    description: development server

paths:
  /companies:
    get:
      summary: Filter companies by specified criteria
      operationId: find
      parameters:
        - name: ids
          in: query
          example: "500, 501"
          schema:
            type: array
            items:
              type: integer
              format: int64
      tags:
        - Company
      responses:
        200:
          description: Successful operation

Before:

parentIDsParam := strings.Split(query.Get("ids"), ",")

After:

mtinfoIDsParam, err := parseInt64ArrayParameter(query.Get("ids"), ",", false)
if err != nil {
	c.errorHandler(w, r, &ParsingError{Err: err}, nil)
	return
}

Other

Fixes minor typos in comments.

Technical committee

@antihax @grokify @kemokemo @jirikuncar @ph4r5h4d

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@wing328
Copy link
Member

wing328 commented Dec 14, 2021

Thanks for the PR. If I understand this correctly, this PR is more bug fixes so it can target current master unless there're breaking changes (without fallback) that I'm not aware of.

@aliakseiz
Copy link
Contributor Author

aliakseiz commented Dec 14, 2021

Thanks for the PR. If I understand this correctly, this PR is more bug fixes so it can target current master unless there're breaking changes (without fallback) that I'm not aware of.

Thanks for review. Indeed there are no breaking changes, it just extends the existing functionality. But it uses some functions, which are not present in master yet (e.g. parseInt64ArrayParameter from here).

Left it in a draft state (forgot about the [WIP] though), because I have some doubts about serialization: changes suggested in this PR will generate expected result only for explode: false.

According to OpenAPI specification v3.1 the default style for query parameters is form (link) and default explode value is true (link).
But DefaultCodegen.java sets it to false, unless specified explicitly (link).

Looking at the *.mustache templates I noticed that Go implementation doesn't use the isExplode attribute. In fact, very few implementations for other languages do.

Coming back to the point, in case explode will be true, generated output will not work as expected, but it will compile at least. Actually, latest master build produces erroneous code even, when query parameter type is array of integers.

I would appreciate if somebody could approve this PR taking the aforementioned limitation into consideration.

@aliakseiz aliakseiz marked this pull request as ready for review December 17, 2021 14:33
@wing328
Copy link
Member

wing328 commented Dec 29, 2021

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@aliakseiz
Copy link
Contributor Author

@wing328 thanks for noticing.
Now my email is linked to a Github account.

@wing328
Copy link
Member

wing328 commented Jan 4, 2022

CircleCI failure (already fixed in the master) not related to this change.

@wing328
Copy link
Member

wing328 commented Jan 4, 2022

@wing328 wing328 merged commit 9c3d4ef into OpenAPITools:6.0.x Jan 4, 2022
@wing328 wing328 added this to the 5.4.0 milestone Jan 31, 2022
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

2 participants