-
-
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
[General] fix InlineModelResolver's logic and use openapi-generator's InlineModelResolver, so that nested "required" works correctly #1200
Conversation
@@ -500,6 +500,7 @@ public Schema modelFromProperty(ObjectSchema object, String path) { | |||
model.setExample(example); | |||
model.setName(object.getName()); | |||
model.setXml(xml); | |||
model.setRequired(object.getRequired()); |
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.
don't forget copy "required" to the separated object
@@ -538,7 +538,6 @@ public ClientOptInput toClientOptInput() { | |||
final List<AuthorizationValue> authorizationValues = AuthParser.parse(auth); | |||
ParseOptions options = new ParseOptions(); | |||
options.setResolve(true); | |||
options.setFlatten(true); |
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 option makes swagger-parser to disassemble inline models into separated models.
but we don't won't to use swagger-parser's InlindeModelResolver, because we use openapi-generator's one.
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java#L818
9f94cd6
to
64d3654
Compare
@@ -596,6 +596,8 @@ definitions: | |||
title: Pet category | |||
description: A category for a pet | |||
type: object | |||
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.
add nested required pattern to test case
@@ -600,6 +600,8 @@ components: | |||
title: Pet category | |||
description: A category for a pet | |||
type: object | |||
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.
add nested required pattern to test case
bf7db5c
to
940f0d2
Compare
@@ -49,7 +49,8 @@ public Category name(String name) { | |||
* Get name | |||
* @return name | |||
**/ | |||
@ApiModelProperty(value = "") | |||
@ApiModelProperty(required = true, value = "") |
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.
it works!
8244f17
to
cdf5cb1
Compare
cdf5cb1
to
7ff34c9
Compare
@wing328 @jimschubert @cbornet @jaz-ah @ackintosh @JFCote @jmini |
01cbf7e
to
0ef509a
Compare
@@ -545,6 +546,9 @@ public Schema makeSchema(String ref, Schema property) { | |||
|
|||
public void copyVendorExtensions(Schema source, Schema target) { | |||
Map<String, Object> vendorExtensions = source.getExtensions(); | |||
if (vendorExtensions == null) { |
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.
sometimes, vendorExtensions is still null...
add null check.
@@ -18,6 +18,18 @@ tags: | |||
- name: user | |||
description: Operations about user | |||
paths: | |||
/foo: |
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 pattern causes NPE at https://github.com/OpenAPITools/openapi-generator/pull/1200/files#r224685854
@fujigon thanks for the PR. Sure, I'll review and let you know if I've any question. |
812b2dc
to
973e398
Compare
@fujigon thanks again for the PR. For the additional test cases, please add to https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml instead of petstore.yaml (as we want to keep petstore.yaml in sync with the one used by the Petstore server) |
7423381
to
c255c7f
Compare
…elResolver, so that nested "required" works correctly
…-with-fake-endpoints-models-for-testing.yaml
74b69cc
to
4cbf907
Compare
4cbf907
to
7af2cd7
Compare
@wing328 Hi! I reflected your review point, thanks :) |
I've reviewed it twice (today and over the weekend) and the change looks good to me. It's a high-quality PR 👍 |
@fujigon thanks for the PR, which is included in the v3.3.2 release: https://twitter.com/oas_generator/status/1057649626101112832 |
… InlineModelResolver, so that nested "required" works correctly (OpenAPITools#1200) * fix InlineModelResolver's logis and use openapi-generator's InlineModelResolver, so that nested "required" works correctly * add "required" to nested model schema * update ensure-up-to-date to include openapi v3's jaxrs * change test required field * fix sample shell script, hide timestamp * fix NPE * move test case to petstore-with-fake-endpoints-models-for-testing.yaml * fix jaxrs-jersey (oas3) example generate shell script to use petstore-with-fake-endpoints-models-for-testing.yaml * add default value * re-generate samples
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\
.master
,3.4.x
,4.0.x
. Default:master
.Description of the PR
This patch fixes #1201