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

error updating an instance because of integer id treated as string #11

Open
spondbob opened this Issue Jul 28, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@spondbob

spondbob commented Jul 28, 2017

When I do an update from its instance

User.findOne({ where: { email: "my@email.com" } }).then(user => {
  user.update({ password })
})

It triggers an error saying

SequelizeDatabaseError: unsupported comparison operator: <int> = <string>

Because it tries to do this query

UPDATE "user" SET "name"='My name',"reset_at"='2017-07-28 07:04:52.688 +00:00',"updated_at"='2017-07-28 07:04:52.706 +00:00' WHERE "id" = '265875070405378049'

Notice quotes between id '265875070405378049' where it should be an integer instead of string. I have the same error when I am trying to update the association such as

user.getProfile().then(profile => {
  profile.update({ name })
})

The closest workaround I found is from this comment by setting require('pg').defaults.parseInt8 = true. I do get an integer id when doing the above query, but the weird thing is I do not get the right id.

For example if my id in db is 265875070405378049 the query produces something like ... WHERE "id" = 265875070405378050. Most of the time it uses an integer +/- 1 of the actual id. Same thing going on when I create a record with association. the User.id will be something like 265923186502860801 but the user_id in Profile will be like 265923186502860800

For the record my model is as simple as

const User = sequelize.define('user', {
  id: {
    type: Sequelize.INTEGER,
    primaryKey: true,
    autoIncrement: true
  },
  name: Sequelize.STRING,
}, {
  underscored: true,
  constraints: false,
  timestamps: true
})
@benesch

This comment has been minimized.

Show comment
Hide comment
@benesch

benesch Jul 28, 2017

Member

Hmm, I thought I fixed this bug. I can investigate more throughly later today, but in the meantime, what version of sequelize-cockroachdb are you using?

Member

benesch commented Jul 28, 2017

Hmm, I thought I fixed this bug. I can investigate more throughly later today, but in the meantime, what version of sequelize-cockroachdb are you using?

@spondbob

This comment has been minimized.

Show comment
Hide comment
@spondbob

spondbob Jul 28, 2017

@benesch I checked out the latest version from npm, the 1.0.2. I will try later checking out from this repo. Is that making any difference?

spondbob commented Jul 28, 2017

@benesch I checked out the latest version from npm, the 1.0.2. I will try later checking out from this repo. Is that making any difference?

@benesch

This comment has been minimized.

Show comment
Hide comment
@benesch

benesch Jul 28, 2017

Member

You'll definitely need 7909492, but I thought that made the cut for 1.2. Perhaps there's another bug lurking.

Member

benesch commented Jul 28, 2017

You'll definitely need 7909492, but I thought that made the cut for 1.2. Perhaps there's another bug lurking.

@benesch

This comment has been minimized.

Show comment
Hide comment
@benesch

benesch Jul 28, 2017

Member

Anyway, to answer your question more thoroughly, the reason that you get a slightly-wrong ID when you enable parseInt8 is because JS numbers cannot represent large 64-bit numbers exactly. (All Numbers in JavaScripts are floats under the hood, so once you've exceeded the 53-bits in the mantissa, you're out of luck.) CockroachDB SERIAL columns almost always return numbers that require more than 53 bits to represent exactly, and so JS silently rounds them to the nearest representable integer.

Sequelize + Postgres handle int8s out of the box because Sequelize treats them as strings, and Postgres is smart enough to translate the string literal '265923186502860801' to a proper int8 (265923186502860801). Cockroach isn't yet (see cockroachdb/cockroach#15903).

I hacked around this limitation at the sequelize adapter layer in 7909492, but perhaps that hack is only sufficient for inserts, and not for updates. I'll do more investigation later.

Member

benesch commented Jul 28, 2017

Anyway, to answer your question more thoroughly, the reason that you get a slightly-wrong ID when you enable parseInt8 is because JS numbers cannot represent large 64-bit numbers exactly. (All Numbers in JavaScripts are floats under the hood, so once you've exceeded the 53-bits in the mantissa, you're out of luck.) CockroachDB SERIAL columns almost always return numbers that require more than 53 bits to represent exactly, and so JS silently rounds them to the nearest representable integer.

Sequelize + Postgres handle int8s out of the box because Sequelize treats them as strings, and Postgres is smart enough to translate the string literal '265923186502860801' to a proper int8 (265923186502860801). Cockroach isn't yet (see cockroachdb/cockroach#15903).

I hacked around this limitation at the sequelize adapter layer in 7909492, but perhaps that hack is only sufficient for inserts, and not for updates. I'll do more investigation later.

benesch added a commit that referenced this issue Aug 17, 2017

convince sequelize to call INTEGER.$stringify on UPDATEs
The previous hack only worked for INSERTs. Convince it to work for
UPDATEs too.

Fixes #11.

benesch added a commit that referenced this issue Aug 18, 2017

convince sequelize to call INTEGER.$stringify on UPDATEs
The previous hack only worked for INSERTs. Convince it to work for
UPDATEs too.

Fixes #11.
@benesch

This comment has been minimized.

Show comment
Hide comment
@benesch

benesch Aug 27, 2017

Member

Good news: we've decided to fix this in CockroachDB. (For details: cockroachdb/cockroach#15903.) The fix will ship in the next alpha release. Leaving this open in the meantime to remind us to remove the half-baked patches I submitted here.

Member

benesch commented Aug 27, 2017

Good news: we've decided to fix this in CockroachDB. (For details: cockroachdb/cockroach#15903.) The fix will ship in the next alpha release. Leaving this open in the meantime to remind us to remove the half-baked patches I submitted here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment