Skip to content

Do not remove schemas with only a default value form allOf schemas #21189

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

Closed
wants to merge 1 commit into from

Conversation

WellingR
Copy link

@WellingR WellingR commented Apr 30, 2025

A schema with only a default was treated as "meta data only" this caused it to be removed from an allOf schema causing the default value to be lost.

This has been fixed in this MR

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (For Windows users, please run the script in Git BASH)
    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.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@wing328
Copy link
Member

wing328 commented Apr 30, 2025

thanks for the PR

can you you please add a test or 2 to the model utils java test file?

@wing328
Copy link
Member

wing328 commented Apr 30, 2025

cc @ross-paypay

@WellingR
Copy link
Author

Sorry for the delay @wing328 , but I have now added a test that reproduced the problem that I had in the inline model resolver tests.

@wing328
Copy link
Member

wing328 commented May 22, 2025

35ba041#diff-b07a3793d0d84981ddc8dc2e080ef2f9c8fa35e4a9063591981d80ab6389455eR424

i think default is just like description, nullable etc which cannot be handled in allOf in inline model resolver at the moment.

@WellingR
Copy link
Author

WellingR commented May 25, 2025

It is true that the inline model resolver does not do anything with this. I have a project which uses a customized generator which accesses the subtypes in allOf. Since #20892 the inline model resolver would remove the subtype that has only a default. I made this change to keep compatibilty and allow extended generators to access this information

@wing328
Copy link
Member

wing328 commented May 25, 2025

can you please share more about your use cases with an example schema? maybe i can come up with a solution

@WellingR
Copy link
Author

We have some customizations in a generator as we prefer the use of Optionals. Which is we use updated templates and have some java code to enrich some data in our customized generator.

One specific feature that we added in our custom generator is to merge a default value into a schema, which can specifically be useful for enums.

See the following snippet (which was also added in the test in this MR)

          schema:
            allOf:
              - $ref: '#/components/schemas/MyEnum'
              - default: "One"

Our custom generator output would pretty much add the default value to the enum definition. Perhaps that is a feature that could even be supported in the inline model resolver.

@wing328
Copy link
Member

wing328 commented May 28, 2025

@WellingR i've filed #21342

can you please give it a try to see if it works for your use cases?

@WellingR
Copy link
Author

@wing328 Thanks, just built the current version on master locally and this does indeed solve my issue. Than you very much.

Looking forward to the next release.

I will close this MR

@WellingR WellingR closed this Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants