PhilWaldmann / openrecord

Make ORMs great again!
https://openrecord.js.org
MIT License
486 stars 38 forks source link

Bugs when using offset / limit #91

Closed arthurfranca closed 5 years ago

arthurfranca commented 5 years ago

Don't know if it should be that way but currently, any offset >= 1 will return a count of 0

(await Model.offset()).length // 5
await Model.offset().count() // 5
await Model.offset(0).count() // 5
(await Model.offset(1)).length // 4
await Model.offset(1).count() // 0!!!

And limit is ignored when counting

(await Model.limit(1)).length // 1
await Model.limit(1).count() // 5!!!
(await Model.limit(1).offset(1)).length // 1
await Model.limit(1).offset(1).count() // 0!!!

Also, updateAll is ignoring both limit and offset.

(await Model.offset(1)).forEach(m => m.update({ updatedAt: new Date() })) // will update 4 records
await Model.offset(1).updateAll({ updatedAt: new Date() })  // will update all 5 records instead of 4!!!
await Model.limit(1).updateAll({ updatedAt: new Date() })  // will update all 5 records instead of 1!!!
await Model.limit(1, 1).updateAll({ updatedAt: new Date() })  // will update all 5 records instead of 1!!!
await Model.limit(1).offset(1).updateAll({ updatedAt: new Date() })  // will update all 5 records instead of 1!!!
PhilWaldmann commented 5 years ago

This is normal SQL behavior. OFFSET and LIMIT will be applied to the result set.
So a SELECT COUNT(*) FROM your_table will return one row. With an offset of 1 you will skip the result and get nothing... This is the behaviour of postgres an sqlite. MySql will ignore the offset and return the count.

regarding updateAll: Yes, that's not implemented. It's not very precise to updated your records this way. It's better to use a where clause to limit the number of affected rows.
e.g.

await Model.where({id_between: [1, 5]}).updateAll({ updatedAt: new Date() })
arthurfranca commented 5 years ago

Yes the count part was more of a wish the framework could bend the query and make it work as length but you are right.

About updateAll, in my app i was already filtering with where but wanted to do something like: where userId = ... and ..., after ordering by updatedAt desc, offset 5 and update the rest removing a premium flag feature as the user will only be able to keep 5 records with the flag after losing the premium status.

I could fetch the records first and then update them but tried to use updateAll to update them with only one trip do database.

PhilWaldmann commented 5 years ago

ahhh, okay!

UPDATE your_table SET foo = 'bar' LIMIT 2

is also no valid SQL. But we could imitate it via

UPDATE your_table SET foo = 'bar' WHERE id IN (SELECT id FROM your_table ORDER BY id LIMIT 2)

I've added the possibility to do this kind of subqueries in UPDATE and DELETE queries.
Checkout the latest version!

arthurfranca commented 5 years ago

Thx that is great! I was about to do it using a mix of .toSql and .raw but now it's way easier.

But i'm getting error at this line if i use .delete or .deleteAll without any filter (where or limit etc), like await Model.deleteAll() to help clear DB on tests

Similarly, also here if using await Model.updateAll({ ... })

TypeError: this.getInternal is not a function

PhilWaldmann commented 5 years ago

You're right. My mistake! Fixed in 2.9.2