encoding/jsonschema: better constraints for additionalProperties a…

…nd `patternProperties` There are a couple of issues with the jsonschema-generated code for `additionalProperties` and `patternProperties`. It generates an extra regexp to exclude fields that have been explicitly defined, but it does not quote regexp metacharacters in that pattern (#3551). When fixing that, I also realised that it does the wrong thing for empty fields, because it does not omit the constraint when there are no fields. Changing the logic here exposed a bug (#3555) in the v2 evaluator where the following expression would not give an error, even though `"BAD)"` is not a valid regexp. The regressions in the jsonschema tests are because of that. Fixes #3551 Signed-off-by: Roger Peppe <[email protected]> Change-Id: I0e21828f679eaf39afd81eaa38d8035ea3d93c71 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1203588 Unity-Result: CUE porcuepine <[email protected]> Reviewed-by: Daniel Martí <[email protected]> TryBot-Result: CUEcueckoo <[email protected]>
cue-lang · Nov 5, 2024 · 85f5ca8 · 85f5ca8
1 parent 1300a2d
commit 85f5ca8
Show file tree

Hide file tree

Showing 13 changed files with 77 additions and 40 deletions.
diff --git a/encoding/jsonschema/constraints_object.go b/encoding/jsonschema/constraints_object.go
@@ -39,7 +39,7 @@ func constraintAdditionalProperties(key string, n cue.Value, s *state) {
 			return
 		}
 		// [!~(properties|patternProperties)]: schema
-		existing := append(s.patterns, excludeFields(obj.Elts))
+		existing := append(s.patterns, excludeFields(obj.Elts)...)
 		f := internal.EmbedStruct(ast.NewStruct(&ast.Field{
 			Label: ast.NewList(ast.NewBinExpr(token.AND, existing...)),
 			Value: s.schema(n),
@@ -87,9 +87,10 @@ func constraintPatternProperties(key string, n cue.Value, s *state) {
 		s.patterns = append(s.patterns,
 			&ast.UnaryExpr{Op: token.NMAT, X: ast.NewString(key)})
 		f := internal.EmbedStruct(ast.NewStruct(&ast.Field{
-			Label: ast.NewList(ast.NewBinExpr(token.AND,
-				&ast.UnaryExpr{Op: token.MAT, X: ast.NewString(key)},
-				existing)),
+			Label: ast.NewList(ast.NewBinExpr(
+				token.AND,
+				append([]ast.Expr{&ast.UnaryExpr{Op: token.MAT, X: ast.NewString(key)}}, existing...)...,
+			)),
 			Value: s.schema(n),
 		}))
 		ast.SetRelPos(f, token.NewSection)

diff --git a/encoding/jsonschema/decode.go b/encoding/jsonschema/decode.go
@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"math"
 	"net/url"
+	"regexp"
 	"regexp/syntax"
 	"sort"
 	"strconv"
@@ -832,33 +833,45 @@ func (s *state) listItems(name string, n cue.Value, allowEmpty bool) (a []cue.Va
 	return a
 }
 
-// excludeFields returns a CUE expression that can be used to exclude the
+// excludeFields returns either an empty slice (if decls is empty)
+// or a slice containing a CUE expression that can be used to exclude the
 // fields of the given declaration in a label expression. For instance, for
 //
 //	{ foo: 1, bar: int }
 //
-// it creates
+// it creates a slice holding the expression
 //
-//	"^(foo|bar)$"
+//	!~ "^(foo|bar)$"
 //
 // which can be used in a label expression to define types for all fields but
 // those existing:
 //
 //	[!~"^(foo|bar)$"]: string
-func excludeFields(decls []ast.Decl) ast.Expr {
-	var a []string
+func excludeFields(decls []ast.Decl) []ast.Expr {
+	if len(decls) == 0 {
+		return nil
+	}
+	var buf strings.Builder
+	first := true
+	buf.WriteString("^(")
 	for _, d := range decls {
 		f, ok := d.(*ast.Field)
 		if !ok {
 			continue
 		}
 		str, _, _ := ast.LabelName(f.Label)
 		if str != "" {
-			a = append(a, str)
+			if !first {
+				buf.WriteByte('|')
+			}
+			buf.WriteString(regexp.QuoteMeta(str))
+			first = false
 		}
 	}
-	re := fmt.Sprintf("^(%s)$", strings.Join(a, "|"))
-	return &ast.UnaryExpr{Op: token.NMAT, X: ast.NewString(re)}
+	buf.WriteString(")$")
+	return []ast.Expr{
+		&ast.UnaryExpr{Op: token.NMAT, X: ast.NewString(buf.String())},
+	}
 }
 
 func bottom() ast.Expr {

diff --git a/encoding/jsonschema/external_teststats.txt b/encoding/jsonschema/external_teststats.txt
@@ -13,8 +13,8 @@ Optional tests
 
 v2:
 	schema extract (pass / total): 230 / 274 = 83.9%
-	tests (pass / total): 1508 / 2372 = 63.6%
-	tests on extracted schemas (pass / total): 1508 / 2258 = 66.8%
+	tests (pass / total): 1498 / 2372 = 63.2%
+	tests on extracted schemas (pass / total): 1498 / 2258 = 66.3%
 
 v3:
 	schema extract (pass / total): 230 / 274 = 83.9%

diff --git a/encoding/jsonschema/testdata/external/tests/draft2019-09/optional/ecmascript-regex.json b/encoding/jsonschema/testdata/external/tests/draft2019-09/optional/ecmascript-regex.json
@@ -547,7 +547,8 @@
 				},
 				"valid": true,
 				"skip": {
-					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`:\n    generated.cue:3:36\n"
+					"v2": "\"l'ecole\": invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`\n",
+					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`:\n    generated.cue:3:25\n"
 				}
 			},
 			{
@@ -732,7 +733,8 @@
 				},
 				"valid": true,
 				"skip": {
-					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`:\n    generated.cue:3:34\n"
+					"v2": "\"42\": invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`\n",
+					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`:\n    generated.cue:3:23\n"
 				}
 			},
 			{

diff --git a/encoding/jsonschema/testdata/external/tests/draft2020-12/optional/ecmascript-regex.json b/encoding/jsonschema/testdata/external/tests/draft2020-12/optional/ecmascript-regex.json
@@ -565,7 +565,8 @@
 				},
 				"valid": true,
 				"skip": {
-					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`:\n    generated.cue:3:36\n"
+					"v2": "\"l'ecole\": invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`\n",
+					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`:\n    generated.cue:3:25\n"
 				}
 			},
 			{
@@ -750,7 +751,8 @@
 				},
 				"valid": true,
 				"skip": {
-					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`:\n    generated.cue:3:34\n"
+					"v2": "\"42\": invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`\n",
+					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`:\n    generated.cue:3:23\n"
 				}
 			},
 			{

diff --git a/encoding/jsonschema/testdata/external/tests/draft4/optional/ecmascript-regex.json b/encoding/jsonschema/testdata/external/tests/draft4/optional/ecmascript-regex.json
@@ -531,7 +531,8 @@
 				},
 				"valid": true,
 				"skip": {
-					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`:\n    generated.cue:2:36\n"
+					"v2": "\"l'ecole\": invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`\n",
+					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`:\n    generated.cue:2:25\n"
 				}
 			},
 			{
@@ -712,7 +713,8 @@
 				},
 				"valid": true,
 				"skip": {
-					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`:\n    generated.cue:2:34\n"
+					"v2": "\"42\": invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`\n",
+					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`:\n    generated.cue:2:23\n"
 				}
 			},
 			{

diff --git a/encoding/jsonschema/testdata/external/tests/draft6/optional/ecmascript-regex.json b/encoding/jsonschema/testdata/external/tests/draft6/optional/ecmascript-regex.json
@@ -531,7 +531,8 @@
 				},
 				"valid": true,
 				"skip": {
-					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`:\n    generated.cue:2:36\n"
+					"v2": "\"l'ecole\": invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`\n",
+					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`:\n    generated.cue:2:25\n"
 				}
 			},
 			{
@@ -712,7 +713,8 @@
 				},
 				"valid": true,
 				"skip": {
-					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`:\n    generated.cue:2:34\n"
+					"v2": "\"42\": invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`\n",
+					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`:\n    generated.cue:2:23\n"
 				}
 			},
 			{

diff --git a/encoding/jsonschema/testdata/external/tests/draft7/optional/ecmascript-regex.json b/encoding/jsonschema/testdata/external/tests/draft7/optional/ecmascript-regex.json
@@ -531,7 +531,8 @@
 				},
 				"valid": true,
 				"skip": {
-					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`:\n    generated.cue:2:36\n"
+					"v2": "\"l'ecole\": invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`\n",
+					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{Letter}`:\n    generated.cue:2:25\n"
 				}
 			},
 			{
@@ -712,7 +713,8 @@
 				},
 				"valid": true,
 				"skip": {
-					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`:\n    generated.cue:2:34\n"
+					"v2": "\"42\": invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`\n",
+					"v3": "invalid regexp: error parsing regexp: invalid character class range: `\\p{digit}`:\n    generated.cue:2:23\n"
 				}
 			},
 			{

diff --git a/encoding/jsonschema/testdata/txtar/issue3176.txtar b/encoding/jsonschema/testdata/txtar/issue3176.txtar
@@ -22,6 +22,6 @@ matchN(1, [{
 	...
 }, strings.MaxRunes(
 	3)]) & (string | {
-	{[=~"^x-" & !~"^()$"]: string}
+	{[=~"^x-"]: string}
 	...
 })
diff --git a/encoding/jsonschema/testdata/txtar/issue3351.txtar b/encoding/jsonschema/testdata/txtar/issue3351.txtar
@@ -75,7 +75,7 @@ _schema: {
 		[string]: _schema
 	}
 	$sort?: matchN(>=1, [_schema, [...number]])
-	{[!~"^($else|$let|$sort)$"]: #["jsone-value"]}
+	{[!~"^(\\$else|\\$let|\\$sort)$"]: #["jsone-value"]}
 
 	#: "jsone-value": matchN(1, [_schema, [..._schema]])
 
@@ -92,10 +92,12 @@ diff old new
  		[string]: _schema
  	}
 -	$sort?: _schema | [...number]
-+	$sort?: matchN(>=1, [_schema, [...number]])
- 	{[!~"^($else|$let|$sort)$"]: #["jsone-value"]}
-
+-	{[!~"^($else|$let|$sort)$"]: #["jsone-value"]}
+-
 -	#: "jsone-value": _schema | [..._schema]
++	$sort?: matchN(>=1, [_schema, [...number]])
++	{[!~"^(\\$else|\\$let|\\$sort)$"]: #["jsone-value"]}
++
 +	#: "jsone-value": matchN(1, [_schema, [..._schema]])
 
  	#: "jsone-array": [...#["jsone-value"]]
@@ -111,7 +113,7 @@ _schema: {
 		[string]: _schema
 	}
 	$sort?: matchN(>=1, [_schema, [...number]])
-	{[!~"^($else|$let|$sort)$"]: #["jsone-value"]}
+	{[!~"^(\\$else|\\$let|\\$sort)$"]: #["jsone-value"]}
 
 	#: "jsone-value": matchN(1, [_schema, [..._schema]])
 
@@ -128,10 +130,12 @@ diff old new
  		[string]: _schema
  	}
 -	$sort?: _schema | [...number]
-+	$sort?: matchN(>=1, [_schema, [...number]])
- 	{[!~"^($else|$let|$sort)$"]: #["jsone-value"]}
-
+-	{[!~"^($else|$let|$sort)$"]: #["jsone-value"]}
+-
 -	#: "jsone-value": _schema | [..._schema]
++	$sort?: matchN(>=1, [_schema, [...number]])
++	{[!~"^(\\$else|\\$let|\\$sort)$"]: #["jsone-value"]}
++
 +	#: "jsone-value": matchN(1, [_schema, [..._schema]])
 
  	#: "jsone-array": [...#["jsone-value"]]

diff --git a/encoding/jsonschema/testdata/txtar/object.txtar b/encoding/jsonschema/testdata/txtar/object.txtar
@@ -90,9 +90,9 @@ patterns?: {
 	...
 }
 patternsNoProps?: {
-	{[=~"^\\P{Lu}" & !~"^()$"]: string}
+	{[=~"^\\P{Lu}"]: string}
 
-	{[=~"^\\P{Lo}" & !~"^()$"]: int}
+	{[=~"^\\P{Lo}"]: int}
 	...
 }
 complex?: {

diff --git a/encoding/jsonschema/testdata/txtar/pattern_with_empty_field.txtar b/encoding/jsonschema/testdata/txtar/pattern_with_empty_field.txtar
@@ -1,6 +1,5 @@
 # This tests the case where there's a patternProperties keyword
 # and no other fields.
-# TODO the empty-field test should fail but currently does not.
 
 -- schema.json --
 {
@@ -15,7 +14,12 @@
 -- out/decode/extract --
 @jsonschema(schema="https://json-schema.org/draft/2020-12/schema")
 
-{[=~".*" & !~"^()$"]: string}
+{[=~".*"]: string}
 ...
--- test/empty-field.json --
+-- test/err-empty-field.json --
 {"": true}
+-- out/decode/testerr/err-empty-field --
+"": conflicting values true and string (mismatched types bool and string):
+    generated.cue:3:1
+    generated.cue:3:12
+    test/err-empty-field.json:1:6
diff --git a/encoding/jsonschema/testdata/txtar/pattern_with_regexp_fields.txtar b/encoding/jsonschema/testdata/txtar/pattern_with_regexp_fields.txtar
@@ -22,9 +22,14 @@
 @jsonschema(schema="http://json-schema.org/draft-07/schema#")
 "c.e"?: bool
 
-{[=~".*" & !~"^(c.e)$"]: string}
+{[=~".*" & !~"^(c\\.e)$"]: string}
 ...
--- test/field.json --
+-- test/err-field.json --
 {
     "cue": 123
 }
+-- out/decode/testerr/err-field --
+cue: conflicting values 123 and string (mismatched types int and string):
+    generated.cue:4:1
+    generated.cue:4:28
+    test/err-field.json:2:12