fix: preserve multi-type type union when schema has subschemas (#954)#1001
fix: preserve multi-type type union when schema has subschemas (#954)#1001ndreno wants to merge 2 commits into
type union when schema has subschemas (#954)#1001Conversation
…decomputer#954) When a schema object had both `type: [a, b, c, ...]` and one of `oneOf`/`anyOf`/`allOf`/`not` on the same level, convert_schema_object matched an earlier arm that wildcarded the `instance_type` field (flagged by a pre-existing TODO: "we probably shouldn't ignore this"). The subschema branches were then converted in isolation, silently discarding the outer type union. For the schema in issue oxidecomputer#954: { "type": ["string", "number", "boolean", "array"], "oneOf": [ { "items": { "type": "string" } }, { "items": { "type": "number" } }, { "items": { "type": "boolean" } } ] } typify produced an enum with only three array variants, losing the string/number/boolean alternatives entirely. The fix tightens the match pattern to `None | Some(Single(_))`, letting `Some(Vec(_))` fall through to the existing merge arm which already folds the outer schema into each subschema branch via `try_merge_with_subschemas`. Single-type + subschema behaviour is preserved, so tolerant handling of schemas whose outer type conflicts with a branch (e.g. the rust-collisions fixture) is unchanged. Adds a fixture with eight cases covering the affected code path: - `type:[...]` combined with oneOf, anyOf, allOf, and not - explicit `type: array` in every oneOf branch (primitives pruned) - partially and fully unsatisfiable oneOf - simultaneous oneOf + allOf - singleton-type regression guard (rust-collisions pattern) No existing workspace fixture exercised `type: [...]` alongside subschemas on the same object, so this fix had zero pre-existing regression coverage.
ahl
left a comment
ahl
left a comment
There was a problem hiding this comment.
Thanks for this. At least one test case is generating the wrong type.
For the $comments I'd ask for a little more brevity (are these LLM-written?).
| #[serde(untagged)] | ||
| pub enum SingleTypeOneOfArrayBranch { | ||
| Object { kind: ::std::string::String }, | ||
| Array([::std::string::String; 2usize]), |
| // Subschemas with no body validation and either no type or a | ||
| // single type. A multi-type (`Vec`) instance_type is deliberately | ||
| // excluded so that it flows through the merge arm below, which | ||
| // folds the type union into each subschema branch. Preserving the | ||
| // earlier behaviour for `None` / `Single` keeps existing tolerant | ||
| // handling of schemas whose outer type may conflict with a branch. |
There was a problem hiding this comment.
| // Subschemas with no body validation and either no type or a | |
| // single type. A multi-type (`Vec`) instance_type is deliberately | |
| // excluded so that it flows through the merge arm below, which | |
| // folds the type union into each subschema branch. Preserving the | |
| // earlier behaviour for `None` / `Single` keeps existing tolerant | |
| // handling of schemas whose outer type may conflict with a branch. | |
| // Subschemas with no additional validation and either no type or a | |
| // single type. A multi-type (`Vec`) instance_type is deliberately | |
| // excluded so that it flows through the merge arm below. |
Per @ahl's review: - Drop the `SingleTypeOneOfArrayBranch` test case. The schema's outer `type: "object"` makes the array branch unsatisfiable, yet codegen still emits the unreachable `Array([String; 2])` variant — i.e. the test was asserting on wrong output. The underlying behaviour (singleton outer type + conflicting branch type not pruned) is worth a separate look but isn't in scope for the oxidecomputer#954 fix. - Trim verbose `$comment` rationales in the remaining cases. - Shorten the match-arm comment in `convert.rs` per the inline suggestion. The match arm itself (`None | Some(SingleOrVec::Single(_))`) and the rest of the oxidecomputer#954 fix are unchanged.
|
Thanks for the review @ahl — pushed
The match arm itself ( The underlying bug ahl's first point hints at — singleton outer |
2 participants
Re-submission of #1000 rebased directly on main. The earlier branch sat on top of other unrelated fork changes that altered doc-comment JSON field ordering, so the committed
.rsfixture didn't match upstream codegen andcargo testfailed after cherry-pick.This branch contains a single commit against current main:
typify-impl/src/convert.rs: narrow the Subschemas match arm frominstance_type: _toNone | Some(SingleOrVec::Single(_))so a multi-typeVec(_)instance_type falls through to the existing merge arm (try_merge_with_subschemas), which folds the outer type union into each subschema branch.typify/tests/schemas/type-array-with-subschemas.{json,rs}: new fixture generated against upstream codegen, 8 cases (oneOf/anyOf/allOf/not+ the singleton-type / rust-collisions regression guard + partially / fully unsatisfiableoneOf+ simultaneousoneOf+allOf).cargo test --workspace,cargo fmt --check,cargo clippy --all-targetsall green locally.Context on the fix itself is in #1000's description.
Closes #954.