Skip to content

fix(what-changed): don't flag object→composition wrap as breaking - #611

Open
corbinbs wants to merge 1 commit into
pb33f:mainfrom
corbinbs:fix/schema-composition-wrap-false-positive
Open

fix(what-changed): don't flag object→composition wrap as breaking#611
corbinbs wants to merge 1 commit into
pb33f:mainfrom
corbinbs:fix/schema-composition-wrap-false-positive

Conversation

@corbinbs

Copy link
Copy Markdown

Summary

what-changed reports false-positive breaking changes when an object schema is refactored into
a oneOf/anyOf composition that still contains that object as one of its branches. No previously
valid value becomes invalid, so the change is not breaking, but the comparator reports the object's
properties/required as removed.

This PR teaches CompareSchemas to recognize that "wrapping" refactor and stop flagging it, while
keeping 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 $ref is never resolved, so
the properties look removed.

# before
Widget:
  type: object
  properties:
    config:
      type: [object, 'null']
      required: [mode]
      properties:
        mode: { type: integer, enum: [1, 2] }

# after — identical schema, just hoisted + referenced
Widget:
  type: object
  properties:
    config:
      oneOf:
      - $ref: '#/components/schemas/Config'
      - type: 'null'
Config:
  type: object
  required: [mode]
  properties:
    mode: { type: integer, enum: [1, 2] }
$ openapi-changes summary before.yaml after.yaml --no-logo
  - Removed ... config properties: [mode]
  - Removed ... config required: [mode]
  ❌  2 Breaking changes

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.

# before
Entry:
  type: object
  required: [kind]
  properties:
    kind: { type: string }

# after — the object is still branch 0; an array alternative was added
Entry:
  anyOf:
  - type: object
    required: [kind]
    properties:
      kind: { type: string }
  - type: array
    items: { type: object }
$ openapi-changes summary before.yaml after.yaml --no-logo
  - Removed ... Entry properties/kind
  - Removed ... Entry required/kind
  ❌  2 Breaking changes

Both reproduce on the current main (and were originally hit with openapi-changes v0.2.8 /
libopenapi v0.37.3, and still on v0.2.10).

Why it's a false positive

Everything valid before is still valid:

  • Case 1: oneOf: [Config, null] is exactly the old nullable object — the properties moved behind
    the $ref, they were not removed.
  • Case 2: the old object is still accepted as the first anyOf branch; an alternative was added.

The default rules already treat adding a oneOf/anyOf as non-breaking. The problem is that the
same restructure is also counted as a removal of the object's properties/required, so it gets
reported 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 a oneOf/anyOf, the field-by-field diff sees the
top-level properties/required disappear (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
$ref branch is keyed by its ref string rather than by resolved content.

The fix

CompareSchemas now detects the wrapping direction and, when found, compares the old object
against the preserved branch directly:

  • Trigger: old side is a plain object with properties; new side is a pure oneOf/anyOf
    wrapper whose branches — resolving local $refs — contain exactly one branch that preserves
    all 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.
  • Genuine changes still surface: because the old object is compared against the preserved branch
    recursively, a real change inside the branch (e.g. a widened enum, an added property) is still
    reported — as breaking or non-breaking per the normal rules. Only the spurious top-level
    properties/required/type removal is suppressed.

It stays conservative via three guards:

  • Directional. Only old-plain → new-composition is matched. The reverse (a composition
    collapsing to a single schema, which can drop alternatives) is left breaking.
  • Nullability preserved. If the old object accepted null, the new composition must still accept
    it (a null branch, or a nullable preserved branch); otherwise the fix does not fire.
  • Unambiguous preservation. Exactly one branch may preserve all of the old object's property
    names. A branch that drops a property is not treated as the preserved object, so real removals stay
    breaking.

The approach mirrors the existing schemaComparisonViewForSimpleAllOfObject and
schemasUseEquivalentSimpleScalarUnion helpers, extending the same "an equivalent restructure is not
breaking" idea to oneOf/anyOf and $ref branches. All new logic is self-contained
(preservedCompositionBranchView plus small predicates); the only changes to existing code are two
call-site tweaks in CompareSchemas.

Tests

Added to what-changed/model/schema_test.go:

  • TestCompareSchemas_InlineObjectWrappedInOneOfRefIsNotBreaking — identical hoist into oneOf: [$ref, null] ⇒ no changes.
  • TestCompareSchemas_ObjectWidenedIntoAnyOfIsNotBreakinganyOf widening ⇒ 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. writeOnly or description on the composition itself) is compared
against 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.

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.75%. Comparing base (0837c9b) to head (619b137).

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     
Flag Coverage Δ
unittests 99.75% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@corbinbs
corbinbs force-pushed the fix/schema-composition-wrap-false-positive branch from db47110 to 5face2e Compare July 23, 2026 22:09
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.
@corbinbs
corbinbs force-pushed the fix/schema-composition-wrap-false-positive branch from 5face2e to 619b137 Compare July 23, 2026 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant