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

Edge case breaking Cursor Pagination #37400

Closed
kamui545 opened this issue May 18, 2021 · 9 comments · Fixed by #37915
Closed

Edge case breaking Cursor Pagination #37400

kamui545 opened this issue May 18, 2021 · 9 comments · Fixed by #37915
Labels

Comments

@kamui545
Copy link
Contributor

kamui545 commented May 18, 2021

  • Laravel Version: 8.41.0
  • PHP Version: 8.0.6
  • Database Driver & Version: mysql Ver 14.14 Distrib 5.7.33-36

Description:

It seems that the new Cursor Pagination from #37216 does not work well when specifying the table used in orderBy / latest (when working with inner join / ambiguous columns).

Edit: Actually, it seems like this is related to pivots

Steps To Reproduce:

I've got the following relationship on my User model, an user can hide other users:

public function hidden()
{
    return $this
        ->belongsToMany(User::class, 'hidden_profiles', 'user_id', 'hidden_id')
        ->withTimestamps();
}

What I would like to do is displaying that list of hidden users, ordered by the latest added.
Simply done like that:

$user->hidden()->latest('created_at')->cursorPaginate();

However, this does not work because of ambiguous columns, both users and hidden_profiles have created_at columns, so we have to give the proper table:

$user->hidden()->latest('hidden_profiles.created_at')->cursorPaginate();

This works, but sadly once you give him a cursor (browsing next page), it will break with the following error:

Only arrays and objects are supported when cursor paginating items.

I will try to work on a patch or at least a repo / failing test to reproduce this easily as soon as I can.

@paras-malhotra Is this a limitation by design? Any idea where I should start?

@paras-malhotra
Copy link
Contributor

@kamui545, cursor pagination needs the values of the columns to construct the query. So, I guess because of the name collision the model may not be hydrated with the value for cursor pagination to use. Perhaps you can alias the value and try using the alias in latest?

In case that doesn't work, it would be great if you could share the migrations/relationship method so that I can reproduce on my end.

@kamui545
Copy link
Contributor Author

@paras-malhotra Yes aliasing was the first thing that came to my mind, sadly once using a cursor (browsing the next page) the app would crash because of SQL errors, it cannot find the alias.

I just finished my repo: https://github.com/kamui545/laravel-cursor

php artisan migrate:fresh --seed
php artisan serve

You should be able to see something like this:

Screen Shot 2021-05-18 at 18 55 23

I tried to cover every cases and highlighted the issues.
It seems like actually the pivot is not populated but merged instead.

Let me know if you need more information, I think the code speaks for itself and if I try to explain it myself I will end up confusing you 😂.

I will continue investigating as well. Thanks !

@vanthao03596
Copy link
Contributor

I see in BelongsToMany relationships have their own paginate and simplePaginate methods while all other relationships use the default implementation from the Builder class. Maybe we need implement cursorPaginate in BelongsToMany relationship ?. Same for HasManyThrough

@driesvints
Copy link
Member

@paras-malhotra have you found time to look into this yet?

@paras-malhotra
Copy link
Contributor

@driesvints, not yet. I'll work on this before the end of the week and submit a PR if there's a good solution to the issue.

@driesvints
Copy link
Member

Sorry to ping you again @paras-malhotra but did you already found a solution to this?

@paras-malhotra
Copy link
Contributor

@driesvints sorry to have not responded. Something urgent has come up. I'll definitely try and work on it this week.

@driesvints
Copy link
Member

@paras-malhotra Don't worry about it 👍

@driesvints
Copy link
Member

I took a very deep dive into all this and came up with a working solution: #37915

@kamui545 I'd appreciate if you could test that PR. It fixed the repo you provided (thanks for that btw).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants