[8.x] Change the order of the bindings for a Sql Server query with a offset and a subquery order by

[thijsvdanker]

As SQL Server moves the OrderBy part to be part of the "select" part of
the query when using an offset, we need to move the Order bindings to be in the right place.

Example

When using a subquery in the OrderBy

User::query()
    ->where('email', ':emailbinding:')
    ->orderBy(Login::select('created_at')->where('name', ':namebinding:')->limit(1))
    ->skip(10)->take(10),  

this in the following query

select * from (select *, row_number() over (order by (select top 1 [created_at] from [logins] where [logins].[name] = ? and [user_id] = [users].[id]) asc) as row_num from [users] where [email] = ?) as temp_table where row_num between 11 and 20 order by row_num

Notice the that [logins].[name] = ? (part of the original order part) comes before the [email = ?] (part of the where clause) part.

The normal order of the bindings is:

    public $bindings = [
        'select' => [],
        'from' => [],
        'join' => [],
        'where' => [],
        'groupBy' => [],
        'having' => [],
        'order' => [],
        'union' => [],
        'unionOrder' => [],
    ];

When getting the bindings, this results in the order bindings coming after the where bindings.
This results in the following query:

select * from (select *, row_number() over (order by (select top 1 [created_at] from [logins] where [logins].[name] = ':emailbinding:' and [user_id] = [users].[id]) asc) as row_num from [users] where [email] = ':namebinding:') as temp_table where row_num between 11 and 20 order by row_num

This pull requests changes the order of the bindings by moving order to come right after selectwhen a offset is used

-- edit: Changed as "limit" to "offset" as that is what determines the current issue.
-- edit 2021-07-21: Fix example to use :namebinding: consistently

[driesvints]

Previous attempts at this over the past week have been either causing breaking changes or have been refused. So please make 100% sure that this actually fixes things without breakages on other apps.

[thijsvdanker]

100% is ambitious but I'll do my best.
@driesvints are there already particular cases you are worried about?

@spawnia, @Javdu10, @madbarron, @arjenvanoostrum @Danil42Russia I noticed you already submitted SQL Server related PR's.
Could you give this patch a try in your application and see if it causes any issues?

[thijsvdanker]

The currently failing test looks unrelated to this change:

 1) Illuminate\Tests\Integration\Queue\ThrottlesExceptionsWithRedisTest::testCircuitResetsAfterSuccess
Mockery\Exception\NoMatchingExpectationException: No matching handler found for Mockery_149_Illuminate_Contracts_Queue_Job::release(599). Either the method was unexpected or its arguments matched no expected argument list for this method

[thijsvdanker]

Without any more proof, this is my current rationale:

The possibilities of breaking anything is more limited then the other PR's this week:

• this code only applies to SQL Server
• this code only applies to queries with a limit

The behavior with this fix is better then the current behavior:

• this only moves the position of the order bindings to be congruent with the move of the order query part
• if there are no order bindings, nothing happens
• if there are order bindings, the old situation
  1. breaks obviously with an exception because of a mismatch in field types (int field gets a string binding)
  2. or results in the wrong results because the wrong bindings are used for the wrong fields

[driesvints]

The currently failing test looks unrelated to this change:

Think you hit a false negative. Restarted them and they look fine now.

[spawnia]

This should be defined in a class constant to avoid instantiating this array every time this method is called.

[thijsvdanker]

Thanks for taking a look @spawnia ! Until we have a green light on the PR I prefer to keep it inline to make it easier to reason about.

[taylorotwell]

Your PR mentions "when a limit is used" but this change is in the "offset" method? Also, does this not change the logic for all order by's when an offset is used? Were all order by clauses that also had an offset broken prior to this PR? This seems geared to a particular cause of sub-query order by's that use a limit.

In other words, I'm pretty apprehensive this is going to break other more common queries that aren't currently broken.

[thijsvdanker]

@taylorotwell

Your PR mentions "when a limit is used" but this change is in the "offset" method?

My bad. This is indeed related to the "offset" part. I first noticed it when using pagination and attributed it to the limit which is clearly wrong.

Also, does this not change the logic for all order by's when an offset is used?

It does change the logic for all order by's of the "offsetted" query as all the order query logic is moved to the select.

Were all order by clauses that also had an offset broken prior to this PR?

No, only the order by clauses that had bindings (subqueries).

In other words, I'm pretty apprehensive this is going to break other more common queries that aren't currently broken.

In understand your hesitance.
Would it help if we would limit this logic to only apply when at least one of the order columns is a subquery?

[taylorotwell]

I do think limiting the logic to order bys that are subqueries is safer.

[thijsvdanker]

👍 I've updated this PR to check for subqueries in the $query->orders first.

[arjenvanoostrum]

Hi,
I've looked at the files and tested some scenarios. The fix solves the issue mentioned (though your example in the first comment mentions :datebinding: which is not found in the resulting query, a bit confusing).

One thing I noticed is that the new binding order is not applied when getBindings() is called before toSql(). So:

$query = User::query()
    ->where('email', ':emailbinding:')
    ->orderBy(Login::select('created_at')->where('name',':namebinding:')->where( 'user_id', 'users.id')->limit(1))
   ->skip(10)->take(10);

$query->getBindings(); // [':emailbinding:', ':namebinding:']
$query->toSql();
$query->getBindings(); // [':namebinding:', ':emailbinding:']

No issue when fired at a database by the framework, however it is confusing when debugging the bindings.

[thijsvdanker]

Thanks for taking a look @arjenvanoostrum

I've looked at the files and tested some scenarios. The fix solves the issue mentioned (though your example in the first comment mentions :datebinding: which is not found in the resulting query, a bit confusing).

I've fixed the example to use :namebinding: consistently.

One thing I noticed is that the new binding order is not applied when getBindings() is called before toSql(). So:

$query = User::query()
    ->where('email', ':emailbinding:')
    ->orderBy(Login::select('created_at')->where('name',':namebinding:')->where( 'user_id', 'users.id')->limit(1))
   ->skip(10)->take(10);

$query->getBindings(); // [':emailbinding:', ':namebinding:']
$query->toSql();
$query->getBindings(); // [':namebinding:', ':emailbinding:']

No issue when fired at a database by the framework, however it is confusing when debugging the bindings.

Good point. This could lead to confusion.
This could be fixed by adding logic to orderBy(), orderByRaw() and offset() or by duplicating logic to getBindings().
orderBy() already contains logic for detecting subqueries.
This would mean we would have to touch code outside of the SQL Server grammar which is unfortunate.

A reason not to go down that road is that the most common ways of debugging queries (debugbar, telescope, ray) would not be suffering from this confusion. They listen to the QueryExecuted event and toSql() has already taken place by then.

My suggestion is:

• to keep it in the SQL Grammar for now
• accept the possible confusion when manually debugging to be of a lesser evil then the bug
• consider the more risky fix for a major release