Skip to content
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

[8.x] Cursor pagination fixes #37915

Merged
merged 6 commits into from
Jul 13, 2021
Merged

[8.x] Cursor pagination fixes #37915

merged 6 commits into from
Jul 13, 2021

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Jul 5, 2021

This PR fixes two three things:

  • Proper column distribution for cursor base pagination when working with pivots (probably just an oversight from [8.x] Add cursor pagination (aka keyset pagination) #37216
  • Allow usage of aliased columns with order by statements when using cursor based pivots
  • Proper attribute retrieval for cursors when combining them with pivot models

Before the second fix, the query builder would generate the following invalid query:

SELECT
    *,
    `friends`.`created_at` AS `friended_at`,
    `users`.*,
    `friends`.`user_id` AS `pivot_user_id`,
    `friends`.`friend_id` AS `pivot_friend_id`,
    `friends`.`created_at` AS `pivot_created_at`,
    `friends`.`updated_at` AS `pivot_updated_at`
FROM
    `users`
    INNER JOIN `friends` ON `users`.`id` = `friends`.`friend_id`
WHERE
    `friends`.`user_id` = ?
    AND `friended_at` < ?
ORDER BY
    `friended_at` DESC
LIMIT 6;

Now it will generate:

SELECT
    *,
    `friends`.`created_at` AS `friended_at`,
    `users`.*,
    `friends`.`user_id` AS `pivot_user_id`,
    `friends`.`friend_id` AS `pivot_friend_id`,
    `friends`.`created_at` AS `pivot_created_at`,
    `friends`.`updated_at` AS `pivot_updated_at`
FROM
    `users`
    INNER JOIN `friends` ON `users`.`id` = `friends`.`friend_id`
WHERE
    `friends`.`user_id` = ?
    AND `friends`.`created_at` < ?
ORDER BY
    `friended_at` DESC
LIMIT 6;

The reason for this is that sql engines need an actual column to sort and can't rely on an aliased column of a result set. By de-aliasing the given columns we circumvent this issue.

I don't think this contains anything breaking but I'm unsure about the usage to detect the column aliasing:

stripos($column, ' as ') !== false

I saw this was used in the base query grammar class as well with the wrap method so I think this will be fine.

This PR will conflict with #37762

Fixes #37400

@driesvints driesvints force-pushed the fix-cursor-pagination branch 3 times, most recently from f939f96 to 3a51209 Compare July 5, 2021 16:23
@driesvints
Copy link
Member Author

@paras-malhotra would appreciate a review if you're willing.

@kamui545
Copy link
Contributor

kamui545 commented Jul 6, 2021

@driesvints Thank you for your hard work, really appreciate it.

Hydration seems to be working properly for the pivot now!

Sadly, I am still having weird behaviour when browsing the "next" page.

On my playground repo, if you're browsing the next page on the cursorPaginate() without alias, you will have the Only arrays and objects are supported when cursor paginating items error.

Using an alias seems to fix the issue on the playground, however, I tried in my own app and I get the same error even with the alias... I've got no idea what's going on, the code of the playground is based from my app and should be similar... I will keep you updated once I figured out... but for now im banging my head against the wall

@driesvints
Copy link
Member Author

I'll need to check in on that on Thursday. I also see it failing but also can't figure it out atm.

@kamui545
Copy link
Contributor

kamui545 commented Jul 6, 2021

Ok so after spending some time debugging, the alias is now working properly on my real app as well. I was sorting on a non-unique column, my records all had the same timestamps and the next page returned no results, failing to build the previous cursor because $this->items->first() was null...

Stupid mistake, just like I did on the playground app: it should be sorting on id instead of created_at to make sure it's unique...

The app is still acting weird when not using an alias though. Eg:

$friends = $this->user()
  ->friends()
  ->latest('friends.id')
  ->cursorPaginate();

I am not sure if there is an easy way to fix this.

Maybe creating an alias automatically when a dot is used would do the trick. Just need to make sure it does not collide.

I will investigate further this week.

@paras-malhotra
Copy link
Contributor

@driesvints I went through the code. I think this PR looks good to go. Definitely seems to have been an oversight on my part to not hydrate the pivot relations and not fetch the original columns for the where clause.

I guess #37762 changed the where clauses entirely but this logic would still need to be applied to that PR.

@driesvints driesvints force-pushed the fix-cursor-pagination branch 2 times, most recently from 06a4b80 to 6cbb48a Compare July 12, 2021 15:07
@driesvints driesvints force-pushed the fix-cursor-pagination branch from 6a84797 to fdec468 Compare July 12, 2021 15:08
@driesvints
Copy link
Member Author

@kamui545 think I got a fix in for the issue you were experiencing. The app you shared is now fully working with this PR. Can you try to verify that?

@paras-malhotra we pushed one more commit to fix the above. It basically will retrieve attributes on a pivot relationship when the table matches with the first part of the dot notation of a parameter name. I'll try some more use case with other relationship queries tomorrow. But this will essentially allow to filter on non-unique column names as long as the table is prefixed.

@driesvints driesvints marked this pull request as ready for review July 13, 2021 12:27
@taylorotwell taylorotwell merged commit 7e0e0d0 into 8.x Jul 13, 2021
@taylorotwell taylorotwell deleted the fix-cursor-pagination branch July 13, 2021 12:38
@kamui545
Copy link
Contributor

@driesvints Thank you again! It seems to fix ordering on ambiguous columns.

However, I noticed another issue: #37999

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

Successfully merging this pull request may close these issues.

Edge case breaking Cursor Pagination
5 participants