Skip to content

[General] fix InlineModelResolver's logic and use openapi-generator's InlineModelResolver, so that nested "required" works correctly#1200

Merged
wing328 merged 10 commits intoOpenAPITools:masterfrom
fuj1g0n:feature/use-openapi's-inline-model-resolver
Oct 23, 2018
Merged

[General] fix InlineModelResolver's logic and use openapi-generator's InlineModelResolver, so that nested "required" works correctly#1200
wing328 merged 10 commits intoOpenAPITools:masterfrom
fuj1g0n:feature/use-openapi's-inline-model-resolver

Conversation

@fuj1g0n
Copy link
Copy Markdown
Contributor

@fuj1g0n fuj1g0n commented Oct 9, 2018 •
edited
Loading

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./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\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This patch fixes #1201

@fuj1g0n fuj1g0n changed the title [WIP] fix InlineModelResolver's logis and use openapi-generator's InlineModelResolver, so that nested "required" works correctly [WIP] fix InlineModelResolver's logic and use openapi-generator's InlineModelResolver, so that nested "required" works correctly Oct 9, 2018
fuj1g0n
fuj1g0n commented Oct 9, 2018
View reviewed changes
Comment thread modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java Outdated
Copy link
Copy Markdown
Contributor Author

@fuj1g0n fuj1g0n Oct 9, 2018

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

fuj1g0n
fuj1g0n commented Oct 9, 2018
View reviewed changes
Comment thread ...les/openapi-generator/src/main/java/org/openapitools/codegen/config/CodegenConfigurator.java Outdated
Copy link
Copy Markdown
Contributor Author

@fuj1g0n fuj1g0n Oct 9, 2018

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

@fuj1g0n fuj1g0n force-pushed the feature/use-openapi's-inline-model-resolver branch from 9f94cd6 to 64d3654 Compare October 9, 2018 04:45
fuj1g0n
fuj1g0n commented Oct 9, 2018
View reviewed changes
Comment thread modules/openapi-generator/src/test/resources/2_0/petstore.yaml Outdated
Copy link
Copy Markdown
Contributor Author

@fuj1g0n fuj1g0n Oct 9, 2018

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

fuj1g0n
fuj1g0n commented Oct 9, 2018
View reviewed changes
Comment thread modules/openapi-generator/src/test/resources/3_0/petstore.yaml Outdated
Copy link
Copy Markdown
Contributor Author

@fuj1g0n fuj1g0n Oct 9, 2018

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

@fuj1g0n fuj1g0n force-pushed the feature/use-openapi's-inline-model-resolver branch 2 times, most recently from bf7db5c to 940f0d2 Compare October 9, 2018 06:32
fuj1g0n
fuj1g0n commented Oct 9, 2018
View reviewed changes
Comment thread samples/client/petstore/spring-cloud/src/main/java/org/openapitools/model/Category.java Outdated
Copy link
Copy Markdown
Contributor Author

@fuj1g0n fuj1g0n Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it works!

@fuj1g0n fuj1g0n force-pushed the feature/use-openapi's-inline-model-resolver branch 2 times, most recently from 8244f17 to cdf5cb1 Compare October 9, 2018 07:18
@fuj1g0n fuj1g0n mentioned this pull request Oct 9, 2018
Closed
@fuj1g0n fuj1g0n force-pushed the feature/use-openapi's-inline-model-resolver branch from cdf5cb1 to 7ff34c9 Compare October 9, 2018 07:48
@fuj1g0n fuj1g0n changed the title [WIP] fix InlineModelResolver's logic and use openapi-generator's InlineModelResolver, so that nested "required" works correctly fix InlineModelResolver's logic and use openapi-generator's InlineModelResolver, so that nested "required" works correctly Oct 9, 2018
@fuj1g0n
Copy link
Copy Markdown
Contributor Author

fuj1g0n commented Oct 9, 2018

@wing328 @jimschubert @cbornet @jaz-ah @ackintosh @JFCote @jmini

@fuj1g0n fuj1g0n changed the title fix InlineModelResolver's logic and use openapi-generator's InlineModelResolver, so that nested "required" works correctly [General] fix InlineModelResolver's logic and use openapi-generator's InlineModelResolver, so that nested "required" works correctly Oct 9, 2018
@fuj1g0n
Copy link
Copy Markdown
Contributor Author

fuj1g0n commented Oct 12, 2018

@wing328 @jimschubert @cbornet @jaz-ah @ackintosh @JFCote @jmini
Could you kindly review this PR, please?
Thanks!

@fuj1g0n fuj1g0n force-pushed the feature/use-openapi's-inline-model-resolver branch from 01cbf7e to 0ef509a Compare October 12, 2018 06:49
fuj1g0n
fuj1g0n commented Oct 12, 2018
View reviewed changes
Comment thread modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java Outdated
Copy link
Copy Markdown
Contributor Author

@fuj1g0n fuj1g0n Oct 12, 2018

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.

fuj1g0n
fuj1g0n commented Oct 12, 2018
View reviewed changes
Comment thread modules/openapi-generator/src/test/resources/3_0/petstore.yaml Outdated
Copy link
Copy Markdown
Contributor Author

@fuj1g0n fuj1g0n Oct 12, 2018

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

@wing328
Copy link
Copy Markdown
Member

wing328 commented Oct 12, 2018

@fujigon thanks for the PR. Sure, I'll review and let you know if I've any question.

@fuj1g0n fuj1g0n force-pushed the feature/use-openapi's-inline-model-resolver branch 2 times, most recently from 812b2dc to 973e398 Compare October 15, 2018 04:57
@wing328
Copy link
Copy Markdown
Member

wing328 commented Oct 15, 2018

@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)

@fuj1g0n fuj1g0n force-pushed the feature/use-openapi's-inline-model-resolver branch 3 times, most recently from 7423381 to c255c7f Compare October 15, 2018 08:46
@fuj1g0n fuj1g0n force-pushed the feature/use-openapi's-inline-model-resolver branch 2 times, most recently from 74b69cc to 4cbf907 Compare October 16, 2018 03:29
@fuj1g0n fuj1g0n force-pushed the feature/use-openapi's-inline-model-resolver branch from 4cbf907 to 7af2cd7 Compare October 16, 2018 07:55
@fuj1g0n
Copy link
Copy Markdown
Contributor Author

fuj1g0n commented Oct 16, 2018

@wing328 Hi! I reflected your review point, thanks :)

@wing328
Copy link
Copy Markdown
Member

wing328 commented Oct 23, 2018

I've reviewed it twice (today and over the weekend) and the change looks good to me. It's a high-quality PR 👍

@wing328 wing328 merged commit bb056cc into OpenAPITools:master Oct 23, 2018
@wing328 wing328 added this to the 3.3.2 milestone Oct 23, 2018
@wing328
Copy link
Copy Markdown
Member

wing328 commented Oct 31, 2018

@fujigon thanks for the PR, which is included in the v3.3.2 release: https://twitter.com/oas_generator/status/1057649626101112832

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
@fuj1g0n @wing328
[General] fix InlineModelResolver's logic and use openapi-generator's…
4fd7e51 
… 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
@fuj1g0n fuj1g0n deleted the feature/use-openapi's-inline-model-resolver branch March 13, 2019 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Feature: Generator Issue: Bug

Projects

None yet

Milestone

3.3.2

Development

Successfully merging this pull request may close these issues.

[General] nested "required" doesn't work

2 participants

@fuj1g0n @wing328