Skip to content

Commit db47110

Browse files
libopenapi contributorcorbinbs
authored andcommitted
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: [<same object>, <new alternative>]`. 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.
1 parent 0837c9b commit db47110

2 files changed

Lines changed: 469 additions & 1 deletion

File tree

what-changed/model/schema.go

Lines changed: 182 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,17 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges {
498498
comparisonLSchema := schemaComparisonViewForSimpleAllOfObject(l, lSchema)
499499
comparisonRSchema := schemaComparisonViewForSimpleAllOfObject(r, rSchema)
500500

501+
// A plain object refactored into a oneOf/anyOf that still contains that object as a branch
502+
// (often behind a local $ref, or an "object OR <alternative>" widening) is not a breaking
503+
// change — everything valid before is still valid. Compare the old object against the
504+
// preserved branch directly so genuine changes are still detected, and suppress the spurious
505+
// top-level properties/required/type removal that the wrapper would otherwise produce.
506+
skipPreservedCompositionDiff := false
507+
if preservedBranch, ok := preservedCompositionBranchView(l, r); ok {
508+
comparisonRSchema = preservedBranch
509+
skipPreservedCompositionDiff = true
510+
}
511+
501512
if low.AreEqual(lSchema, rSchema) {
502513
// there is no point going on, we know nothing changed!
503514
return nil
@@ -512,7 +523,8 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges {
512523
skipSimpleScalarUnionDiff := schemasUseEquivalentSimpleScalarUnion(l, r)
513524

514525
// check schema core properties for changes.
515-
checkSchemaPropertyChanges(comparisonLSchema, comparisonRSchema, l, r, &changes, sc, skipSimpleScalarUnionDiff)
526+
checkSchemaPropertyChanges(comparisonLSchema, comparisonRSchema, l, r, &changes, sc,
527+
skipSimpleScalarUnionDiff || skipPreservedCompositionDiff)
516528

517529
// now for the confusing part, there is also a schema's 'properties' property to parse.
518530
// inception, eat your heart out.
@@ -544,6 +556,12 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges {
544556
lanyOf = nil
545557
ranyOf = nil
546558
}
559+
if skipPreservedCompositionDiff {
560+
// The preserved branch is now folded into comparisonRSchema; don't also diff the wrapper
561+
// composition itself (it would resurface as an added oneOf/anyOf).
562+
roneOf = nil
563+
ranyOf = nil
564+
}
547565

548566
props := checkMappedSchemaOfASchema(lProperties, rProperties, &changes)
549567
sc.SchemaPropertyChanges = props
@@ -2108,6 +2126,169 @@ func extractScalarTypeName(node *yaml.Node) (string, bool) {
21082126
return node.Value, true
21092127
}
21102128

2129+
// preservedCompositionBranchView detects the "wrapping" refactor where an OLD plain object schema is
2130+
// reshaped into a NEW oneOf/anyOf composition that still contains that object as one of its branches
2131+
// (commonly behind a local $ref when a schema is hoisted into a reusable component, or an
2132+
// "object OR <alternative>" widening). Because the original object is still one of the accepted
2133+
// shapes, every previously-valid value remains valid, so nothing was removed — but a naive
2134+
// node-level diff sees the top-level `properties`/`required`/`type` vanish and reports them as
2135+
// breaking removals.
2136+
//
2137+
// When the wrap is detected, the resolved "preserved" branch schema is returned so the caller can
2138+
// compare the OLD object against that branch directly. That keeps genuine, e.g. property-level,
2139+
// changes visible while dropping the spurious top-level removal signals. Only the wrapping direction
2140+
// (l = old, r = new) is handled: the reverse (a composition collapsing to a single schema) can drop
2141+
// alternatives and must remain breaking, so it is intentionally not matched here.
2142+
func preservedCompositionBranchView(l, r *base.SchemaProxy) (*base.Schema, bool) {
2143+
if l == nil || r == nil || l.IsReference() || r.IsReference() {
2144+
return nil, false
2145+
}
2146+
lSchema := l.Schema()
2147+
rSchema := r.Schema()
2148+
if lSchema == nil || rSchema == nil {
2149+
return nil, false
2150+
}
2151+
2152+
// OLD must be a plain object carrying properties and no composition of its own.
2153+
if !schemaIsPlainObjectWithProperties(lSchema) {
2154+
return nil, false
2155+
}
2156+
2157+
// NEW must be a pure oneOf/anyOf wrapper (exactly one of the two, no competing top-level keys).
2158+
branches, ok := schemaWrappingCompositionBranches(rSchema)
2159+
if !ok {
2160+
return nil, false
2161+
}
2162+
2163+
// Find exactly one branch that preserves all of OLD's property names (the object, possibly
2164+
// evolved), resolving local $refs. Any additional branches are treated as added alternatives.
2165+
var primary *base.Schema
2166+
primaryCount := 0
2167+
compositionAcceptsNull := false
2168+
for _, branch := range branches {
2169+
if branch.Value == nil || base.CheckSchemaProxyForCircularRefs(branch.Value) {
2170+
return nil, false
2171+
}
2172+
branchSchema := branch.Value.Schema()
2173+
if branchSchema == nil {
2174+
return nil, false
2175+
}
2176+
if schemaTypeAcceptsNull(branchSchema) {
2177+
compositionAcceptsNull = true
2178+
}
2179+
if schemaPreservesObjectProperties(lSchema, branchSchema) {
2180+
primary = branchSchema
2181+
primaryCount++
2182+
}
2183+
}
2184+
if primaryCount != 1 || primary == nil {
2185+
return nil, false
2186+
}
2187+
2188+
// Nullability must be preserved: if OLD accepted null, NEW must still accept it (via a null
2189+
// branch or the preserved branch itself), otherwise dropping null is a real narrowing.
2190+
if schemaTypeAcceptsNull(lSchema) && !compositionAcceptsNull && !schemaTypeAcceptsNull(primary) {
2191+
return nil, false
2192+
}
2193+
2194+
return primary, true
2195+
}
2196+
2197+
// schemaWrappingCompositionBranches returns the branches of a schema that is a pure oneOf/anyOf
2198+
// wrapper (exactly one of oneOf/anyOf, and no top-level properties/allOf/prefixItems/not that would
2199+
// make it more than a wrapper). allOf is intentionally excluded — it is handled by
2200+
// schemaComparisonViewForSimpleAllOfObject.
2201+
func schemaWrappingCompositionBranches(schema *base.Schema) ([]low.ValueReference[*base.SchemaProxy], bool) {
2202+
if schema == nil {
2203+
return nil, false
2204+
}
2205+
if schemaHasProperties(schema) || len(schema.AllOf.Value) > 0 ||
2206+
len(schema.PrefixItems.Value) > 0 || schema.Not.Value != nil {
2207+
return nil, false
2208+
}
2209+
oneOf := schema.OneOf.Value
2210+
anyOf := schema.AnyOf.Value
2211+
switch {
2212+
case len(oneOf) > 0 && len(anyOf) == 0:
2213+
return oneOf, true
2214+
case len(anyOf) > 0 && len(oneOf) == 0:
2215+
return anyOf, true
2216+
default:
2217+
return nil, false
2218+
}
2219+
}
2220+
2221+
// schemaPreservesObjectProperties reports whether branch is an object that keeps every property name
2222+
// declared on l (branch may add or widen properties — those are surfaced by the normal recursive
2223+
// diff — but it must not drop any, which is what distinguishes the preserved object from an
2224+
// unrelated alternative branch).
2225+
func schemaPreservesObjectProperties(l, branch *base.Schema) bool {
2226+
if !schemaHasProperties(branch) || !schemaTypeIncludesObject(branch) {
2227+
return false
2228+
}
2229+
branchNames := make(map[string]struct{}, branch.Properties.Value.Len())
2230+
for k := range branch.Properties.Value.FromOldest() {
2231+
branchNames[k.Value] = struct{}{}
2232+
}
2233+
for k := range l.Properties.Value.FromOldest() {
2234+
if _, ok := branchNames[k.Value]; !ok {
2235+
return false
2236+
}
2237+
}
2238+
return true
2239+
}
2240+
2241+
func schemaIsPlainObjectWithProperties(schema *base.Schema) bool {
2242+
return schemaHasProperties(schema) && schemaTypeIncludesObject(schema) && !schemaHasComposition(schema)
2243+
}
2244+
2245+
func schemaHasProperties(schema *base.Schema) bool {
2246+
return schema != nil && schema.Properties.Value != nil && schema.Properties.Value.Len() > 0
2247+
}
2248+
2249+
func schemaHasComposition(schema *base.Schema) bool {
2250+
if schema == nil {
2251+
return false
2252+
}
2253+
return len(schema.OneOf.Value) > 0 || len(schema.AnyOf.Value) > 0 || len(schema.AllOf.Value) > 0 ||
2254+
len(schema.PrefixItems.Value) > 0 || schema.Not.Value != nil
2255+
}
2256+
2257+
// schemaTypeIncludesObject reports whether the schema's declared type admits objects. A schema with
2258+
// no explicit type but with properties is treated as an object.
2259+
func schemaTypeIncludesObject(schema *base.Schema) bool {
2260+
if schema == nil {
2261+
return false
2262+
}
2263+
if schema.Type.IsEmpty() {
2264+
return schemaHasProperties(schema)
2265+
}
2266+
if schema.Type.Value.IsB() {
2267+
for _, t := range schema.Type.Value.B {
2268+
if t.Value == "object" {
2269+
return true
2270+
}
2271+
}
2272+
return false
2273+
}
2274+
return schema.Type.Value.A == "object"
2275+
}
2276+
2277+
func schemaTypeAcceptsNull(schema *base.Schema) bool {
2278+
if schema == nil || schema.Type.IsEmpty() {
2279+
return false
2280+
}
2281+
if schema.Type.Value.IsB() {
2282+
for _, t := range schema.Type.Value.B {
2283+
if t.Value == "null" {
2284+
return true
2285+
}
2286+
}
2287+
return false
2288+
}
2289+
return schema.Type.Value.A == "null"
2290+
}
2291+
21112292
func checkExamples(lSchema *base.Schema, rSchema *base.Schema, changes *[]*Change) {
21122293
if lSchema == nil && rSchema == nil {
21132294
return

0 commit comments

Comments
 (0)