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

[9.x] Cursor pagination: convert original column to expression #41003

Conversation

gdebrauwer
Copy link
Contributor

@gdebrauwer gdebrauwer commented Feb 14, 2022

Imagine you write the following query:

User::query()
    ->select('*')
    ->selectSub('CONCAT(firstname, \' \', lastname)', 'fullname')
    ->orderBy('fullname')
    ->orderBy('id')
    ->cursorPaginate()

According to the documentation, this should work:

Query expressions in "order by" clauses are supported only if they are aliased and added to the "select" clause as well.

But this query will only work when fetching the first page. When you fetch the users of the second page / next cursor, you will get the following error:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'CONCAT(firstname, ' ', lastname)' in 'where clause'

This is caused by the fact that the raw output of the getOriginalColumnNameForCursorPagination method is provided to a where method. This means that the original column value is interpreted as a real column name. That is not the case when aliasing a dynamic column.

By wrapping the result of the getOriginalColumnNameForCursorPagination method in an \Illuminate\Database\Query\Expression object, we ensure that the original column value is correctly interpreted.

@driesvints
Copy link
Member

Seems like tests are still failing here. Please fix the failing tests and mark as ready.

@driesvints driesvints marked this pull request as draft February 14, 2022 12:51
@gdebrauwer gdebrauwer marked this pull request as ready for review February 14, 2022 13:04
@taylorotwell
Copy link
Member

Won't this break columns that actually need to be quoted?

@gdebrauwer
Copy link
Contributor Author

gdebrauwer commented Feb 14, 2022

Oh I did not know that in certain scenario's the columns need to be quoted 🤔 I changed the logic so it checks if the original column is an expression. It does this by checking if the original column starts with a ( and ends with a ).

@gdebrauwer
Copy link
Contributor Author

To do this I also needed to use the grammar of the builder in the getOriginalColumnNameForCursorPagination. This is because the alias may already be wrapped in quotes and the current check $parameter === $alias would not pass in that case, because $parameter is not quoted while the $alias might be.

I'm not sure if it is allowed to use the grammar in that place 🤔

@GrahamCampbell
Copy link
Member

It does this by checking if the original column starts with a ( and ends with a ).

That logic looks wrong to me. Different sql engines have different rules.

@gdebrauwer
Copy link
Contributor Author

I chose to use that logic because the selectSub method also just wraps the subquery in parentheses, regardless of the sql engine.

return $this->selectRaw(
'('.$query.') as '.$this->grammar->wrap($as), $bindings
);

@taylorotwell taylorotwell merged commit bf90720 into laravel:9.x Feb 16, 2022
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.

4 participants