[core] Resolve oneOf discriminator property type from child schemas#24172
Open
Ignacio-Vidal wants to merge 1 commit into
Open
[core] Resolve oneOf discriminator property type from child schemas#24172Ignacio-Vidal wants to merge 1 commit into
Ignacio-Vidal wants to merge 1 commit into
Conversation
3a3cbe2 to
0a6c7f0
Compare
Contributor
Author
|
@Mattias-Sehlstedt - This is an enhancement to your recent contribution 439f608 (introduce DiscriminatorUtils). Could you review it please? @wing328 - Could you review i? Given you approved 439f608 too |
4532046 to
02c7380
Compare
Contributor
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
02c7380 to
7b0dc26
Compare
getDiscriminatorPropertyType resolved the discriminator property's type only from the oneOf/anyOf schema's own properties, defaulting to "string" when the property was not declared there. For a pure oneOf interface whose discriminator property lives on a shared base that the children inherit via allOf, this produced a String getter type that clashed with the enum the subtypes actually expose. Add the resolution to DiscriminatorUtils (alongside the existing getDiscriminatorPropertyType / getDiscriminatorSchema): getDiscriminatorPropertyType now falls back to the mapped child schemas, chasing the discriminator property through each child's allOf members (getDiscriminatorPropertyTypeFromChildren / getDiscriminatorSchemaDeep). The allOf descent tracks visited schemas to guard against a cyclic allOf composition recursing infinitely. The fallback only fires when the own-properties lookup is empty, so the type now resolves to the enum ref (e.g. PetType) when a child declares the discriminator property as a $ref, and otherwise still falls back to "string". DefaultCodegen.getDiscriminatorPropertyType becomes a thin binding that maps the resolved schema ref name to a generator-specific model name and applies the "string" default, as those depend on this codegen's instance state. The first resolution branch is unchanged, so existing specs are unaffected: regenerating the full sample suite produces no diffs.
4483ce2 to
310c3f0
Compare
Contributor
|
Hi, see #24158 and #24156 for similar changes. For your change I would say that the same as in #24156 (comment) applies (i.e., is it really necessary to do two recursive passes?). |
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.
What
getDiscriminatorPropertyTyperesolved the discriminator property's type only from theoneOf/anyOfschema's own properties, defaulting to"string"otherwise. For a pureoneOf/anyOfinterface whose discriminator property lives on a shared base the children inherit viaallOf, this produced aStringgetter that clashed with the enum the subtypes actually expose.This adds a fallback (in
DiscriminatorUtils) that resolves the type from the mapped child schemas, chasing the property through each child'sallOfmembers. It only fires when the previous code would have returned"string", so existing specs are unaffected — regenerating the full sample suite produces no diffs.Scenarios covered
$refon the schema's own properties (unchanged path)PetType)DiscriminatorUtilsTest,DefaultCodegenTestoneOfchildren inherit viaallOfPetType(from children)DiscriminatorUtilsTest,DefaultCodegenTestanyOfchildren inherit viaallOfPetType(from children)DiscriminatorUtilsTestallOfPetType(recursive descent)DiscriminatorUtilsTeststring(no$ref, no enum)"string"(fallback, unchanged)DiscriminatorUtilsTest,DefaultCodegenTestSimples example where the
discriminatoris in thebaseobject that is separate from the schema that containsoneOfPR 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 discriminator type resolution for oneOf/anyOf interfaces by deriving the property type from child schemas (via allOf) with cycle-safe recursion. Getter types now use the correct enum (e.g., PetType) instead of "string", avoiding interface vs subtype mismatches.
DiscriminatorUtils.getDiscriminatorPropertyType(OpenAPI, ...)to check the schema’s own$reffirst, then fall back to oneOf/anyOf children viagetDiscriminatorPropertyTypeFromChildrenandgetDiscriminatorSchemaDeep(supports multi-level allOf).getDiscriminatorSchemaDeepto handle cyclic allOf graphs safely.DefaultCodegen.getDiscriminatorPropertyTypeto delegate to the util, map to model names, and keep the "string" fallback; addedDiscriminatorUtilsTest, expandedDefaultCodegenTest, and new YAMLs (oneOf/anyOf, nested base, cyclic, string fallback). Samples regenerate with no diffs.Written for commit 310c3f0. Summary will update on new commits.