auto uuid scanning #726
auto uuid scanning #726
Conversation
..for the UUID OID? We already could scan/value with our UUID type for BYTEA, columns, so I gather we're now enabling it to work with the special UUID type from the standard postgres extension? I'm all for controlling the implemention, but to document motivation, what are some of the considerations with making our own scanner vs. one of the existing like https://pkg.go.dev/github.com/jackc/pgx/v5@v5.5.5/pgtype#UUID or https://github.com/jackc/pgx/wiki/UUID-Support? |
|
|
||
| uuid := UUID{} | ||
| copy(uuid[:], src) | ||
| reflect.ValueOf(target).Elem().Set(reflect.ValueOf(uuid)) |
There was a problem hiding this comment.
What's this doing? In the case where !ok above, what's an example with concrete types where this does something sensible?
I'm not sure where target comes from or is used later
There was a problem hiding this comment.
I'll document this. When testing, it sometimes did not pass a UUID, but instead passed []byte. I believe this is because of how we pass uuids internally (such as in the vote store), where we convert them to []byte.
core/types/uuid.go
Outdated
| } | ||
|
|
||
| func (u *UUID) PreferredFormat() int16 { | ||
| return pgtype.TextFormatCode |
There was a problem hiding this comment.
The UUID type built in to pgype prefers binary.
Will you document some of the decisions in this code and/or like to reference implementations? There's a lot of logic add with no obvious reasons, although I'm sure there are indeed good ones.
There was a problem hiding this comment.
Oh this is a mistake, meant to be binary
The main reason is to control what gets returned to the client. @KwilLuke thought it was weird that UUIDs were not returned as strings (I agree). Doing this allows us to json marshal our own UUID type without iterating over all of the results to convert it. |
tidy tidy main downgrade pgx
|
While creating tests for this, I noticed that we actually had a bug for sending arrays via flags, as well as with RLP encoding. I have added into this PR fixes for these. These are breaking changes, but given the other breaking changes in this PR, I figured it was not a big deal. I also could not think of a way to properly support arrays without doing this. The last outstanding thing on this PR is some ridiculous bug with writing UUIDs to postgres. For some reason, in one of the integration tests, I get a primary key violation for having duplicate primary keys, despite the fact that they certainly aren't. Will finish this up tomorrow for the beta. |
| // EncodeValue encodes a value to its detected type. | ||
| // It will reflect the value of the passed argument to determine its type. | ||
| func EncodeValue(v any) (*EncodedValue, error) { | ||
| if v == nil { | ||
| return &EncodedValue{ | ||
| Type: DataType{ | ||
| Name: types.NullType.Name, | ||
| }, | ||
| Data: nil, | ||
| }, nil | ||
| } | ||
|
|
||
| // encodeScalar encodes a scalar value into a byte slice. | ||
| // It also returns the data type of the value. | ||
| encodeScalar := func(v any) ([]byte, *types.DataType, error) { | ||
| switch t := v.(type) { | ||
| case string: | ||
| return []byte(t), types.TextType, nil |
There was a problem hiding this comment.
With our client tooling at least, I only ever see v being a string.
Backing up through core/client.encodeTuple -> core/client.(*Client).Execute (or Call), I see kwil-cli preparing inputs from where the any is always going to be a string (or []string which ends up here too).
Am I reading this wrong? Or is this planning for different client use?
Also, I'm noting that on client both call and execute use encodeTuple this way, but server side only in txapp does execute have the complementary decoding with core/types/transactions.(*EncodedValue).Decode
| @@ -228,16 +227,19 @@ func (c *Client) ExecuteAction(ctx context.Context, dbid string, action string, | |||
| // It can take any number of inputs, and if multiple tuples of inputs are passed, | |||
| // it will execute them in the same transaction. | |||
| func (c *Client) Execute(ctx context.Context, dbid string, procedure string, tuples [][]any, opts ...clientType.TxOpt) (transactions.TxHash, error) { | |||
There was a problem hiding this comment.
kwil-cli still uses the deprecated ExecuteAction instead of this one directly.
| case string: | ||
| return []byte(t), types.TextType, nil | ||
| case int, int16, int32, int64, int8, uint, uint16, uint32, uint64, uint8: | ||
| i64, err := conv.Int64(v) |
There was a problem hiding this comment.
Our internal/conv if possible, please.
There was a problem hiding this comment.
We can't because this is in core.
There was a problem hiding this comment.
Oh, well we definitely don't want to use stockton here. We can move conv to core or copy/paste what's needed here.
There was a problem hiding this comment.
I've got a way around it, will push up
| if e.Type.IsArray { | ||
| decoded := make([]any, 0) | ||
| for _, elem := range e.Data { | ||
| dec, err := decodeScalar(elem, e.Type.toTypes()) |
There was a problem hiding this comment.
Probably spare some conversions and allocations and just pass e.Type.Name to decodeScalar because that's all it needs. Array context is already established here.
|
|
||
| // check if it is an array | ||
| if e.Type.IsArray { | ||
| decoded := make([]any, 0) |
There was a problem hiding this comment.
| decoded := make([]any, 0) | |
| decoded := make([]any, 0, len(e.Data)) |
|
Closing in favor of #761. I have addressed your comments thus far there as well. |
This PR makes it such that our UUID types are automatically scanned from the database. It also adds json marshalling for our UUID types, so that they are sent as strings over json.