-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Fix bug in ModelUtils.getParentName resulting in wrong inner Models for oneOf-composed schemas #21799
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
Conversation
Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors. Let me know if you need help fixing it. |
Thank you very much, adding the email address fixed it! |
since there was a failing check that is probably due to the branch being based on an outdated master, I synced the branch. |
please
then i will try to test your fix before merging |
…nt to narrow down observed bug)
…sed schema instead of null when there are multiple elements and it is not clear which one should be parent
6007e7e
to
e0786cd
Compare
https://github.com/OpenAPITools/openapi-generator/actions/runs/17371620947/job/49309557740?pr=21799 please run the command to update the samples one more time |
did a test locally and all tests passed |
we will later revise the test instead if it still occurs after merging into master thanks for the PR to fix the bug |
update: merged #21865 to disable the tests (which only failed in Windows but not Linux) |
The
ModelUtils.getParentName
function description states:It returned the first parent candidate instead. This resulted in a weird bug where generated client models did not contain all fields of their subschemas mentioned in
oneOf
(fixes #21773 , in particular this affects at least Java Microprofile and Vertx generators).This PR contains some new tests:
ModelUtilsTest
)JavaCodegenTest
(and also inDefaultCodegenTest
to narrow it down)Also, the
oneOf
-Test was changed to contain three schemas, because at first I thought the issue was with the number of schemas and in general it is probably better to test something accepting a list with three instead of two elements.Running the scripts to update samples and docs did not change anything (as was to be expected).
Please tell me if I should
Also, since I found this mostly via debugging and am not proficient with the terminology and edge cases of this project, my fix follows the philosophy to touch/change as little as possible, however, somebody with more experience than me might want to clean up that function since there seems to be some unreachable/unused code according to my IDE.
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.0
minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)"fixes #123"
present in the PR description)@bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger @karismann @Zomzog @lwlee2608 @martin-mfg