Skip to content

uint64 equality and eager loading bug #399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mickeyreiss opened this issue Oct 3, 2018 · 2 comments
Closed

uint64 equality and eager loading bug #399

mickeyreiss opened this issue Oct 3, 2018 · 2 comments

Comments

@mickeyreiss
Copy link
Contributor

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
Copy link

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 golang/go#18417 and general driver support (e.g.: go-sql-driver/mysql#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: styque@4c7a446

@aarondl
Copy link
Member

aarondl commented Oct 10, 2018

@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).

@aarondl aarondl closed this as completed Oct 10, 2018
aarondl added a commit that referenced this issue Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants