-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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-server] Add ability to handle nullable query param #17051
Conversation
# Conflicts: # samples/openapi3/server/petstore/go/go-petstore/.openapi-generator/VERSION # samples/server/petstore/go-api-server/.openapi-generator/VERSION # samples/server/petstore/go-chi-server/.openapi-generator/VERSION
@@ -23,7 +23,7 @@ func New{{classname}}Service() {{classname}}Servicer { | |||
{{#isDeprecated}} | |||
// Deprecated | |||
{{/isDeprecated}} | |||
func (s *{{classname}}Service) {{nickname}}(ctx context.Context{{#allParams}}, {{paramName}} {{dataType}}{{/allParams}}) (ImplResponse, error) { | |||
func (s *{{classname}}Service) {{nickname}}(ctx context.Context{{#allParams}}, {{paramName}} {{#isNullable}}*{{/isNullable}}{{dataType}}{{/allParams}}) (ImplResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think isNullable
is behaving as you expect it to here. The Pet Store Specs have many non-required params that would show this change but none of them are showing up in this diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you provide a specific example? are they marked as nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One specific example from openapi-generator/samples/server/petstore/go-api-server/api/openapi.yaml
is the booleanTest
query parameter in deleteUser
.
- description: boolean query parameter
explode: true
in: query
name: boolean_test
required: false
schema:
type: boolean
style: form
This should result in the the function signature for deleteUser
having a *bool
parameter for the function signature in openapi-generator/samples/server/petstore/go-api-server/go/api_user_service.go
There are numerous query parameters in that example file that have optional query parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object is not nullable, please see https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#schema-object
modules/openapi-generator/src/main/resources/go-server/controller-api.mustache
Show resolved
Hide resolved
To fix other import issues with os workaround in the future. ref: #11097 |
modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/GoServerCodegen.java
Show resolved
Hide resolved
modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/GoServerCodegen.java
Outdated
Show resolved
Hide resolved
@@ -23,7 +23,7 @@ func New{{classname}}Service() {{classname}}Servicer { | |||
{{#isDeprecated}} | |||
// Deprecated | |||
{{/isDeprecated}} | |||
func (s *{{classname}}Service) {{nickname}}(ctx context.Context{{#allParams}}, {{paramName}} {{dataType}}{{/allParams}}) (ImplResponse, error) { | |||
func (s *{{classname}}Service) {{nickname}}(ctx context.Context{{#allParams}}, {{paramName}} {{#isNullable}}*{{/isNullable}}{{dataType}}{{/allParams}}) (ImplResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One specific example from openapi-generator/samples/server/petstore/go-api-server/api/openapi.yaml
is the booleanTest
query parameter in deleteUser
.
- description: boolean query parameter
explode: true
in: query
name: boolean_test
required: false
schema:
type: boolean
style: form
This should result in the the function signature for deleteUser
having a *bool
parameter for the function signature in openapi-generator/samples/server/petstore/go-api-server/go/api_user_service.go
There are numerous query parameters in that example file that have optional query parameters.
Please be patient, I'm still fixing test issues. I'll request for a review via the "re-request review" button |
I'm still researching and reading about how What is currently giving me hesitation is comparing this implementation to how the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examples should be added/edited to cover the new variations of nullable bools, nullable enums, nullable numbers, etc.
} else { | ||
{{#required}} | ||
c.errorHandler(w, r, &RequiredError{Field: "{{baseName}}"}, nil) | ||
return | ||
{{/required}} | ||
{{^required}} | ||
{{#defaultValue}} | ||
param := {{^isString}}{{dataType}}({{/isString}}{{defaultValue}}{{^isString}}){{/isString}} | ||
{{paramName}}Param = {{#isNullable}}&{{/isNullable}}param | ||
{{/defaultValue}} | ||
{{/required}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of having empty else
clauses in code. If a parameter isn't required and doesn't have a default value, this case will create an empty else
clause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that you are not a fan of empty else block, but is purely cosmetic and does not affect the compiled code and logic. Feel free to suggest fixes or create a PR to update it
{ | ||
param, err := ReadFormFileToTempFile(r, "file") | ||
if err != nil { | ||
c.errorHandler(w, r, &ParsingError{Err: err}, nil) | ||
return | ||
} | ||
|
||
fileParam = param | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code is odd as it is using {
to just scope a variable. I have not been of fan of these as they are not common practice and are typically used in a "clever" way to solve a problem.
I know it's a supported feature of the Go language, but it is typically abused in my experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this is uncommon, but this is purely cosmetic and does not affect the semantics of the code.
This will need to be fixed with the larger problem as stated above #17051 (comment)
* fix null request body NPE * fix typo
…ols#17057) * Remove checks for jackson and jsonb from okhttp-gson templates * regenerate samples
* retain deprecated in allof schema handling * add test
* added handling for boolean content type * adapted addition to newer version of template * [python] updated samples
* Update base image of Dockerfile * Make github workflow "Docker tests" manually triggerable * Don't possibly downgrade TLS version * Update Dockerimage in ``run-in-docker.sh`` * Use Maven's non interactive mode inside workflow * Don't spam log * Use java 17 because it won't compile with 21 * Removed hard memory limit as memory should be controlled by the container * Update hub dockerfiles
…Tools#16918) * add socks5 proxy support (requires additional import) * updated examples * build samples. updated to support pydantic python option * rename sock to socks for correct protocol name * add proxy headers for pydantic * fixed param changes from conflict resolution
For model classes with model_something fields, pydantic raises a warning by default: `Field "model_something" has conflict with protected namespace "model_".`. These warnings make no sense here, because most users of the generator have established APIs that they cannot change to conform to pydantic's safety rules. Pydantic will raise an error if we ever conflict with a current attribute like `model_dump`.
* [typescript-axios] Upgrade to axios@^1 * Try fixing tests
* added .net8 * change .net version in the github action * upgrade manual sample
…APITools#17077) * Add passgenau digital logo * Add passgenau digital to users * Add wemakeai logo * Add we-make.ai to users
* refactor: Clean up _response_types_map formatting It matches black's behavior of having trailing commas now. * test: Add test to reproduce OpenAPITools#16967 * fix: deserialize responses even if no returnType Closes OpenAPITools#16967 * refactor: Simplify ApiException subclasses * refactor: Move exception subtype choice to ApiException * feat: Deserialize error responses and add to exceptions * test: Add for error responses with model
* add bearer auth API to echo-api * run generate-samples.sh * add resttemplate echo-api sample * add bearer auth test * remove @ignore
* use jdk17 images in Dockerfile * use amazoncorretto:17.0.8-alpine3.18
Moving to another PR due to bad merge #17321 |
Add the ability to handle nullable query param
nullable query params will now be passed as a pointer to the servicer.
non-nullable query params will still be passed as value.
There is no breaking changes in this PR
PR checklist
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*
.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master
(upcoming 7.1.0 minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)