Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #611 +/- ##
==========================================
- Coverage 99.78% 99.75% -0.03%
==========================================
Files 283 283
Lines 34456 34545 +89
==========================================
+ Hits 34382 34461 +79
- Misses 46 55 +9
- Partials 28 29 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
db47110 to
5face2e
Compare
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.
5face2e to
619b137
Compare
| oneOf := schema.OneOf.Value | ||
| anyOf := schema.AnyOf.Value | ||
| switch { | ||
| case len(oneOf) > 0 && len(anyOf) == 0: |
There was a problem hiding this comment.
I don’t think oneOf and anyOf can be treated the same here. An overlapping oneOf branch can make a previously valid value invalid. Could we split these paths and only suppress oneOf when the branches are proven disjoint?
| if schema == nil { | ||
| return nil, false | ||
| } | ||
| if schemaHasProperties(schema) || len(schema.AllOf.Value) > 0 || |
There was a problem hiding this comment.
This isn’t quite a pure wrapper yet. Sibling constraints such as maxProperties still apply, but swapping in the branch drops them from the comparison. Could we only take this shortcut when the wrapper has no assertion keywords?
| return nil, false | ||
| } | ||
|
|
||
| // Nullability must be preserved: if OLD accepted null, NEW must still accept it (via a null |
There was a problem hiding this comment.
This only protects null. An old type: [object, string] can lose string here and still be reported unchanged. Could we verify that the complete old type set is preserved?
daveshanley
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the false positive is real, and the intended cases look good.
I found three cases where the shortcut can hide genuine breaking changes: overlapping oneOf branches, sibling assertions on the wrapper, and dropped non-null members from the old type union. CompareSchemas reports no change for all three, so I do not think we can merge this as-is.
Suggested next steps:
- Handle
anyOfandoneOfseparately. - Keep the
oneOfshortcut to provably disjoint branches, at minimum object-or-null. - Require a genuinely assertion-free wrapper, or compare its sibling constraints.
- Verify the complete old type set is preserved.
- Add regression tests for the three cases above.
Once those are covered, the overall shape looks reasonable. Thanks again — this is a good bug to fix.
Summary
what-changedreports false-positive breaking changes when an object schema is refactored intoa
oneOf/anyOfcomposition that still contains that object as one of its branches. No previouslyvalid value becomes invalid, so the change is not breaking, but the comparator reports the object's
properties/requiredas removed.This PR teaches
CompareSchemasto recognize that "wrapping" refactor and stop flagging it, whilekeeping every genuine change (and the reverse, narrowing direction) fully reported.
The two shapes that trigger it
1. An inline object hoisted into a reusable component, referenced via
oneOf: [$ref, {type: null}].The referenced component is identical to the old inline object, but the
$refis never resolved, sothe properties look removed.
2. An object widened into
anyOf: [<same object>, <new alternative>].The old object is preserved as the first branch, but the top-level node changed from an object to an
anyOf, so its properties look removed.Both reproduce on the current
main(and were originally hit withopenapi-changesv0.2.8 /libopenapi v0.37.3, and still on v0.2.10).
Why it's a false positive
Everything valid before is still valid:
oneOf: [Config, null]is exactly the old nullable object — the properties moved behindthe
$ref, they were not removed.anyOfbranch; an alternative was added.The default rules already treat adding a
oneOf/anyOfas non-breaking. The problem is that thesame restructure is also counted as a removal of the object's
properties/required, so it getsreported twice — once (correctly) as a non-breaking composition addition, and once (incorrectly) as a
breaking removal.
Root cause
Comparison runs on the low-level node model. When the old side is a plain object (
properties/required/type) and the new side is aoneOf/anyOf, the field-by-field diff sees thetop-level
properties/requireddisappear (they now live inside a branch, possibly behind a$ref) and reports them removed. Nothing correlates the old object with the preserved branch, and a$refbranch is keyed by its ref string rather than by resolved content.The fix
CompareSchemasnow detects the wrapping direction and, when found, compares the old objectagainst the preserved branch directly:
oneOf/anyOfwrapper whose branches — resolving local
$refs — contain exactly one branch that preservesall of the old object's property names. That branch becomes the comparison view for the new side;
the wrapper composition is not separately re-reported.
recursively, a real change inside the branch (e.g. a widened
enum, an added property) is stillreported — as breaking or non-breaking per the normal rules. Only the spurious top-level
properties/required/typeremoval is suppressed.It stays conservative via three guards:
collapsing to a single schema, which can drop alternatives) is left breaking.
null, the new composition must still acceptit (a
nullbranch, or a nullable preserved branch); otherwise the fix does not fire.names. A branch that drops a property is not treated as the preserved object, so real removals stay
breaking.
The approach mirrors the existing
schemaComparisonViewForSimpleAllOfObjectandschemasUseEquivalentSimpleScalarUnionhelpers, extending the same "an equivalent restructure is notbreaking" idea to
oneOf/anyOfand$refbranches. All new logic is self-contained(
preservedCompositionBranchViewplus small predicates); the only changes to existing code are twocall-site tweaks in
CompareSchemas.Tests
Added to
what-changed/model/schema_test.go:TestCompareSchemas_InlineObjectWrappedInOneOfRefIsNotBreaking— identical hoist intooneOf: [$ref, null]⇒ no changes.TestCompareSchemas_ObjectWidenedIntoAnyOfIsNotBreaking—anyOfwidening ⇒ 0 breaking.TestCompareSchemas_WrappedRefWithInnerWideningSurfacesNonBreakingChange— hoist + inner enum widening ⇒ change surfaced, 0 breaking.TestCompareSchemas_AnyOfUnwrappedToObjectStaysBreaking— reverse direction stays breaking.TestCompareSchemas_ObjectReplacedByNonPreservingAnyOfStaysBreaking— non-preserving replacement stays breaking.TestCompareSchemas_WrapDroppingNullabilityStaysBreaking— dropped nullability stays breaking.The full
what-changed/...suite passes.Note / possible follow-up
When the preserved branch is used as the comparison view, an annotation placed on the wrapper node
rather than the branch (e.g.
writeOnlyordescriptionon the composition itself) is comparedagainst the branch and can show as non-breaking add/remove noise. This does not affect
breaking-change detection. Happy to refine the handling if you'd prefer the wrapper-level keywords be
carried onto the comparison view.