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

Support allOf schemas referring to oneOf schemas #318

Merged
merged 7 commits into from
Feb 28, 2022

Conversation

julienrf
Copy link
Contributor

@julienrf julienrf commented Jan 9, 2022

Fixes #317

@julienrf julienrf marked this pull request as ready for review January 9, 2022 23:11
@julienrf
Copy link
Contributor Author

julienrf commented Jan 9, 2022

I have pushed a minimal set of changes that fix my issue. I am not 100% this is the right way to solve the problem, though, any advice is welcome!

@julienrf
Copy link
Contributor Author

May I interest some reviewers in reviewing this PR?

@timur27
Copy link
Contributor

timur27 commented Jan 22, 2022

Hi @julienrf ! Appreciate your effort while contributing to the project. I reviewed the code and left one comment. Should you have any questions to the comment, please let me know :)

Thank you!

@julienrf
Copy link
Contributor Author

Thank you @timur27 for the review and improved solution! I’ve just pushed a commit that follows your suggestion.

@julienrf
Copy link
Contributor Author

julienrf commented Feb 3, 2022

Hey @timur27, may I request another review?

@timur27
Copy link
Contributor

timur27 commented Feb 3, 2022

Hey @timur27, may I request another review?

I see there are some test failures, could you address them? :) @julienrf

@julienrf
Copy link
Contributor Author

Hi @timur27,
I followed your suggestion but I am not sure that we should treat the oneOf schemas like the allOf schemas. In particular, I don’t think we should call addSchema on them because a one of schema must be at most one of the alternative schemas, unlike anyOf and allOf schemas. Doing so breaks a lot of tests that rely on getOneOf and getDiscriminator to manipulate the underlying mapping of a one of schema.
So, I have added back my initial solution, which I think is correct.

@julienrf
Copy link
Contributor Author

Friendly reminder: it would be great if I could get a review :)

@timur27
Copy link
Contributor

timur27 commented Feb 21, 2022

Could anyone help us move this PR forward? Maybe @joschi (forgive me the direct ping please :) )
If I could help anyhow, don't hesitate to let me know.

Copy link
Contributor

@joschi joschi left a comment

Choose a reason for hiding this comment

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

@julienrf LGTM, thanks for your contribution!

@joschi joschi added this to the 2.1.0 milestone Feb 28, 2022
@joschi joschi merged commit 199e30a into OpenAPITools:master Feb 28, 2022
@julienrf julienrf deleted the fix/317 branch February 28, 2022 12:32
@joschi joschi linked an issue Mar 15, 2022 that may be closed by this pull request
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.

[bug] support of breaking changes for "anyOf" NullPointerException when comparing composed schemas
3 participants