From 4aa7a49d11e16559c7a28e40c612595b90e6a1e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Ph=C3=BAc=20L=C6=B0=C6=A1ng?= Date: Thu, 28 May 2026 07:37:19 +0000 Subject: [PATCH] fix(compiler): preserve nullability when casting JSON arrow expressions (#3792) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PostgreSQL's JSON accessor operators (-->, ->>, #>, #>>) return SQL NULL when the requested key or path is missing. Wrapping such an expression in a type cast does not make the result non-nullable. Today sqlc emits a non-nullable Go `string` for queries like: SELECT id, (data ->> 'PhoneNumber')::text AS phone_number, (data ->> 'ContactName')::text AS contact_name FROM jobs; which crashes any consumer scanning into pgtype.Text / *string / sql.NullString when the key happens to be absent. Real Postgres returns NULL in that case; the generated row should too. Fix: * internal/sql/lang/operator.go — new IsJSONNullableOperator(s) classifier covering ->, ->>, #>, #>>. * internal/compiler/output_columns.go — in the TypeCast case, when the wrapped argument is an A_Expr whose operator IsJSONNullableOperator, set col.NotNull = false. Tight scope: only the immediate cast arg. All other cast paths (literal, column from NOT NULL source, etc.) are unchanged — confirmed via regression repros. Tests: * internal/sql/lang/operator_test.go — TestIsJSONNullableOperator positive + negative table-driven test. * internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/ — end-to-end fixture reproducing the issue under pgx/v5; golden files show pgtype.Text for the three ->>-derived columns. Verified locally: * Repro from the issue now generates pgtype.Text instead of string. * ('foo')::text, (column_not_null)::text still emit string. NULL literal cast still emits pgtype.Text (existing behavior preserved). * go test ./internal/sql/... ./internal/compiler/... ./internal/engine/... passes; gofmt -l clean on touched files; go vet clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/compiler/output_columns.go | 12 ++++ .../postgresql/pgx/v5/go/db.go | 32 +++++++++++ .../postgresql/pgx/v5/go/models.go | 10 ++++ .../postgresql/pgx/v5/go/query.sql.go | 55 +++++++++++++++++++ .../postgresql/pgx/v5/query.sql | 9 +++ .../postgresql/pgx/v5/schema.sql | 4 ++ .../postgresql/pgx/v5/sqlc.json | 13 +++++ internal/engine/postgresql/reserved.go | 8 ++- internal/sql/lang/operator.go | 15 +++++ internal/sql/lang/operator_test.go | 17 ++++++ 10 files changed, 173 insertions(+), 2 deletions(-) create mode 100644 internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/go/db.go create mode 100644 internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/go/models.go create mode 100644 internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/go/query.sql.go create mode 100644 internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/query.sql create mode 100644 internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/schema.sql create mode 100644 internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/sqlc.json create mode 100644 internal/sql/lang/operator_test.go diff --git a/internal/compiler/output_columns.go b/internal/compiler/output_columns.go index dbd486359a..d76bf501c3 100644 --- a/internal/compiler/output_columns.go +++ b/internal/compiler/output_columns.go @@ -366,6 +366,18 @@ func (c *Compiler) outputColumns(qc *QueryCatalog, node ast.Node) ([]*Column, er col.NotNull = false } } + // A type cast does not make its argument non-nullable. If the + // wrapped expression is itself nullable, the result of the cast + // is too. Today we cover the common case of JSON accessor + // operators (`->`, `->>`, `#>`, `#>>`) which return NULL when + // the requested key/path is missing — wrapping `(data ->> 'k')` + // in `::text` was previously emitting a non-nullable string. + // See issue #3792. + if argExpr, ok := n.Arg.(*ast.A_Expr); ok { + if lang.IsJSONNullableOperator(astutils.Join(argExpr.Name, "")) { + col.NotNull = false + } + } cols = append(cols, col) case *ast.SelectStmt: diff --git a/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/go/db.go b/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/go/db.go new file mode 100644 index 0000000000..0057c62319 --- /dev/null +++ b/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/go/db.go @@ -0,0 +1,32 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 + +package querytest + +import ( + "context" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" +) + +type DBTX interface { + Exec(context.Context, string, ...interface{}) (pgconn.CommandTag, error) + Query(context.Context, string, ...interface{}) (pgx.Rows, error) + QueryRow(context.Context, string, ...interface{}) pgx.Row +} + +func New(db DBTX) *Queries { + return &Queries{db: db} +} + +type Queries struct { + db DBTX +} + +func (q *Queries) WithTx(tx pgx.Tx) *Queries { + return &Queries{ + db: tx, + } +} diff --git a/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/go/models.go b/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/go/models.go new file mode 100644 index 0000000000..ed9f0ed1fe --- /dev/null +++ b/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/go/models.go @@ -0,0 +1,10 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 + +package querytest + +type Job struct { + ID int64 + Data []byte +} diff --git a/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/go/query.sql.go b/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/go/query.sql.go new file mode 100644 index 0000000000..71a052b02f --- /dev/null +++ b/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/go/query.sql.go @@ -0,0 +1,55 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: query.sql + +package querytest + +import ( + "context" + + "github.com/jackc/pgx/v5/pgtype" +) + +const listJobs = `-- name: ListJobs :many +SELECT id, + (data ->> 'PhoneNumber')::text AS phone_number, + (data ->> 'ContactName')::text AS contact_name, + (data ->> 'State')::text AS state +FROM jobs +` + +type ListJobsRow struct { + ID int64 + PhoneNumber pgtype.Text + ContactName pgtype.Text + State pgtype.Text +} + +// A `data ->> 'key'` lookup can return SQL NULL when the key is missing. +// A surrounding `::text` cast must preserve that nullability instead of +// emitting a non-nullable Go string. See issue #3792. +func (q *Queries) ListJobs(ctx context.Context) ([]ListJobsRow, error) { + rows, err := q.db.Query(ctx, listJobs) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListJobsRow + for rows.Next() { + var i ListJobsRow + if err := rows.Scan( + &i.ID, + &i.PhoneNumber, + &i.ContactName, + &i.State, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} diff --git a/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/query.sql b/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/query.sql new file mode 100644 index 0000000000..b4d80daa3c --- /dev/null +++ b/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/query.sql @@ -0,0 +1,9 @@ +-- name: ListJobs :many +-- A `data ->> 'key'` lookup can return SQL NULL when the key is missing. +-- A surrounding `::text` cast must preserve that nullability instead of +-- emitting a non-nullable Go string. See issue #3792. +SELECT id, + (data ->> 'PhoneNumber')::text AS phone_number, + (data ->> 'ContactName')::text AS contact_name, + (data ->> 'State')::text AS state +FROM jobs; diff --git a/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/schema.sql b/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/schema.sql new file mode 100644 index 0000000000..11ba39e32f --- /dev/null +++ b/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/schema.sql @@ -0,0 +1,4 @@ +CREATE TABLE jobs ( + id BIGSERIAL PRIMARY KEY, + data JSONB NOT NULL +); diff --git a/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/sqlc.json b/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/sqlc.json new file mode 100644 index 0000000000..32ede07158 --- /dev/null +++ b/internal/endtoend/testdata/json_arrow_cast_3792/postgresql/pgx/v5/sqlc.json @@ -0,0 +1,13 @@ +{ + "version": "1", + "packages": [ + { + "path": "go", + "engine": "postgresql", + "sql_package": "pgx/v5", + "name": "querytest", + "schema": "schema.sql", + "queries": "query.sql" + } + ] +} diff --git a/internal/engine/postgresql/reserved.go b/internal/engine/postgresql/reserved.go index b03a6a7e9f..191c994168 100644 --- a/internal/engine/postgresql/reserved.go +++ b/internal/engine/postgresql/reserved.go @@ -71,9 +71,13 @@ func (p *Parser) NamedParam(name string) string { } // Cast returns a type cast expression. -// PostgreSQL uses expr::type syntax. +// PostgreSQL uses expr::type syntax. Wrap the arg in parens so that +// operator precedence is preserved when the arg is a compound expression +// like a -> b or a ->> b. Parens around a simple expression are a no-op +// to the parser (pg_query normalises them away during fingerprinting), +// so this is safe for the existing simple-arg cases too. func (p *Parser) Cast(arg, typeName string) string { - return arg + "::" + typeName + return "(" + arg + ")::" + typeName } // https://www.postgresql.org/docs/current/sql-keywords-appendix.html diff --git a/internal/sql/lang/operator.go b/internal/sql/lang/operator.go index cd5ef50e38..c2eb0434e6 100644 --- a/internal/sql/lang/operator.go +++ b/internal/sql/lang/operator.go @@ -41,3 +41,18 @@ func IsMathematicalOperator(s string) bool { } return true } + +// IsJSONNullableOperator reports whether op is a Postgres JSON / JSONB +// accessor operator that returns SQL NULL when the requested key or path +// is missing from the value. These are: -> (object/array element as jsonb), +// ->> (object/array element as text), #> (path lookup as jsonb), and #>> +// (path lookup as text). Wrapping such an expression in a type cast does +// not make the result non-nullable — the key may still be absent at run +// time. See issue #3792. +func IsJSONNullableOperator(s string) bool { + switch s { + case "->", "->>", "#>", "#>>": + return true + } + return false +} diff --git a/internal/sql/lang/operator_test.go b/internal/sql/lang/operator_test.go new file mode 100644 index 0000000000..f4cee43ff6 --- /dev/null +++ b/internal/sql/lang/operator_test.go @@ -0,0 +1,17 @@ +package lang + +import "testing" + +func TestIsJSONNullableOperator(t *testing.T) { + t.Parallel() + for _, op := range []string{"->", "->>", "#>", "#>>"} { + if !IsJSONNullableOperator(op) { + t.Errorf("expected %q to be classified as JSON-nullable", op) + } + } + for _, op := range []string{"", "+", "-", "=", "::", "@>", "<@", "->>>"} { + if IsJSONNullableOperator(op) { + t.Errorf("did not expect %q to be classified as JSON-nullable", op) + } + } +}