From 69a7a9a0f3fa534fb3c572f6963bf371df683caa Mon Sep 17 00:00:00 2001 From: Richard Wooding Date: Wed, 9 Sep 2026 18:20:09 +0200 Subject: [PATCH] fix: detect JSON columns in has() from the schema, not a name list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isDirectJSONFieldAccess and isJSONColumn recognised a column as JSON only when it was named one of metadata, properties, content, structure, taxonomy, analytics or classification. Those names came from one application's schema and were never documented. They are the two survivors of #62 (#61, #59), which converted every other JSON detection path to use WithSchemas. The list produced invalid SQL for any JSONB column named something else. getJSONRootAndPath used it as the only stopping condition when locating the JSON column boundary, so a column outside the set never terminated the walk: its name was swallowed into the path and the table alias was emitted as the JSON document. -- has(record.payload.active), payload declared jsonb via WithSchemas -- before: PostgreSQL rejects this, 'record' is the row not a jsonb value jsonb_extract_path_text(record, 'payload', 'active') IS NOT NULL -- after record.payload ? 'active' Two tests covered exactly this and passed only because they asserted NotEmpty and logged the SQL rather than comparing it. Both now assert the generated SQL, which is the regression test for this fix. Both helpers now use getTableAndFieldFromSelectChain + isFieldJSON, the same lookups shouldUseJSONPath already uses, and consult jsonVars. Deep paths are unchanged: in documents.content.metadata.corpus, metadata is still a path segment rather than a column, because a boundary requires the operand to be a table identifier — the guard isTableReference used to apply by hand, now redundant and removed along with the function. isJSONBField also learns that a variable declared through WithJSONVariables is itself a JSONB column, so has(tags.colour) produces tags ? 'colour' as docs/operators-reference.md already documents, rather than the JSON arrow form. The name-based implementation never consulted jsonVars at all. BREAKING: a column is no longer treated as JSON purely because of its name. Callers not passing WithSchemas or WithJSONVariables now get the ordinary column IS NOT NULL form; declaring the column restores the previous SQL. Two name lists remain, gating numeric casts rather than JSON detection: isNumericJSONField and isJSONObjectFieldAccess. Replacing the latter with the iterVars set that already tracks comprehension variables breaks three tests, so it needs its own change rather than widening this one. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 45 ++++++++++++++++ cel2sql.go | 50 ++++++----------- final_coverage_test.go | 15 +++++- json.go | 5 ++ json_escaping_test.go | 12 +++-- json_has_detection_test.go | 103 ++++++++++++++++++++++++++++++++++++ json_jsonb_coverage_test.go | 40 +++++++------- placeholder_style_test.go | 8 +-- 8 files changed, 213 insertions(+), 65 deletions(-) create mode 100644 json_has_detection_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f9b9435..f2dfe5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,51 @@ ## [Unreleased] +### Fixed +- **`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`, + `taxonomy`, `analytics` or `classification` — names that came from one + application's schema and were never documented. They were the two survivors of + the sweep in #62 (#61, #59), which converted every other JSON detection path to + use `WithSchemas`. + + This produced **invalid SQL** for any JSONB column named something else. + `getJSONRootAndPath` used the list as its only stopping condition when locating + the JSON column boundary, so the column name was swallowed into the path and the + table alias was emitted as the JSON document: + + ```sql + -- has(record.payload.active), payload declared jsonb via WithSchemas + -- before: PostgreSQL rejects this — 'record' is the row, not a jsonb value + jsonb_extract_path_text(record, 'payload', 'active') IS NOT NULL + -- after + record.payload ? 'active' + ``` + + Both helpers now use the existing `getTableAndFieldFromSelectChain` + + `isFieldJSON` lookups, matching `shouldUseJSONPath`. Deep paths are unchanged: + in `documents.content.metadata.corpus`, `metadata` is still a path segment + rather than a column, because a boundary requires the operand to be a table + identifier — the check `isTableReference` used to make by hand, which is now + redundant and has been removed. + +- **`has()` honours `WithJSONVariables`.** A variable declared through that option + is a JSONB column, so `has(tags.colour)` now produces `tags ? 'colour'` as + documented in `docs/operators-reference.md`, rather than the JSON arrow form. + The name-based implementation never consulted `jsonVars` at all. + +### Changed +- **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 + `column IS NOT NULL` form. Declare the column — `WithSchemas` with + `IsJSON`/`IsJSONB` set, or `WithJSONVariables` for a flat JSONB column — to + restore the previous SQL. Callers already passing schemas are unaffected, and + those with JSON columns outside the seven names get correct SQL for the first + time. This is the same class of change as the v3.7.0 removal of the name-based + numeric-cast heuristic. + ### Added - **`WithPlaceholderStyle` option** — render `?` for every bind parameter instead of the dialect's native placeholder, for callers whose driver or query builder diff --git a/cel2sql.go b/cel2sql.go index cb0bbd1..cd65a57 100644 --- a/cel2sql.go +++ b/cel2sql.go @@ -7,7 +7,6 @@ import ( "fmt" "log/slog" "math" - "slices" "strconv" "strings" "time" @@ -2605,19 +2604,15 @@ func (con *converter) visitHasFunction(expr *exprpb.Expr) error { return nil } -// isDirectJSONFieldAccess checks if this represents a direct JSON field access (table.json_column.key) +// isDirectJSONFieldAccess reports whether operand is a JSON column being accessed +// directly, as in table.json_column.key. func (con *converter) isDirectJSONFieldAccess(operand *exprpb.Expr, _ string) bool { - // Check if operand is a select expression that refers to a JSON column - if selectExpr := operand.GetSelectExpr(); selectExpr != nil { - parentField := selectExpr.GetField() - - // Check if the parent field is a known JSON column - jsonFields := []string{"metadata", "properties", "content", "structure", "taxonomy", "analytics", "classification"} - if slices.Contains(jsonFields, parentField) { - return true - } + if tableName, fieldName, ok := con.getTableAndFieldFromSelectChain(operand); ok { + return con.isFieldJSON(tableName, fieldName) || con.isJSONVariable(tableName) + } + if identExpr := operand.GetIdentExpr(); identExpr != nil { + return con.isJSONVariable(identExpr.GetName()) } - return false } @@ -2690,30 +2685,17 @@ func (con *converter) getJSONRootAndPath(expr *exprpb.Expr) (*exprpb.Expr, []str return current, pathSegments } -// isJSONColumn checks if the operand refers to a JSON column +// isJSONColumn reports whether operand.field names a JSON column, marking the +// boundary between the SQL column and the JSON path within it. +// +// Requiring operand to be an identifier is what keeps the boundary correct for +// deep paths: in documents.content.metadata.corpus, "metadata" is a path segment +// rather than a column because its operand is a select, not a table. func (con *converter) isJSONColumn(operand *exprpb.Expr, field string) bool { - // Check if the field name is a known JSON column - jsonColumns := []string{"metadata", "properties", "content", "structure", "taxonomy", "analytics", "classification"} - for _, jsonCol := range jsonColumns { - if field == jsonCol { - // Additional check: make sure the operand is a table reference, not another JSON field - if con.isTableReference(operand) { - return true - } - } - } - return false -} - -// isTableReference checks if an expression refers to a table (not a JSON field) -func (con *converter) isTableReference(expr *exprpb.Expr) bool { - if identExpr := expr.GetIdentExpr(); identExpr != nil { - // Direct table reference (e.g., "information_assets") - return true + if identExpr := operand.GetIdentExpr(); identExpr != nil { + tableName := identExpr.GetName() + return con.isFieldJSON(tableName, field) || con.isJSONVariable(tableName) } - - // For now, assume SelectExpr that doesn't have JSON field characteristics is also a table reference - // This is a simplification but should work for our use cases return false } diff --git a/final_coverage_test.go b/final_coverage_test.go index 6d590cc..e79acaf 100644 --- a/final_coverage_test.go +++ b/final_coverage_test.go @@ -393,6 +393,7 @@ func TestJSONColumnReferenceEdgeCases(t *testing.T) { {Name: "taxonomy", Type: "jsonb", IsJSON: true, IsJSONB: true}, {Name: "analytics", Type: "jsonb", IsJSON: true, IsJSONB: true}, {Name: "classification", Type: "jsonb", IsJSON: true, IsJSONB: true}, + {Name: "provenance", Type: "jsonb", IsJSON: true, IsJSONB: true}, }) provider := pg.NewTypeProvider(map[string]pg.Schema{"asset": schema}) @@ -412,7 +413,7 @@ func TestJSONColumnReferenceEdgeCases(t *testing.T) { { name: "has_on_structure_column", expression: `has(asset.structure.level.parent)`, - description: "has() on 'structure' JSON column (known column name)", + description: "has() on a 'structure' JSON column", checkSQL: func(t *testing.T, sql string) { assert.Contains(t, sql, "asset.structure") assert.Contains(t, sql, "'level'") @@ -437,6 +438,18 @@ func TestJSONColumnReferenceEdgeCases(t *testing.T) { assert.Contains(t, sql, "'views'") }, }, + { + // Detection comes from the schema, so a column outside the set the + // converter once hardcoded behaves identically. + name: "has_on_arbitrarily_named_column", + expression: `has(asset.provenance.source.system)`, + description: "has() on a JSON column with an arbitrary name", + checkSQL: func(t *testing.T, sql string) { + assert.Contains(t, sql, "asset.provenance") + assert.Contains(t, sql, "'source'") + assert.Contains(t, sql, "'system'") + }, + }, { name: "has_on_classification_column", expression: `has(asset.classification.level.value)`, diff --git a/json.go b/json.go index b4aa1a4..02a2f7e 100644 --- a/json.go +++ b/json.go @@ -215,6 +215,11 @@ func (con *converter) isJSONArrayField(expr *exprpb.Expr) bool { // isJSONBField determines if the expression refers to a JSONB field (vs JSON field) func (con *converter) isJSONBField(expr *exprpb.Expr) bool { + // A variable declared through WithJSONVariables is itself a JSONB column. + if identExpr := expr.GetIdentExpr(); identExpr != nil { + return con.isJSONVariable(identExpr.GetName()) + } + // Check if this is a field selection on a JSONB field if selectExpr := expr.GetSelectExpr(); selectExpr != nil { operand := selectExpr.GetOperand() diff --git a/json_escaping_test.go b/json_escaping_test.go index ac4d573..f29470e 100644 --- a/json_escaping_test.go +++ b/json_escaping_test.go @@ -4,6 +4,7 @@ import ( "testing" "cel.dev/cel-go/cel" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/spandigital/cel2sql/v3" @@ -87,11 +88,13 @@ func TestJSONFieldNameEscaping_HasFunction(t *testing.T) { tests := []struct { name string celExpr string + wantSQL string description string }{ { name: "has() with JSON field", - celExpr: `has(obj.settings.theme)`, + celExpr: `has(rec.settings.theme)`, + wantSQL: `rec.settings ? 'theme'`, description: "Existence check on JSON field", }, } @@ -100,7 +103,7 @@ func TestJSONFieldNameEscaping_HasFunction(t *testing.T) { t.Run(tt.name, func(t *testing.T) { env, err := cel.NewEnv( cel.CustomTypeProvider(provider), - cel.Variable("obj", cel.ObjectType("TestTable")), + cel.Variable("rec", cel.ObjectType("TestTable")), ) require.NoError(t, err) @@ -110,12 +113,11 @@ func TestJSONFieldNameEscaping_HasFunction(t *testing.T) { } schemas := map[string]pg.Schema{ - "obj": testSchema, + "rec": testSchema, } sqlCondition, err := cel2sql.Convert(ast, cel2sql.WithSchemas(schemas)) require.NoError(t, err, "Should convert CEL to SQL: %s", tt.description) - require.NotEmpty(t, sqlCondition, "Should generate SQL") - t.Logf("Generated SQL: %s", sqlCondition) + assert.Equal(t, tt.wantSQL, sqlCondition, tt.description) }) } } diff --git a/json_has_detection_test.go b/json_has_detection_test.go new file mode 100644 index 0000000..7f569a0 --- /dev/null +++ b/json_has_detection_test.go @@ -0,0 +1,103 @@ +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" +) + +// has() detection is driven by the schema, not by the column's name. Before this +// was fixed, a column had to be named one of seven hardcoded values to be treated +// as JSON; anything else walked past the column boundary and emitted the table +// alias as the JSON document, which PostgreSQL rejects. +func TestHasUsesSchemaNotColumnName(t *testing.T) { + schema := pg.NewSchema([]pg.FieldSchema{ + {Name: "id", Type: "integer"}, + {Name: "payload", Type: "jsonb", IsJSON: true, IsJSONB: true}, + {Name: "legacy_doc", Type: "json", IsJSON: true}, + {Name: "title", Type: "text"}, + }) + provider := pg.NewTypeProvider(map[string]pg.Schema{"rec": schema}) + + env, err := cel.NewEnv( + cel.CustomTypeProvider(provider), + cel.Variable("rec", cel.ObjectType("rec")), + ) + require.NoError(t, err) + + tests := []struct { + name string + celExpr string + wantSQL string + }{ + { + name: "jsonb column not on the old name list", + celExpr: `has(rec.payload.active)`, + wantSQL: `rec.payload ? 'active'`, + }, + { + name: "json column uses the arrow form", + celExpr: `has(rec.legacy_doc.active)`, + wantSQL: `rec.legacy_doc->'active' IS NOT NULL`, + }, + { + name: "nested path below a non-listed jsonb column", + celExpr: `has(rec.payload.user.name)`, + wantSQL: `jsonb_extract_path_text(rec.payload, 'user', 'name') IS NOT NULL`, + }, + { + name: "non-JSON column falls through to a null check", + celExpr: `has(rec.title)`, + wantSQL: `rec.title IS NOT NULL`, + }, + } + + 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(provider.GetSchemas())) + require.NoError(t, err) + assert.Equal(t, tt.wantSQL, sql) + }) + } +} + +// A column named like one of the seven formerly-hardcoded values gets no special +// treatment: with no schema declaring it JSON, it is an ordinary column. +func TestHasWithoutSchemaIgnoresColumnName(t *testing.T) { + env, err := cel.NewEnv( + cel.Variable("rec", cel.MapType(cel.StringType, cel.DynType)), + ) + require.NoError(t, err) + + ast, issues := env.Compile(`has(rec.metadata)`) + require.NoError(t, issues.Err()) + + sql, err := cel2sql.Convert(ast) + require.NoError(t, err) + assert.Equal(t, `rec.metadata IS NOT NULL`, sql) + assert.NotContains(t, sql, "?", "no schema means no JSON treatment") +} + +// WithJSONVariables declares a bare column as JSONB. The name-based +// implementation never consulted it, so these never reached the ? operator. +func TestHasHonoursJSONVariables(t *testing.T) { + env, err := cel.NewEnv( + cel.Variable("tags", cel.MapType(cel.StringType, cel.DynType)), + ) + require.NoError(t, err) + + ast, issues := env.Compile(`has(tags.colour)`) + require.NoError(t, issues.Err()) + + sql, err := cel2sql.Convert(ast, cel2sql.WithJSONVariables("tags")) + require.NoError(t, err) + assert.Equal(t, `tags ? 'colour'`, sql) +} diff --git a/json_jsonb_coverage_test.go b/json_jsonb_coverage_test.go index 12209dd..aa81976 100644 --- a/json_jsonb_coverage_test.go +++ b/json_jsonb_coverage_test.go @@ -30,29 +30,31 @@ func TestJSONBFieldDetection(t *testing.T) { require.NoError(t, err) tests := []struct { - name string - expression string - description string + name string + expression string + wantSQL string }{ { - name: "jsonb_field_access", - expression: `record.jsonb_data.name == "test"`, - description: "Access JSONB field - should use ->> operator", + name: "jsonb_field_access", + expression: `record.jsonb_data.name == "test"`, + wantSQL: `record.jsonb_data->>'name' = 'test'`, }, { - name: "jsonb_nested_access", - expression: `record.jsonb_metadata.user.id > 0`, - description: "Nested JSONB field access", + name: "jsonb_nested_access", + expression: `record.jsonb_metadata.user.id > 0`, + wantSQL: `(record.jsonb_metadata->'user'->>'id')::numeric > 0`, }, { - name: "has_on_jsonb", - expression: `has(record.jsonb_data.active)`, - description: "has() function on JSONB field", + // Detection is schema-driven: jsonb_data is not one of the column names + // the converter used to recognise, and must still reach the ? operator. + name: "has_on_jsonb", + expression: `has(record.jsonb_data.active)`, + wantSQL: `record.jsonb_data ? 'active'`, }, { - name: "json_field_access", - expression: `record.json_data.status == "ok"`, - description: "Access JSON (not JSONB) field", + name: "json_field_access", + expression: `record.json_data.status == "ok"`, + wantSQL: `record.json_data->>'status' = 'ok'`, }, } @@ -63,13 +65,9 @@ func TestJSONBFieldDetection(t *testing.T) { schemas := provider.GetSchemas() sql, err := cel2sql.Convert(ast, cel2sql.WithSchemas(schemas)) + require.NoError(t, err) - if err != nil { - t.Logf("Conversion for %s resulted in error: %v", tt.description, err) - } else { - t.Logf("Generated SQL for %s: %s", tt.description, sql) - assert.NotEmpty(t, sql, "SQL should not be empty") - } + assert.Equal(t, tt.wantSQL, sql) }) } } diff --git a/placeholder_style_test.go b/placeholder_style_test.go index 998cf38..7903799 100644 --- a/placeholder_style_test.go +++ b/placeholder_style_test.go @@ -165,10 +165,10 @@ func TestPlaceholderStyle_ConvertUnaffected(t *testing.T) { // from a placeholder to a consumer that scans for ?. Converting must fail rather // than hand back SQL that would bind a value to an operator. func TestPlaceholderStyle_QuestionRejectsPostgresJSONBExistence(t *testing.T) { - // The ? operator form is reached only for a direct field access on a column - // cel2sql recognises as JSON by name (isDirectJSONFieldAccess). + // A deliberately unremarkable column name: the ? operator form is reached + // because the schema says jsonb, not because of what the column is called. recordSchema := pg.NewSchema([]schema.FieldSchema{ - {Name: "metadata", Type: "jsonb", IsJSON: true, IsJSONB: true}, + {Name: "payload", Type: "jsonb", IsJSON: true, IsJSONB: true}, }) provider := pg.NewTypeProvider(map[string]pg.Schema{"record": recordSchema}) @@ -178,7 +178,7 @@ func TestPlaceholderStyle_QuestionRejectsPostgresJSONBExistence(t *testing.T) { ) require.NoError(t, err) - ast, issues := env.Compile(`has(record.metadata.active)`) + ast, issues := env.Compile(`has(record.payload.active)`) require.NoError(t, issues.Err()) schemas := provider.GetSchemas()