Merge #143735

143735: jsonpath: validate regular expression during parsing r=normanchenn a=normanchenn

Previously, regular expressions in jsonpath queries were only validated during query execution. This could lead to queries with invalid regex patterns being accepted during parsing, only to fail later during execution.

This change moves regex validation to parse time, ensuring that any jsonpath query containing an invalid regular expression will fail immediately during parsing. This matches what postgres does when they handle regex.

Informs: #143730
Closes: #143693
Release note: None.

Co-authored-by: Norman Chen <[email protected]>

Author: craig[bot]

pkg/sql/logictest/testdata/logic_test/jsonpath

@@ -150,6 +150,9 @@ SELECT '$  .  a  ?  (  @  .  b  ==  1  )  .  c  ?  (  @  .  d  ==  2  )  '::JSON
 ----
 $."a"?((@."b" == 1))."c"?((@."d" == 2))
 
+statement error pgcode 2201B pq: could not parse .* invalid regular expression: error parsing regexp: missing closing \)
+SELECT '$ ? (@ like_regex "(invalid pattern")'::JSONPATH
+
 ## When we allow table creation
 
 # statement ok

pkg/sql/sem/builtins/builtins.go

@@ -12184,9 +12184,7 @@ func makeTimestampStatementBuiltinOverload(withOutputTZ bool, withInputTZ bool)
 	}
 }
 
-func makeJsonpathExists(
-	_ context.Context, evalCtx *eval.Context, args tree.Datums,
-) (tree.Datum, error) {
+func makeJsonpathExists(_ context.Context, _ *eval.Context, args tree.Datums) (tree.Datum, error) {
 	target := tree.MustBeDJSON(args[0])
 	path := tree.MustBeDJsonpath(args[1])
 	vars := tree.EmptyDJSON
@@ -12197,7 +12195,7 @@ func makeJsonpathExists(
 	if len(args) > 3 {
 		silent = tree.MustBeDBool(args[3])
 	}
-	exists, err := jsonpath.JsonpathExists(evalCtx, target, path, vars, silent)
+	exists, err := jsonpath.JsonpathExists(target, path, vars, silent)
 	if err != nil {
 		return nil, err
 	}

pkg/sql/sem/builtins/generator_builtins.go

@@ -1624,8 +1624,6 @@ var jsonObjectKeysImpl = makeGeneratorOverload(
 var jsonPathQueryGeneratorType = types.Jsonb
 
 type jsonPathQueryGenerator struct {
-	evalCtx *eval.Context
-
 	target tree.DJSON
 	path   tree.DJsonpath
 	vars   tree.DJSON
@@ -1636,7 +1634,7 @@ type jsonPathQueryGenerator struct {
 }
 
 func makeJsonpathQueryGenerator(
-	_ context.Context, evalCtx *eval.Context, args tree.Datums,
+	_ context.Context, _ *eval.Context, args tree.Datums,
 ) (eval.ValueGenerator, error) {
 	target := tree.MustBeDJSON(args[0])
 	path := tree.MustBeDJsonpath(args[1])
@@ -1652,11 +1650,10 @@ func makeJsonpathQueryGenerator(
 		silent = tree.MustBeDBool(args[3])
 	}
 	return &jsonPathQueryGenerator{
-		evalCtx: evalCtx,
-		target:  target,
-		path:    path,
-		vars:    vars,
-		silent:  silent,
+		target: target,
+		path:   path,
+		vars:   vars,
+		silent: silent,
 	}, nil
 }
 
@@ -1667,7 +1664,7 @@ func (g *jsonPathQueryGenerator) ResolvedType() *types.T {
 
 // Start implements the eval.ValueGenerator interface.
 func (g *jsonPathQueryGenerator) Start(_ context.Context, _ *kv.Txn) error {
-	jsonb, err := jsonpath.JsonpathQuery(g.evalCtx, g.target, g.path, g.vars, g.silent)
+	jsonb, err := jsonpath.JsonpathQuery(g.target, g.path, g.vars, g.silent)
 	if err != nil {
 		return err
 	}

pkg/util/jsonpath/eval/BUILD.bazel

@@ -15,7 +15,6 @@ go_library(
     deps = [
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",
-        "//pkg/sql/sem/eval",
         "//pkg/sql/sem/tree",
         "//pkg/util/errorutil/unimplemented",
         "//pkg/util/json",

pkg/util/jsonpath/eval/eval.go

@@ -6,7 +6,6 @@
 package eval
 
 import (
-	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/cockroach/pkg/util/json"
@@ -21,8 +20,6 @@ var (
 )
 
 type jsonpathCtx struct {
-	evalCtx *eval.Context
-
 	// Root of the given JSON object ($). We store this because we will need to
 	// support queries with multiple root elements (ex. $.a ? ($.b == "hello").
 	root   json.JSON
@@ -31,7 +28,7 @@ type jsonpathCtx struct {
 }
 
 func JsonpathQuery(
-	evalCtx *eval.Context, target tree.DJSON, path tree.DJsonpath, vars tree.DJSON, silent tree.DBool,
+	target tree.DJSON, path tree.DJsonpath, vars tree.DJSON, silent tree.DBool,
 ) ([]tree.DJSON, error) {
 	parsedPath, err := parser.Parse(string(path))
 	if err != nil {
@@ -40,10 +37,9 @@ func JsonpathQuery(
 	expr := parsedPath.AST
 
 	ctx := &jsonpathCtx{
-		evalCtx: evalCtx,
-		root:    target.JSON,
-		vars:    vars.JSON,
-		strict:  expr.Strict,
+		root:   target.JSON,
+		vars:   vars.JSON,
+		strict: expr.Strict,
 	}
 	// When silent is true, overwrite the strict mode.
 	if bool(silent) {
@@ -62,9 +58,9 @@ func JsonpathQuery(
 }
 
 func JsonpathExists(
-	evalCtx *eval.Context, target tree.DJSON, path tree.DJsonpath, vars tree.DJSON, silent tree.DBool,
+	target tree.DJSON, path tree.DJsonpath, vars tree.DJSON, silent tree.DBool,
 ) (tree.DBool, error) {
-	j, err := JsonpathQuery(evalCtx, target, path, vars, silent)
+	j, err := JsonpathQuery(target, path, vars, silent)
 	if err != nil {
 		return false, err
 	}

pkg/util/jsonpath/eval/operation.go

@@ -12,6 +12,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/util/json"
 	"github.com/cockroachdb/cockroach/pkg/util/jsonpath"
+	"github.com/cockroachdb/cockroach/pkg/util/jsonpath/parser"
 	"github.com/cockroachdb/errors"
 )
 
@@ -124,7 +125,7 @@ func (ctx *jsonpathCtx) evalRegex(
 	}
 
 	regexOp := op.Right.(jsonpath.Regex)
-	r, err := ctx.evalCtx.ReCache.GetRegexp(regexOp)
+	r, err := parser.ReCache.GetRegexp(regexOp)
 	if err != nil {
 		return jsonpathBoolUnknown, err
 	}

pkg/util/jsonpath/parser/jsonpath.y

@@ -4,6 +4,8 @@ package parser
 import (
   "strconv"
 
+  "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+  "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
   "github.com/cockroachdb/cockroach/pkg/sql/scanner"
   "github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
   "github.com/cockroachdb/cockroach/pkg/util/json"
@@ -124,6 +126,16 @@ func unaryOp(op jsonpath.OperationType, left jsonpath.Path) jsonpath.Operation {
   }
 }
 
+func regexBinaryOp(left jsonpath.Path, regex string) (jsonpath.Operation, error) {
+  r := jsonpath.Regex{Regex: regex}
+  _, err := ReCache.GetRegexp(r)
+  if err != nil {
+    return jsonpath.Operation{}, pgerror.Wrapf(err, pgcode.InvalidRegularExpression,
+      "invalid regular expression")
+  }
+  return binaryOp(jsonpath.OpLikeRegex, left, r), nil
+}
+
 %}
 
 %union{
@@ -398,8 +410,11 @@ predicate:
   }
 | expr LIKE_REGEX STRING
   {
-    regex := jsonpath.Regex{Regex: $3}
-    $$.val = binaryOp(jsonpath.OpLikeRegex, $1.path(), regex)
+    regex, err := regexBinaryOp($1.path(), $3)
+    if err != nil {
+      return setErr(jsonpathlex, err)
+    }
+    $$.val = regex
   }
 | expr LIKE_REGEX STRING FLAG STRING
   {

pkg/util/jsonpath/parser/parse.go

@@ -22,6 +22,10 @@ func init() {
 	}
 }
 
+var (
+	ReCache = tree.NewRegexpCache(64)
+)
+
 type Parser struct {
 	scanner    scanner.JSONPathScanner
 	lexer      lexer

pkg/util/jsonpath/parser/testdata/jsonpath

@@ -494,6 +494,14 @@ parse
 ----
 (1 + (2 * (-4))) -- normalized!
 
+error
+$ ? (@ like_regex "(invalid pattern")
+----
+at or near ")": syntax error: invalid regular expression: error parsing regexp: missing closing ): `(invalid pattern`
+DETAIL: source SQL:
+$ ? (@ like_regex "(invalid pattern")
+                                    ^
+
 # postgres allows floats as array indexes
 # parse
 # $.abc[1.0]