Fix handle ptr pk in indirectAsKey #1139
Conversation
This commit addresses an issue where the `getRealValue` function was not correctly handling fields that implement the `driver.Valuer` interface. The code now checks if the field implements `driver.Valuer` and, if so, retrieves the underlying value. It then checks the Kind of type to determine whether it can be returned directly. fix uptrace#1138
21a4a38 to
f25f7b0
Compare
The bun tag now includes the type definition "type:uuid" for pointer primary keys, ensuring correct database schema generation. Also removed WithForeignKeys call when creating the table, as it's not needed.
4651511 to
0f33259
Compare
0f33259 to
287b0e3
Compare
| default: | ||
| return v | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we should always use the value returned by driver.Valuer even if it is not comparable.
I suggest to do this:
val, err := valuer.Value()
if err != nil {
slog.Error(...)
}
return val // might be nil
There was a problem hiding this comment.
Maybe we don't need to log because the panic will come later
There was a problem hiding this comment.
It might panic on nil value (or not), but you won't see the error message in app logs which will make debugging slightly more complicated... Up to you.
model_table_has_many.go
Outdated
| @@ -172,5 +172,11 @@ func indirectAsKey(field reflect.Value) interface{} { | |||
| if field.IsNil() { | |||
| return nil | |||
| } | |||
|
|
|||
| if valuer, ok := field.Interface().(driver.Valuer); ok { | |||
| v, _ := valuer.Value() // No need to record logs, will panic later | |||
There was a problem hiding this comment.
Do we need to be compatible with cases similar to #1107?
Suppose the user is using *ULID, if we don’t handle this specifically,
it will panic because [16]byte is unhashable in Go.
There was a problem hiding this comment.
We only handle pointer-based driver.Valuer implementation. What do you think?
There was a problem hiding this comment.
In your test case, the struct itself is not comparable, but the result returned by Value is comparable.
So it would have panicked before, but now it won’t.
In the example I provided, the struct(array) itself is comparable,
but the result returned(slice) by Value is not comparable.
So it wouldn’t have panicked before, but now it will.
type ULID [16]byte
func (id *ULID) Scan(any) error { return nil }
func (id *ULID) Value() (driver.Value, error) { return []byte{}, nil }
type User struct {
ID *ULID `bun:",pk"`
Name string
Posts []*Post `bun:"rel:has-many,join:id=user_id"`
}
type Post struct {
PostID string `bun:",pk"`
UserID *ULID
}Maybe I misunderstood, but this isn’t related to whether it’s pointer-based, is it?
There was a problem hiding this comment.
Interfaces like driver.Valuer, json.Marshaler, and most other encoding interfaces are generally implemented directly on values rather than pointers.
Protobuf generated code has another characteristic, DoNotCopy , in addition to DoNotCompare , this imposes a strict requirement that all methods must be pointer-based when extending the generated code.
Deviating from this best practice will inevitably lead to more issues. For example, just like now.
I will fix it later.
No description provided.