Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential regression in sql.Scanner handling when using pgx.CollectRows #2229

Closed
calumj opened this issue Jan 13, 2025 · 1 comment · Fixed by #2236
Closed

Potential regression in sql.Scanner handling when using pgx.CollectRows #2229

calumj opened this issue Jan 13, 2025 · 1 comment · Fixed by #2236
Labels

Comments

@calumj
Copy link

calumj commented Jan 13, 2025

Describe the bug

As of v5.7.2 When using pgx.CollectRows to collect rows into a struct that contains a pointer to a struct that implements sql.Scanner, if the column in the database is NULL, the corresponding struct is no longer nil.

  • v5.7.1 - the example below prints testData[1].Value was nil struct and the testData[1].Value is nil as expected.
  • v5.7.2 - the example below prints expected testData[1].Value to be nil struct and the testData[1].Value is no longer nil.

This seems to have been introduced with #2151, is this an expected change?
Is it possible to work around this to ensure that the pointer values are still nil?

To Reproduce
Steps to reproduce the behavior:

If possible, please provide runnable example such as:

package main

import (
	"context"
	"database/sql/driver"
	"encoding/json"
	"errors"
	"fmt"
	"log/slog"
	"os"

	embeddedpostgres "github.com/fergusstrange/embedded-postgres"
	"github.com/jackc/pgx/v5"
	"github.com/jackc/pgx/v5/pgxpool"
)

type TestData struct {
	Id    string     `db:"id"`
	Value *TestValue `db:"value"`
}

func (e *TestData) RewriteQuery(ctx context.Context, conn *pgx.Conn, sql string, args []any) (newSQL string, newArgs []any, err error) {
	return `
		INSERT INTO
			pgx_test (
				id,
				value
			)
		VALUES
			($1, $2)
	`, []any{e.Id, e.Value}, nil
}

type TestValue struct {
	Data string `json:"data"`
}

func (t *TestValue) Scan(src interface{}) error {
	if src == nil {
		return nil
	}

	var source []byte
	switch val := src.(type) {
	case string:
		source = []byte(val)
	case []byte:
		source = val
	default:
		return errors.New("incompatible source type")
	}

	if err := json.Unmarshal(source, &t); err != nil {
		return fmt.Errorf("unable to unmarshal source data: %w", err)
	}

	return nil
}

func (t *TestValue) Value() (driver.Value, error) {
	if t == nil {
		return nil, nil
	}

	testValueBytes, err := json.Marshal(t)
	if err != nil {
		return nil, fmt.Errorf("unable to marshal test value data: %w", err)
	}

	return testValueBytes, nil
}

func main() {
	ctx := context.TODO()

	logger := slog.New(slog.NewJSONHandler(os.Stdout, nil))
	slog.SetDefault(logger)

	postgres := embeddedpostgres.NewDatabase(embeddedpostgres.DefaultConfig().Port(5555).Logger(nil))
	if err := postgres.Start(); err != nil {
		slog.Error("unable to start embedded postgres", "error", err.Error())
		os.Exit(1)
	}
	defer func() {
		if err := postgres.Stop(); err != nil {
			slog.Error("unable to stop embedded postgres", "error", err.Error())
			os.Exit(1)
		}
	}()

	test(ctx)

}

func test(ctx context.Context) {

	connectionUrl := "postgres://postgres:[email protected]:5555/postgres"

	pgxConfig, err := pgxpool.ParseConfig(connectionUrl)
	if err != nil {
		slog.Error("cannot parse postgres config", "error", err.Error())
		return
	}

	pool, err := pgxpool.NewWithConfig(ctx, pgxConfig)
	if err != nil {
		slog.Error("unable to create pgx connection pool", "error", err.Error())
		return
	}

	if _, err := pool.Exec(ctx, `CREATE TABLE "public"."pgx_test" ("id" text NOT NULL PRIMARY KEY, "value" JSONB);`); err != nil {
		slog.Error("unable to create table 'pgx_test'", "error", err.Error())
		return
	}

	first := &TestData{
		Id: "first",
		Value: &TestValue{
			Data: "test",
		},
	}

	if _, err := pool.Exec(ctx, "", first); err != nil {
		slog.Error("unable to insert first value into table 'pgx_test'", "error", err.Error())
		return
	}

	second := &TestData{
		Id:    "second",
		Value: nil,
	}

	if _, err := pool.Exec(ctx, "", second); err != nil {
		slog.Error("unable to insert second value into table 'pgx_test'", "error", err.Error())
		return
	}

	rows, err := pool.Query(ctx, "SELECT id, value from pgx_test")
	if err != nil {
		slog.Error("unable to query rows from table 'pgx_test'", "error", err.Error())
		return
	}

	testData, err := pgx.CollectRows(rows, pgx.RowToAddrOfStructByName[TestData])
	if err != nil {
		slog.Error("unable to collect rows from table 'pgx_test'", "error", err.Error())
		return
	}

	if testData[0].Value == nil || testData[0].Value.Data != "test" {
		slog.Error("expected testData[0].Value.Data to be 'test'")
		return
	}
	slog.Info("testData[0].Value.Data was 'test'")

	if testData[1].Value != nil {
		slog.Error("expected testData[1].Value to be nil struct")
		return
	}

	slog.Info("testData[1].Value was nil struct")
}
  • Previous behaviour - go mod tidy && go get github.com/jackc/pgx/[email protected] && go run main.go
  • New behaviour - go mod tidy && go get github.com/jackc/pgx/[email protected] && go run main.go

Expected behavior
I expected the behaviour to continue between versions, and that the NULL column would map to a nil struct value.

Actual behavior
The NULL column no longer produces a nil struct value.

Version

  • Go: $ go version -> go version go1.23.1 darwin/arm64

Additional context
Add any other context about the problem here.

@felix-roehrich
Copy link
Contributor

Simpler reproduction

type Foo struct {
	Bar string
}

func (f *Foo) Scan(src any) error {
	log.Print("scan")
	if src == nil {
		return nil
	}

	return json.Unmarshal(src.([]byte), f)
}

conn, err := pgx.Connect(context.Background(), "connString")
if err != nil {
  panic(err)
}

var dst *Foo
err = conn.QueryRow(context.Background(), "select null::jsonb").Scan(&dst)
if err != nil {
  panic(err)
}
log.Print(dst)

This is a side effect of now using scanPlanSQLScanner instead of scanPlanJSONToJSONUnmarshal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants