diff --git a/CHANGELOG.md b/CHANGELOG.md index f2dfe5a..c7fe5e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,35 @@ ## [Unreleased] ### Fixed +- **Comprehension variables are JSON because of what they range over, not what + they are called.** `isJSONObjectFieldAccess` decided that field access on a + comprehension variable should extract from a JSON document whenever the + variable was named `attr`, `item`, `element`, `obj`, `feature` or `review`. + Both directions were wrong: a `jsonb[]` column iterated as `row` was treated as + a composite and produced `row.status`, while an array of a composite type + iterated as `item` produced `item->>'name'` for a real column. The name even + leaked outside comprehensions — with a schema declaring `name` as `text`, + `item.name.size()` rendered as `LENGTH(item->>'name')`. + + A comprehension variable bound to JSON documents is now treated exactly like + one declared through `WithJSONVariables`, scoped to the comprehension body, so + the existing JSON path machinery handles it. Nested access benefits in + particular: `r.metadata.active` over a `jsonb[]` now routes through the path + builder to `r->'metadata'->>'active'`, where the name-based branch would have + extracted the first segment as text and then dotted into it. + +- **Numeric casts on JSON values no longer depend on the field's name.** + `isNumericJSONField` carried a list of nineteen field names — `level`, `score`, + `price`, `rating`, `megapixels`, `vram`, `helpful` and so on — that decided + whether an extracted JSON value was wrapped in `::numeric`. The cast is already + driven by the compared type in `visitCall`, which is where v3.7.0 left it when + it removed the matching heuristic from `visitIdent`; the list was redundant. + `(item->>'price')::numeric > 10` is unchanged, now because 10 is numeric rather + than because the field is called `price`. + + With these two gone, no hardcoded column- or variable-name list remains in the + converter. + - **`has()` now detects JSON columns from the schema instead of a hardcoded name list.** `isDirectJSONFieldAccess` and `isJSONColumn` recognised a column as JSON only when it was named one of `metadata`, `properties`, `content`, `structure`, @@ -37,6 +66,14 @@ The name-based implementation never consulted `jsonVars` at all. ### Changed +- **BREAKING: field access on a comprehension variable follows the schema.** + Iterating an array of a composite type now yields column access (`e.name`) and + iterating a `jsonb`/`jsonb[]` range yields extraction (`e->>'name'`), whichever + the variable is called. Callers who relied on a variable name to force JSON + treatment should declare the range as JSON in `WithSchemas`; callers who + happened to name a plain variable `item` or `obj` get column access, which is + what the schema already said. + - **BREAKING: `has()` on a JSON column requires the column to be declared.** Callers who relied on a column being treated as JSON purely because of its name, without passing `WithSchemas` or `WithJSONVariables`, now get the ordinary diff --git a/cel2sql.go b/cel2sql.go index cd65a57..079a35b 100644 --- a/cel2sql.go +++ b/cel2sql.go @@ -499,7 +499,11 @@ type converter struct { // iterVars are the comprehension iteration variables in scope. A // reference to one is written by the dialect, which knows whether its own // source bound the alias to a value or to a row. - iterVars map[string]bool + iterVars map[string]bool + // jsonIterVars marks those iteration variables whose range yields JSON + // values rather than rows or scalars, so field access on them extracts + // from the document instead of naming a composite field. + jsonIterVars map[string]bool parameterize bool // Enable parameterized output parameters []any // Collected parameters for parameterized queries paramCount int // Parameter counter for placeholders @@ -559,6 +563,13 @@ func (con *converter) visit(expr *exprpb.Expr) error { return newConversionErrorf(errMsgUnsupportedExpression, "expr type: %T, id: %d", expr.ExprKind, expr.Id) } +// isJSONSQLType reports whether a declared SQL type is a JSON document type. +// Schemas loaded by a provider set IsJSON explicitly, but hand-built ones often +// only carry the type name. +func isJSONSQLType(sqlType string) bool { + return strings.EqualFold(sqlType, "json") || strings.EqualFold(sqlType, "jsonb") +} + // isFieldJSON checks if a field in a table is a JSON/JSONB type using schema information func (con *converter) isFieldJSON(tableName, fieldName string) bool { if con.schemas == nil { @@ -2159,7 +2170,7 @@ func (con *converter) visitAllComprehension(expr *exprpb.Expr, info *Comprehensi // // Restored afterwards, since comprehensions nest and an inner one may // reuse a name. - defer con.bindIterVar(info.IterVar)() + defer con.bindIterVar(info.IterVar, con.iterRangeYieldsJSON(comprehension.GetIterRange()))() iterRange := comprehension.GetIterRange() @@ -2194,7 +2205,7 @@ func (con *converter) visitExistsComprehension(expr *exprpb.Expr, info *Comprehe // // Restored afterwards, since comprehensions nest and an inner one may // reuse a name. - defer con.bindIterVar(info.IterVar)() + defer con.bindIterVar(info.IterVar, con.iterRangeYieldsJSON(comprehension.GetIterRange()))() iterRange := comprehension.GetIterRange() @@ -2228,7 +2239,7 @@ func (con *converter) visitExistsOneComprehension(expr *exprpb.Expr, info *Compr // // Restored afterwards, since comprehensions nest and an inner one may // reuse a name. - defer con.bindIterVar(info.IterVar)() + defer con.bindIterVar(info.IterVar, con.iterRangeYieldsJSON(comprehension.GetIterRange()))() iterRange := comprehension.GetIterRange() @@ -2263,7 +2274,7 @@ func (con *converter) visitMapComprehension(expr *exprpb.Expr, info *Comprehensi // // Restored afterwards, since comprehensions nest and an inner one may // reuse a name. - defer con.bindIterVar(info.IterVar)() + defer con.bindIterVar(info.IterVar, con.iterRangeYieldsJSON(comprehension.GetIterRange()))() iterRange := comprehension.GetIterRange() @@ -2307,7 +2318,7 @@ func (con *converter) visitFilterComprehension(expr *exprpb.Expr, info *Comprehe // // Restored afterwards, since comprehensions nest and an inner one may // reuse a name. - defer con.bindIterVar(info.IterVar)() + defer con.bindIterVar(info.IterVar, con.iterRangeYieldsJSON(comprehension.GetIterRange()))() iterRange := comprehension.GetIterRange() @@ -2526,49 +2537,19 @@ func (con *converter) visitSelect(expr *exprpb.Expr) error { // Check if we should use JSON path operators // We need to determine if the operand is a JSON/JSONB field - useJSONPath := con.shouldUseJSONPath(sel.GetOperand(), fieldName) - useJSONObjectAccess := con.isJSONObjectFieldAccess(expr) - - // Check if this is a nested JSON path that requires special handling - if useJSONPath && !useJSONObjectAccess { + if con.shouldUseJSONPath(sel.GetOperand(), fieldName) { // Use the specialized JSON path builder for nested access return con.buildJSONPath(expr) } nested := !sel.GetTestOnly() && isBinaryOrTernaryOperator(sel.GetOperand()) - writeBase := func() error { - return con.visitMaybeNested(sel.GetOperand(), nested) - } - - switch { - case useJSONPath: - // Use dialect-specific JSON field access (text extraction) - if err := con.dialect.WriteJSONFieldAccess(&con.str, writeBase, fieldName, true); err != nil { - return err - } - case useJSONObjectAccess: - // Use dialect-specific JSON object field access in comprehensions - isNumeric := con.isNumericJSONField(fieldName) - if isNumeric { - con.str.WriteString("(") - } - if err := con.dialect.WriteJSONFieldAccess(&con.str, writeBase, fieldName, true); err != nil { - return err - } - if isNumeric { - // Close parentheses and add numeric cast - con.str.WriteString(")") - con.dialect.WriteCastToNumeric(&con.str) - } - default: - // Regular field selection - if err := writeBase(); err != nil { - return err - } - con.str.WriteString(".") - con.str.WriteString(fieldName) + // Regular field selection + if err := con.visitMaybeNested(sel.GetOperand(), nested); err != nil { + return err } + con.str.WriteString(".") + con.str.WriteString(fieldName) return nil } @@ -2771,13 +2752,23 @@ func (con *converter) writeComprehensionSource(iterRange *exprpb.Expr) error { // Scoped rather than accumulated: comprehensions nest, and an inner one may // reuse a name the outer one bound differently. The returned function restores // whatever the name meant before. -func (con *converter) bindIterVar(name string) func() { +func (con *converter) bindIterVar(name string, yieldsJSON bool) func() { if con.iterVars == nil { con.iterVars = map[string]bool{} } + if con.jsonIterVars == nil { + con.jsonIterVars = map[string]bool{} + } had := con.iterVars[name] + prevJSON, hadJSON := con.jsonIterVars[name] con.iterVars[name] = true + con.jsonIterVars[name] = yieldsJSON return func() { + if hadJSON { + con.jsonIterVars[name] = prevJSON + } else { + delete(con.jsonIterVars, name) + } if had { return } @@ -2785,6 +2776,44 @@ func (con *converter) bindIterVar(name string) func() { } } +// iterRangeYieldsJSON reports whether unnesting this range binds the iteration +// variable to a JSON value. A jsonb array yields documents, so field access on +// the variable must extract from them; an array of a composite type yields rows, +// where the same access names a real column. +func (con *converter) iterRangeYieldsJSON(iterRange *exprpb.Expr) bool { + if iterRange == nil { + return false + } + if identExpr := iterRange.GetIdentExpr(); identExpr != nil { + return con.isJSONVariable(identExpr.GetName()) + } + if tableName, fieldName, ok := con.getTableAndFieldFromSelectChain(iterRange); ok { + return con.isJSONVariable(tableName) || con.fieldYieldsJSONElements(tableName, fieldName) + } + return false +} + +// fieldYieldsJSONElements reports whether iterating this field produces JSON +// documents. The element type decides it: a jsonb[] column unnests to jsonb +// values, while an array of a composite type unnests to rows. +func (con *converter) fieldYieldsJSONElements(tableName, fieldName string) bool { + if con.schemas == nil { + return false + } + tableSchema, ok := con.schemas[tableName] + if !ok { + return false + } + field, ok := tableSchema.FindField(fieldName) + if !ok { + return false + } + if field.ElementType != "" { + return isJSONSQLType(field.ElementType) + } + return field.IsJSON || isJSONSQLType(field.Type) +} + func (con *converter) visitMaybeNested(expr *exprpb.Expr, nested bool) error { if nested { con.str.WriteString("(") diff --git a/comprehension_json_vars_test.go b/comprehension_json_vars_test.go new file mode 100644 index 0000000..19ab5d2 --- /dev/null +++ b/comprehension_json_vars_test.go @@ -0,0 +1,154 @@ +package cel2sql_test + +import ( + "testing" + + "cel.dev/cel-go/cel" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/spandigital/cel2sql/v3" + "github.com/spandigital/cel2sql/v3/pg" +) + +// Whether a comprehension variable accesses JSON is decided by what it ranges +// over, not by what it is called. The converter used to key this off a list of +// variable names — attr, item, element, obj, feature, review — so a jsonb array +// iterated as `row` was treated as a composite, and a composite array iterated +// as `item` was treated as JSON. +func TestComprehensionVarJSONnessComesFromTheRange(t *testing.T) { + employee := pg.NewSchema([]pg.FieldSchema{ + {Name: "name", Type: "text"}, + {Name: "salary", Type: "bigint"}, + }) + data := pg.NewSchema([]pg.FieldSchema{ + {Name: "docs", Type: "jsonb", Repeated: true}, + {Name: "items", Type: "jsonb"}, + {Name: "scores", Type: "bigint", Repeated: true}, + }) + + provider := pg.NewTypeProvider(map[string]pg.Schema{ + "Employee": employee, + "data": data, + }) + schemas := map[string]pg.Schema{"data": data} + + env, err := cel.NewEnv( + cel.CustomTypeProvider(provider), + cel.Variable("data", cel.ObjectType("data")), + cel.Variable("staff", cel.ListType(cel.ObjectType("Employee"))), + ) + require.NoError(t, err) + + tests := []struct { + name string + celExpr string + wantSQL string + }{ + { + // jsonb[] unnests to documents, so field access extracts from them — + // and "row" is not a name the old list knew about. + name: "json array with an unlisted variable name", + celExpr: `data.docs.exists(row, row.status == "open")`, + wantSQL: `EXISTS (SELECT 1 FROM UNNEST(data.docs) AS row WHERE row->>'status' = 'open')`, + }, + { + // A composite array unnests to rows, so the same access names a column — + // even though "item" was on the old list. + name: "composite array with a formerly listed variable name", + celExpr: `staff.exists(item, item.name == "admin")`, + wantSQL: `EXISTS (SELECT 1 FROM UNNEST(staff) AS item WHERE item.name = 'admin')`, + }, + { + name: "scalar array is untouched", + celExpr: `data.scores.exists(x, x > 10)`, + wantSQL: `EXISTS (SELECT 1 FROM UNNEST(data.scores) AS x WHERE x > 10)`, + }, + { + // Numeric casting comes from the compared type, not from the field name. + name: "numeric comparison casts without a name list", + celExpr: `data.docs.exists(row, row.score > 100)`, + wantSQL: `EXISTS (SELECT 1 FROM UNNEST(data.docs) AS row WHERE (row->>'score')::numeric > 100)`, + }, + { + // The documented example from docs/json-support.md. It produced this + // same SQL before, but only because the variable was called "item"; + // now it is because items is declared jsonb. (Which unnest function + // suits a jsonb array value is a separate, pre-existing question.) + name: "jsonb array value", + celExpr: `data.items.filter(item, item.price > 10)`, + wantSQL: `ARRAY(SELECT item FROM UNNEST(data.items) AS item WHERE (item->>'price')::numeric > 10)`, + }, + { + // Nested paths route through the JSON path builder rather than + // extracting the first segment as text and then dotting into it. + name: "nested access on a JSON element", + celExpr: `data.docs.exists(row, row.meta.active == "yes")`, + wantSQL: `EXISTS (SELECT 1 FROM UNNEST(data.docs) AS row WHERE row->'meta'->>'active' = 'yes')`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ast, issues := env.Compile(tt.celExpr) + require.NoError(t, issues.Err()) + + sql, err := cel2sql.Convert(ast, cel2sql.WithSchemas(schemas)) + require.NoError(t, err) + assert.Equal(t, tt.wantSQL, sql) + }) + } +} + +// Outside a comprehension a variable is just a variable, whatever it is called. +func TestFormerlyListedVariableNamesGetNoSpecialTreatment(t *testing.T) { + schema := pg.NewSchema([]pg.FieldSchema{{Name: "name", Type: "text"}}) + schemas := map[string]pg.Schema{} + + for _, varName := range []string{"attr", "item", "element", "obj", "feature", "review"} { + t.Run(varName, func(t *testing.T) { + schemas[varName] = schema + defer delete(schemas, varName) + + env, err := cel.NewEnv( + cel.CustomTypeProvider(pg.NewTypeProvider(map[string]pg.Schema{varName: schema})), + cel.Variable(varName, cel.ObjectType(varName)), + ) + require.NoError(t, err) + + ast, issues := env.Compile(varName + `.name == "x"`) + require.NoError(t, issues.Err()) + + sql, err := cel2sql.Convert(ast, cel2sql.WithSchemas(schemas)) + require.NoError(t, err) + assert.Equal(t, varName+`.name = 'x'`, sql) + }) + } +} + +// The binding is scoped: an inner comprehension may reuse a name the outer one +// bound to a different kind of collection, and the outer meaning must survive. +func TestNestedComprehensionsRestoreOuterBinding(t *testing.T) { + data := pg.NewSchema([]pg.FieldSchema{ + {Name: "docs", Type: "jsonb", Repeated: true}, + {Name: "scores", Type: "bigint", Repeated: true}, + }) + schemas := map[string]pg.Schema{"data": data} + + env, err := cel.NewEnv( + cel.CustomTypeProvider(pg.NewTypeProvider(schemas)), + cel.Variable("data", cel.ObjectType("data")), + ) + require.NoError(t, err) + + // The inner comprehension rebinds "x" to a scalar; the outer "x" is a JSON + // document and must still extract after the inner one closes. + ast, issues := env.Compile(`data.docs.exists(x, data.scores.exists(x, x > 1) && x.status == "open")`) + require.NoError(t, issues.Err()) + + sql, err := cel2sql.Convert(ast, cel2sql.WithSchemas(schemas)) + require.NoError(t, err) + + assert.Contains(t, sql, `x->>'status' = 'open'`, "outer JSON binding restored after the inner one") + assert.Contains(t, sql, `AS x WHERE x > 1`, "inner scalar binding not treated as JSON") +} diff --git a/coverage_80_test.go b/coverage_80_test.go index fd0ab5c..062dc55 100644 --- a/coverage_80_test.go +++ b/coverage_80_test.go @@ -511,9 +511,11 @@ func TestVisitFilterComprehension_EdgeCases(t *testing.T) { expected: "ARRAY(SELECT x FROM UNNEST(data.numbers) AS x WHERE x > 10 AND x < 20 OR x > 30 AND x < 40 OR x = 50)", }, { + // records is jsonb[], so r is bound to a document: field access on it + // extracts from JSON rather than naming a composite field. name: "filter with nested field access on JSON", expr: "data.records.filter(r, r.metadata.active == true)", - expected: "ARRAY(SELECT r FROM UNNEST(data.records) AS r WHERE r.metadata.active IS TRUE)", + expected: "ARRAY(SELECT r FROM UNNEST(data.records) AS r WHERE r->'metadata'->>'active' IS TRUE)", }, { name: "filter with multiple string conditions", diff --git a/json.go b/json.go index 02a2f7e..fdaa8b3 100644 --- a/json.go +++ b/json.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "log/slog" - "slices" exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1" ) @@ -62,7 +61,12 @@ func (con *converter) shouldUseJSONPath(operand *exprpb.Expr, _ string) bool { // isJSONVariable checks if a variable name was declared as JSONB via WithJSONVariables. func (con *converter) isJSONVariable(name string) bool { - return con.jsonVars != nil && con.jsonVars[name] + if con.jsonVars != nil && con.jsonVars[name] { + return true + } + // A comprehension variable bound to JSON documents behaves exactly like one + // declared through WithJSONVariables, for as long as its body is written. + return con.jsonIterVars[name] } // hasJSONFieldInChain checks if there's a JSON field anywhere in the select expression chain @@ -106,13 +110,6 @@ func (con *converter) isJSONTextExtraction(expr *exprpb.Expr) bool { return false } -// isNumericJSONField checks if a JSON field name typically contains numeric values -func (con *converter) isNumericJSONField(fieldName string) bool { - numericFields := []string{"level", "score", "value", "count", "amount", "price", "rating", "age", "size", "capacity", "megapixels", "cores", "threads", "ram", "storage", "vram", "weight", "frequency", "helpful"} - - return slices.Contains(numericFields, fieldName) -} - // isNestedJSONAccess checks if this is nested JSON field access like settings.permissions func (con *converter) isNestedJSONAccess(expr *exprpb.Expr) bool { if selectExpr := expr.GetSelectExpr(); selectExpr != nil { @@ -171,25 +168,6 @@ func (con *converter) buildJSONPathForArray(expr *exprpb.Expr) error { }, field, false) } -// isJSONObjectFieldAccess determines if this is a JSON object field access in comprehensions -func (con *converter) isJSONObjectFieldAccess(expr *exprpb.Expr) bool { - if selectExpr := expr.GetSelectExpr(); selectExpr != nil { - operand := selectExpr.GetOperand() - - // Check if the operand is an identifier that could be a comprehension variable - if identExpr := operand.GetIdentExpr(); identExpr != nil { - // Common comprehension variable names that access JSON objects - jsonObjectVars := []string{"attr", "item", "element", "obj", "feature", "review"} - identName := identExpr.GetName() - - if slices.Contains(jsonObjectVars, identName) { - return true - } - } - } - return false -} - // isJSONArrayField determines if the expression refers to a JSON/JSONB array field func (con *converter) isJSONArrayField(expr *exprpb.Expr) bool { // Check if this is a field selection on a JSON field diff --git a/string_functions_test.go b/string_functions_test.go index 1fd0e98..57e2fd2 100644 --- a/string_functions_test.go +++ b/string_functions_test.go @@ -88,8 +88,9 @@ func TestIssue85_SizeOutsideComprehension(t *testing.T) { sql, err := cel2sql.Convert(ast, cel2sql.WithSchemas(schemas)) require.NoError(t, err) - // Note: item.name will be treated as a struct field, generating item->>'name' for JSON - assert.Equal(t, "LENGTH(item->>'name') > 10", sql) + // name is a text column, so it is addressed as a column. The variable being + // called "item" no longer implies JSON. + assert.Equal(t, "LENGTH(item.name) > 10", sql) } // Comprehensive tests for all string extension functions