From 619b137492ac0e54c193b24cd05f84d8204a0bb9 Mon Sep 17 00:00:00 2001 From: Brian Corbin Date: Thu, 23 Jul 2026 18:17:39 -0400 Subject: [PATCH] fix(what-changed): don't flag object->composition wrap as breaking When an object schema is refactored into a oneOf/anyOf that still contains that object as one of its branches, every value valid before is still valid, so the change is not breaking. Two common cases were reported as breaking: 1. An inline object hoisted into a reusable component and referenced via `oneOf: [$ref, {type: null}]`. The node diff never resolved the $ref, so the object's properties/required looked removed. 2. An object widened into `anyOf: [, ]`. The top-level node changed from an object to an anyOf, so its properties looked removed. CompareSchemas now detects the wrapping direction (old = plain object, new = oneOf/anyOf that preserves that object as a branch, following local $refs) and compares the old object against the preserved branch directly. Genuine, e.g. property-level, changes are still surfaced; only the spurious top-level properties/required/type removal is suppressed. Nullability must be preserved (a null-accepting branch) or the fix does not fire. The reverse direction (a composition collapsing to a single schema, which can drop alternatives) is intentionally left breaking. Adds regression tests for both non-breaking cases plus guards proving unwrapping, non-preserving replacements, genuine property removal, and dropped nullability all stay breaking. --- what-changed/model/schema.go | 178 +++++++++++++- what-changed/model/schema_test.go | 395 ++++++++++++++++++++++++++++++ 2 files changed, 572 insertions(+), 1 deletion(-) diff --git a/what-changed/model/schema.go b/what-changed/model/schema.go index 77a497ba..57cd080e 100644 --- a/what-changed/model/schema.go +++ b/what-changed/model/schema.go @@ -498,6 +498,17 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { comparisonLSchema := schemaComparisonViewForSimpleAllOfObject(l, lSchema) comparisonRSchema := schemaComparisonViewForSimpleAllOfObject(r, rSchema) + // A plain object refactored into a oneOf/anyOf that still contains that object as a branch + // (often behind a local $ref, or an "object OR " widening) is not a breaking + // change — everything valid before is still valid. Compare the old object against the + // preserved branch directly so genuine changes are still detected, and suppress the spurious + // top-level properties/required/type removal that the wrapper would otherwise produce. + skipPreservedCompositionDiff := false + if preservedBranch, ok := preservedCompositionBranchView(l, r); ok { + comparisonRSchema = preservedBranch + skipPreservedCompositionDiff = true + } + if low.AreEqual(lSchema, rSchema) { // there is no point going on, we know nothing changed! return nil @@ -512,7 +523,8 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { skipSimpleScalarUnionDiff := schemasUseEquivalentSimpleScalarUnion(l, r) // check schema core properties for changes. - checkSchemaPropertyChanges(comparisonLSchema, comparisonRSchema, l, r, &changes, sc, skipSimpleScalarUnionDiff) + checkSchemaPropertyChanges(comparisonLSchema, comparisonRSchema, l, r, &changes, sc, + skipSimpleScalarUnionDiff || skipPreservedCompositionDiff) // now for the confusing part, there is also a schema's 'properties' property to parse. // inception, eat your heart out. @@ -544,6 +556,12 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { lanyOf = nil ranyOf = nil } + if skipPreservedCompositionDiff { + // The preserved branch is now folded into comparisonRSchema; don't also diff the wrapper + // composition itself (it would resurface as an added oneOf/anyOf). + roneOf = nil + ranyOf = nil + } props := checkMappedSchemaOfASchema(lProperties, rProperties, &changes) sc.SchemaPropertyChanges = props @@ -2108,6 +2126,164 @@ func extractScalarTypeName(node *yaml.Node) (string, bool) { return node.Value, true } +// preservedCompositionBranchView detects the "wrapping" refactor where an OLD plain object schema is +// reshaped into a NEW oneOf/anyOf composition that still contains that object as one of its branches +// (commonly behind a local $ref when a schema is hoisted into a reusable component, or an +// "object OR " widening). Because the original object is still one of the accepted +// shapes, every previously-valid value remains valid, so nothing was removed — but a naive +// node-level diff sees the top-level `properties`/`required`/`type` vanish and reports them as +// breaking removals. +// +// When the wrap is detected, the resolved "preserved" branch schema is returned so the caller can +// compare the OLD object against that branch directly. That keeps genuine, e.g. property-level, +// changes visible while dropping the spurious top-level removal signals. Only the wrapping direction +// (l = old, r = new) is handled: the reverse (a composition collapsing to a single schema) can drop +// alternatives and must remain breaking, so it is intentionally not matched here. +func preservedCompositionBranchView(l, r *base.SchemaProxy) (*base.Schema, bool) { + if l == nil || r == nil || l.IsReference() || r.IsReference() { + return nil, false + } + // OLD must be a plain object carrying properties and no composition of its own. + // (nil schemas are handled by the predicate helpers, which treat nil as "no".) + lSchema := l.Schema() + rSchema := r.Schema() + if !schemaIsPlainObjectWithProperties(lSchema) { + return nil, false + } + + // NEW must be a pure oneOf/anyOf wrapper (exactly one of the two, no competing top-level keys). + branches, ok := schemaWrappingCompositionBranches(rSchema) + if !ok { + return nil, false + } + + // Find exactly one branch that preserves all of OLD's property names (the object, possibly + // evolved), resolving local $refs. Any additional branches are treated as added alternatives. + var primary *base.Schema + primaryCount := 0 + compositionAcceptsNull := false + for _, branch := range branches { + if branch.Value == nil || base.CheckSchemaProxyForCircularRefs(branch.Value) { + return nil, false + } + // A nil resolved branch schema is handled by the predicate helpers below (treated as "no"). + branchSchema := branch.Value.Schema() + if schemaTypeAcceptsNull(branchSchema) { + compositionAcceptsNull = true + } + if schemaPreservesObjectProperties(lSchema, branchSchema) { + primary = branchSchema + primaryCount++ + } + } + if primaryCount != 1 || primary == nil { + return nil, false + } + + // Nullability must be preserved: if OLD accepted null, NEW must still accept it (via a null + // branch or the preserved branch itself), otherwise dropping null is a real narrowing. + if schemaTypeAcceptsNull(lSchema) && !compositionAcceptsNull && !schemaTypeAcceptsNull(primary) { + return nil, false + } + + return primary, true +} + +// schemaWrappingCompositionBranches returns the branches of a schema that is a pure oneOf/anyOf +// wrapper (exactly one of oneOf/anyOf, and no top-level properties/allOf/prefixItems/not that would +// make it more than a wrapper). allOf is intentionally excluded — it is handled by +// schemaComparisonViewForSimpleAllOfObject. +func schemaWrappingCompositionBranches(schema *base.Schema) ([]low.ValueReference[*base.SchemaProxy], bool) { + if schema == nil { + return nil, false + } + if schemaHasProperties(schema) || len(schema.AllOf.Value) > 0 || + len(schema.PrefixItems.Value) > 0 || schema.Not.Value != nil { + return nil, false + } + oneOf := schema.OneOf.Value + anyOf := schema.AnyOf.Value + switch { + case len(oneOf) > 0 && len(anyOf) == 0: + return oneOf, true + case len(anyOf) > 0 && len(oneOf) == 0: + return anyOf, true + default: + return nil, false + } +} + +// schemaPreservesObjectProperties reports whether branch is an object that keeps every property name +// declared on l (branch may add or widen properties — those are surfaced by the normal recursive +// diff — but it must not drop any, which is what distinguishes the preserved object from an +// unrelated alternative branch). +func schemaPreservesObjectProperties(l, branch *base.Schema) bool { + if !schemaHasProperties(branch) || !schemaTypeIncludesObject(branch) { + return false + } + branchNames := make(map[string]struct{}, branch.Properties.Value.Len()) + for k := range branch.Properties.Value.FromOldest() { + branchNames[k.Value] = struct{}{} + } + for k := range l.Properties.Value.FromOldest() { + if _, ok := branchNames[k.Value]; !ok { + return false + } + } + return true +} + +func schemaIsPlainObjectWithProperties(schema *base.Schema) bool { + return schemaHasProperties(schema) && schemaTypeIncludesObject(schema) && !schemaHasComposition(schema) +} + +func schemaHasProperties(schema *base.Schema) bool { + return schema != nil && schema.Properties.Value != nil && schema.Properties.Value.Len() > 0 +} + +func schemaHasComposition(schema *base.Schema) bool { + if schema == nil { + return false + } + return len(schema.OneOf.Value) > 0 || len(schema.AnyOf.Value) > 0 || len(schema.AllOf.Value) > 0 || + len(schema.PrefixItems.Value) > 0 || schema.Not.Value != nil +} + +// schemaTypeIncludesObject reports whether the schema's declared type admits objects. A schema with +// no explicit type but with properties is treated as an object. +func schemaTypeIncludesObject(schema *base.Schema) bool { + if schema == nil { + return false + } + if schema.Type.IsEmpty() { + return schemaHasProperties(schema) + } + if schema.Type.Value.IsB() { + for _, t := range schema.Type.Value.B { + if t.Value == "object" { + return true + } + } + return false + } + return schema.Type.Value.A == "object" +} + +func schemaTypeAcceptsNull(schema *base.Schema) bool { + if schema == nil || schema.Type.IsEmpty() { + return false + } + if schema.Type.Value.IsB() { + for _, t := range schema.Type.Value.B { + if t.Value == "null" { + return true + } + } + return false + } + return schema.Type.Value.A == "null" +} + func checkExamples(lSchema *base.Schema, rSchema *base.Schema, changes *[]*Change) { if lSchema == nil && rSchema == nil { return diff --git a/what-changed/model/schema_test.go b/what-changed/model/schema_test.go index a8197143..3ada67fe 100644 --- a/what-changed/model/schema_test.go +++ b/what-changed/model/schema_test.go @@ -7422,3 +7422,398 @@ components: // TotalBreakingChanges should count the vocabulary change assert.Equal(t, 1, changes.TotalBreakingChanges()) } + +// An inline object hoisted into a reusable component and referenced via `oneOf: [$ref, null]` is an +// identical, non-breaking restructure: the referenced schema is byte-identical to the old inline +// object and the null branch preserves nullability. The naive diff used to report the object's +// properties/required as removed (breaking) because it never resolved the $ref. +func TestCompareSchemas_InlineObjectWrappedInOneOfRefIsNotBreaking(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Widget: + type: object + properties: + config: + type: + - object + - 'null' + required: + - mode + - items + properties: + mode: + type: integer + enum: [1, 2] + items: + type: array + items: + type: object + required: [kind] + properties: + kind: + type: string` + + right := `openapi: 3.1.0 +components: + schemas: + Widget: + type: object + properties: + config: + oneOf: + - $ref: '#/components/schemas/Config' + - type: 'null' + Config: + type: object + required: + - mode + - items + properties: + mode: + type: integer + enum: [1, 2] + items: + type: array + items: + type: object + required: [kind] + properties: + kind: + type: string` + + leftDoc, rightDoc := test_BuildDoc(left, right) + lSchemaProxy := leftDoc.Components.Value.FindSchema("Widget").Value + rSchemaProxy := rightDoc.Components.Value.FindSchema("Widget").Value + + changes := CompareSchemas(lSchemaProxy, rSchemaProxy) + // The restructure is equivalent, so there are no changes at all — and definitely no breaking ones. + assert.Nil(t, changes) + assert.Equal(t, 0, changes.TotalChanges()) + assert.Equal(t, 0, changes.TotalBreakingChanges()) +} + +// Widening an object into `anyOf: [, ]` still accepts every value the +// object accepted before, so it is not breaking. The naive diff used to report the object's +// properties/required as removed because the top-level node changed from an object to an anyOf. +func TestCompareSchemas_ObjectWidenedIntoAnyOfIsNotBreaking(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Entry: + type: object + required: [kind] + properties: + kind: + type: string + enum: [alpha, beta]` + + right := `openapi: 3.1.0 +components: + schemas: + Entry: + anyOf: + - type: object + required: [kind] + properties: + kind: + type: string + enum: [alpha, beta] + - type: array + items: + type: object + required: [kind] + properties: + kind: + type: string + enum: [alpha, beta]` + + leftDoc, rightDoc := test_BuildDoc(left, right) + lSchemaProxy := leftDoc.Components.Value.FindSchema("Entry").Value + rSchemaProxy := rightDoc.Components.Value.FindSchema("Entry").Value + + changes := CompareSchemas(lSchemaProxy, rSchemaProxy) + assert.Equal(t, 0, changes.TotalBreakingChanges()) +} + +// The combined case: hoisted into a $ref AND the referenced schema was widened (an enum gained a +// member). The wrapper itself is non-breaking, and the genuine inner change must still be surfaced +// (as a non-breaking enum addition) rather than hidden. +func TestCompareSchemas_WrappedRefWithInnerWideningSurfacesNonBreakingChange(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Widget: + type: object + properties: + config: + type: + - object + - 'null' + required: + - mode + properties: + mode: + type: integer + enum: [1, 2]` + + right := `openapi: 3.1.0 +components: + schemas: + Widget: + type: object + properties: + config: + oneOf: + - $ref: '#/components/schemas/Config' + - type: 'null' + Config: + type: object + required: + - mode + properties: + mode: + type: integer + enum: [1, 2, 3]` + + leftDoc, rightDoc := test_BuildDoc(left, right) + lSchemaProxy := leftDoc.Components.Value.FindSchema("Widget").Value + rSchemaProxy := rightDoc.Components.Value.FindSchema("Widget").Value + + changes := CompareSchemas(lSchemaProxy, rSchemaProxy) + assert.NotNil(t, changes) + // The enum gaining a member is surfaced as a change, but it is not breaking. + assert.Greater(t, changes.TotalChanges(), 0) + assert.Equal(t, 0, changes.TotalBreakingChanges()) +} + +// Guard: the reverse direction (a composition collapsing to a single object, dropping an +// alternative) is a real narrowing and must stay breaking — the fix must not fire here. +func TestCompareSchemas_AnyOfUnwrappedToObjectStaysBreaking(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Entry: + anyOf: + - type: object + required: [kind] + properties: + kind: + type: string + - type: array + items: + type: object` + + right := `openapi: 3.1.0 +components: + schemas: + Entry: + type: object + required: [kind] + properties: + kind: + type: string` + + leftDoc, rightDoc := test_BuildDoc(left, right) + lSchemaProxy := leftDoc.Components.Value.FindSchema("Entry").Value + rSchemaProxy := rightDoc.Components.Value.FindSchema("Entry").Value + + changes := CompareSchemas(lSchemaProxy, rSchemaProxy) + assert.NotNil(t, changes) + assert.Greater(t, changes.TotalBreakingChanges(), 0) +} + +// Guard: an object replaced by an anyOf whose branches do NOT preserve the original object's +// properties is a genuine change (the original property is gone) and must stay breaking. +func TestCompareSchemas_ObjectReplacedByNonPreservingAnyOfStaysBreaking(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Entry: + type: object + required: [kind] + properties: + kind: + type: string` + + right := `openapi: 3.1.0 +components: + schemas: + Entry: + anyOf: + - type: object + required: [label] + properties: + label: + type: string + - type: array + items: + type: object` + + leftDoc, rightDoc := test_BuildDoc(left, right) + lSchemaProxy := leftDoc.Components.Value.FindSchema("Entry").Value + rSchemaProxy := rightDoc.Components.Value.FindSchema("Entry").Value + + changes := CompareSchemas(lSchemaProxy, rSchemaProxy) + assert.NotNil(t, changes) + assert.Greater(t, changes.TotalBreakingChanges(), 0) +} + +// Guard: dropping nullability while hoisting into a ref is a real narrowing (null was accepted, now +// it isn't). The fix must not fire because the composition has no null-accepting branch. +func TestCompareSchemas_WrapDroppingNullabilityStaysBreaking(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Widget: + type: object + properties: + config: + type: + - object + - 'null' + required: + - mode + properties: + mode: + type: integer` + + right := `openapi: 3.1.0 +components: + schemas: + Widget: + type: object + properties: + config: + oneOf: + - $ref: '#/components/schemas/Config' + Config: + type: object + required: + - mode + properties: + mode: + type: integer` + + leftDoc, rightDoc := test_BuildDoc(left, right) + lSchemaProxy := leftDoc.Components.Value.FindSchema("Widget").Value + rSchemaProxy := rightDoc.Components.Value.FindSchema("Widget").Value + + changes := CompareSchemas(lSchemaProxy, rSchemaProxy) + assert.NotNil(t, changes) + assert.Greater(t, changes.TotalBreakingChanges(), 0) +} + +// Guard: a composition branch that is a circular $ref must not be followed (it would recurse). The +// preservation shortcut bails out, so the comparison falls back to the normal diff without panicking. +func TestCompareSchemas_WrapWithCircularRefBranchIsHandled(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Widget: + type: object + properties: + config: + type: object + required: [mode] + properties: + mode: + type: integer` + + right := `openapi: 3.1.0 +components: + schemas: + Widget: + type: object + properties: + config: + oneOf: + - $ref: '#/components/schemas/Node' + - type: 'null' + Node: + type: object + required: [mode] + properties: + mode: + type: integer + child: + $ref: '#/components/schemas/Node'` + + leftDoc, rightDoc := test_BuildDoc(left, right) + lSchemaProxy := leftDoc.Components.Value.FindSchema("Widget").Value + rSchemaProxy := rightDoc.Components.Value.FindSchema("Widget").Value + + assert.NotPanics(t, func() { + CompareSchemas(lSchemaProxy, rSchemaProxy) + }) +} + +// Directly exercises the small predicate helpers behind preservedCompositionBranchView, including +// nil and edge-typed inputs that the normal comparison path never produces. +func TestPreservedCompositionWrapHelpers(t *testing.T) { + low.ClearHashCache() + spec := `openapi: 3.1.0 +components: + schemas: + BothComposition: + oneOf: + - type: string + anyOf: + - type: integer + EmptyWrapper: + description: no type, no properties, no composition + ScalarUnionNoNull: + type: + - string + - number + ScalarUnionWithNull: + type: + - string + - 'null' + UntypedWithProps: + properties: + a: + type: string + OneOfWrapper: + oneOf: + - type: string + - type: 'null'` + + doc, _ := test_BuildDoc(spec, spec) + get := func(name string) *base.Schema { + return doc.Components.Value.FindSchema(name).Value.Schema() + } + + // schemaWrappingCompositionBranches + _, ok := schemaWrappingCompositionBranches(nil) + assert.False(t, ok) + _, ok = schemaWrappingCompositionBranches(get("BothComposition")) + assert.False(t, ok) // both oneOf and anyOf -> not a pure wrapper + _, ok = schemaWrappingCompositionBranches(get("EmptyWrapper")) + assert.False(t, ok) // neither oneOf nor anyOf + branches, ok := schemaWrappingCompositionBranches(get("OneOfWrapper")) + assert.True(t, ok) + assert.Len(t, branches, 2) + + // schemaHasComposition + assert.False(t, schemaHasComposition(nil)) + assert.True(t, schemaHasComposition(get("BothComposition"))) + + // schemaTypeIncludesObject + assert.False(t, schemaTypeIncludesObject(nil)) + assert.False(t, schemaTypeIncludesObject(get("ScalarUnionNoNull"))) // type array without object + assert.True(t, schemaTypeIncludesObject(get("UntypedWithProps"))) // no type but has properties + + // schemaTypeAcceptsNull + assert.False(t, schemaTypeAcceptsNull(nil)) + assert.False(t, schemaTypeAcceptsNull(get("EmptyWrapper"))) // no type + assert.False(t, schemaTypeAcceptsNull(get("ScalarUnionNoNull"))) // type array without null + assert.True(t, schemaTypeAcceptsNull(get("ScalarUnionWithNull"))) +}