From c22316fafc67564bc6be589ac984fb2f16059c5d Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 06:38:32 +0300 Subject: [PATCH] fix(compilers/openapi): keep bounds beside a preserved union MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A schema that co-declares oneOf/anyOf with structural keywords lowers its structural body and keeps the union verbatim. When that body reduces to a shared primitive, the position hoists an alias so the preserved union has a node of its own to attach to — and that alias was interned with nil constraints, so every value constraint written at the position was dropped in silence: A: {type: string, minLength: 3, oneOf: [{minLength: 1}, {minLength: 2}]} lowered to a scalar with no constraints at all, and no diagnostic named minLength. Ownership is what made it terminal. hoistDeclarationHome resolves to whatever node the pointer already owns and returns early, so the fallback that carries constraints for every other position never ran. A position that also wrote an unhomed keyword happened to be rescued, because preserveUnhomedKeywords interns its alias through the constraint-reading path first — which is why this went unnoticed. Read the position's constraints and hand them to internAlias, as lowerComponentSchema, hoistDeclarationHome, hoistSubSchema and preserveUnhomedKeywords all already do. Their diagnostics are routed too, so the co-declared-bound reconciliation now reports at these positions, where before nothing read the bounds to report on. The corpus had the same blind spot as the code: no committed spec put a constraint beside a union kept verbatim, so nothing moved when the drop was introduced. allof-oneof-cooccurrence gains that shape beside the model-body case it already carried. --- compilers/openapi/conformance_test.go | 11 ++++ .../openapi/internal/schema/compose_test.go | 66 +++++++++++++++++++ compilers/openapi/internal/schema/schema.go | 8 ++- .../allof-oneof-cooccurrence.golden.json | 53 ++++++++++++++- .../openapi/allof-oneof-cooccurrence.yaml | 11 ++++ 5 files changed, 146 insertions(+), 3 deletions(-) diff --git a/compilers/openapi/conformance_test.go b/compilers/openapi/conformance_test.go index 17ae7d9..8d07828 100644 --- a/compilers/openapi/conformance_test.go +++ b/compilers/openapi/conformance_test.go @@ -641,6 +641,9 @@ func assertAllOfRequiredOnly(t *testing.T, doc *ir.Document, _ []ir.Diagnostic) // assertAllOfOneOfCooccurrence pins both halves of the co-declared composition // rule: §4.3 distributes a union whose branches all name referents, and §4.8 // keeps one with an inline branch verbatim rather than distributing it halfway. +// The verbatim half is covered over a model body, which owns a node already, and +// over a scalar one, which does not — there the alias hoisted for the union +// carries what the position wrote beside it too. // The outside reference to a branch pointer pins the third thing: a composed // variant is Morphic's own node, so it cannot be taken by, or take from, a $ref. func assertAllOfOneOfCooccurrence(t *testing.T, doc *ir.Document, _ []ir.Diagnostic) { @@ -662,6 +665,14 @@ func assertAllOfOneOfCooccurrence(t *testing.T, doc *ir.Document, _ []ir.Diagnos require.True(t, ok, "and the union it could not absorb survives beside it") assert.Equal(t, ir.ReasonDegradedLowering, entry.Reason) + bounded, ok := doc.Types[namedID("BoundedKinds")].(*ir.Scalar) + require.True(t, ok, "a body that is not a model reduces to a shared primitive and hoists an alias") + _, ok = bounded.Unmodeled["openapi:oneOf"] + require.True(t, ok, "which is the node the kept union sits on") + require.NotNil(t, bounded.Constraints, "and the bounds written beside the union sit on it too") + require.NotNil(t, bounded.Constraints.MinLength) + assert.Equal(t, int64(3), *bounded.Constraints.MinLength) + outsider, ok := doc.Types[namedID("Outsider")].(*ir.Model) require.True(t, ok) require.Len(t, outsider.Properties, 1) diff --git a/compilers/openapi/internal/schema/compose_test.go b/compilers/openapi/internal/schema/compose_test.go index a44e5d0..ee48612 100644 --- a/compilers/openapi/internal/schema/compose_test.go +++ b/compilers/openapi/internal/schema/compose_test.go @@ -1959,6 +1959,72 @@ func TestOneOf_CoDeclaredNotDistributedReasons(t *testing.T) { "each declined shape is reported once; got %+v", diags) } +// TestOneOf_CoDeclaredKeepsTheBoundsWrittenBesideIt pins that keeping a union +// verbatim does not cost the position the value constraints written beside it. +// The alias exists so the union attaches to a node this pointer owns rather than +// to the shared primitive the body reduced to, and owning a node is what stops +// hoistDeclarationHome hoisting the alias that would otherwise carry the bounds +// — so this alias has to carry them itself, as every other hoist here does +// (GitHub #343). +// +// Both routes to the verbatim lowering hoist the same alias, so both are covered: +// the bounds went the same way whichever reason kept the union. The third case +// covers what reading them also produces — bounds nothing reads are bounds +// nothing can report on, so the co-declared-bound reconciliation reached no such +// position until now. +func TestOneOf_CoDeclaredKeepsTheBoundsWrittenBesideIt(t *testing.T) { + t.Parallel() + three := int64(3) + ten, five := ir.BigVal("10"), ir.BigVal("5") + cases := []struct { + name, schemas string + reason ir.UnmodeledReason + want ir.Constraints + wantDiag string + }{ + { + name: "a validation-only union", + schemas: " A: {type: string, minLength: 3, oneOf: [{minLength: 1}, {minLength: 2}]}\n", + reason: ir.ReasonValidationOnly, + want: ir.Constraints{MinLength: &three}, + }, + { + name: "a union kept as a degraded lowering", + schemas: " A: {type: number, minimum: 10, multipleOf: 5, oneOf: [{type: string}, {type: integer}]}\n", + reason: ir.ReasonDegradedLowering, + want: ir.Constraints{Min: &ten, MultipleOf: &five}, + }, + { + name: "co-declared bounds beside a union", + schemas: " A: {type: number, minimum: 10, exclusiveMinimum: 0, oneOf: [{minLength: 1}, {minLength: 2}]}\n", + reason: ir.ReasonValidationOnly, + want: ir.Constraints{Min: &ten}, + wantDiag: "kept minimum as the tighter of the two", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + doc, diags := lowerSpec(t, componentSpec(tc.schemas)) + requireNoErrorDiags(t, diags) + + sc, ok := typeByName(doc, "A").(*ir.Scalar) + require.True(t, ok, "the preserved union hoists an alias over the shared primitive") + entry, ok := sc.Unmodeled["openapi:oneOf"] + require.True(t, ok, "and keeps the union on it") + assert.Equal(t, tc.reason, entry.Reason) + require.NotNil(t, sc.Constraints, "while keeping the bounds written beside it") + assert.Empty(t, cmp.Diff(tc.want, *sc.Constraints)) + if tc.wantDiag == "" { + return + } + assert.Contains(t, + diagMessageAt(t, diags, diag.DegradedConstruct, ir.SeverityInfo, "/components/schemas/A"), + tc.wantDiag, "reading the bounds is what reports on them") + }) + } +} + // TestUnionCombinators_PassedOverBranchSetIsKept covers the preference nothing // used to record (GitHub #35). unionBranches takes oneOf whenever it is written // and falls back to anyOf only when it is not, so a schema declaring both lost diff --git a/compilers/openapi/internal/schema/schema.go b/compilers/openapi/internal/schema/schema.go index d06d1eb..a44918e 100644 --- a/compilers/openapi/internal/schema/schema.go +++ b/compilers/openapi/internal/schema/schema.go @@ -380,8 +380,12 @@ func lowerBesideUnmodeledUnion(c lowering.Ctx, ts *compile.Types, anchors *Ancho if got, _ := ts.Lookup(pointer); got != inner { // The structural body reduced to a shared/aliased target; hoist an alias // so the preserved union attaches to a node this pointer owns, never to a - // shared primitive. - owner = internAlias(c, ts, pointer, hint, ir.TypeRef{Target: inner}, nil) + // shared primitive. The alias carries the position's value constraints for + // the reason hoistByteScalar records: owning the node is what stops + // hoistDeclarationHome hoisting the alias that would otherwise carry them. + cons, consDiags := schemaConstraints(c, s, pointer) + diags = append(diags, consDiags...) + owner = internAlias(c, ts, pointer, hint, ir.TypeRef{Target: inner}, cons) } return owner, append(diags, preserveUnionSiblings(c, ts, owner, s, pointer, reason, why)...) } diff --git a/testdata/conformance/openapi/allof-oneof-cooccurrence.golden.json b/testdata/conformance/openapi/allof-oneof-cooccurrence.golden.json index c43e993..80c2b91 100644 --- a/testdata/conformance/openapi/allof-oneof-cooccurrence.golden.json +++ b/testdata/conformance/openapi/allof-oneof-cooccurrence.golden.json @@ -251,6 +251,48 @@ "positional": false, "inputOnly": false }, + "t/openapi/components/schemas/BoundedKinds": { + "kind": "scalar", + "id": "t/openapi/components/schemas/BoundedKinds", + "name": { + "source": "BoundedKinds", + "canonical": "bounded_kinds" + }, + "anonymous": false, + "docs": {}, + "sensitive": false, + "unmodeled": { + "openapi:oneOf": { + "reason": "degraded_lowering", + "value": [ + { + "$ref": "#/components/schemas/A" + }, + { + "type": "string" + } + ], + "provenance": { + "source": 0, + "pointer": "/components/schemas/BoundedKinds/oneOf" + } + } + }, + "provenance": { + "source": 0, + "pointer": "/components/schemas/BoundedKinds" + }, + "base": { + "target": "t/prim/string", + "nullable": false + }, + "constraints": { + "exclusiveMin": false, + "exclusiveMax": false, + "minLength": 3, + "uniqueItems": false + } + }, "t/openapi/components/schemas/Combo": { "kind": "union", "id": "t/openapi/components/schemas/Combo", @@ -542,13 +584,22 @@ "source": 0, "pointer": "/components/schemas/MixedKinds" } + }, + { + "severity": "info", + "code": "openapi/degraded-construct", + "message": "oneOf/anyOf co-declared with structural keywords intersects with them, and the body is not a model, so it carries no composition to distribute into; union branches kept verbatim under Unmodeled", + "provenance": { + "source": 0, + "pointer": "/components/schemas/BoundedKinds" + } } ], "sources": [ { "format": "openapi@3.1", "path": "allof-oneof-cooccurrence.yaml", - "hash": "cb7fb6bc613222b0211fc363b9028e7739b44a1d14b04f74cf709b8d603655f2" + "hash": "4befdad1ab433d53b430993675f127b586c7ec0bfb765dca31c66d5c819b4c10" } ] } diff --git a/testdata/conformance/openapi/allof-oneof-cooccurrence.yaml b/testdata/conformance/openapi/allof-oneof-cooccurrence.yaml index f085da9..22b77c2 100644 --- a/testdata/conformance/openapi/allof-oneof-cooccurrence.yaml +++ b/testdata/conformance/openapi/allof-oneof-cooccurrence.yaml @@ -32,6 +32,17 @@ components: oneOf: - {$ref: '#/components/schemas/A'} - {type: string} + # The same conjunction over a body that is not a model. It reduces to the + # shared string primitive, so the position hoists an alias for the kept union + # to sit on — and that alias has to carry the bounds written here too, since + # owning a node is what stops the declaration-home fallback carrying them + # (GitHub #343). + BoundedKinds: + type: string + minLength: 3 + oneOf: + - {$ref: '#/components/schemas/A'} + - {type: string} # A branch pointer denotes the branch schema, and a reference to it must get # that schema — not the variant Morphic composes for the same branch, which # is a node of its own with no pointer a $ref can name.