breaking change in v5: Value() no longer called on nil args

[kszafran]

Describe the bug
v5 introduced this line https://github.com/jackc/pgx/blob/master/extended_query_builder.go#L25 with the result being that Valuer.Value() is no longer called on nil arguments.

To Reproduce
I have some types like the following:

type MyThing struct {
	// ...some fields
}

type MyThings []MyThing

func (i MyThings) Value() (driver.Value, error) {
	if len(i) == 0 {
		return []byte("[]"), nil
	}
	return json.Marshal(i)
}

func (i *MyThings) Scan(value any) error {
	bs, ok := value.([]byte)
	if !ok {
		return fmt.Errorf("failed to scan MyThings, expected []byte, got: %T", value)
	}
	return json.Unmarshal(bs, &i)
}

and a column like things JSONB NOT NULL. In v4, if I passed MyThings(nil) as a query arg, Value() would be called and an empty JSONB array would be stored. In v5, Value() is not called and I end up with an error:

ERROR: null value in column \"things\" of relation \"test\" violates not-null constraint (SQLSTATE 23502)

Expected behavior
Valuer.Value() should be called on typed nil values. Otherwise, this breaking change should be described in the changelog.

Actual behavior
This line https://github.com/jackc/pgx/blob/master/extended_query_builder.go#L25 turns all types nils into nil interfaces, which prevents calling Valuer.Value().

Version

• Go: go version go1.19.2 darwin/amd64
• PostgreSQL: PostgreSQL 14.3 on x86_64-pc-linux-musl, compiled by gcc (Alpine 11.2.1_git20220219) 11.2.1 20220219, 64-bit
• pgx: v5.0.4

Additional context
Question: I'm planning on eventually moving to the pgx native interface. Would I encounter the same behavior there, or would I be able to provide default values for nils?

[jackc]

I think the v5 behavior is correct.

To be honest I'm surprised that was v4's behavior. This means that pointer to a driver.Valuer would behave differently that a pointer to anything else. And it risks a panic if the driver.Valuer wasn't expecting to be called with a nil receiver.

Furthermore, allowing typed nils would cascade through the entire type system. Normalizing nils as is done now removes a lot of edge cases.

Valuer.Value() should be called on typed nil values. Otherwise, this breaking change should be described in the changelog.

That's fair. I've added a note (a968ce3).

[kszafran]

I wonder how other database/sql drivers handle nil values (e.g. mysql drivers). We were using lib/pq before, and I think it was also calling Valuer.Value() on nil arguments.

[joshma]

We have this assumption spread across our codebase, so it's currently blocking our upgrade to v5.

We have non-null constraints on our JSONB columns, preferring a JSON "null" over a SQL null just to remove a redundant state. But now all our nil maps are getting stored as SQL null, breaking the non-null constraints. The only solution seems to be to find all call sites and initiate an empty map.

Does anyone happen to have a workaround, to restore the old behavior?

[jackc]

@joshma Sorry, but I don't think there is an easy way to do this. The typed nil into nil conversion happens early in the encoding process and the rest of the system expects to not receive any typed nils.

...

I started writing about the harder way to do this, but as I thought about it a little more, I determined it wouldn't work either. So I actually don't know any way to do this.

[joshma]

😢 - well I appreciate the time and consideration!

[kszafran]

@joshma Looks like this has been fixed in 5.6.0: #2019