New Eager Loading Bug!

[danfinlay]

Hey @ben-ng, it's been a while, but I think I found a bug in eager-loading when using it with pagination!

If I have a bunch of posts and their users, and I want to paginate, I might make a query like this:

geddy.model.Post.all({}, {includes:'Person', limit:100, skip:100, sort:{createdAt:'desc'}}, callback);

which will first fetch the IDs of the relevant posts in an appropriate way, something like:

select post.id, post.createdAt from posts limit 100 skip 100 order by post.createdAt desc;

Which is working fine. The problem is that currently, the follow-up detail request KEEPS the offset, even when querying by ID, which means it skips all the correct results, and returns an empty array:

select post.*, person.* from posts post left outer join people person where ID in (1, 2, 3, 4,...)
ORDER BY post.created_at DESC, OFFSET 100;

This breaks pagination when using eager loaded associations! I think I've boiled it down it should be a quick fix, so I might take a look in there, but I'm sure it would be easier for someone who already has worked with this.

[ben-ng]

Woah! Nice find, I'll take a look after work 😊

[danfinlay]

My guess is that this condition should be modified to say:

    if (skip && !query.opts.includes) {

I might have time to check if that works after lunch. Haven't gotten Model's tests running locally somehow. Need to take the time.

[danfinlay]

Actually that could be the pre-query, not sure..

[ben-ng]

had an unusually long day at work, going to have to defer this to tomorrow 😞

ping me on twitter if i forget! i have it on my to-do list.

[danfinlay]

Yeah no hard feelings, I had a long day too. Hope you beat those tasks back soon!

[danfinlay]

I wrote a test for what I described, but it's passing. If I can't replicate this soon I'll just close it, maybe something else is going on.

These tests seem to suggest that having an offset of 2 will wrap around the set if there are only two results for the query.