From 2a36a7032e6efa9d84d09e84a19a5561091f335f Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 18 May 2024 07:41:10 -0500 Subject: [PATCH 1/9] Fix encode driver.Valuer on pointer pgx v5 introduced nil normalization for typed nils. This means that []byte(nil) is normalized to nil at the edge of the encoding system. This simplified encoding logic as nil could be encoded as NULL and type specific handling was unneeded. However, database/sql compatibility requires Value to be called on a nil pointer that implements driver.Valuer. This was broken by normalizing to nil. This commit changes the normalization logic to not normalize pointers that directly implement driver.Valuer to nil. It still normalizes pointers that implement driver.Valuer through implicit derefence. e.g. type T struct{} func (t *T) Value() (driver.Value, error) { return nil, nil } type S struct{} func (s S) Value() (driver.Value, error) { return nil, nil } (*T)(nil) will not be normalized to nil but (*S)(nil) will be. https://github.com/jackc/pgx/issues/1566 --- internal/anynil/anynil.go | 38 +++++++++++++++++++++++++---- pgtype/doc.go | 10 ++++++++ query_test.go | 50 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 4 deletions(-) diff --git a/internal/anynil/anynil.go b/internal/anynil/anynil.go index 9a48c1a84..314e2bbb2 100644 --- a/internal/anynil/anynil.go +++ b/internal/anynil/anynil.go @@ -1,17 +1,47 @@ package anynil -import "reflect" +import ( + "database/sql/driver" + "reflect" +) -// Is returns true if value is any type of nil. e.g. nil or []byte(nil). +// valuerReflectType is a reflect.Type for driver.Valuer. It has confusing syntax because reflect.TypeOf returns nil +// when it's argument is a nil interface value. So we use a pointer to the interface and call Elem to get the actual +// type. Yuck. +// +// This can be simplified in Go 1.22 with reflect.TypeFor. +// +// var valuerReflectType = reflect.TypeFor[driver.Valuer]() +var valuerReflectType = reflect.TypeOf((*driver.Valuer)(nil)).Elem() + +// Is returns true if value is any type of nil except a pointer that directly implements driver.Valuer. e.g. nil, +// []byte(nil), and a *T where T implements driver.Valuer get normalized to nil but a *T where *T implements +// driver.Valuer does not. func Is(value any) bool { if value == nil { return true } refVal := reflect.ValueOf(value) - switch refVal.Kind() { + kind := refVal.Kind() + switch kind { case reflect.Chan, reflect.Func, reflect.Map, reflect.Ptr, reflect.UnsafePointer, reflect.Interface, reflect.Slice: - return refVal.IsNil() + if !refVal.IsNil() { + return false + } + + if kind == reflect.Ptr { + if _, ok := value.(driver.Valuer); ok { + // The pointer will be considered to implement driver.Valuer even if it is actually implemented on the value. + // But we only want to consider it nil if it is implemented on the pointer. So check if what the pointer points + // to implements driver.Valuer. + if !refVal.Type().Elem().Implements(valuerReflectType) { + return false + } + } + } + + return true default: return false } diff --git a/pgtype/doc.go b/pgtype/doc.go index ec9270acb..8e5023038 100644 --- a/pgtype/doc.go +++ b/pgtype/doc.go @@ -139,6 +139,16 @@ Compatibility with database/sql pgtype also includes support for custom types implementing the database/sql.Scanner and database/sql/driver.Valuer interfaces. +Encoding Typed Nils + +pgtype normalizes typed nils (e.g. []byte(nil)) into nil. nil is always encoded is the SQL NULL value without going +through the Codec system. This means that Codecs and other encoding logic does not have to handle nil or *T(nil). + +However, database/sql compatibility requires Value to be called on a pointer that implements driver.Valuer. See +https://github.com/golang/go/issues/8415 and +https://github.com/golang/go/commit/0ce1d79a6a771f7449ec493b993ed2a720917870. Therefore, pointers that implement +driver.Valuer are not normalized to nil. + Child Records pgtype's support for arrays and composite records can be used to load records and their children in a single query. See diff --git a/query_test.go b/query_test.go index df044cdea..550e8cb89 100644 --- a/query_test.go +++ b/query_test.go @@ -4,6 +4,8 @@ import ( "bytes" "context" "database/sql" + "database/sql/driver" + "encoding/json" "errors" "fmt" "os" @@ -1171,6 +1173,54 @@ func TestConnQueryDatabaseSQLDriverValuerWithAutoGeneratedPointerReceiver(t *tes ensureConnValid(t, conn) } +type nilAsEmptyJSONObject struct { + ID string + Name string +} + +func (v *nilAsEmptyJSONObject) Value() (driver.Value, error) { + if v == nil { + return "{}", nil + } + + return json.Marshal(v) +} + +// https://github.com/jackc/pgx/issues/1566 +func TestConnQueryDatabaseSQLDriverValuerCalledOnPointerImplementers(t *testing.T) { + t.Parallel() + + conn := mustConnectString(t, os.Getenv("PGX_TEST_DATABASE")) + defer closeConn(t, conn) + + mustExec(t, conn, "create temporary table t(v json not null)") + + var v *nilAsEmptyJSONObject + commandTag, err := conn.Exec(context.Background(), `insert into t(v) values($1)`, v) + require.NoError(t, err) + require.Equal(t, "INSERT 0 1", commandTag.String()) + + var s string + err = conn.QueryRow(context.Background(), "select v from t").Scan(&s) + require.NoError(t, err) + require.Equal(t, "{}", s) + + _, err = conn.Exec(context.Background(), `delete from t`) + require.NoError(t, err) + + v = &nilAsEmptyJSONObject{ID: "1", Name: "foo"} + commandTag, err = conn.Exec(context.Background(), `insert into t(v) values($1)`, v) + require.NoError(t, err) + require.Equal(t, "INSERT 0 1", commandTag.String()) + + var v2 *nilAsEmptyJSONObject + err = conn.QueryRow(context.Background(), "select v from t").Scan(&v2) + require.NoError(t, err) + require.Equal(t, v, v2) + + ensureConnValid(t, conn) +} + func TestConnQueryDatabaseSQLDriverScannerWithBinaryPgTypeThatAcceptsSameType(t *testing.T) { t.Parallel() From 3b7fa4ce87fb5dfd1fe1a9942b9a09cda5957dcb Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 18 May 2024 16:43:24 -0500 Subject: [PATCH 2/9] Use go 1.20 in go.mod Future commit will be using bytes.Clone which was implemented in Go 1.20. Also update README.md to reflect that minimum supported Go version is 1.21. But only requiring Go 1.20 in go.mod to avoid needlessly breaking old Go when it still works. --- README.md | 2 +- go.mod | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 49f2c3d78..0cf2c2916 100644 --- a/README.md +++ b/README.md @@ -92,7 +92,7 @@ See the presentation at Golang Estonia, [PGX Top to Bottom](https://www.youtube. ## Supported Go and PostgreSQL Versions -pgx supports the same versions of Go and PostgreSQL that are supported by their respective teams. For [Go](https://golang.org/doc/devel/release.html#policy) that is the two most recent major releases and for [PostgreSQL](https://www.postgresql.org/support/versioning/) the major releases in the last 5 years. This means pgx supports Go 1.20 and higher and PostgreSQL 12 and higher. pgx also is tested against the latest version of [CockroachDB](https://www.cockroachlabs.com/product/). +pgx supports the same versions of Go and PostgreSQL that are supported by their respective teams. For [Go](https://golang.org/doc/devel/release.html#policy) that is the two most recent major releases and for [PostgreSQL](https://www.postgresql.org/support/versioning/) the major releases in the last 5 years. This means pgx supports Go 1.21 and higher and PostgreSQL 12 and higher. pgx also is tested against the latest version of [CockroachDB](https://www.cockroachlabs.com/product/). ## Version Policy diff --git a/go.mod b/go.mod index c8430a417..e27bf8a48 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/jackc/pgx/v5 -go 1.19 +go 1.20 require ( github.com/jackc/pgpassfile v1.0.0 From fec45c802ba9f19107aa24f9aea46f2f21577621 Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 18 May 2024 17:00:41 -0500 Subject: [PATCH 3/9] Refactor appendParamsForQueryExecModeExec Extract logic for finding OID and converting argument to encodable value. This is in preparation for a future change for better supporting nil driver.Valuer values. --- extended_query_builder.go | 93 ++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 50 deletions(-) diff --git a/extended_query_builder.go b/extended_query_builder.go index 9c9de5b2c..0056cec7c 100644 --- a/extended_query_builder.go +++ b/extended_query_builder.go @@ -161,58 +161,51 @@ func (eqb *ExtendedQueryBuilder) chooseParameterFormatCode(m *pgtype.Map, oid ui // no way to safely use binary or to specify the parameter OIDs. func (eqb *ExtendedQueryBuilder) appendParamsForQueryExecModeExec(m *pgtype.Map, args []any) error { for _, arg := range args { - if arg == nil { - err := eqb.appendParam(m, 0, TextFormatCode, arg) - if err != nil { - return err - } - } else { - dt, ok := m.TypeForValue(arg) - if !ok { - var tv pgtype.TextValuer - if tv, ok = arg.(pgtype.TextValuer); ok { - t, err := tv.TextValue() - if err != nil { - return err - } - - dt, ok = m.TypeForOID(pgtype.TextOID) - if ok { - arg = t - } - } - } - if !ok { - var dv driver.Valuer - if dv, ok = arg.(driver.Valuer); ok { - v, err := dv.Value() - if err != nil { - return err - } - dt, ok = m.TypeForValue(v) - if ok { - arg = v - } - } - } - if !ok { - var str fmt.Stringer - if str, ok = arg.(fmt.Stringer); ok { - dt, ok = m.TypeForOID(pgtype.TextOID) - if ok { - arg = str.String() - } - } - } - if !ok { - return &unknownArgumentTypeQueryExecModeExecError{arg: arg} - } - err := eqb.appendParam(m, dt.OID, TextFormatCode, arg) - if err != nil { - return err - } + oid, modArg, err := eqb.oidAndArgForQueryExecModeExec(m, arg) + if err != nil { + return err + } + + err = eqb.appendParam(m, oid, pgtype.TextFormatCode, modArg) + if err != nil { + return err } } return nil } + +func (eqb *ExtendedQueryBuilder) oidAndArgForQueryExecModeExec(m *pgtype.Map, arg any) (uint32, any, error) { + if arg == nil { + return 0, arg, nil + } + + if dt, ok := m.TypeForValue(arg); ok { + return dt.OID, arg, nil + } + + if textValuer, ok := arg.(pgtype.TextValuer); ok { + tv, err := textValuer.TextValue() + if err != nil { + return 0, nil, err + } + + return pgtype.TextOID, tv, nil + } + + if dv, ok := arg.(driver.Valuer); ok { + v, err := dv.Value() + if err != nil { + return 0, nil, err + } + if dt, ok := m.TypeForValue(v); ok { + return dt.OID, v, nil + } + } + + if str, ok := arg.(fmt.Stringer); ok { + return pgtype.TextOID, str.String(), nil + } + + return 0, nil, &unknownArgumentTypeQueryExecModeExecError{arg: arg} +} From 13beb380f51e3d744e27e7d55a3078ac8753ccf9 Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 18 May 2024 17:17:46 -0500 Subject: [PATCH 4/9] Fix encode driver.Valuer on nil-able non-pointers https://github.com/jackc/pgx/issues/1566 https://github.com/jackc/pgx/issues/1860 https://github.com/jackc/pgx/pull/2019#discussion_r1605806751 --- extended_query_builder.go | 5 ++ internal/anynil/anynil.go | 20 +++---- pgtype/doc.go | 6 +- query_test.go | 119 ++++++++++++++++++++++++++++++++++++-- 4 files changed, 130 insertions(+), 20 deletions(-) diff --git a/extended_query_builder.go b/extended_query_builder.go index 0056cec7c..522a70e38 100644 --- a/extended_query_builder.go +++ b/extended_query_builder.go @@ -198,6 +198,11 @@ func (eqb *ExtendedQueryBuilder) oidAndArgForQueryExecModeExec(m *pgtype.Map, ar if err != nil { return 0, nil, err } + + if v == nil { + return 0, nil, nil + } + if dt, ok := m.TypeForValue(v); ok { return dt.OID, v, nil } diff --git a/internal/anynil/anynil.go b/internal/anynil/anynil.go index 314e2bbb2..967dcf415 100644 --- a/internal/anynil/anynil.go +++ b/internal/anynil/anynil.go @@ -14,9 +14,8 @@ import ( // var valuerReflectType = reflect.TypeFor[driver.Valuer]() var valuerReflectType = reflect.TypeOf((*driver.Valuer)(nil)).Elem() -// Is returns true if value is any type of nil except a pointer that directly implements driver.Valuer. e.g. nil, -// []byte(nil), and a *T where T implements driver.Valuer get normalized to nil but a *T where *T implements -// driver.Valuer does not. +// Is returns true if value is any type of nil unless it implements driver.Valuer. *T is not considered to implement +// driver.Valuer if it is only implemented by T. func Is(value any) bool { if value == nil { return true @@ -30,14 +29,13 @@ func Is(value any) bool { return false } - if kind == reflect.Ptr { - if _, ok := value.(driver.Valuer); ok { - // The pointer will be considered to implement driver.Valuer even if it is actually implemented on the value. - // But we only want to consider it nil if it is implemented on the pointer. So check if what the pointer points - // to implements driver.Valuer. - if !refVal.Type().Elem().Implements(valuerReflectType) { - return false - } + if _, ok := value.(driver.Valuer); ok { + if kind == reflect.Ptr { + // The type assertion will succeed if driver.Valuer is implemented on T or *T. Check if it is implemented on T + // to see if it is not implemented on *T. + return refVal.Type().Elem().Implements(valuerReflectType) + } else { + return false } } diff --git a/pgtype/doc.go b/pgtype/doc.go index 8e5023038..2039fcf1d 100644 --- a/pgtype/doc.go +++ b/pgtype/doc.go @@ -144,10 +144,10 @@ Encoding Typed Nils pgtype normalizes typed nils (e.g. []byte(nil)) into nil. nil is always encoded is the SQL NULL value without going through the Codec system. This means that Codecs and other encoding logic does not have to handle nil or *T(nil). -However, database/sql compatibility requires Value to be called on a pointer that implements driver.Valuer. See +However, database/sql compatibility requires Value to be called on T(nil) when T implements driver.Valuer. Therefore, +driver.Valuer values are not normalized to nil unless it is a *T(nil) where driver.Valuer is implemented on T. See https://github.com/golang/go/issues/8415 and -https://github.com/golang/go/commit/0ce1d79a6a771f7449ec493b993ed2a720917870. Therefore, pointers that implement -driver.Valuer are not normalized to nil. +https://github.com/golang/go/commit/0ce1d79a6a771f7449ec493b993ed2a720917870. Child Records diff --git a/query_test.go b/query_test.go index 550e8cb89..a6a26ad77 100644 --- a/query_test.go +++ b/query_test.go @@ -1173,12 +1173,12 @@ func TestConnQueryDatabaseSQLDriverValuerWithAutoGeneratedPointerReceiver(t *tes ensureConnValid(t, conn) } -type nilAsEmptyJSONObject struct { +type nilPointerAsEmptyJSONObject struct { ID string Name string } -func (v *nilAsEmptyJSONObject) Value() (driver.Value, error) { +func (v *nilPointerAsEmptyJSONObject) Value() (driver.Value, error) { if v == nil { return "{}", nil } @@ -1187,7 +1187,7 @@ func (v *nilAsEmptyJSONObject) Value() (driver.Value, error) { } // https://github.com/jackc/pgx/issues/1566 -func TestConnQueryDatabaseSQLDriverValuerCalledOnPointerImplementers(t *testing.T) { +func TestConnQueryDatabaseSQLDriverValuerCalledOnNilPointerImplementers(t *testing.T) { t.Parallel() conn := mustConnectString(t, os.Getenv("PGX_TEST_DATABASE")) @@ -1195,7 +1195,7 @@ func TestConnQueryDatabaseSQLDriverValuerCalledOnPointerImplementers(t *testing. mustExec(t, conn, "create temporary table t(v json not null)") - var v *nilAsEmptyJSONObject + var v *nilPointerAsEmptyJSONObject commandTag, err := conn.Exec(context.Background(), `insert into t(v) values($1)`, v) require.NoError(t, err) require.Equal(t, "INSERT 0 1", commandTag.String()) @@ -1208,12 +1208,119 @@ func TestConnQueryDatabaseSQLDriverValuerCalledOnPointerImplementers(t *testing. _, err = conn.Exec(context.Background(), `delete from t`) require.NoError(t, err) - v = &nilAsEmptyJSONObject{ID: "1", Name: "foo"} + v = &nilPointerAsEmptyJSONObject{ID: "1", Name: "foo"} commandTag, err = conn.Exec(context.Background(), `insert into t(v) values($1)`, v) require.NoError(t, err) require.Equal(t, "INSERT 0 1", commandTag.String()) - var v2 *nilAsEmptyJSONObject + var v2 *nilPointerAsEmptyJSONObject + err = conn.QueryRow(context.Background(), "select v from t").Scan(&v2) + require.NoError(t, err) + require.Equal(t, v, v2) + + ensureConnValid(t, conn) +} + +type nilSliceAsEmptySlice []byte + +func (j nilSliceAsEmptySlice) Value() (driver.Value, error) { + if len(j) == 0 { + return []byte("[]"), nil + } + + return []byte(j), nil +} + +func (j *nilSliceAsEmptySlice) UnmarshalJSON(data []byte) error { + *j = bytes.Clone(data) + return nil +} + +// https://github.com/jackc/pgx/issues/1860 +func TestConnQueryDatabaseSQLDriverValuerCalledOnNilSliceImplementers(t *testing.T) { + t.Parallel() + + conn := mustConnectString(t, os.Getenv("PGX_TEST_DATABASE")) + defer closeConn(t, conn) + + mustExec(t, conn, "create temporary table t(v json not null)") + + var v nilSliceAsEmptySlice + commandTag, err := conn.Exec(context.Background(), `insert into t(v) values($1)`, v) + require.NoError(t, err) + require.Equal(t, "INSERT 0 1", commandTag.String()) + + var s string + err = conn.QueryRow(context.Background(), "select v from t").Scan(&s) + require.NoError(t, err) + require.Equal(t, "[]", s) + + _, err = conn.Exec(context.Background(), `delete from t`) + require.NoError(t, err) + + v = nilSliceAsEmptySlice(`{"name": "foo"}`) + commandTag, err = conn.Exec(context.Background(), `insert into t(v) values($1)`, v) + require.NoError(t, err) + require.Equal(t, "INSERT 0 1", commandTag.String()) + + var v2 nilSliceAsEmptySlice + err = conn.QueryRow(context.Background(), "select v from t").Scan(&v2) + require.NoError(t, err) + require.Equal(t, v, v2) + + ensureConnValid(t, conn) +} + +type nilMapAsEmptyObject map[string]any + +func (j nilMapAsEmptyObject) Value() (driver.Value, error) { + if j == nil { + return []byte("{}"), nil + } + + return json.Marshal(j) +} + +func (j *nilMapAsEmptyObject) UnmarshalJSON(data []byte) error { + var m map[string]any + err := json.Unmarshal(data, &m) + if err != nil { + return err + } + + *j = m + + return nil +} + +// https://github.com/jackc/pgx/pull/2019#discussion_r1605806751 +func TestConnQueryDatabaseSQLDriverValuerCalledOnNilMapImplementers(t *testing.T) { + t.Parallel() + + conn := mustConnectString(t, os.Getenv("PGX_TEST_DATABASE")) + defer closeConn(t, conn) + + mustExec(t, conn, "create temporary table t(v json not null)") + + var v nilMapAsEmptyObject + commandTag, err := conn.Exec(context.Background(), `insert into t(v) values($1)`, v) + require.NoError(t, err) + require.Equal(t, "INSERT 0 1", commandTag.String()) + + var s string + err = conn.QueryRow(context.Background(), "select v from t").Scan(&s) + require.NoError(t, err) + require.Equal(t, "{}", s) + + _, err = conn.Exec(context.Background(), `delete from t`) + require.NoError(t, err) + + v = nilMapAsEmptyObject{"name": "foo"} + commandTag, err = conn.Exec(context.Background(), `insert into t(v) values($1)`, v) + require.NoError(t, err) + require.Equal(t, "INSERT 0 1", commandTag.String()) + + var v2 nilMapAsEmptyObject err = conn.QueryRow(context.Background(), "select v from t").Scan(&v2) require.NoError(t, err) require.Equal(t, v, v2) From cf6074fe5c7c43fc5c73d56761d20f73f2bc378c Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 18 May 2024 20:37:25 -0500 Subject: [PATCH 5/9] Remove unused anynil.Normalize --- internal/anynil/anynil.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/internal/anynil/anynil.go b/internal/anynil/anynil.go index 967dcf415..5d5065ea4 100644 --- a/internal/anynil/anynil.go +++ b/internal/anynil/anynil.go @@ -45,14 +45,6 @@ func Is(value any) bool { } } -// Normalize converts typed nils (e.g. []byte(nil)) into untyped nil. Other values are returned unmodified. -func Normalize(v any) any { - if Is(v) { - return nil - } - return v -} - // NormalizeSlice converts all typed nils (e.g. []byte(nil)) in s into untyped nils. Other values are unmodified. s is // mutated in place. func NormalizeSlice(s []any) { From c1075bfff0752a01d946f40a15e0893f6637c579 Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 18 May 2024 20:59:01 -0500 Subject: [PATCH 6/9] Remove some special casing for QueryExecModeExec --- extended_query_builder.go | 79 ++++----------------------------------- 1 file changed, 8 insertions(+), 71 deletions(-) diff --git a/extended_query_builder.go b/extended_query_builder.go index 522a70e38..152a24446 100644 --- a/extended_query_builder.go +++ b/extended_query_builder.go @@ -1,7 +1,6 @@ package pgx import ( - "database/sql/driver" "fmt" "github.com/jackc/pgx/v5/internal/anynil" @@ -26,7 +25,14 @@ func (eqb *ExtendedQueryBuilder) Build(m *pgtype.Map, sd *pgconn.StatementDescri anynil.NormalizeSlice(args) if sd == nil { - return eqb.appendParamsForQueryExecModeExec(m, args) + for i := range args { + err := eqb.appendParam(m, 0, pgtype.TextFormatCode, args[i]) + if err != nil { + err = fmt.Errorf("failed to encode args[%d]: %w", i, err) + return err + } + } + return nil } if len(sd.ParamOIDs) != len(args) { @@ -145,72 +151,3 @@ func (eqb *ExtendedQueryBuilder) chooseParameterFormatCode(m *pgtype.Map, oid ui return m.FormatCodeForOID(oid) } - -// appendParamsForQueryExecModeExec appends the args to eqb. -// -// Parameters must be encoded in the text format because of differences in type conversion between timestamps and -// dates. In QueryExecModeExec we don't know what the actual PostgreSQL type is. To determine the type we use the -// Go type to OID type mapping registered by RegisterDefaultPgType. However, the Go time.Time represents both -// PostgreSQL timestamp[tz] and date. To use the binary format we would need to also specify what the PostgreSQL -// type OID is. But that would mean telling PostgreSQL that we have sent a timestamp[tz] when what is needed is a date. -// This means that the value is converted from text to timestamp[tz] to date. This means it does a time zone conversion -// before converting it to date. This means that dates can be shifted by one day. In text format without that double -// type conversion it takes the date directly and ignores time zone (i.e. it works). -// -// Given that the whole point of QueryExecModeExec is to operate without having to know the PostgreSQL types there is -// no way to safely use binary or to specify the parameter OIDs. -func (eqb *ExtendedQueryBuilder) appendParamsForQueryExecModeExec(m *pgtype.Map, args []any) error { - for _, arg := range args { - oid, modArg, err := eqb.oidAndArgForQueryExecModeExec(m, arg) - if err != nil { - return err - } - - err = eqb.appendParam(m, oid, pgtype.TextFormatCode, modArg) - if err != nil { - return err - } - } - - return nil -} - -func (eqb *ExtendedQueryBuilder) oidAndArgForQueryExecModeExec(m *pgtype.Map, arg any) (uint32, any, error) { - if arg == nil { - return 0, arg, nil - } - - if dt, ok := m.TypeForValue(arg); ok { - return dt.OID, arg, nil - } - - if textValuer, ok := arg.(pgtype.TextValuer); ok { - tv, err := textValuer.TextValue() - if err != nil { - return 0, nil, err - } - - return pgtype.TextOID, tv, nil - } - - if dv, ok := arg.(driver.Valuer); ok { - v, err := dv.Value() - if err != nil { - return 0, nil, err - } - - if v == nil { - return 0, nil, nil - } - - if dt, ok := m.TypeForValue(v); ok { - return dt.OID, v, nil - } - } - - if str, ok := arg.(fmt.Stringer); ok { - return pgtype.TextOID, str.String(), nil - } - - return 0, nil, &unknownArgumentTypeQueryExecModeExecError{arg: arg} -} From 6ea2d248a38eda25a686ebca7ff9ea4c313f513c Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 18 May 2024 21:01:34 -0500 Subject: [PATCH 7/9] Remove anynil.NormalizeSlice anynil.Is was already being called in all paths that anynil.NormalizeSlice was used. --- conn.go | 2 -- extended_query_builder.go | 2 -- internal/anynil/anynil.go | 10 ---------- 3 files changed, 14 deletions(-) diff --git a/conn.go b/conn.go index a9cb3163f..311721459 100644 --- a/conn.go +++ b/conn.go @@ -10,7 +10,6 @@ import ( "strings" "time" - "github.com/jackc/pgx/v5/internal/anynil" "github.com/jackc/pgx/v5/internal/sanitize" "github.com/jackc/pgx/v5/internal/stmtcache" "github.com/jackc/pgx/v5/pgconn" @@ -755,7 +754,6 @@ optionLoop: } c.eqb.reset() - anynil.NormalizeSlice(args) rows := c.getRows(ctx, sql, args) var err error diff --git a/extended_query_builder.go b/extended_query_builder.go index 152a24446..0cf3d28c8 100644 --- a/extended_query_builder.go +++ b/extended_query_builder.go @@ -22,8 +22,6 @@ type ExtendedQueryBuilder struct { func (eqb *ExtendedQueryBuilder) Build(m *pgtype.Map, sd *pgconn.StatementDescription, args []any) error { eqb.reset() - anynil.NormalizeSlice(args) - if sd == nil { for i := range args { err := eqb.appendParam(m, 0, pgtype.TextFormatCode, args[i]) diff --git a/internal/anynil/anynil.go b/internal/anynil/anynil.go index 5d5065ea4..061680e08 100644 --- a/internal/anynil/anynil.go +++ b/internal/anynil/anynil.go @@ -44,13 +44,3 @@ func Is(value any) bool { return false } } - -// NormalizeSlice converts all typed nils (e.g. []byte(nil)) in s into untyped nils. Other values are unmodified. s is -// mutated in place. -func NormalizeSlice(s []any) { - for i := range s { - if Is(s[i]) { - s[i] = nil - } - } -} From 79cab4640f4ba70c4be37ff2071ed4180ba8b63a Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 18 May 2024 21:06:23 -0500 Subject: [PATCH 8/9] Only use anynil inside of pgtype --- extended_query_builder.go | 5 ----- pgtype/pgtype.go | 4 +++- values.go | 9 --------- 3 files changed, 3 insertions(+), 15 deletions(-) diff --git a/extended_query_builder.go b/extended_query_builder.go index 0cf3d28c8..526b0e953 100644 --- a/extended_query_builder.go +++ b/extended_query_builder.go @@ -3,7 +3,6 @@ package pgx import ( "fmt" - "github.com/jackc/pgx/v5/internal/anynil" "github.com/jackc/pgx/v5/pgconn" "github.com/jackc/pgx/v5/pgtype" ) @@ -117,10 +116,6 @@ func (eqb *ExtendedQueryBuilder) reset() { } func (eqb *ExtendedQueryBuilder) encodeExtendedParamValue(m *pgtype.Map, oid uint32, formatCode int16, arg any) ([]byte, error) { - if anynil.Is(arg) { - return nil, nil - } - if eqb.paramValueBytes == nil { eqb.paramValueBytes = make([]byte, 0, 128) } diff --git a/pgtype/pgtype.go b/pgtype/pgtype.go index 2be11e820..810eb5771 100644 --- a/pgtype/pgtype.go +++ b/pgtype/pgtype.go @@ -9,6 +9,8 @@ import ( "net/netip" "reflect" "time" + + "github.com/jackc/pgx/v5/internal/anynil" ) // PostgreSQL oids for common types @@ -1912,7 +1914,7 @@ func newEncodeError(value any, m *Map, oid uint32, formatCode int16, err error) // (nil, nil). The caller of Encode is responsible for writing the correct NULL value or the length of the data // written. func (m *Map) Encode(oid uint32, formatCode int16, value any, buf []byte) (newBuf []byte, err error) { - if value == nil { + if anynil.Is(value) { return nil, nil } diff --git a/values.go b/values.go index cab717d0a..6e2ff3003 100644 --- a/values.go +++ b/values.go @@ -3,7 +3,6 @@ package pgx import ( "errors" - "github.com/jackc/pgx/v5/internal/anynil" "github.com/jackc/pgx/v5/internal/pgio" "github.com/jackc/pgx/v5/pgtype" ) @@ -15,10 +14,6 @@ const ( ) func convertSimpleArgument(m *pgtype.Map, arg any) (any, error) { - if anynil.Is(arg) { - return nil, nil - } - buf, err := m.Encode(0, TextFormatCode, arg, []byte{}) if err != nil { return nil, err @@ -30,10 +25,6 @@ func convertSimpleArgument(m *pgtype.Map, arg any) (any, error) { } func encodeCopyValue(m *pgtype.Map, buf []byte, oid uint32, arg any) ([]byte, error) { - if anynil.Is(arg) { - return pgio.AppendInt32(buf, -1), nil - } - sp := len(buf) buf = pgio.AppendInt32(buf, -1) argBuf, err := m.Encode(oid, BinaryFormatCode, arg, buf) From 9ca9203afbd95ed53bf29eb0d3ac42f20ae5d17f Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 18 May 2024 22:34:09 -0500 Subject: [PATCH 9/9] Move typed nil handling to Map.Encode from anynil The new logic checks for any type of nil at the beginning of Encode and then either treats it as NULL or calls the driver.Valuer method if appropriate. This should preserve the existing nil normalization while restoring the ability to encode nil driver.Valuer values. --- internal/anynil/anynil.go | 46 --------------------------- pgtype/array_codec.go | 3 +- pgtype/doc.go | 6 ++-- pgtype/pgtype.go | 67 ++++++++++++++++++++++++++++++++++++--- 4 files changed, 67 insertions(+), 55 deletions(-) delete mode 100644 internal/anynil/anynil.go diff --git a/internal/anynil/anynil.go b/internal/anynil/anynil.go deleted file mode 100644 index 061680e08..000000000 --- a/internal/anynil/anynil.go +++ /dev/null @@ -1,46 +0,0 @@ -package anynil - -import ( - "database/sql/driver" - "reflect" -) - -// valuerReflectType is a reflect.Type for driver.Valuer. It has confusing syntax because reflect.TypeOf returns nil -// when it's argument is a nil interface value. So we use a pointer to the interface and call Elem to get the actual -// type. Yuck. -// -// This can be simplified in Go 1.22 with reflect.TypeFor. -// -// var valuerReflectType = reflect.TypeFor[driver.Valuer]() -var valuerReflectType = reflect.TypeOf((*driver.Valuer)(nil)).Elem() - -// Is returns true if value is any type of nil unless it implements driver.Valuer. *T is not considered to implement -// driver.Valuer if it is only implemented by T. -func Is(value any) bool { - if value == nil { - return true - } - - refVal := reflect.ValueOf(value) - kind := refVal.Kind() - switch kind { - case reflect.Chan, reflect.Func, reflect.Map, reflect.Ptr, reflect.UnsafePointer, reflect.Interface, reflect.Slice: - if !refVal.IsNil() { - return false - } - - if _, ok := value.(driver.Valuer); ok { - if kind == reflect.Ptr { - // The type assertion will succeed if driver.Valuer is implemented on T or *T. Check if it is implemented on T - // to see if it is not implemented on *T. - return refVal.Type().Elem().Implements(valuerReflectType) - } else { - return false - } - } - - return true - default: - return false - } -} diff --git a/pgtype/array_codec.go b/pgtype/array_codec.go index c1863b32a..bf5f6989a 100644 --- a/pgtype/array_codec.go +++ b/pgtype/array_codec.go @@ -6,7 +6,6 @@ import ( "fmt" "reflect" - "github.com/jackc/pgx/v5/internal/anynil" "github.com/jackc/pgx/v5/internal/pgio" ) @@ -230,7 +229,7 @@ func (c *ArrayCodec) PlanScan(m *Map, oid uint32, format int16, target any) Scan // target / arrayScanner might be a pointer to a nil. If it is create one so we can call ScanIndexType to plan the // scan of the elements. - if anynil.Is(target) { + if isNil, _ := isNilDriverValuer(target); isNil { arrayScanner = reflect.New(reflect.TypeOf(target).Elem()).Interface().(ArraySetter) } diff --git a/pgtype/doc.go b/pgtype/doc.go index 2039fcf1d..d56c1dc70 100644 --- a/pgtype/doc.go +++ b/pgtype/doc.go @@ -141,11 +141,11 @@ interfaces. Encoding Typed Nils -pgtype normalizes typed nils (e.g. []byte(nil)) into nil. nil is always encoded is the SQL NULL value without going -through the Codec system. This means that Codecs and other encoding logic does not have to handle nil or *T(nil). +pgtype encodes untyped and typed nils (e.g. nil and []byte(nil)) to the SQL NULL value without going through the Codec +system. This means that Codecs and other encoding logic do not have to handle nil or *T(nil). However, database/sql compatibility requires Value to be called on T(nil) when T implements driver.Valuer. Therefore, -driver.Valuer values are not normalized to nil unless it is a *T(nil) where driver.Valuer is implemented on T. See +driver.Valuer values are only considered NULL when *T(nil) where driver.Valuer is implemented on T not on *T. See https://github.com/golang/go/issues/8415 and https://github.com/golang/go/commit/0ce1d79a6a771f7449ec493b993ed2a720917870. diff --git a/pgtype/pgtype.go b/pgtype/pgtype.go index 810eb5771..408295683 100644 --- a/pgtype/pgtype.go +++ b/pgtype/pgtype.go @@ -9,8 +9,6 @@ import ( "net/netip" "reflect" "time" - - "github.com/jackc/pgx/v5/internal/anynil" ) // PostgreSQL oids for common types @@ -1914,8 +1912,17 @@ func newEncodeError(value any, m *Map, oid uint32, formatCode int16, err error) // (nil, nil). The caller of Encode is responsible for writing the correct NULL value or the length of the data // written. func (m *Map) Encode(oid uint32, formatCode int16, value any, buf []byte) (newBuf []byte, err error) { - if anynil.Is(value) { - return nil, nil + if isNil, callNilDriverValuer := isNilDriverValuer(value); isNil { + if callNilDriverValuer { + newBuf, err = (&encodePlanDriverValuer{m: m, oid: oid, formatCode: formatCode}).Encode(value, buf) + if err != nil { + return nil, newEncodeError(value, m, oid, formatCode, err) + } + + return newBuf, nil + } else { + return nil, nil + } } plan := m.PlanEncode(oid, formatCode, value) @@ -1970,3 +1977,55 @@ func (w *sqlScannerWrapper) Scan(src any) error { return w.m.Scan(t.OID, TextFormatCode, bufSrc, w.v) } + +// canBeNil returns true if value can be nil. +func canBeNil(value any) bool { + refVal := reflect.ValueOf(value) + kind := refVal.Kind() + switch kind { + case reflect.Chan, reflect.Func, reflect.Map, reflect.Ptr, reflect.UnsafePointer, reflect.Interface, reflect.Slice: + return true + default: + return false + } +} + +// valuerReflectType is a reflect.Type for driver.Valuer. It has confusing syntax because reflect.TypeOf returns nil +// when it's argument is a nil interface value. So we use a pointer to the interface and call Elem to get the actual +// type. Yuck. +// +// This can be simplified in Go 1.22 with reflect.TypeFor. +// +// var valuerReflectType = reflect.TypeFor[driver.Valuer]() +var valuerReflectType = reflect.TypeOf((*driver.Valuer)(nil)).Elem() + +// isNilDriverValuer returns true if value is any type of nil unless it implements driver.Valuer. *T is not considered to implement +// driver.Valuer if it is only implemented by T. +func isNilDriverValuer(value any) (isNil bool, callNilDriverValuer bool) { + if value == nil { + return true, false + } + + refVal := reflect.ValueOf(value) + kind := refVal.Kind() + switch kind { + case reflect.Chan, reflect.Func, reflect.Map, reflect.Ptr, reflect.UnsafePointer, reflect.Interface, reflect.Slice: + if !refVal.IsNil() { + return false, false + } + + if _, ok := value.(driver.Valuer); ok { + if kind == reflect.Ptr { + // The type assertion will succeed if driver.Valuer is implemented on T or *T. Check if it is implemented on *T + // by checking if it is not implemented on *T. + return true, !refVal.Type().Elem().Implements(valuerReflectType) + } else { + return true, true + } + } + + return true, false + default: + return false, false + } +}