volatiletech / sqlboiler

Generate a Go ORM tailored to your database schema.
BSD 3-Clause "New" or "Revised" License
6.73k stars 544 forks source link

uint64 equality and eager loading bug #399

Closed mickeyreiss closed 6 years ago

mickeyreiss commented 6 years ago

We are facing a strange issue where eager loads on nullable uint64 fields, such that eager loads of related models completely fail:

primitive type of a (int64) was not the same primitive type as b (uint64)

We have narrowed this down to a possible bug in null/int64.go:

// Value implements the driver Valuer interface.
func (u Uint64) Value() (driver.Value, error) {
    if !u.Valid {
        return nil, nil
    }
    return int64(u.Uint64), nil
}

We also dug into reflect.go and found some potentially suspect code. Apparently unsigned numbers are casted to signed numbers before == in upgradeNumericTypes. Maybe this is correct, but we didn't see an explicit comment around comparing unsigned and signed values.

Is it correct to cast the uint64 to an int64 here?

We use mysql bigint unsigned for our primary keys and foreign relations, and we are seeing this in a number of situations where we are eager loading a model with a bigint unsigned foreign key to the model being queried.

We tried the obvious thing..hacking the line above to uint64, but then other errors crop up. We presume that the bug is consistent in a number of places in the generated models.

P.S. Does the Value bug above lead to a underflow issues for negative numbers?

cc/ @jthreatt4

fridolin-koch commented 6 years ago

We are facing the same issue. I'm not sure what the exact reason for the cast to int64 is, I suspect it is somehow related with https://github.com/golang/go/issues/18417 and general driver support (e.g.: https://github.com/go-sql-driver/mysql/pull/838) for uint64? (correct me if I'm wrong :) )

I mitigated the issue by modifying upgradeNumericTypes which seems to fix the issue, but I'm not sure if this will have any side effect as I'm not really familiar with the code base.

This is the change: https://github.com/styque/sqlboiler/commit/4c7a446d2914e59b679a911a2718c28a8da5a45d

aarondl commented 6 years ago

@mickeyreiss The problem starts out here: https://golang.org/pkg/database/sql/driver/#Value

It's not acceptable for Value() to return a uint64 as per the interface definition. The only types we can use are:

int64
float64
bool
[]byte
string
time.Time

Now, there should be no underflow/overflow occurring here because we are just modifying the representation but the bits should stay the same: https://play.golang.org/p/SdVS-5biJqS

I actually think that @fridolin-koch's solution is correct. There's just a missing case because if it is a uint64 we do need to change it into an int64 to be compatible with whatever is coming back from the Value interface. I've made this change in the dev branch now (pushing very soon).