encoding/jsonschema: fix oneOf with unconstrained elements

The existing logic incorrectly elided top elements and did not include a matchN when there were potentially multiple matches and no non-type constraints in the elements. Fixes #3586 Signed-off-by: Roger Peppe <[email protected]> Change-Id: Ieb591f5049246c0b6bf6fa39744256832c386a6f Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1204511 Reviewed-by: Daniel Martí <[email protected]> TryBot-Result: CUEcueckoo <[email protected]>
cue-lang · Nov 25, 2024 · 956a558 · 956a558
1 parent 15f57d8
commit 956a558
Show file tree

Hide file tree

Showing 9 changed files with 46 additions and 104 deletions.
diff --git a/encoding/jsonschema/constraints_combinator.go b/encoding/jsonschema/constraints_combinator.go
@@ -115,7 +115,7 @@ func constraintAnyOf(key string, n cue.Value, s *state) {
 func constraintOneOf(key string, n cue.Value, s *state) {
 	var types cue.Kind
 	var knownTypes cue.Kind
-	hasSome := false
+	needsConstraint := false
 	items := s.listItems("oneOf", n, false)
 	if len(items) == 0 {
 		s.errf(n, "oneOf requires at least one subschema")
@@ -128,22 +128,24 @@ func constraintOneOf(key string, n cue.Value, s *state) {
 			// Nothing is allowed; omit
 			continue
 		}
-		types |= sub.allowedTypes
 
 		// TODO: make more finegrained by making it two pass.
 		if sub.hasConstraints() {
-			hasSome = true
-		}
-
-		if !isAny(x) {
-			knownTypes |= sub.knownTypes
-			a = append(a, x)
+			needsConstraint = true
+		} else if (types & sub.allowedTypes) != 0 {
+			// If there's overlap between the
+			// uncontrained elements, we'll definitely
+			// need to add a constraint.
+			needsConstraint = true
 		}
+		types |= sub.allowedTypes
+		knownTypes |= sub.knownTypes
+		a = append(a, x)
 	}
 	// TODO if there are no elements in the oneOf, validation
 	// should fail.
 	s.allowedTypes &= types
-	if len(a) > 0 && hasSome {
+	if len(a) > 0 && needsConstraint {
 		s.knownTypes &= knownTypes
 		if len(a) == 1 {
 			// Only one possibility. Use that.

diff --git a/encoding/jsonschema/external_teststats.txt b/encoding/jsonschema/external_teststats.txt
@@ -1,13 +1,13 @@
 # Generated by CUE_UPDATE=1 go test. DO NOT EDIT
 v2:
 	schema extract (pass / total): 1013 / 1363 = 74.3%
-	tests (pass / total): 3659 / 4803 = 76.2%
-	tests on extracted schemas (pass / total): 3659 / 3865 = 94.7%
+	tests (pass / total): 3676 / 4803 = 76.5%
+	tests on extracted schemas (pass / total): 3676 / 3865 = 95.1%
 
 v3:
 	schema extract (pass / total): 1013 / 1363 = 74.3%
-	tests (pass / total): 3649 / 4803 = 76.0%
-	tests on extracted schemas (pass / total): 3649 / 3865 = 94.4%
+	tests (pass / total): 3666 / 4803 = 76.3%
+	tests on extracted schemas (pass / total): 3666 / 3865 = 94.9%
 
 Optional tests
 

diff --git a/encoding/jsonschema/testdata/external/tests/draft2019-09/oneOf.json b/encoding/jsonschema/testdata/external/tests/draft2019-09/oneOf.json
@@ -81,11 +81,7 @@
 			{
 				"description": "any value is invalid",
 				"data": "foo",
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},
@@ -121,11 +117,7 @@
 			{
 				"description": "any value is invalid",
 				"data": "foo",
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},
@@ -143,11 +135,7 @@
 			{
 				"description": "any value is invalid",
 				"data": "foo",
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},
@@ -231,11 +219,7 @@
 			{
 				"description": "both valid - invalid",
 				"data": 123,
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},

diff --git a/encoding/jsonschema/testdata/external/tests/draft2020-12/oneOf.json b/encoding/jsonschema/testdata/external/tests/draft2020-12/oneOf.json
@@ -81,11 +81,7 @@
 			{
 				"description": "any value is invalid",
 				"data": "foo",
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},
@@ -121,11 +117,7 @@
 			{
 				"description": "any value is invalid",
 				"data": "foo",
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},
@@ -143,11 +135,7 @@
 			{
 				"description": "any value is invalid",
 				"data": "foo",
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},
@@ -231,11 +219,7 @@
 			{
 				"description": "both valid - invalid",
 				"data": 123,
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},

diff --git a/encoding/jsonschema/testdata/external/tests/draft4/oneOf.json b/encoding/jsonschema/testdata/external/tests/draft4/oneOf.json
@@ -143,11 +143,7 @@
 			{
 				"description": "both valid - invalid",
 				"data": 123,
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},

diff --git a/encoding/jsonschema/testdata/external/tests/draft6/oneOf.json b/encoding/jsonschema/testdata/external/tests/draft6/oneOf.json
@@ -78,11 +78,7 @@
 			{
 				"description": "any value is invalid",
 				"data": "foo",
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},
@@ -116,11 +112,7 @@
 			{
 				"description": "any value is invalid",
 				"data": "foo",
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},
@@ -137,11 +129,7 @@
 			{
 				"description": "any value is invalid",
 				"data": "foo",
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},
@@ -223,11 +211,7 @@
 			{
 				"description": "both valid - invalid",
 				"data": 123,
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},

diff --git a/encoding/jsonschema/testdata/external/tests/draft7/oneOf.json b/encoding/jsonschema/testdata/external/tests/draft7/oneOf.json
@@ -78,11 +78,7 @@
 			{
 				"description": "any value is invalid",
 				"data": "foo",
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},
@@ -116,11 +112,7 @@
 			{
 				"description": "any value is invalid",
 				"data": "foo",
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},
@@ -137,11 +129,7 @@
 			{
 				"description": "any value is invalid",
 				"data": "foo",
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},
@@ -223,11 +211,7 @@
 			{
 				"description": "both valid - invalid",
 				"data": 123,
-				"valid": false,
-				"skip": {
-					"v2": "unexpected success",
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},

diff --git a/encoding/jsonschema/testdata/txtar/oneof_with_unconstrained-1.txtar b/encoding/jsonschema/testdata/txtar/oneof_with_unconstrained-1.txtar
@@ -1,5 +1,4 @@
 Issue 3586, first test case.
-TODO fix this
 
 -- schema.json --
 {
@@ -9,6 +8,11 @@ TODO fix this
     ]
 }
 -- out/decode/extract --
-_
--- test/string.json --
+matchN(1, [_, _])
+-- test/err-string.json --
 "something"
+-- out/decode/testerr/err-string --
+invalid value "something" (does not satisfy matchN): 2 matched, expected 1:
+    generated.cue:1:1
+    generated.cue:1:8
+    test/err-string.json:1:1
diff --git a/encoding/jsonschema/testdata/txtar/oneof_with_unconstrained-2.txtar b/encoding/jsonschema/testdata/txtar/oneof_with_unconstrained-2.txtar
@@ -1,5 +1,4 @@
 Issue 3586, second test case.
-TODO fix this
 
 -- schema.json --
 {
@@ -16,6 +15,11 @@ TODO fix this
     ]
 }
 -- out/decode/extract --
-number | string
--- test/string.json --
+matchN(1, [string, number | string])
+-- test/err-string.json --
 "something"
+-- out/decode/testerr/err-string --
+invalid value "something" (does not satisfy matchN): 2 matched, expected 1:
+    generated.cue:1:1
+    generated.cue:1:8
+    test/err-string.json:1:1