diff --git a/encoding/jsonschema/generate.go b/encoding/jsonschema/generate.go index e522c6d86..5f651bcdf 100644 --- a/encoding/jsonschema/generate.go +++ b/encoding/jsonschema/generate.go @@ -304,6 +304,21 @@ type generator struct { // unique ensures that all items are comparable with // simple equality. unique *uniqueItems + + // redirectFrom and redirectTo are set temporarily when inlining + // a non-definition into a closed definition. References to + // redirectFrom are resolved as redirectTo instead, so that + // recursive self-references within the inlined body generate + // $ref to the enclosing definition rather than the open + // non-definition. + // + // When a redirect is active, other non-definitions encountered + // in a closed context are also inlined at the property level + // (handling mutual recursion). inliningNonDef prevents unbounded + // recursion by limiting property-level inlining to one level deep. + redirectFrom *CUERef + redirectTo *CUERef + inliningNonDef bool } // Note this type definition is defined further away from [siblings] @@ -373,6 +388,17 @@ func (g *generator) makeItem0(v cue.Value, mode closedMode) item { Path: path, } if actualRef, _, ok := g.defs.Get2(ref); ok { + if g.redirectFrom != nil && mode != open { + if (cueRefHasher{}).Equal(ref, g.redirectFrom) { + return g.redirectTo + } + if !isDefinition(path) && !g.inliningNonDef { + g.inliningNonDef = true + result := g.makeItem0(v, mode) + g.inliningNonDef = false + return result + } + } return actualRef } g.defs.Set(ref, internItem{}) // Prevent infinite loops on cycles. @@ -380,7 +406,23 @@ func (g *generator) makeItem0(v cue.Value, mode closedMode) item { if isDefinition(path) { defMode = closedRecursively } - g.defs.Set(ref, g.makeItem(v, defMode)) + defItem := g.makeItem(v, defMode) + if defMode != open { + if innerRef, ok := defItem.Value().(*CUERef); ok && !isDefinition(innerRef.Path) { + savedFrom, savedTo := g.redirectFrom, g.redirectTo + g.redirectFrom = innerRef + g.redirectTo = ref + defItem = g.makeItem(innerRef.Inst.LookupPath(innerRef.Path), defMode) + g.redirectFrom, g.redirectTo = savedFrom, savedTo + } + } + g.defs.Set(ref, defItem) + if g.redirectFrom != nil && mode != open && !isDefinition(path) && !g.inliningNonDef { + g.inliningNonDef = true + result := g.makeItem0(v, mode) + g.inliningNonDef = false + return result + } return ref case cue.AndOp: if v.Kind() == cue.StructKind { @@ -414,6 +456,11 @@ func (g *generator) makeItem0(v cue.Value, mode closedMode) item { return &itemAnyOf{ elems: mapSlice(args, func(v cue.Value) internItem { return g.makeItem(v, mode) }), } + case cue.SpreadOp: + // SpreadOp opens its operand (e.g. #T...). The struct processing + // after this switch handles it correctly by iterating the spread's fields. + // Note: the reason this works is because this causes the logic + // to ignore whether the spread value is a reference or not. case cue.RegexMatchOp, cue.NotRegexMatchOp: re, err := args[0].String() @@ -966,8 +1013,12 @@ func (g *generator) makeStructItem(v cue.Value, mode closedMode) item { } hasUniversalConstraint := false for v := range valueConjuncts(v) { - pkg, _ := v.ReferencePath() - if pkg.Exists() || v.Kind() != cue.StructKind { + pkg, path := v.ReferencePath() + if pkg.Exists() && mode != open && !isDefinition(path) && v.Kind() == cue.StructKind { + // In a closed context, inline non-definition references so + // their properties become local to additionalProperties. + v = pkg.LookupPath(path) + } else if pkg.Exists() || v.Kind() != cue.StructKind { // This conjunct is a reference or some other non-struct literal. // Let's keep it as such. allOf.elems = append(allOf.elems, g.makeItem(v, open)) @@ -1096,7 +1147,12 @@ func (g *generator) makeStructItem(v cue.Value, mode closedMode) item { delete(props.patternProperties, "") } if mode != open && !g.cfg.ExplicitOpen && props.additionalProperties.Value() == nil { - props.additionalProperties = g.unique.intern(&itemFalse{}) + // Note: additionalProperties is lexical (applies only to fields + // it's directly adjacent too) so it only makes sense to apply it + // when the struct is genuinely empty or there are properties locally. + if len(props.properties) > 0 || len(allOf.elems) == 0 { + props.additionalProperties = g.unique.intern(&itemFalse{}) + } } props.required = slices.Sorted(maps.Keys(required)) hasObjectConstraints := diff --git a/encoding/jsonschema/testdata/generate/closedness_nondef.txtar b/encoding/jsonschema/testdata/generate/closedness_nondef.txtar index 14c189017..d9fcb7ffb 100644 --- a/encoding/jsonschema/testdata/generate/closedness_nondef.txtar +++ b/encoding/jsonschema/testdata/generate/closedness_nondef.txtar @@ -1,10 +1,6 @@ # Tests for non-definition reference closedness. # When a definition's body is solely a reference to a non-definition, # the definition should still be closed. -# -# BUG: WrapsHidden and WrapsShared should have additionalProperties: false -# because they're definitions. Currently they're just a $ref to the -# open non-definition, losing closedness. -- test.cue -- package test @@ -35,12 +31,9 @@ package datatest hiddenValid: { data: {t01: {h: 1}} } -// BUG: this should not be accepted. #WrapsHidden is a definition -// wrapping an open non-definition. The definition should be closed, -// but the generated schema just uses $ref to the open non-definition, -// losing closedness. hiddenExtra: { - data: {t01: {h: 1, extra: true}} + data: {t01: {h: 1, extra: true}} + error: true } sharedOpenExtra: { data: {t02open: {v: 1, extra: true}} @@ -48,18 +41,15 @@ sharedOpenExtra: { sharedClosedValid: { data: {t02closed: {v: 1}} } -// BUG: this should not be accepted (same issue as hiddenExtra). sharedClosedExtra: { - data: {t02closed: {v: 1, extra: true}} + data: {t02closed: {v: 1, extra: true}} + error: true } conjunctionOpenExtra: { data: {t03open: {h: 1, v: 1, extra: true}} } -// BUG: this correctly rejects extra properties, but the generated schema -// is structurally wrong: additionalProperties: false sits alongside $ref -// with no local properties, rejecting all fields rather than only extra ones. conjunctionClosedExtra: { - data: {t03closed: {h: 1, v: 1, extra: true}} + data: {t03closed: {h: 1, v: 1, extra: true}} error: true } conjunctionOfDisjunctionsOpenExtra: { @@ -78,13 +68,16 @@ conjunctionOfDisjunctionsClosedExtra: { $schema: "https://json-schema.org/draft/2020-12/schema" $defs: { WrapsConjunction: { - allOf: [{ - $ref: "#/$defs/_hidden" - }, { - type: "object" - additionalProperties: false - $ref: "#/$defs/_shared" - }] + type: "object" + additionalProperties: false + properties: { + h: { + type: "integer" + } + v: { + type: "integer" + } + } } WrapsConjunctionOfDisjunctions: { allOf: [{ @@ -112,10 +105,22 @@ conjunctionOfDisjunctionsClosedExtra: { }] } WrapsHidden: { - $ref: "#/$defs/_hidden" + type: "object" + additionalProperties: false + properties: { + h: { + type: "integer" + } + } } WrapsShared: { - $ref: "#/$defs/_shared" + type: "object" + additionalProperties: false + properties: { + v: { + type: "integer" + } + } } "_hidden": { type: "object" @@ -188,24 +193,16 @@ conjunctionOfDisjunctionsClosedExtra: { } -- out/generate-v3/hiddenValid -- -- out/generate-v3/hiddenExtra -- +hiddenExtra.data.t01.extra: field not allowed: + ./datatest/testdata.cue:6:22 -- out/generate-v3/sharedOpenExtra -- -- out/generate-v3/sharedClosedValid -- -- out/generate-v3/sharedClosedExtra -- +sharedClosedExtra.data.t02closed.extra: field not allowed: + ./datatest/testdata.cue:16:28 -- out/generate-v3/conjunctionClosedExtra -- -conjunctionClosedExtra.data.t03closed: invalid value {h:1,v:1,extra:true} (does not satisfy matchN): 1 matched, expected 2: - 1:88 - ./datatest/testdata.cue:29:21 - 1:792 conjunctionClosedExtra.data.t03closed.extra: field not allowed: - 1:88 - ./datatest/testdata.cue:29:41 -conjunctionClosedExtra.data.t03closed.h: field not allowed: - 1:88 - ./datatest/testdata.cue:29:25 -conjunctionClosedExtra.data.t03closed.v: field not allowed: - 1:88 - ./datatest/testdata.cue:29:31 - 1:592 + ./datatest/testdata.cue:23:33 -- out/generate-v3/conjunctionOpenExtra -- -- out/generate-v3/conjunctionOfDisjunctionsClosedExtra -- -- out/generate-v3/conjunctionOfDisjunctionsOpenExtra -- diff --git a/encoding/jsonschema/testdata/generate/closedness_recursive.txtar b/encoding/jsonschema/testdata/generate/closedness_recursive.txtar index 496c40afc..131a9d8d9 100644 --- a/encoding/jsonschema/testdata/generate/closedness_recursive.txtar +++ b/encoding/jsonschema/testdata/generate/closedness_recursive.txtar @@ -1,8 +1,4 @@ # Tests for closedness with recursive references. -# -# BUG: WrapsNode should have additionalProperties: false because -# it's a definition. Currently it's just a $ref to the open -# non-definition. -- test.cue -- package test @@ -93,30 +89,28 @@ nodeExtra: { wrapsNodeValid: { data: {t05: {value: 1, next: {value: 2}}} } -// BUG: this should not be accepted. #WrapsNode is a definition -// wrapping an open recursive non-definition. The definition should -// be closed. wrapsNodeExtra: { - data: {t05: {value: 1, extra: true}} + data: {t05: {value: 1, extra: true}} + error: true } -// BUG: this should not be accepted because #WrapsNode is a definition -// wrapping a recursive non-definition, and nested values should be closed. wrapsNodeNextExtra: { data: {t05: {value: 1, next: {value: 2, extra: true}}} + error: true } closedLeftValid: { data: {t06: {value: 1, right: {value: "hello", left: {value: 2}}}} } -// BUG: these should not be accepted. #ClosedLeft is a definition -// wrapping open mutually-recursive non-definitions. closedLeftExtra: { data: {t06: {value: 1, extra: true}} + error: true } closedLeftRightExtra: { data: {t06: {value: 1, right: {value: "hello", extra: true}}} + error: true } closedLeftRightLeftExtra: { data: {t06: {value: 1, right: {value: "hello", left: {value: 2, extra: true}}}} + error: true } -- out/generate-v3/schema -- { @@ -147,7 +141,25 @@ closedLeftRightLeftExtra: { } } ClosedLeft: { - $ref: "#/$defs/_left" + type: "object" + additionalProperties: false + properties: { + right: { + type: "object" + additionalProperties: false + properties: { + left: { + $ref: "#/$defs/ClosedLeft" + } + value: { + type: "string" + } + } + } + value: { + type: "integer" + } + } } Expr: { anyOf: [{ @@ -184,7 +196,16 @@ closedLeftRightLeftExtra: { } } WrapsNode: { - $ref: "#/$defs/_node" + type: "object" + additionalProperties: false + properties: { + next: { + $ref: "#/$defs/WrapsNode" + } + value: { + type: "integer" + } + } } "_left": { type: "object" @@ -260,21 +281,31 @@ mutualDeepExtra.data.t02.b.extra: field not allowed: -- out/generate-v3/exprExtra -- exprExtra.data.t03: conflicting values {op:"+",left:1,right:2,extra:true} and int (mismatched types struct and int): ./datatest/testdata.cue:28:14 - 1:351 - 1:1109 + 1:548 + 1:1406 exprExtra.data.t03: invalid value {op:"+",left:1,right:2,extra:true} (does not satisfy matchN): 0 matched, expected >=1: - 1:341 + 1:538 ./datatest/testdata.cue:28:14 - 1:1109 + 1:1406 exprExtra.data.t03.extra: field not allowed: - 1:341 + 1:538 ./datatest/testdata.cue:28:50 -- out/generate-v3/nodeValid -- -- out/generate-v3/nodeExtra -- -- out/generate-v3/wrapsNodeValid -- -- out/generate-v3/wrapsNodeExtra -- +wrapsNodeExtra.data.t05.extra: field not allowed: + ./datatest/testdata.cue:41:26 +-- out/generate-v3/wrapsNodeNextExtra -- +wrapsNodeNextExtra.data.t05.next.extra: field not allowed: + ./datatest/testdata.cue:45:42 -- out/generate-v3/closedLeftExtra -- +closedLeftExtra.data.t06.extra: field not allowed: + ./datatest/testdata.cue:52:25 -- out/generate-v3/closedLeftRightExtra -- +closedLeftRightExtra.data.t06.right.extra: field not allowed: + ./datatest/testdata.cue:56:49 -- out/generate-v3/closedLeftRightLeftExtra -- +closedLeftRightLeftExtra.data.t06.right.left.extra: field not allowed: + ./datatest/testdata.cue:60:66 -- out/generate-v3/closedLeftValid -- --- out/generate-v3/wrapsNodeNextExtra -- diff --git a/encoding/jsonschema/testdata/generate/closedness_root.txtar b/encoding/jsonschema/testdata/generate/closedness_root.txtar index 6747d36da..1c9114f23 100644 --- a/encoding/jsonschema/testdata/generate/closedness_root.txtar +++ b/encoding/jsonschema/testdata/generate/closedness_root.txtar @@ -1,13 +1,9 @@ # Tests for root-level definition reference closedness. # # Issue 4356: when the root value is solely a reference to a definition, -# additionalProperties: false and type: "object" should not appear at -# the root because they conflict with the $ref (additionalProperties -# only considers properties in the same schema object, and the root -# has none). -# -# BUG(4356): the type and additionalProperties keywords should -# not be present at the root; the root should be solely a $ref. +# additionalProperties: false should not appear at the root because it +# conflicts with the $ref (additionalProperties only considers properties +# in the same schema object, and the root has none). -- test.cue -- package test @@ -34,7 +30,6 @@ package test } } } - type: "object" - additionalProperties: false - $ref: "#/$defs/T" + type: "object" + $ref: "#/$defs/T" } diff --git a/encoding/jsonschema/testdata/generate/closedness_root_required.txtar b/encoding/jsonschema/testdata/generate/closedness_root_required.txtar index 5c6503a2b..45e4b0d01 100644 --- a/encoding/jsonschema/testdata/generate/closedness_root_required.txtar +++ b/encoding/jsonschema/testdata/generate/closedness_root_required.txtar @@ -1,11 +1,6 @@ # Tests for root-level definition reference with required fields. # # Issue 4356: same as closedness_root but with required fields. -# -# BUG(4356): the type and additionalProperties keywords should -# not be present at the root; the root should be solely a $ref. -# -#brokenRoundTrip -- test.cue -- package test @@ -33,7 +28,6 @@ package test required: ["a", "b"] } } - type: "object" - additionalProperties: false - $ref: "#/$defs/T" + type: "object" + $ref: "#/$defs/T" }