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

Always append primary key field to ORDER BY clause to prevent already occurred results on other page numbers #6695

Open
ServerExe opened this issue Jan 9, 2025 · 1 comment
Milestone

Comments

@ServerExe
Copy link

ServerExe commented Jan 9, 2025

Background
I was struggling a lot yesterday when investigating an issue on my index page displaying results on pages which I already had on previous ones during pagination. In total I have 251 records with the default page size of 20 records per page. Because of searching a term, I had 26 results. When moving to the second page, there were 6 results shown as expected. But after a while I realized that the number of results was just an illusion. There was 1 record missing in the UI and 1 which appeared again on the second page. I couldn't find it during pagination neither on the first page nor on the second one. But in the database it was existent.

So, I started investigating if this is an EasyAdmin bug. Fortunately, it wasn't! I debugged and checked the query which was getting assembled after the pagination in the AbstractCrudController. The Query seemed okay. By the way: I was sorting my index page by a non-unique column + I had the search field filled. I was able to reproduce the issue on the MySQL instance. So it was clear, it can't be related to EasyAdmin. After some research on the web I came across several reported StackOverflow threads and finally I found the answer in the documentation of MySQL: https://dev.mysql.com/doc/refman/8.4/en/limit-optimization.html

If multiple rows have identical values in the ORDER BY columns, the server is free to return those rows in any order, and may do so differently depending on the overall execution plan. In other words, the sort order of those rows is nondeterministic with respect to the nonordered columns.

One factor that affects the execution plan is LIMIT, so an ORDER BY query with and without LIMIT may return rows in different orders.

In other words: Sorting + Pagination does not mean that each page will be a unique set of records. The Limit-Optimizer will always decide how to sort. This can lead to results which you already had before, because the uniqueness of ordering is not guaranteed if you sort by non unique columns -> nondeterministic.

The fix is to always append at least a unique column to the ORDER BY clause. As a workaround I did this in each of my crud controllers by setting:

public function configureCrud(Crud $crud): Crud
{
    return parent::configureCrud($crud)->setDefaultSort(['name' => 'ASC', 'id' => 'ASC']);
}

So, even if you sort manually by any column, the above default columns will always be appended by EasyAdmin to the query (even if you sort by "name", it will be present twice in the ORDER BY clause, but this is okay, I guess). This solution ensures that your results are always correct even if using search + filter + pagination.

Suggestion
Would it be possible to always append the primary key column as a default sort field? I am open for discussion because I don't know the downsides of such a solution.

@ServerExe ServerExe changed the title Always append primary key field to ORDER BY clause to prevent same already occurred results on other pages when paginating Always append primary key field to ORDER BY clause to prevent already occurred results on other page numbers Jan 9, 2025
@javiereguiluz javiereguiluz added this to the 4.x milestone Jan 18, 2025
@javiereguiluz
Copy link
Collaborator

Yes, I'd like to fix this ... but as you said, we need to be careful to not add something that breaks the feature for other folks.

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

No branches or pull requests

2 participants