From 603460021bbb3ef02cd3b815f72a5857867e4682 Mon Sep 17 00:00:00 2001 From: quobix Date: Tue, 6 Oct 2026 08:56:03 -0400 Subject: [PATCH 1/2] fix(what-changed): compare object composition refactors safely --- issue611_test.go | 41 ++ what-changed/model/schema.go | 10 +- what-changed/model/schema_composition_wrap.go | 145 +++++ .../model/schema_composition_wrap_test.go | 498 ++++++++++++++++++ 4 files changed, 692 insertions(+), 2 deletions(-) create mode 100644 issue611_test.go create mode 100644 what-changed/model/schema_composition_wrap.go create mode 100644 what-changed/model/schema_composition_wrap_test.go diff --git a/issue611_test.go b/issue611_test.go new file mode 100644 index 00000000..c074187a --- /dev/null +++ b/issue611_test.go @@ -0,0 +1,41 @@ +// Copyright 2026 Princess B33f Heavy Industries / Dave Shanley +// SPDX-License-Identifier: MIT + +package libopenapi + +import ( + "fmt" + "testing" + + "github.com/pb33f/testify/assert" + "github.com/pb33f/testify/require" +) + +func TestIssue611CompareDocumentsObjectComposition(t *testing.T) { + const object = `{"type":"object","required":["id"],"properties":{"id":{"type":"string"}}}` + const nullable = `{"type":["object","null"],"required":["id"],"properties":{"id":{"type":"string"}}}` + const stringUnion = `{"type":["object","string"],"required":["id"],"properties":{"id":{"type":"string"}}}` + const spec = `{"openapi":"3.1.0","info":{"title":"Composition","version":"1.0.0"},"paths":{},"components":{"schemas":{"Entry":%s}}}` + for _, tc := range []struct { + name, old, next string + changed, breaking bool + }{ + {"equivalent nullable refactor", nullable, `{"oneOf":[` + object + `,{"type":"null"}]}`, false, false}, + {"array widening", object, `{"anyOf":[` + object + `,{"type":"array"}]}`, true, false}, + {"null widening", object, `{"oneOf":[` + object + `,{"type":"null"}]}`, true, false}, + {"overlapping oneOf", object, `{"oneOf":[` + object + `,{"type":"object"}]}`, true, true}, + {"wrapper constraint", object, `{"anyOf":[` + object + `,{"type":"null"}],"maxProperties":0}`, true, true}, + {"dropped string alternative", stringUnion, `{"anyOf":[` + object + `,{"type":"null"}]}`, true, true}, + } { + t.Run(tc.name, func(t *testing.T) { + left, err := NewDocument([]byte(fmt.Sprintf(spec, tc.old))) + require.NoError(t, err) + right, err := NewDocument([]byte(fmt.Sprintf(spec, tc.next))) + require.NoError(t, err) + changes, err := CompareDocuments(left, right) + require.NoError(t, err) + assert.Equal(t, tc.changed, changes.TotalChanges() > 0) + assert.Equal(t, tc.breaking, changes.TotalBreakingChanges() > 0) + }) + } +} diff --git a/what-changed/model/schema.go b/what-changed/model/schema.go index 2aaa0a94..5ff42aa6 100644 --- a/what-changed/model/schema.go +++ b/what-changed/model/schema.go @@ -496,10 +496,15 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { lSchema := l.Schema() rSchema := r.Schema() + preserved := preservedObjectBranch(l, r, lSchema, rSchema) + if preserved != nil { + checkObjectCompositionAlternatives(lSchema, rSchema, preserved, &changes) + rSchema = preserved + } comparisonLSchema := schemaComparisonViewForSimpleAllOfObject(l, lSchema) comparisonRSchema := schemaComparisonViewForSimpleAllOfObject(r, rSchema) - if low.AreEqual(lSchema, rSchema) { + if low.AreEqual(lSchema, rSchema) && len(changes) == 0 { // there is no point going on, we know nothing changed! return nil } @@ -513,7 +518,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 || preserved != nil) // now for the confusing part, there is also a schema's 'properties' property to parse. // inception, eat your heart out. diff --git a/what-changed/model/schema_composition_wrap.go b/what-changed/model/schema_composition_wrap.go new file mode 100644 index 00000000..e37f70c3 --- /dev/null +++ b/what-changed/model/schema_composition_wrap.go @@ -0,0 +1,145 @@ +// Copyright 2026 Princess B33f Heavy Industries / Dave Shanley +// SPDX-License-Identifier: MIT + +package model + +import ( + "github.com/pb33f/libopenapi/datamodel/low/base" + "github.com/pb33f/libopenapi/datamodel/low/v3" +) + +// preservedObjectBranch returns a comparison view for an object moved into a +// composition. Uncertain wrappers retain the normal structural comparison. +func preservedObjectBranch(l, r *base.SchemaProxy, old, wrapper *base.Schema) *base.Schema { + if l.IsReference() || r.IsReference() || old == nil || wrapper == nil || + old.Properties.Value == nil || old.Properties.Value.Len() == 0 || hasObjectComposition(old) { + return nil + } + oldNull, ok := objectOrNullType(old) + if !ok { + return nil + } + branches := wrapper.AnyOf.Value + oneOf := schemaNodeHasSingleKey(r.GetValueNode(), v3.OneOfLabel) + if oneOf { + branches = wrapper.OneOf.Value + if len(branches) != 2 { + return nil + } + } else if !schemaNodeHasSingleKey(r.GetValueNode(), v3.AnyOfLabel) { + return nil + } + + var selected *base.Schema + hasNull := false + for _, entry := range branches { + proxy := entry.Value + if proxy == nil || base.CheckSchemaProxyForCircularRefs(proxy) { + return nil + } + branch := proxy.Schema() + if branch == nil { + return nil + } + // A pure null alternative proves null admission without evaluating a schema. + if !proxy.IsReference() && schemaNodeHasSingleKey(proxy.GetValueNode(), v3.TypeLabel) && + branch.Type.Value.A == "null" { + hasNull = true + continue + } + branchNull, object := objectOrNullType(branch) + if object && !branchNull && !hasObjectComposition(branch) { + if selected != nil { + return nil + } + selected = branch + } else if oneOf { + // Only object versus null is proven disjoint here. Other oneOf + // alternatives can reject values by matching more than once. + return nil + } + } + if (oldNull || oneOf) && !hasNull { + return nil + } + return selected +} + +// objectOrNullType accepts only explicit object types, optionally with null. +// Other unions and OAS 3.0 nullable need a wider proof than this shortcut makes. +func objectOrNullType(schema *base.Schema) (nullable, ok bool) { + if schema.Nullable.Value || schema.Type.IsEmpty() { + return false, false + } + if schema.Type.Value.IsA() { + return false, schema.Type.Value.A == "object" + } + object := false + for _, value := range schema.Type.Value.B { + switch value.Value { + case "object": + object = true + case "null": + nullable = true + default: + return false, false + } + } + return nullable, object +} + +func hasObjectComposition(schema *base.Schema) bool { + 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 +} + +// Keep genuine widening visible while suppressing the moved object's removal. +func checkObjectCompositionAlternatives(old, wrapper, selected *base.Schema, changes *[]*Change) { + branches, label := wrapper.AnyOf.Value, v3.AnyOfLabel + if len(wrapper.OneOf.Value) > 0 { + branches, label = wrapper.OneOf.Value, v3.OneOfLabel + } + oldNull, _ := objectOrNullType(old) + oldNull = oldNull && objectConstraintsAdmitNull(old) + for _, entry := range branches { + proxy := entry.Value + branch := proxy.Schema() + if branch == selected || (oldNull && !proxy.IsReference() && + schemaNodeHasSingleKey(proxy.GetValueNode(), v3.TypeLabel) && branch.Type.Value.A == "null") { + continue + } + CreateChange(changes, ObjectAdded, label, nil, proxy.GetValueNode(), + schemaCompositionChangeBreaking(label, ObjectAdded), nil, proxy) + } +} + +// Object assertions do not restrict null. Other assertions need an explicit +// proof before an added null alternative can be treated as already present. +var nullableObjectKeys = map[string]struct{}{ + v3.TypeLabel: {}, v3.PropertiesLabel: {}, v3.RequiredLabel: {}, + v3.AdditionalPropertiesLabel: {}, v3.PatternPropertiesLabel: {}, + v3.PropertyNamesLabel: {}, v3.MinPropertiesLabel: {}, v3.MaxPropertiesLabel: {}, + v3.DependentSchemasLabel: {}, base.DependentRequiredLabel: {}, v3.UnevaluatedPropertiesLabel: {}, + v3.TitleLabel: {}, v3.DescriptionLabel: {}, v3.DefaultLabel: {}, + v3.ExampleLabel: {}, v3.ExamplesLabel: {}, v3.DeprecatedLabel: {}, + v3.ReadOnlyLabel: {}, v3.WriteOnlyLabel: {}, v3.XMLLabel: {}, v3.ExternalDocsLabel: {}, + v3.EnumLabel: {}, v3.ConstLabel: {}, +} + +func objectConstraintsAdmitNull(schema *base.Schema) bool { + if !schemaNodeHasOnlyAllowedKeys(schema.RootNode, nullableObjectKeys) { + return false + } + if schema.Const.Value != nil && schema.Const.Value.Tag != "!!null" { + return false + } + if !schema.Enum.IsEmpty() { + for _, value := range schema.Enum.Value { + if value.Value.Tag == "!!null" { + return true + } + } + return false + } + return true +} diff --git a/what-changed/model/schema_composition_wrap_test.go b/what-changed/model/schema_composition_wrap_test.go new file mode 100644 index 00000000..4602af6c --- /dev/null +++ b/what-changed/model/schema_composition_wrap_test.go @@ -0,0 +1,498 @@ +// Copyright 2026 Princess B33f Heavy Industries / Dave Shanley +// SPDX-License-Identifier: MIT + +package model + +import ( + "strings" + "testing" + + "github.com/pb33f/go-yaml" + "github.com/pb33f/libopenapi/datamodel" + "github.com/pb33f/libopenapi/datamodel/low" + "github.com/pb33f/libopenapi/datamodel/low/v3" + "github.com/pb33f/testify/assert" + "github.com/pb33f/testify/require" +) + +const wrapObject = `type: object +required: [id] +properties: + id: + type: string` + +func wrapDocument(schema string) string { + return "openapi: 3.1.0\ninfo: {title: Composition, version: 1.0.0}\npaths: {}\ncomponents:\n schemas:\n Entry:\n " + strings.ReplaceAll(schema, "\n", "\n ") +} + +func wrapAlternative(keyword, object, alternative string) string { + return keyword + ":\n - " + strings.ReplaceAll(object, "\n", "\n ") + "\n - " + strings.ReplaceAll(alternative, "\n", "\n ") +} + +func TestCompareSchemas_ObjectCompositionSafety(t *testing.T) { + cases := []struct { + name, old, next string + }{ + {"overlapping oneOf", wrapObject, wrapAlternative("oneOf", wrapObject, "type: object")}, + {"wrapper assertion", wrapObject, wrapAlternative("anyOf", wrapObject, "type: 'null'") + "\nmaxProperties: 0"}, + {"wrapper unknown assertion", wrapObject, wrapAlternative("anyOf", wrapObject, "type: 'null'") + "\nx-validation: restricted"}, + {"dropped string type", strings.Replace(wrapObject, "type: object", "type: [object, string]", 1), wrapAlternative("anyOf", wrapObject, "type: 'null'")}, + {"old nullable", wrapObject + "\nnullable: true", wrapAlternative("anyOf", wrapObject, "type: array")}, + {"nullable oneOf overlap", wrapObject, wrapAlternative("oneOf", wrapObject+"\nnullable: true", "type: 'null'")}, + {"duplicate null oneOf", strings.Replace(wrapObject, "type: object", "type: [object, 'null']", 1), wrapAlternative("oneOf", wrapObject, "type: 'null'") + "\n - type: 'null'"}, + {"null branch with constraint", strings.Replace(wrapObject, "type: object", "type: [object, 'null']", 1), wrapAlternative("anyOf", wrapObject, "type: 'null'\nnot: {}")}, + {"ambiguous object branches", wrapObject, wrapAlternative("anyOf", wrapObject, wrapObject)}, + {"removed property", wrapObject, wrapAlternative("anyOf", "type: object", "type: array")}, + {"added requirement", wrapObject, wrapAlternative("anyOf", strings.Replace(wrapObject, "required: [id]", "required: [id, name]", 1), "type: array")}, + {"narrowed property", wrapObject + "\n maxLength: 20", wrapAlternative("anyOf", wrapObject+"\n maxLength: 3", "type: array")}, + {"narrowed object", wrapObject + "\nmaxProperties: 20", wrapAlternative("anyOf", wrapObject+"\nmaxProperties: 1", "type: array")}, + {"reverse direction", wrapAlternative("anyOf", wrapObject, "type: array"), wrapObject}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + left, right := test_BuildDoc(wrapDocument(tc.old), wrapDocument(tc.next)) + l := left.Components.Value.FindSchema("Entry").Value + r := right.Components.Value.FindSchema("Entry").Value + changes := CompareSchemas(l, r) + require.NotNil(t, changes) + assert.Positive(t, changes.TotalBreakingChanges()) + }) + } +} + +func TestCompareSchemas_ObjectCompositionPreservesChangesAndNodes(t *testing.T) { + for _, keyword := range []string{"oneOf", "anyOf"} { + t.Run(keyword, func(t *testing.T) { + old := wrapObject + "\nxml: {name: before}\nexamples: [{id: old}]" + next := wrapObject + "\nxml: {name: after}\nexamples: [{id: old}, {id: new}]" + left, right := test_BuildDoc(wrapDocument(old), wrapDocument(wrapAlternative(keyword, next, "type: 'null'"))) + l := left.Components.Value.FindSchema("Entry").Value + r := right.Components.Value.FindSchema("Entry").Value + beforeL, err := yaml.Marshal(l.GetValueNode()) + require.NoError(t, err) + beforeR, err := yaml.Marshal(r.GetValueNode()) + require.NoError(t, err) + changes := CompareSchemas(l, r) + require.NotNil(t, changes) + require.NotNil(t, changes.XMLChanges) + var exampleChange bool + for _, change := range changes.GetAllChanges() { + if change.Property == "examples" { + exampleChange = true + } + } + assert.True(t, exampleChange) + afterL, err := yaml.Marshal(l.GetValueNode()) + require.NoError(t, err) + afterR, err := yaml.Marshal(r.GetValueNode()) + require.NoError(t, err) + assert.Equal(t, beforeL, afterL) + assert.Equal(t, beforeR, afterR) + assert.Equal(t, changes.TotalChanges(), CompareSchemas(l, r).TotalChanges()) + }) + } +} + +func BenchmarkPreservedObjectBranch(b *testing.B) { + for _, schema := range []struct{ name, value string }{ + {"ordinary", wrapObject}, + {"wrapped", wrapAlternative("oneOf", wrapObject, "type: 'null'")}, + } { + b.Run(schema.name, func(b *testing.B) { + left, right := test_BuildDoc(wrapDocument(wrapObject), wrapDocument(schema.value)) + l := left.Components.Value.FindSchema("Entry").Value + r := right.Components.Value.FindSchema("Entry").Value + ls, rs := l.Schema(), r.Schema() + preservedObjectBranch(l, r, ls, rs) + b.ReportAllocs() + b.ResetTimer() + for b.Loop() { + preservedObjectBranch(l, r, ls, rs) + } + }) + } +} + +func TestPreservedObjectBranch_UnresolvedExternalBranch(t *testing.T) { + left, _ := test_BuildDoc(wrapDocument(wrapObject), wrapDocument(wrapObject)) + info, err := datamodel.ExtractSpecInfo([]byte(wrapDocument("anyOf:\n - $ref: './unresolved.yaml#/Thing'"))) + require.NoError(t, err) + config := datamodel.NewDocumentConfiguration() + config.SkipExternalRefResolution = true + right, err := v3.CreateDocumentFromConfig(info, config) + require.NoError(t, err) + l := left.Components.Value.FindSchema("Entry").Value + r := right.Components.Value.FindSchema("Entry").Value + require.NotNil(t, r.Schema()) + require.Nil(t, r.Schema().AnyOf.Value[0].Value.Schema()) + assert.Nil(t, preservedObjectBranch(l, r, l.Schema(), r.Schema())) +} + +func TestCompareSchemas_ObjectCompositionReportsWidening(t *testing.T) { + for _, keyword := range []string{"oneOf", "anyOf"} { + left, right := test_BuildDoc(wrapDocument(wrapObject), wrapDocument(wrapAlternative(keyword, wrapObject, "type: 'null'"))) + changes := CompareSchemas(left.Components.Value.FindSchema("Entry").Value, right.Components.Value.FindSchema("Entry").Value) + require.NotNil(t, changes) + require.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, ObjectAdded, changes.GetAllChanges()[0].ChangeType) + assert.Equal(t, keyword, changes.GetAllChanges()[0].Property) + assert.Zero(t, changes.TotalBreakingChanges()) + } +} + +func TestCompareSchemas_ObjectCompositionNullAssertions(t *testing.T) { + for _, keyword := range []string{"oneOf", "anyOf"} { + for _, tc := range []struct { + name, assertion string + widening bool + }{ + {"enum excludes null", "enum: [{id: ok}]", true}, + {"const excludes null", "const: {id: ok}", true}, + {"enum includes null", "enum: [{id: ok}, null]", false}, + {"const is null", "const: null", false}, + {"conditional excludes null", "if: {type: 'null'}\nthen: false", true}, + {"unknown constraint", "customConstraint: true", true}, + } { + t.Run(keyword+"/"+tc.name, func(t *testing.T) { + object := wrapObject + "\n" + tc.assertion + old := strings.Replace(object, "type: object", "type: [object, 'null']", 1) + left, right := test_BuildDoc(wrapDocument(old), wrapDocument(wrapAlternative(keyword, object, "type: 'null'"))) + changes := CompareSchemas(left.Components.Value.FindSchema("Entry").Value, right.Components.Value.FindSchema("Entry").Value) + assert.Equal(t, tc.widening, changes.TotalChanges() > 0) + assert.Zero(t, 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) + }) +} From ccf29e001db1b5e7f0d291aa1a11b0835710d1e0 Mon Sep 17 00:00:00 2001 From: quobix Date: Tue, 6 Oct 2026 09:02:59 -0400 Subject: [PATCH 2/2] fix(what-changed): retain structural diffs for changed value constraints --- issue611_test.go | 1 + what-changed/model/schema_composition_wrap.go | 7 +++++++ what-changed/model/schema_composition_wrap_test.go | 2 ++ 3 files changed, 10 insertions(+) diff --git a/issue611_test.go b/issue611_test.go index c074187a..fb011a2a 100644 --- a/issue611_test.go +++ b/issue611_test.go @@ -26,6 +26,7 @@ func TestIssue611CompareDocumentsObjectComposition(t *testing.T) { {"overlapping oneOf", object, `{"oneOf":[` + object + `,{"type":"object"}]}`, true, true}, {"wrapper constraint", object, `{"anyOf":[` + object + `,{"type":"null"}],"maxProperties":0}`, true, true}, {"dropped string alternative", stringUnion, `{"anyOf":[` + object + `,{"type":"null"}]}`, true, true}, + {"narrowed object enum", `{"type":"object","properties":{"id":{"type":"string"}},"enum":[{"id":"a"},{"id":"b"}]}`, `{"anyOf":[{"type":"object","properties":{"id":{"type":"string"}},"enum":[{"id":"a"}]},{"type":"null"}]}`, true, true}, } { t.Run(tc.name, func(t *testing.T) { left, err := NewDocument([]byte(fmt.Sprintf(spec, tc.old))) diff --git a/what-changed/model/schema_composition_wrap.go b/what-changed/model/schema_composition_wrap.go index e37f70c3..00d6e7b5 100644 --- a/what-changed/model/schema_composition_wrap.go +++ b/what-changed/model/schema_composition_wrap.go @@ -4,6 +4,7 @@ package model import ( + "github.com/pb33f/libopenapi/datamodel/low" "github.com/pb33f/libopenapi/datamodel/low/base" "github.com/pb33f/libopenapi/datamodel/low/v3" ) @@ -62,6 +63,12 @@ func preservedObjectBranch(l, r *base.SchemaProxy, old, wrapper *base.Schema) *b if (oldNull || oneOf) && !hasNull { return nil } + // The structural comparison must remain when root value constraints change: + // the ordinary enum comparator cannot distinguish object-valued entries. + if selected != nil && (low.GenerateHashString(old.Enum.ValueNode) != low.GenerateHashString(selected.Enum.ValueNode) || + low.GenerateHashString(old.Const.ValueNode) != low.GenerateHashString(selected.Const.ValueNode)) { + return nil + } return selected } diff --git a/what-changed/model/schema_composition_wrap_test.go b/what-changed/model/schema_composition_wrap_test.go index 4602af6c..ac08df46 100644 --- a/what-changed/model/schema_composition_wrap_test.go +++ b/what-changed/model/schema_composition_wrap_test.go @@ -46,6 +46,8 @@ func TestCompareSchemas_ObjectCompositionSafety(t *testing.T) { {"added requirement", wrapObject, wrapAlternative("anyOf", strings.Replace(wrapObject, "required: [id]", "required: [id, name]", 1), "type: array")}, {"narrowed property", wrapObject + "\n maxLength: 20", wrapAlternative("anyOf", wrapObject+"\n maxLength: 3", "type: array")}, {"narrowed object", wrapObject + "\nmaxProperties: 20", wrapAlternative("anyOf", wrapObject+"\nmaxProperties: 1", "type: array")}, + {"narrowed object enum", wrapObject + "\nenum: [{id: a}, {id: b}]", wrapAlternative("anyOf", wrapObject+"\nenum: [{id: a}]", "type: 'null'")}, + {"changed object const", wrapObject + "\nconst: {id: a}", wrapAlternative("oneOf", wrapObject+"\nconst: {id: b}", "type: 'null'")}, {"reverse direction", wrapAlternative("anyOf", wrapObject, "type: array"), wrapObject}, } for _, tc := range cases {