Closed idaunis closed 2 years ago
Hello! Thank you for opening this issue!
I think in your example you don't even want to wrap your struct, you would want your rows
to look like that:
type UUID struct {
pgtype.UUID
}
q := `SELECT id FROM ...`
var rows []UUID
if err := pgxscan.Select(ctx, pg.Client(tx), &rows, q); err != nil {
return nil, err
}
The problem is, currently, it's not possible for scany to handle that, but I would like to change that. See that issue https://github.com/georgysavva/scany/issues/33
I think, instead of merging the changes that you proposed https://github.com/georgysavva/scany/pull/38 (they look a little bit hacky to me) we can look at your problem at a different angle and solve it in a more fundamental way. What do you think?
Hi Georgy,
We can solve the other issue #33, but this is completely different. This is not about scanning one column only. I put this example to make it simple but what I'm describing is something like this:
type UUID struct {
pgtype.UUID
}
// custom type marshaling for protobuf
func (u UUID) MarshalTo(data []byte) (n int, err error) {
if u.Zero() {
return 0, nil
}
bytes, _ := u.UUID.MarshalBinary()
copy(data, bytes)
return 16, nil
}
func (u *UUID) Unmarshal(data []byte) (err error) {
...
type User struct {
ID *UUID
Name string
Email string
...
}
q := `SELECT id, name, email, ... FROM ...`
var rows []User
if err := pgxscan.Select(ctx, pg.Client(tx), &rows, q); err != nil {
return nil, err
}
Hello and thank you for sharing another example!
In the second example, you don't embed the UUID
type into User
struct, you nest it as ID
field.
In that case, scany should be able to scan into the User
type as expected (I just tested that)
Strange, that doesn't work for us. If you look at the test on PR #38. The test would not pass without the fix.
Hi Georgy, in the previous example if you define ID *UUID
(as a pointer) you'll see it fails to assign the value
Hi Greg, in the previous example if you define
ID *UUID
(as a pointer) you'll see it fails to assign the value
This is a known limitation of scany + pgx integration. Check that docs section: https://pkg.go.dev/github.com/georgysavva/scany@v0.2.8/pgxscan#hdr-Note_about_pgx_custom_types
Let me explain how I see this whole situation.
When you wrap (embed) pgtype.UUID
and create your own UUID
struct, you can use that struct with scany because UUID
embeds all methods of pgtype.UUID
and hence, when scany passes UUID
object to pgx
, it still implements the pgtype Decoder interface and everything works. So you can easily wrap pgtype structs into custom ones and still use them with scany.
The test case in your PR is different, you wrap (embed) the CustomString
type that doesn't implement any Decoder interface for pgx, actually, you could simply use the default string
type instead of CustomString
and the situation would be absolutely the same for scany. Scany passes an object of NestedAnonymous
to pgx and since it doesn't implement any Decoder interface pgx can't handle it. In order for that case to work with the current scany version, NestedAnonymous
or CustomString
types should implement the Decoder methods to simulate the case with pgtype.UUID
.
Could you please explain your use case better, do you really need to wrap primitive types like string
and use them with scany? If yes, I would need to think of a good solution for that, because the fix you proposed causes panics on some input data.
If you are happy with just wrapping pgtypes like pgtype.UUID
we don't need to do anything for now since it works already.
Thanks!
Thank you Georgy,
Our use case is with either custom structs that implement Decoder interface or inherited via embedding. Not with primitives like string
. But in our use case we use pointers since the same struct is being used with other serializers like json that take advantage of the nil pointer as another state.
Thank you again for your help!
Hi Georgy,
I updated PR: #38 to support pointers in custom types instead. This resolves our fundamental issue. What do yo think?
Thank you Gregory,
Our use case is with either custom structs that implement Decoder interface or inherited via embedding. Not with primitives like
string
. But in our use case we use pointers since the same struct is being used with other serializers like json that take advantage of the nil pointer as another state.Thank you again for your help!
You don't need to specify the pgtype.UUID by pointer to understand is it a NULL or not, it has the special Status
field and you can do the check in your JSON marshalling/unmarshalling:
if user.ID.Status == pgtype.Null {
...
}
But, I totally agree with you that scany should work with pgtype structs whether by pointer or not. Ideally, the pgx library itself, should handle that case when it receives **pgtype.UUID
(pointer to pointer) as database/sql
does it.
I would like to ask the pgx author to support that, what do you think?
The fix that you proposed in your PR breaks some existing logic see that PR https://github.com/georgysavva/scany/pull/42 for test cases that are broken by your changes. This happens because if you have a NULLable JSON column and you have a corresponding field for that:
type Model struct
Foo *JSONData
}
Scany must pass it to the underlying database library as **JSONData
, to be able to handle the NULL case, by nil
state. The custom pgtype types are the only exception for that because pgx doesn't need them to be **pgtype.UUID
, since they have the special Status
field that tells you is it a NULL or not.
So if it's not possible to fix it on the pgx side, the fix on the scany side should check that the pointer type implements the pgtype Decoder interface and because dbscan should know nothing about pgx interfaces it's a bit harder to implement and should be configured from the pgxscan package. This is how I see it at least.
The issue is not with the pgx library. I used pgtype.UUID
as an example but it can be any struct that implements Scan
. In fact we use google.UUID
. I updated the #38 to make sure the struct implements the Scanner interface and support your new test cases. Let me know what do you think.
You are correct that the situation is the same for all types that implement the sql.Scanner
interface, not only pgtypes. But this is only true when we use pgx via its native interface. If we consider database/sql
and pgx
as a driver (or it could be lib/pq
instead of pgx
), when you still must pass types that implement sql.Scanner
as **google.UUID
(pointer to pointer), because it's the only way for the database/sql
to handle NULL case.
And since dbscan
is an abstraction, and it used by both pgxscan
(pgx native interface) and sqlscan
(database/sql
library) your recent changes break that logic for database/sql
integration.
@idaunis here, I opened an issue in pgx library for sql.Scanner
case: https://github.com/jackc/pgx/issues/1000
Thank you Georgy. It makes sense to me. If we want to keep intact the NULL logic the way to fix this right is in the pgx library as database/sql is already working correctly.
Hi @georgysavva, This test in #46 passes with the fixes on https://github.com/jackc/pgtype/pull/104
Hi @idaunis @georgysavva we are experience a similar issue when attempting to scan into a custom *UUID field. Should https://github.com/jackc/pgtype/pull/104 unblock this issue.
@xomaczar could you provide details on your particular case so I could help?
In the case need to wrap a structure anonymously to simply implement some interfaces or custom methods, scany loses the column mapping throwing the error:
scany: scan row into struct fields: can't scan into dest[1]: cannot assign ...
I propose the following fix: #38