From 2a36a7032e6efa9d84d09e84a19a5561091f335f Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 18 May 2024 07:41:10 -0500 Subject: [PATCH] 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 9a48c1a8..314e2bbb 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 ec9270ac..8e502303 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 df044cde..550e8cb8 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()