-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
[BUG] [Groovy] Fix client to work with services that support other formats in addition to JSON #22908
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
[BUG] [Groovy] Fix client to work with services that support other formats in addition to JSON #22908
Conversation
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.
No issues found across 10 files
| if (bodyParams != null) { | ||
| request.body = bodyParams | ||
| } | ||
| request.accept = ['application/json'] |
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.
thanks for the PR
according to the description. the change should support other formats in addition to JSON but looks like it's now hard-code to application/json.
should it use the accept provided in the spec instead?
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.
Thank you so much for reviewing @wing328.
according to the description. the change should support other formats in addition to JSON but looks like it's now hard-code to application/json.
My intention was to force the server to respond in JSON format by specifying that header in the request, because all of the servers listed in the official document at least support JSON.
But yes, it is clear that adding new format support is better as you mentioned, since there can be a service only supporting XML in the wild. Let me give it a try.
should it use the accept provided in the spec instead?
Agreed, I'll fix it accordingly.
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.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="modules/openapi-generator/src/main/resources/Groovy/modelEnum.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/Groovy/modelEnum.mustache:12">
P2: @XmlEnumValue expects a String literal, but boolean enums render @XmlEnumValue(true) without quotes, which will not compile in XML-enabled generation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="samples/client/petstore/groovy/src/main/groovy/org/openapitools/api/ApiUtils.groovy">
<violation number="1" location="samples/client/petstore/groovy/src/main/groovy/org/openapitools/api/ApiUtils.groovy:75">
P2: Recursive array parsing still calls the old 3-argument parse signature, which will throw MissingMethodException after the method signature was expanded to include `response`. Array responses will fail to deserialize.</violation>
</file>
<file name="modules/openapi-generator/src/main/resources/Groovy/ApiUtils.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/Groovy/ApiUtils.mustache:101">
P2: XML parsing short-circuits array handling; XML responses for array operations will return a single instance instead of a collection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
samples/client/petstore/groovy/src/main/groovy/org/openapitools/api/ApiUtils.groovy
Outdated
Show resolved
Hide resolved
58a9620 to
9bd583a
Compare
|
I'll try to test locally tomorrow and merge if no question/feedback from anyone. Thanks again for the contribution. |
|
Thanks a lot for reviewing and checking @wing328! Build the Groovy generator and its sample, then against https://petstore.swagger.io/ to ensure prioritizing "application/json" to "application/xml": Regenerate the client enabling the XML feature and ensure it works with the XML was surely sent during the transaction above, as follows: |
|
@wing328 Thanks! I'll rebase it shortly. |
|
Yes. Please adopt the tests accordingly. |
…rmats in addition to JSON
9bd583a to
c3fa247
Compare
|
I've just pushed a rebased and squashed commit. |
|
all tests passed let's give it a try thanks for the contribution |
This PR fixes the Groovy client to add the Accept header since it only supports JSON wire format as of now.
It also includes some fixes for the sample tests, because the current client implementation doesn't seem to deal with the enum conversion and the redirect from http to https correctly.
You can ensure its behavior as follows:
Build openapi-generator and generate the Groovy sample with JDK 11+:
Run the sample tests against the generated client with JDK 8:
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(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)fixes [BUG] [Groovy] Generated client can't work with the openapi-petstore service by default #22907
No one is in the Groovy technical committee as of now, so let me ping @wing328
Summary by cubic
Set the Accept header per operation using each operation’s produces list (preferring application/json), and add JAXB-based XML request/response support so Groovy clients work with servers that offer JSON or XML. Fixes #22907.
New Features
Bug Fixes
Written for commit c3fa247. Summary will update on new commits.