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"))) +}