From df210316bbdc218802154425a375180387161a5b Mon Sep 17 00:00:00 2001 From: Richard Wooding Date: Wed, 9 Sep 2026 18:47:01 +0200 Subject: [PATCH] fix: derive comprehension JSON access from the range, not variable names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two hardcoded name lists remained after #193, both in the comprehension path. Removing them leaves none in the converter. isJSONObjectFieldAccess treated field access on a comprehension variable as JSON extraction 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; an array of a composite type iterated as "item" produced item->>'name' for a real column. The name leaked outside comprehensions too: with a schema declaring name as text, item.name.size() rendered as LENGTH(item->>'name'). A comprehension variable is now bound along with whether its range yields JSON, decided by the element type of the range, and a variable bound that way is treated exactly like one declared through WithJSONVariables for as long as the body is being written. That reuses the existing JSON path machinery rather than duplicating it, which nested access needs: r.metadata.active over a jsonb[] now routes through the path builder to r->'metadata'->>'active', where the name-based branch extracted the first segment as text and then dotted into it, producing r->>'metadata'.active. The binding is scoped like iterVars, since comprehensions nest and an inner one may reuse a name the outer bound to a different kind of collection. isNumericJSONField carried nineteen field names — level, score, price, rating, megapixels, vram, helpful and so on — deciding whether an extracted JSON value was wrapped in ::numeric. visitCall already drives that from the compared type, which is where v3.7.0 left it when it removed the matching heuristic from visitIdent, so the list was redundant. (item->>'price')::numeric > 10 is unchanged, now because 10 is numeric rather than because the field is called price. With the branch gone, visitSelect loses its three-way switch. Two tests pinned the old behaviour and are corrected: a filter over a jsonb[] column expected r.metadata.active for a document, and the issue-85 test expected item->>'name' for a text column. BREAKING: field access on a comprehension variable follows the schema. Iterating a composite array yields column access and iterating a jsonb range yields extraction, whichever the variable is called. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 37 ++++++++ cel2sql.go | 115 +++++++++++++++--------- comprehension_json_vars_test.go | 154 ++++++++++++++++++++++++++++++++ coverage_80_test.go | 4 +- json.go | 34 ++----- string_functions_test.go | 5 +- 6 files changed, 275 insertions(+), 74 deletions(-) create mode 100644 comprehension_json_vars_test.go 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