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()