[8.x] Add proper paging offset when possible to sql server

[joelharkes]

Sql server does not support paging without a order by column.

But the current implementation leads to issues when using GUID (or any other primary key column that is not ordered) column as ID's and generating those id's in laravel. (sql orders id's differently than alphabetically, so the order of the models inserted is differently than the order of ID's).

Current setup

Old setup when paging and sorting:

page1:

SELECT TOP 50 [users].* FROM [users] WHERE [users].[deleted_at] IS NULL ORDER BY [id] ASC

page2:

SELECT * FROM (SELECT [users].*, row_number() OVER (ORDER BY [id] ASC) AS row_num FROM [users] WHERE [users].[deleted_at] IS NULL) AS temp_table WHERE row_num between 51 and 100 ORDER BY row_num

Proposed solution

page1:

SELECT TOP 50 [users].* FROM [users] WHERE [users].[deleted_at] IS NULL ORDER BY [id] ASC

page2:

SELECT [users].* FROM [users] WHERE [users].[deleted_at] IS NULL ORDER BY [id] ASC OFFSET 50 ROWS FETCH NEXT 50 ROWS ONLY

Edge cases

• OFFSET 50 ROWS FETCH NEXT 10 is a feature added to sql server in 2012. But you are already using features that are not supported by older versions.
• this only works when you provide an order column as this is mandatory in sql server.

improvements

• instead of needing to query the whole table, now it will stop after Offset+Limit. (eg page 2 on 50 results = after 100 results instead of the whole table).
• when ordering you will no longer get the issue of rows being ordered differently on the first and subsequent pages.

comments

please don't close my pull request without proper feedback, but actually help me land this pull request (previous PR got closed on just 1 tiny comment).

[joelharkes]

Ideally you'd always want to add an orderby when using SQL (maybe best to add this to documentation) as without it could still lead to issues now.

Preferable user would fallback to using just the id column to sort on.

[joelharkes]

Extra solution could be to always order by 1 if no order is provided eg:

select * from users order by 1 asc

Drawbacks:

• Order is not guaranteed: if you select name from users order by 1 you order on name

benefits:

• You can always user proper fetch and limit never having this issue again.

Either way to completely tackle this issue a user should always provide an order by statement in the query builder when requesting paged data.

[taylorotwell]

Does this PR contain any breaking changes at all? Even minor ones.

Does this new offset stuff have to be all caps? I don't think any of our other query sections are.

[joelharkes]

@taylorotwell current setup, nothing is breaking. all-caps are not necessary microsoft always documents their operators in uppercase so thats where i got it from. want me to make it lowercase?

as said: this feature is from sql server 2012 but i don't think you currently support any lower version anyways?

[driesvints]

@joelharkes we support 2017 and up: https://laravel.com/docs/8.x/database#introduction

[joelharkes]

Thanks for merging :)

[henryavila]

@taylorotwell , Hi, This break my app. Now no Pagination is working (Laravel Nova). I'm usgin SqlServer

[henryavila]

@joelharkes we support 2017 and up: https://laravel.com/docs/8.x/database#introduction

But with the add of this feature, all apps that use older version of Sql Server will stop working (apps that are working fine today). It's a breaking change!

[X-Coder264]

@henryavila That's not how the support policy works. Laravel 8 officially supports only SQL Server 2017+, meaning that it's not officially guaranteed that Laravel 8 will work on anything below that so this is not a breaking change (as all officially suported SQL server versions work fine with this change) and basically it's pure luck that your Laravel 8 app worked with SQL Server 2008 at all until now.

[henryavila]

Hi @X-Coder264, I agree with you. We are making our best to update SQL Server to 2019 as soon as possible, but on big companies, its not so easy, and definitly, not fast.

Laravel 9 is almost been released, so if this change could be released on Laravel 9, will help other users like me, the app will not brake and we can still install security/minor update on Laravel 8.

[PaolaRuby]

@henryavila overwrite SqlServerGrammar.php with your own custom grammar(old version of the file) on providers, or rollback to old laravel framework version till you upgrade your database
app('db.connection')->setQueryGrammar(new CustomGrammar())

[joelharkes]

sorry @henryavila

Good to know that the sql server version you are using is no longer officially supported by microsoft and thus is very dangerous to use. Out of security perspective an enormous liability.

as mentioned: the previous Laravel implementation actually broke our paging as the sorting order wasn't 100% reliable. (you should still always provide an order now when paging as it could still lead to sorting issues!)

[joelharkes]

I would suggest to throw an exception when no sorting is provided in SQL server grammar when paging but i think this will never land as it could seriously break other people's apps.

[PaolaRuby]

#39977 (comment)