Repository navigation
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #25522
With
defaultToEmptyContainer,DefaultCodegen.fromPropertysets the empty-container default on the resolved schema. For a$refproperty that is the shared component schema, and forallOfcomposition it is the shared inline property schema. Every later property that resolves to the same schema then finds a default already set, skips its own required check, and gets an initializer too. The first reference's required flag decides for all of them, so the output depends on property order.The default is now taken off the schema again once
toDefaultValueandtoDefaultValueWithParamhave read it, and the schema's default-set flag is restored. Each property is decided by its own required flag, and the exportedapi/openapi.yamlno longer carriesdefault: []entries the input never declared. Parameter and response metadata, which read the schema default again afterfromProperty, now see the declared default only; the regenerated Java sample's API files are unchanged by that.I chose this over cloning the schema before the update:
ModelUtils.cloneSchemachanges untyped{}schemas, which costUser.anyTypePropandAdditionalPropertiesClass.anytype1their= nullinitializer in the okhttp-gson sample. Happy to switch if you prefer the clone.The okhttp-gson sample (the only config with this option) changes in two places. Its
api/openapi.yamlloses the leakeddefault: []lines.RequiredNullableBody.objectItemsNullableloses= new HashMap<>(): the config hasmap?and nomaprule, so a required non-nullable map gets no default; it only had one becauseNullableClasswas generated first and leaked it through the sharedallOfschema.NullableClass.objectItemsNullablekeeps its initializer.Testing
-g java --library resttemplateanddefaultToEmptyContainer=arrayon master givesoptionalStrings = new ArrayList<>(), andoptionalStrings;after swapping the two properties. With the fix both orders giveoptionalStrings;andrequiredStrings = new ArrayList<>().JavaClientCodegenTest#testDefaultToEmptyContainerDoesNotLeakAcrossSharedSchemaRefs_issue25522fails on master at theoptionalStrings;assertion, and fails at theapi/openapi.yamlassertion when only the flag restore is removed. It passes with the fix.JavaClientCodegenTest,DefaultCodegenTest,SpringCodegenTest,DefaultGeneratorTest,JavaJAXRSSpecServerCodegenTest: 1225 tests, 0 failures, 0 errors.bin/configs/java-okhttp-gson.yamlwith the master jar (no change) and with the fixed jar (the two files above). No other config usesdefaultToEmptyContainer.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.
Summary by cubic
Fixes #25522:
defaultToEmptyContainerno longer leaks empty-container defaults across schemas shared via$reforallOf, so each property's initializer is decided by its own required flag instead of the first property that resolved the shared schema.The default is now read and then removed from the schema again, restoring the declared state. The fix restores the schema's flag rather than cloning because
ModelUtils.cloneSchemachanges untyped{}schemas and would drop= nullinitializers in the okhttp-gson sample.Side effects
api/openapi.yamlno longer containsdefault: []entries the input never declared.RequiredNullableBody.objectItemsNullablein the okhttp-gson sample loses its= new HashMap<>()initializer; it only had one becauseNullableClassleaked it through the sharedallOfschema.Testing
testDefaultToEmptyContainerDoesNotLeakAcrossSharedSchemaRefs_issue25522fails on master and passes with the fix.Written for commit d5dfe7d. Summary will update on new commits.