pgx
pgx copied to clipboard
jackc
Implementations of driver.Valuer are no longer called for nil receivers in v5
The v5 changelog mentions this:
Previously, a type that implemented driver.Valuer would have the Value method called even on a nil pointer. All nils whether typed or untyped now represent NULL.
That's a huge change in behavior from v4. Previously, it was very simple to implement custom types which would serialize themselves into a non-SQL-NULL value from a nil receiver.
CREATE TABLE tab (col text NOT NULL);
type MySlice []string
func (m MySlice) Value() (driver.Value, error) {
if len(m) == 0 {
return "[]", nil
}
encoded, err := json.Marshal(&m)
return string(encoded), errors.WithStack(err)
}
func main() {
var m MySlice
_, err := conn.Exec(context.Background(), "INSERT INTO tab (col) VALUES ($1)", m))
}
This would work in v4, but fails in v5 with a NOT NULL constraint violation, because (MySlice).Value() is never called.
Is there a comparably simple way to achieve this pattern in pgx/v5?
Thanks for your work and this great library!
It seems like your concerns were partially answered and resolved in subsequent minor and patch releases, right? At least ours were with the latest 5.3.1.
Sorry, I actually don't know any good way for an application to get that behavior back.
Typed nils caused a lot of headaches in pgtype and normalizing them at the border simplified things quite a bit. It wasn't clear to me at the time that this would have negative downstream effects. While calling methods with a nil receiver is allowed in Go, it almost always is an error. But I suppose it is a reasonable thing to consider with your example type.
As far as changes to pgx that could improve this use case, there are only two options that come to mind.
- A new interface that when implemented signals that typed nils should not be normalized.
- A flag that turns off the nil normalization.
Not sure either of these is a good idea though. I'm not confident that any typed nils allowed past the border wouldn't crash later in code not expecting a nil. Maybe if it was only enabled for database/sql though...
It seems like your concerns were partially answered and resolved in subsequent minor and patch releases, right? At least ours were with the latest 5.3.1.
The patch used in gobuffalo/pop does use github.com/jackc/pgx/v5 v5.3.1 so I think the problem persists.
It wasn't clear to me at the time that this would have negative downstream effects. While calling methods with a nil receiver is allowed in Go, it almost always is an error. But I suppose it is a reasonable thing to consider with your example type.
The most common use case for this behavior is custom types that transform e.g. JSON json.RawMessage []byte into a format (string) the database can understand, with a sensible default (e.g. an empty object). This is often used when having a larger struct such as
type JSONRawMessage []byte
// Scan ...
// Value ...
type MyPayload struct {
ID string `json:"id"`
SomeJSON JSONRawMessage `json:"some_json"`
}
that would be hydrated by a JSON unmarshaller. Here, the JSON unmarshaller will set SomeJSON to a typed nil if the some_json key is omitted in the original JSON:
{
"id": "foo"
}
A global flag would probably be the "easiest" way from a consumer perspective to keep backwards compatibility, in particular for cases where pgx is used in an DBAL library such as pop or others. A type assertion would certainly be more elegant, but will potentailly cause downstream issues when users simply update the underlying DBAL.
So just out of curiosity I disabled the typed nil to nil conversion to see what would happen. I did this by changing all the functions in the anynil internal package to no-ops.
It broke TestConnQueryDatabaseSQLDriverValuerWithAutoGeneratedPointerReceiver which was originally added as a regression test for #339. Presumably, the old fix for that could be reimplemented.
It also caused test failures with JSON encoding of typed nils. Typed nils would be encoded as the JSON null rather than the SQL null. I think that also is a reversion to v4 behavior.
It also broke TestMapScanPtrToPtrToSlice but as that is testing scanning rather than parameter encoding, it probably wouldn't be affected by an actual change.
There's actually less failures than I expected, though that could be due lack of extensive tests for typed nils.
If this is to be configured, then based on where anynil is called from, the configuration would have to eventually be part of pgtype.Map. This could be a flag or maybe even a NilPolicy or NilHandler interface. The flag would be simpler of course, but making the nil logic pluggable would allow for more fine-grained control such as only allow typed nils of type T rather than all types (and that type could be specified there instead of requiring T to implement a new interface which may not always be possible). But maybe that's getting too complicated.
There's actually less failures than I expected, though that could be due lack of extensive tests for typed nils.
That's nice to hear! I'm happy to test any changes in PGX against our existing code bases to see if anything breaks - just let me know the correct git hash for go mod replace :)
@jackc do you have an idea on how would be the best way to proceed? We can help if you slightly guide us on where we should be focusing on. Thanks! 🤞
do you have an idea on how would be the best way to proceed? We can help if you slightly guide us on where we should be focusing on. Thanks! 🤞
@glerchundi If you want, you can prototype the approach in my previous comment. I think most of it is pretty straight forward. The part I'm not sure about is how to configure it. I'm nervous about nils leaking in and causing panics, so I kind of lean toward something that can change the behavior per type rather than just a flag, but that might be overcomplicating it..
Ok, thanks for the info @jackc. Lets see what we can do on our side and will let you know. Although it could overcomplicate it, I’m also in favor of per type approach.
We did not found a free slot to jump into this yet. Hopefully in September.
This also breaks gorm for a long time now, the current "workaround" is to add exclude-primitives every time a new version is tagged: https://github.com/go-gorm/postgres/issues/159 However, it doesn't work reliably so I stopped updating dependencies for a while now.
Had to update exclude-directives again to fix broken builds. The current list, when using gorm and postgres is as follows:
exclude (
gorm.io/driver/postgres v1.4.6
gorm.io/driver/postgres v1.4.7
gorm.io/driver/postgres v1.4.8
gorm.io/driver/postgres v1.5.0
gorm.io/driver/postgres v1.5.1
gorm.io/driver/postgres v1.5.2
gorm.io/driver/postgres v1.5.3
gorm.io/driver/postgres v1.5.4
gorm.io/gorm v1.24.4
gorm.io/gorm v1.24.5
gorm.io/gorm v1.24.6
gorm.io/gorm v1.25.0
gorm.io/gorm v1.25.1
gorm.io/gorm v1.25.2
gorm.io/gorm v1.25.3
gorm.io/gorm v1.25.4
gorm.io/gorm v1.25.5
)
@jackc @glerchundi - I added a PR for this. When you get a chance, could you take a look and let me know if it is on the right track. Thank you!
I think I have a solution in #2019. Basically, it just skips nil normalization on any (*T)(nil) where Value is a method on *T, but not on T. My previous ideas were over complicating it because I wasn't thinking about using the distinction between Value being implemented on the pointer vs. the value.