From 3695ee3a8c2d9340dd71940152b65fed02b2a564 Mon Sep 17 00:00:00 2001 From: Brandur Date: Thu, 15 May 2025 23:03:34 -0700 Subject: [PATCH 1/4] SQLite: Coerce jsonb columns to json before returning to Go code This one follows up the discussion in #3953 to try and make the `jsonb` data type in SQLite usable (see discussion there, but I believe that it's currently not). According the SQLite docs on JSONB [1], it's considered a format that's internal to the database itself, and no attempt should be made to parse it elsewhere: > JSONB is not intended as an external format to be used by > applications. JSONB is designed for internal use by SQLite only. > Programmers do not need to understand the JSONB format in order to use > it effectively. Applications should access JSONB only through the JSON > SQL functions, not by looking at individual bytes of the BLOB. Currently, when trying to use a `jsonb` column in SQLite, sqlc ends up returning the internal binary data, which ends up being unparsable in Go: riverdrivertest.go:3030: Error Trace: /Users/brandur/Documents/projects/river/internal/riverinternaltest/riverdrivertest/riverdrivertest.go:3030 Error: Not equal: expected: []byte{0x7b, 0x22, 0x66, 0x6f, 0x6f, 0x22, 0x3a, 0x20, 0x22, 0x62, 0x61, 0x72, 0x22, 0x7d} actual : []byte{0x8c, 0x37, 0x66, 0x6f, 0x6f, 0x37, 0x62, 0x61, 0x72} Diff: --- Expected +++ Actual @@ -1,3 +1,3 @@ -([]uint8) (len=14) { - 00000000 7b 22 66 6f 6f 22 3a 20 22 62 61 72 22 7d |{"foo": "bar"}| +([]uint8) (len=9) { + 00000000 8c 37 66 6f 6f 37 62 61 72 |.7foo7bar| } Test: TestDriverRiverSQLite/QueueCreateOrSetUpdatedAt/InsertsANewQueueWithDefaultUpdatedAt The fix is that we should make sure to coerce `jsonb` columns back to `json` before returning. That's what this pull request does, intercepting `SELECT *` and wrapping `jsonb` columns with a `json(...)` invocation. I also assign `json` and `jsonb` a `[]byte` data type by default so they don't end up as `any`, which isn't very useful. `[]byte` is consistent with the default for `pgx/v5`. [1] https://sqlite.org/jsonb.html --- internal/codegen/golang/sqlite_type.go | 3 ++ internal/compiler/expand.go | 12 +++++ internal/endtoend/testdata/jsonb/pgx/go/db.go | 32 ++++++++++++ .../endtoend/testdata/jsonb/pgx/go/models.go | 12 +++++ .../testdata/jsonb/pgx/go/query.sql.go | 50 +++++++++++++++++++ .../endtoend/testdata/jsonb/pgx/query.sql | 15 ++++++ .../endtoend/testdata/jsonb/pgx/schema.sql | 7 +++ .../endtoend/testdata/jsonb/pgx/sqlc.json | 13 +++++ .../endtoend/testdata/jsonb/sqlite/go/db.go | 31 ++++++++++++ .../testdata/jsonb/sqlite/go/models.go | 12 +++++ .../testdata/jsonb/sqlite/go/query.sql.go | 50 +++++++++++++++++++ .../endtoend/testdata/jsonb/sqlite/query.sql | 15 ++++++ .../endtoend/testdata/jsonb/sqlite/schema.sql | 7 +++ .../endtoend/testdata/jsonb/sqlite/sqlc.json | 12 +++++ 14 files changed, 271 insertions(+) create mode 100644 internal/endtoend/testdata/jsonb/pgx/go/db.go create mode 100644 internal/endtoend/testdata/jsonb/pgx/go/models.go create mode 100644 internal/endtoend/testdata/jsonb/pgx/go/query.sql.go create mode 100644 internal/endtoend/testdata/jsonb/pgx/query.sql create mode 100644 internal/endtoend/testdata/jsonb/pgx/schema.sql create mode 100644 internal/endtoend/testdata/jsonb/pgx/sqlc.json create mode 100644 internal/endtoend/testdata/jsonb/sqlite/go/db.go create mode 100644 internal/endtoend/testdata/jsonb/sqlite/go/models.go create mode 100644 internal/endtoend/testdata/jsonb/sqlite/go/query.sql.go create mode 100644 internal/endtoend/testdata/jsonb/sqlite/query.sql create mode 100644 internal/endtoend/testdata/jsonb/sqlite/schema.sql create mode 100644 internal/endtoend/testdata/jsonb/sqlite/sqlc.json diff --git a/internal/codegen/golang/sqlite_type.go b/internal/codegen/golang/sqlite_type.go index 92c13557f6..cb7348385f 100644 --- a/internal/codegen/golang/sqlite_type.go +++ b/internal/codegen/golang/sqlite_type.go @@ -56,6 +56,9 @@ func sqliteType(req *plugin.GenerateRequest, options *opts.Options, col *plugin. } return "sql.NullTime" + case "json", "jsonb": + return "[]byte" + case "any": return "interface{}" diff --git a/internal/compiler/expand.go b/internal/compiler/expand.go index 60e654b696..2f04e9a6f1 100644 --- a/internal/compiler/expand.go +++ b/internal/compiler/expand.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/sqlc-dev/sqlc/internal/config" + "github.com/sqlc-dev/sqlc/internal/engine/sqlite" "github.com/sqlc-dev/sqlc/internal/source" "github.com/sqlc-dev/sqlc/internal/sql/ast" "github.com/sqlc-dev/sqlc/internal/sql/astutils" @@ -149,6 +150,17 @@ func (c *Compiler) expandStmt(qc *QueryCatalog, raw *ast.RawStmt, node ast.Node) if counts[cname] > 1 { cname = tableName + "." + cname } + // Under SQLite, neither json nor jsonb are real data types, and + // rather just of type blob, so database drivers just return + // whatever raw binary is stored as values. This is a problem + // for jsonb, which is considered an internal format to SQLite + // and no attempt should be made to parse it outside of the + // database itself. For jsonb columns in SQLite, wrap returned + // columns in `json(col)` to coerce the internal binary format + // to JSON parsable by the user-space application. + if _, ok := c.parser.(*sqlite.Parser); ok && column.DataType == "jsonb" { + cname = "json(" + cname + ")" + } cols = append(cols, cname) } } diff --git a/internal/endtoend/testdata/jsonb/pgx/go/db.go b/internal/endtoend/testdata/jsonb/pgx/go/db.go new file mode 100644 index 0000000000..e83d6a948c --- /dev/null +++ b/internal/endtoend/testdata/jsonb/pgx/go/db.go @@ -0,0 +1,32 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.29.0 + +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/jsonb/pgx/go/models.go b/internal/endtoend/testdata/jsonb/pgx/go/models.go new file mode 100644 index 0000000000..1932ce7f53 --- /dev/null +++ b/internal/endtoend/testdata/jsonb/pgx/go/models.go @@ -0,0 +1,12 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.29.0 + +package querytest + +type Foo struct { + A []byte + B []byte + C []byte + D []byte +} diff --git a/internal/endtoend/testdata/jsonb/pgx/go/query.sql.go b/internal/endtoend/testdata/jsonb/pgx/go/query.sql.go new file mode 100644 index 0000000000..1d7532f6ec --- /dev/null +++ b/internal/endtoend/testdata/jsonb/pgx/go/query.sql.go @@ -0,0 +1,50 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.29.0 +// source: query.sql + +package querytest + +import ( + "context" +) + +const insertFoo = `-- name: InsertFoo :exec +INSERT INTO foo ( + a, + b, + c, + d +) VALUES ( + $1, + $2, + $3, + $4 +) RETURNING a, b, c, d +` + +type InsertFooParams struct { + A []byte + B []byte + C []byte + D []byte +} + +func (q *Queries) InsertFoo(ctx context.Context, arg InsertFooParams) error { + _, err := q.db.Exec(ctx, insertFoo, + arg.A, + arg.B, + arg.C, + arg.D, + ) + return err +} + +const selectFoo = `-- name: SelectFoo :exec +SELECT a, b, c, d FROM foo +` + +func (q *Queries) SelectFoo(ctx context.Context) error { + _, err := q.db.Exec(ctx, selectFoo) + return err +} diff --git a/internal/endtoend/testdata/jsonb/pgx/query.sql b/internal/endtoend/testdata/jsonb/pgx/query.sql new file mode 100644 index 0000000000..6959bd1a70 --- /dev/null +++ b/internal/endtoend/testdata/jsonb/pgx/query.sql @@ -0,0 +1,15 @@ +-- name: InsertFoo :exec +INSERT INTO foo ( + a, + b, + c, + d +) VALUES ( + @a, + @b, + @c, + @d +) RETURNING *; + +-- name: SelectFoo :exec +SELECT * FROM foo; diff --git a/internal/endtoend/testdata/jsonb/pgx/schema.sql b/internal/endtoend/testdata/jsonb/pgx/schema.sql new file mode 100644 index 0000000000..6b4a1bb0fd --- /dev/null +++ b/internal/endtoend/testdata/jsonb/pgx/schema.sql @@ -0,0 +1,7 @@ +CREATE TABLE foo ( + a json not null, + b jsonb not null, + c json, + d jsonb +); + diff --git a/internal/endtoend/testdata/jsonb/pgx/sqlc.json b/internal/endtoend/testdata/jsonb/pgx/sqlc.json new file mode 100644 index 0000000000..32ede07158 --- /dev/null +++ b/internal/endtoend/testdata/jsonb/pgx/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/endtoend/testdata/jsonb/sqlite/go/db.go b/internal/endtoend/testdata/jsonb/sqlite/go/db.go new file mode 100644 index 0000000000..a92cd6e8eb --- /dev/null +++ b/internal/endtoend/testdata/jsonb/sqlite/go/db.go @@ -0,0 +1,31 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.29.0 + +package querytest + +import ( + "context" + "database/sql" +) + +type DBTX interface { + ExecContext(context.Context, string, ...interface{}) (sql.Result, error) + PrepareContext(context.Context, string) (*sql.Stmt, error) + QueryContext(context.Context, string, ...interface{}) (*sql.Rows, error) + QueryRowContext(context.Context, string, ...interface{}) *sql.Row +} + +func New(db DBTX) *Queries { + return &Queries{db: db} +} + +type Queries struct { + db DBTX +} + +func (q *Queries) WithTx(tx *sql.Tx) *Queries { + return &Queries{ + db: tx, + } +} diff --git a/internal/endtoend/testdata/jsonb/sqlite/go/models.go b/internal/endtoend/testdata/jsonb/sqlite/go/models.go new file mode 100644 index 0000000000..1932ce7f53 --- /dev/null +++ b/internal/endtoend/testdata/jsonb/sqlite/go/models.go @@ -0,0 +1,12 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.29.0 + +package querytest + +type Foo struct { + A []byte + B []byte + C []byte + D []byte +} diff --git a/internal/endtoend/testdata/jsonb/sqlite/go/query.sql.go b/internal/endtoend/testdata/jsonb/sqlite/go/query.sql.go new file mode 100644 index 0000000000..fdde010458 --- /dev/null +++ b/internal/endtoend/testdata/jsonb/sqlite/go/query.sql.go @@ -0,0 +1,50 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.29.0 +// source: query.sql + +package querytest + +import ( + "context" +) + +const insertFoo = `-- name: InsertFoo :exec +INSERT INTO foo ( + a, + b, + c, + d +) VALUES ( + ?1, + ?2, + ?3, + ?4 +) RETURNING a, json(b), c, json(d) +` + +type InsertFooParams struct { + A []byte + B []byte + C []byte + D []byte +} + +func (q *Queries) InsertFoo(ctx context.Context, arg InsertFooParams) error { + _, err := q.db.ExecContext(ctx, insertFoo, + arg.A, + arg.B, + arg.C, + arg.D, + ) + return err +} + +const selectFoo = `-- name: SelectFoo :exec +SELECT a, json(b), c, json(d) FROM foo +` + +func (q *Queries) SelectFoo(ctx context.Context) error { + _, err := q.db.ExecContext(ctx, selectFoo) + return err +} diff --git a/internal/endtoend/testdata/jsonb/sqlite/query.sql b/internal/endtoend/testdata/jsonb/sqlite/query.sql new file mode 100644 index 0000000000..6959bd1a70 --- /dev/null +++ b/internal/endtoend/testdata/jsonb/sqlite/query.sql @@ -0,0 +1,15 @@ +-- name: InsertFoo :exec +INSERT INTO foo ( + a, + b, + c, + d +) VALUES ( + @a, + @b, + @c, + @d +) RETURNING *; + +-- name: SelectFoo :exec +SELECT * FROM foo; diff --git a/internal/endtoend/testdata/jsonb/sqlite/schema.sql b/internal/endtoend/testdata/jsonb/sqlite/schema.sql new file mode 100644 index 0000000000..6b4a1bb0fd --- /dev/null +++ b/internal/endtoend/testdata/jsonb/sqlite/schema.sql @@ -0,0 +1,7 @@ +CREATE TABLE foo ( + a json not null, + b jsonb not null, + c json, + d jsonb +); + diff --git a/internal/endtoend/testdata/jsonb/sqlite/sqlc.json b/internal/endtoend/testdata/jsonb/sqlite/sqlc.json new file mode 100644 index 0000000000..cd66df063b --- /dev/null +++ b/internal/endtoend/testdata/jsonb/sqlite/sqlc.json @@ -0,0 +1,12 @@ +{ + "version": "1", + "packages": [ + { + "path": "go", + "engine": "sqlite", + "name": "querytest", + "schema": "schema.sql", + "queries": "query.sql" + } + ] +} From 967f6533ab4b8578bf7b29051f307fc0bb6651ea Mon Sep 17 00:00:00 2001 From: Brandur Date: Sun, 25 May 2025 17:41:30 -0600 Subject: [PATCH 2/4] Make SQLite json/jsonb values `json.RawMessage` instead of `[]byte` This in response to code review feedback, it's a little more correct to make json/jsonb values `json.RawMessage` instead of `[]byte`. The former is a form of the latter, but better represents that the value is meant to be JSON. --- internal/codegen/golang/sqlite_type.go | 2 +- internal/endtoend/testdata/jsonb/sqlite/go/models.go | 12 ++++++++---- .../endtoend/testdata/jsonb/sqlite/go/query.sql.go | 9 +++++---- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/internal/codegen/golang/sqlite_type.go b/internal/codegen/golang/sqlite_type.go index cb7348385f..8a22aaa262 100644 --- a/internal/codegen/golang/sqlite_type.go +++ b/internal/codegen/golang/sqlite_type.go @@ -57,7 +57,7 @@ func sqliteType(req *plugin.GenerateRequest, options *opts.Options, col *plugin. return "sql.NullTime" case "json", "jsonb": - return "[]byte" + return "json.RawMessage" case "any": return "interface{}" diff --git a/internal/endtoend/testdata/jsonb/sqlite/go/models.go b/internal/endtoend/testdata/jsonb/sqlite/go/models.go index 1932ce7f53..7c4f7cd8c7 100644 --- a/internal/endtoend/testdata/jsonb/sqlite/go/models.go +++ b/internal/endtoend/testdata/jsonb/sqlite/go/models.go @@ -4,9 +4,13 @@ package querytest +import ( + "encoding/json" +) + type Foo struct { - A []byte - B []byte - C []byte - D []byte + A json.RawMessage + B json.RawMessage + C json.RawMessage + D json.RawMessage } diff --git a/internal/endtoend/testdata/jsonb/sqlite/go/query.sql.go b/internal/endtoend/testdata/jsonb/sqlite/go/query.sql.go index fdde010458..9c0858a9c3 100644 --- a/internal/endtoend/testdata/jsonb/sqlite/go/query.sql.go +++ b/internal/endtoend/testdata/jsonb/sqlite/go/query.sql.go @@ -7,6 +7,7 @@ package querytest import ( "context" + "encoding/json" ) const insertFoo = `-- name: InsertFoo :exec @@ -24,10 +25,10 @@ INSERT INTO foo ( ` type InsertFooParams struct { - A []byte - B []byte - C []byte - D []byte + A json.RawMessage + B json.RawMessage + C json.RawMessage + D json.RawMessage } func (q *Queries) InsertFoo(ctx context.Context, arg InsertFooParams) error { From 05552309cd3f65af1931e7705f8137d2c1df7c7e Mon Sep 17 00:00:00 2001 From: Brandur Date: Sun, 25 May 2025 17:43:00 -0600 Subject: [PATCH 3/4] Introduce `Selector` interface for generating column expressions This is response to code review feedback, add a new `Selector `interface whose job it is to provide an engine-agnostic way of generating output expressions for when selecting column values with `SELECT ...` or `RETURNING ...`. This is exclusively needed for SQLite for the time being, which uses it to wrap all output `jsonb` column values with a call to `json(...)` so that values are coerced to a publicly usable format before being returned. [1] https://github.com/sqlc-dev/sqlc/pull/3968#discussion_r2101626274 --- internal/compiler/engine.go | 5 +++++ internal/compiler/expand.go | 17 +++++----------- internal/engine/sqlite/selector.go | 21 ++++++++++++++++++++ internal/engine/sqlite/selector_test.go | 20 +++++++++++++++++++ internal/sql/selector/selector.go | 26 +++++++++++++++++++++++++ internal/sql/selector/selector_test.go | 20 +++++++++++++++++++ 6 files changed, 97 insertions(+), 12 deletions(-) create mode 100644 internal/engine/sqlite/selector.go create mode 100644 internal/engine/sqlite/selector_test.go create mode 100644 internal/sql/selector/selector.go create mode 100644 internal/sql/selector/selector_test.go diff --git a/internal/compiler/engine.go b/internal/compiler/engine.go index d263637d9f..2332776a36 100644 --- a/internal/compiler/engine.go +++ b/internal/compiler/engine.go @@ -13,6 +13,7 @@ import ( "github.com/sqlc-dev/sqlc/internal/engine/sqlite" "github.com/sqlc-dev/sqlc/internal/opts" "github.com/sqlc-dev/sqlc/internal/sql/catalog" + "github.com/sqlc-dev/sqlc/internal/sql/selector" ) type Compiler struct { @@ -23,6 +24,7 @@ type Compiler struct { result *Result analyzer analyzer.Analyzer client dbmanager.Client + selector selector.Selector schema []string } @@ -39,12 +41,15 @@ func NewCompiler(conf config.SQL, combo config.CombinedSettings) (*Compiler, err case config.EngineSQLite: c.parser = sqlite.NewParser() c.catalog = sqlite.NewCatalog() + c.selector = sqlite.NewSelector() case config.EngineMySQL: c.parser = dolphin.NewParser() c.catalog = dolphin.NewCatalog() + c.selector = selector.NewDefaultSelector() case config.EnginePostgreSQL: c.parser = postgresql.NewParser() c.catalog = postgresql.NewCatalog() + c.selector = selector.NewDefaultSelector() if conf.Database != nil { if conf.Analyzer.Database == nil || *conf.Analyzer.Database { c.analyzer = analyzer.Cached( diff --git a/internal/compiler/expand.go b/internal/compiler/expand.go index 2f04e9a6f1..17771070f4 100644 --- a/internal/compiler/expand.go +++ b/internal/compiler/expand.go @@ -6,7 +6,6 @@ import ( "strings" "github.com/sqlc-dev/sqlc/internal/config" - "github.com/sqlc-dev/sqlc/internal/engine/sqlite" "github.com/sqlc-dev/sqlc/internal/source" "github.com/sqlc-dev/sqlc/internal/sql/ast" "github.com/sqlc-dev/sqlc/internal/sql/astutils" @@ -150,17 +149,11 @@ func (c *Compiler) expandStmt(qc *QueryCatalog, raw *ast.RawStmt, node ast.Node) if counts[cname] > 1 { cname = tableName + "." + cname } - // Under SQLite, neither json nor jsonb are real data types, and - // rather just of type blob, so database drivers just return - // whatever raw binary is stored as values. This is a problem - // for jsonb, which is considered an internal format to SQLite - // and no attempt should be made to parse it outside of the - // database itself. For jsonb columns in SQLite, wrap returned - // columns in `json(col)` to coerce the internal binary format - // to JSON parsable by the user-space application. - if _, ok := c.parser.(*sqlite.Parser); ok && column.DataType == "jsonb" { - cname = "json(" + cname + ")" - } + + // This is important for SQLite in particular which needs to + // wrap jsonb column values with `json(colname)` so they're in a + // publicly usable format (i.e. not jsonb). + cname = c.selector.ColumnExpr(cname, column.DataType) cols = append(cols, cname) } } diff --git a/internal/engine/sqlite/selector.go b/internal/engine/sqlite/selector.go new file mode 100644 index 0000000000..c2fc144590 --- /dev/null +++ b/internal/engine/sqlite/selector.go @@ -0,0 +1,21 @@ +package sqlite + +type Selector struct{} + +func NewSelector() *Selector { + return &Selector{} +} + +func (s *Selector) ColumnExpr(name string, dataType string) string { + // Under SQLite, neither json nor jsonb are real data types, and rather just + // of type blob, so database drivers just return whatever raw binary is + // stored as values. This is a problem for jsonb, which is considered an + // internal format to SQLite and no attempt should be made to parse it + // outside of the database itself. For jsonb columns in SQLite, wrap values + // in `json(col)` to coerce the internal binary format to JSON parsable by + // the user-space application. + if dataType == "jsonb" { + return "json(" + name + ")" + } + return name +} diff --git a/internal/engine/sqlite/selector_test.go b/internal/engine/sqlite/selector_test.go new file mode 100644 index 0000000000..3a8fb2c056 --- /dev/null +++ b/internal/engine/sqlite/selector_test.go @@ -0,0 +1,20 @@ +package sqlite + +import "testing" + +func TestSelectorColumnExpr(t *testing.T) { + t.Parallel() + + selector := NewSelector() + + expectExpr := func(expected, name, dataType string) { + if actual := selector.ColumnExpr(name, dataType); expected != actual { + t.Errorf("Expected %v, got %v for data type %v", expected, actual, dataType) + } + } + + expectExpr("my_column", "my_column", "integer") + expectExpr("my_column", "my_column", "json") + expectExpr("json(my_column)", "my_column", "jsonb") + expectExpr("my_column", "my_column", "text") +} diff --git a/internal/sql/selector/selector.go b/internal/sql/selector/selector.go new file mode 100644 index 0000000000..63f8ebc864 --- /dev/null +++ b/internal/sql/selector/selector.go @@ -0,0 +1,26 @@ +package selector + +// Selector is an interface used by a compiler for generating expressions for +// output columns in a `SELECT ...` or `RETURNING ...` statement. +// +// This interface is exclusively needed at the moment for SQLite, which must +// wrap output `jsonb` columns with a `json(column_name)` invocation so that a +// publicly consumable format (i.e. not jsonb) is returned. +type Selector interface { + // ColumnExpr generates output to be used in a `SELECT ...` or `RETURNING + // ...` statement based on input column name and metadata. + ColumnExpr(name string, dataType string) string +} + +// DefaultSelector is a Selector implementation that does the simpliest possible +// pass through when generating column expressions. Its use is suitable for all +// database engines not requiring additional customization. +type DefaultSelector struct{} + +func NewDefaultSelector() *DefaultSelector { + return &DefaultSelector{} +} + +func (s *DefaultSelector) ColumnExpr(name string, dataType string) string { + return name +} diff --git a/internal/sql/selector/selector_test.go b/internal/sql/selector/selector_test.go new file mode 100644 index 0000000000..44d5f767fa --- /dev/null +++ b/internal/sql/selector/selector_test.go @@ -0,0 +1,20 @@ +package selector + +import "testing" + +func TestDefaultSelectorColumnExpr(t *testing.T) { + t.Parallel() + + selector := NewDefaultSelector() + + expectExpr := func(expected, name, dataType string) { + if actual := selector.ColumnExpr(name, dataType); expected != actual { + t.Errorf("Expected %v, got %v for data type %v", expected, actual, dataType) + } + } + + expectExpr("my_column", "my_column", "integer") + expectExpr("my_column", "my_column", "json") + expectExpr("my_column", "my_column", "jsonb") + expectExpr("my_column", "my_column", "text") +} From c62c6e78843ada0f137960d455baf638390b5c7b Mon Sep 17 00:00:00 2001 From: Brandur Date: Sun, 25 May 2025 17:56:52 -0600 Subject: [PATCH 4/4] Make selectors internal to the `compiler` package This is based on original code review feedback that I'd misread initially. The selector interface doesn't need to be an outside package for any reason (it's used only internal to the compiler), and this lets us improve it somewhat by taking a full `*Column` struct rather than having to make it a `dataType string` (because `Column` is internal to `compiler` and it would otherwise introduce dependency cycles). --- internal/compiler/engine.go | 9 +++-- internal/compiler/expand.go | 2 +- internal/compiler/selector.go | 46 +++++++++++++++++++++++++ internal/compiler/selector_test.go | 35 +++++++++++++++++++ internal/engine/sqlite/selector.go | 21 ----------- internal/engine/sqlite/selector_test.go | 20 ----------- internal/sql/selector/selector.go | 26 -------------- internal/sql/selector/selector_test.go | 20 ----------- 8 files changed, 86 insertions(+), 93 deletions(-) create mode 100644 internal/compiler/selector.go create mode 100644 internal/compiler/selector_test.go delete mode 100644 internal/engine/sqlite/selector.go delete mode 100644 internal/engine/sqlite/selector_test.go delete mode 100644 internal/sql/selector/selector.go delete mode 100644 internal/sql/selector/selector_test.go diff --git a/internal/compiler/engine.go b/internal/compiler/engine.go index 2332776a36..f742bfd999 100644 --- a/internal/compiler/engine.go +++ b/internal/compiler/engine.go @@ -13,7 +13,6 @@ import ( "github.com/sqlc-dev/sqlc/internal/engine/sqlite" "github.com/sqlc-dev/sqlc/internal/opts" "github.com/sqlc-dev/sqlc/internal/sql/catalog" - "github.com/sqlc-dev/sqlc/internal/sql/selector" ) type Compiler struct { @@ -24,7 +23,7 @@ type Compiler struct { result *Result analyzer analyzer.Analyzer client dbmanager.Client - selector selector.Selector + selector selector schema []string } @@ -41,15 +40,15 @@ func NewCompiler(conf config.SQL, combo config.CombinedSettings) (*Compiler, err case config.EngineSQLite: c.parser = sqlite.NewParser() c.catalog = sqlite.NewCatalog() - c.selector = sqlite.NewSelector() + c.selector = newSQLiteSelector() case config.EngineMySQL: c.parser = dolphin.NewParser() c.catalog = dolphin.NewCatalog() - c.selector = selector.NewDefaultSelector() + c.selector = newDefaultSelector() case config.EnginePostgreSQL: c.parser = postgresql.NewParser() c.catalog = postgresql.NewCatalog() - c.selector = selector.NewDefaultSelector() + c.selector = newDefaultSelector() if conf.Database != nil { if conf.Analyzer.Database == nil || *conf.Analyzer.Database { c.analyzer = analyzer.Cached( diff --git a/internal/compiler/expand.go b/internal/compiler/expand.go index 17771070f4..c60b7618b2 100644 --- a/internal/compiler/expand.go +++ b/internal/compiler/expand.go @@ -153,7 +153,7 @@ func (c *Compiler) expandStmt(qc *QueryCatalog, raw *ast.RawStmt, node ast.Node) // This is important for SQLite in particular which needs to // wrap jsonb column values with `json(colname)` so they're in a // publicly usable format (i.e. not jsonb). - cname = c.selector.ColumnExpr(cname, column.DataType) + cname = c.selector.ColumnExpr(cname, column) cols = append(cols, cname) } } diff --git a/internal/compiler/selector.go b/internal/compiler/selector.go new file mode 100644 index 0000000000..04d118ff9c --- /dev/null +++ b/internal/compiler/selector.go @@ -0,0 +1,46 @@ +package compiler + +// selector is an interface used by a compiler for generating expressions for +// output columns in a `SELECT ...` or `RETURNING ...` statement. +// +// This interface is exclusively needed at the moment for SQLite, which must +// wrap output `jsonb` columns with a `json(column_name)` invocation so that a +// publicly consumable format (i.e. not jsonb) is returned. +type selector interface { + // ColumnExpr generates output to be used in a `SELECT ...` or `RETURNING + // ...` statement based on input column name and metadata. + ColumnExpr(name string, column *Column) string +} + +// defaultSelector is a selector implementation that does the simpliest possible +// pass through when generating column expressions. Its use is suitable for all +// database engines not requiring additional customization. +type defaultSelector struct{} + +func newDefaultSelector() *defaultSelector { + return &defaultSelector{} +} + +func (s *defaultSelector) ColumnExpr(name string, column *Column) string { + return name +} + +type sqliteSelector struct{} + +func newSQLiteSelector() *sqliteSelector { + return &sqliteSelector{} +} + +func (s *sqliteSelector) ColumnExpr(name string, column *Column) string { + // Under SQLite, neither json nor jsonb are real data types, and rather just + // of type blob, so database drivers just return whatever raw binary is + // stored as values. This is a problem for jsonb, which is considered an + // internal format to SQLite and no attempt should be made to parse it + // outside of the database itself. For jsonb columns in SQLite, wrap values + // in `json(col)` to coerce the internal binary format to JSON parsable by + // the user-space application. + if column.DataType == "jsonb" { + return "json(" + name + ")" + } + return name +} diff --git a/internal/compiler/selector_test.go b/internal/compiler/selector_test.go new file mode 100644 index 0000000000..e460dd281c --- /dev/null +++ b/internal/compiler/selector_test.go @@ -0,0 +1,35 @@ +package compiler + +import "testing" + +func TestSelector(t *testing.T) { + t.Parallel() + + selectorExpectColumnExpr := func(t *testing.T, selector selector, expected, name string, column *Column) { + if actual := selector.ColumnExpr(name, column); expected != actual { + t.Errorf("Expected %v, got %v for data type %v", expected, actual, column.DataType) + } + } + + t.Run("DefaultSelectorColumnExpr", func(t *testing.T) { + t.Parallel() + + selector := newDefaultSelector() + + selectorExpectColumnExpr(t, selector, "my_column", "my_column", &Column{DataType: "integer"}) + selectorExpectColumnExpr(t, selector, "my_column", "my_column", &Column{DataType: "json"}) + selectorExpectColumnExpr(t, selector, "my_column", "my_column", &Column{DataType: "jsonb"}) + selectorExpectColumnExpr(t, selector, "my_column", "my_column", &Column{DataType: "text"}) + }) + + t.Run("SQLiteSelectorColumnExpr", func(t *testing.T) { + t.Parallel() + + selector := newSQLiteSelector() + + selectorExpectColumnExpr(t, selector, "my_column", "my_column", &Column{DataType: "integer"}) + selectorExpectColumnExpr(t, selector, "my_column", "my_column", &Column{DataType: "json"}) + selectorExpectColumnExpr(t, selector, "json(my_column)", "my_column", &Column{DataType: "jsonb"}) + selectorExpectColumnExpr(t, selector, "my_column", "my_column", &Column{DataType: "text"}) + }) +} diff --git a/internal/engine/sqlite/selector.go b/internal/engine/sqlite/selector.go deleted file mode 100644 index c2fc144590..0000000000 --- a/internal/engine/sqlite/selector.go +++ /dev/null @@ -1,21 +0,0 @@ -package sqlite - -type Selector struct{} - -func NewSelector() *Selector { - return &Selector{} -} - -func (s *Selector) ColumnExpr(name string, dataType string) string { - // Under SQLite, neither json nor jsonb are real data types, and rather just - // of type blob, so database drivers just return whatever raw binary is - // stored as values. This is a problem for jsonb, which is considered an - // internal format to SQLite and no attempt should be made to parse it - // outside of the database itself. For jsonb columns in SQLite, wrap values - // in `json(col)` to coerce the internal binary format to JSON parsable by - // the user-space application. - if dataType == "jsonb" { - return "json(" + name + ")" - } - return name -} diff --git a/internal/engine/sqlite/selector_test.go b/internal/engine/sqlite/selector_test.go deleted file mode 100644 index 3a8fb2c056..0000000000 --- a/internal/engine/sqlite/selector_test.go +++ /dev/null @@ -1,20 +0,0 @@ -package sqlite - -import "testing" - -func TestSelectorColumnExpr(t *testing.T) { - t.Parallel() - - selector := NewSelector() - - expectExpr := func(expected, name, dataType string) { - if actual := selector.ColumnExpr(name, dataType); expected != actual { - t.Errorf("Expected %v, got %v for data type %v", expected, actual, dataType) - } - } - - expectExpr("my_column", "my_column", "integer") - expectExpr("my_column", "my_column", "json") - expectExpr("json(my_column)", "my_column", "jsonb") - expectExpr("my_column", "my_column", "text") -} diff --git a/internal/sql/selector/selector.go b/internal/sql/selector/selector.go deleted file mode 100644 index 63f8ebc864..0000000000 --- a/internal/sql/selector/selector.go +++ /dev/null @@ -1,26 +0,0 @@ -package selector - -// Selector is an interface used by a compiler for generating expressions for -// output columns in a `SELECT ...` or `RETURNING ...` statement. -// -// This interface is exclusively needed at the moment for SQLite, which must -// wrap output `jsonb` columns with a `json(column_name)` invocation so that a -// publicly consumable format (i.e. not jsonb) is returned. -type Selector interface { - // ColumnExpr generates output to be used in a `SELECT ...` or `RETURNING - // ...` statement based on input column name and metadata. - ColumnExpr(name string, dataType string) string -} - -// DefaultSelector is a Selector implementation that does the simpliest possible -// pass through when generating column expressions. Its use is suitable for all -// database engines not requiring additional customization. -type DefaultSelector struct{} - -func NewDefaultSelector() *DefaultSelector { - return &DefaultSelector{} -} - -func (s *DefaultSelector) ColumnExpr(name string, dataType string) string { - return name -} diff --git a/internal/sql/selector/selector_test.go b/internal/sql/selector/selector_test.go deleted file mode 100644 index 44d5f767fa..0000000000 --- a/internal/sql/selector/selector_test.go +++ /dev/null @@ -1,20 +0,0 @@ -package selector - -import "testing" - -func TestDefaultSelectorColumnExpr(t *testing.T) { - t.Parallel() - - selector := NewDefaultSelector() - - expectExpr := func(expected, name, dataType string) { - if actual := selector.ColumnExpr(name, dataType); expected != actual { - t.Errorf("Expected %v, got %v for data type %v", expected, actual, dataType) - } - } - - expectExpr("my_column", "my_column", "integer") - expectExpr("my_column", "my_column", "json") - expectExpr("my_column", "my_column", "jsonb") - expectExpr("my_column", "my_column", "text") -}