Merge pull request #4438 from trusek/2.12.x

#4437 MSSQL adds a redundant ORDER BY clause when with subquery with …
doctrine · Dec 7, 2020 · 5a7befb · 5a7befb
2 parents 98233ab + 7aac084
commit 5a7befb
Show file tree

Hide file tree

Showing 3 changed files with 82 additions and 13 deletions.
diff --git a/lib/Doctrine/DBAL/Platforms/SQLServer2012Platform.php b/lib/Doctrine/DBAL/Platforms/SQLServer2012Platform.php
@@ -101,19 +101,7 @@ protected function doModifyLimitQuery($query, $limit, $offset = null)
         }
 
         // Queries using OFFSET... FETCH MUST have an ORDER BY clause
-        // Find the position of the last instance of ORDER BY and ensure it is not within a parenthetical statement
-        // but can be in a newline
-        $matches      = [];
-        $matchesCount = preg_match_all('/[\\s]+order\\s+by\\s/im', $query, $matches, PREG_OFFSET_CAPTURE);
-        $orderByPos   = false;
-        if ($matchesCount > 0) {
-            $orderByPos = $matches[0][$matchesCount - 1][1];
-        }
-
-        if (
-            $orderByPos === false
-            || substr_count($query, '(', $orderByPos) - substr_count($query, ')', $orderByPos)
-        ) {
+        if ($this->shouldAddOrderBy($query)) {
             if (preg_match('/^SELECT\s+DISTINCT/im', $query)) {
                 // SQL Server won't let us order by a non-selected column in a DISTINCT query,
                 // so we have to do this madness. This says, order by the first column in the
@@ -143,4 +131,35 @@ protected function doModifyLimitQuery($query, $limit, $offset = null)
 
         return $query;
     }
+
+    /**
+     * @param string $query
+     */
+    private function shouldAddOrderBy($query): bool
+    {
+        // Find the position of the last instance of ORDER BY and ensure it is not within a parenthetical statement
+        // but can be in a newline
+        $matches      = [];
+        $matchesCount = preg_match_all('/[\\s]+order\\s+by\\s/im', $query, $matches, PREG_OFFSET_CAPTURE);
+        if ($matchesCount === 0) {
+            return true;
+        }
+
+        // ORDER BY instance may be in a subquery after ORDER BY
+        // e.g. SELECT col1 FROM test ORDER BY (SELECT col2 from test ORDER BY col2)
+        // if in the searched query ORDER BY clause was found where
+        // number of open parentheses after the occurrence of the clause is equal to
+        // number of closed brackets after the occurrence of the clause,
+        // it means that ORDER BY is included in the query being checked
+        while ($matchesCount > 0) {
+            $orderByPos          = $matches[0][--$matchesCount][1];
+            $openBracketsCount   = substr_count($query, '(', $orderByPos);
+            $closedBracketsCount = substr_count($query, ')', $orderByPos);
+            if ($openBracketsCount === $closedBracketsCount) {
+                return false;
+            }
+        }
+
+        return true;
+    }
 }
diff --git a/tests/Doctrine/Tests/DBAL/Functional/ModifyLimitQueryTest.php b/tests/Doctrine/Tests/DBAL/Functional/ModifyLimitQueryTest.php
@@ -2,11 +2,16 @@
 
 namespace Doctrine\Tests\DBAL\Functional;
 
+use Doctrine\DBAL\Platforms\DB2Platform;
+use Doctrine\DBAL\Platforms\OraclePlatform;
+use Doctrine\DBAL\Platforms\SQLServer2012Platform;
+use Doctrine\DBAL\Platforms\SQLServerPlatform;
 use Doctrine\DBAL\Schema\Table;
 use Doctrine\Tests\DbalFunctionalTestCase;
 
 use function array_change_key_case;
 use function count;
+use function preg_replace;
 
 use const CASE_LOWER;
 
@@ -195,4 +200,39 @@ private function assertLimitResult(
             self::assertCount(count($expectedResults), $data);
         }
     }
+
+    public function testLimitWhenOrderByWithSubqueryWithOrderBy(): void
+    {
+        $platform = $this->connection->getDatabasePlatform();
+        if ($platform instanceof DB2Platform) {
+            self::markTestSkipped('DB2 cannot handle ORDER BY in subquery');
+        }
+
+        if ($platform instanceof OraclePlatform) {
+            $this->markTestSkipped('Oracle cannot handle ORDER BY in subquery');
+        }
+
+        $this->connection->insert('modify_limit_table2', ['test_int' => 3]);
+        $this->connection->insert('modify_limit_table2', ['test_int' => 1]);
+        $this->connection->insert('modify_limit_table2', ['test_int' => 2]);
+
+        $subquery = 'SELECT test_int FROM modify_limit_table2 T2 WHERE T1.id=T2.id ORDER BY test_int';
+
+        // [SQL Server]The ORDER BY clause is invalid in views, inline functions, derived tables, subqueries,
+        // and common table expressions, unless TOP, OFFSET or FOR XML is also specified.
+        if (
+            $platform instanceof SQLServerPlatform
+            && ! $platform instanceof SQLServer2012Platform
+        ) {
+            $subquery = preg_replace('/^SELECT\s/i', 'SELECT TOP 1 ', $subquery);
+        }
+
+        if ($platform instanceof SQLServer2012Platform) {
+            $subquery .= ' OFFSET 0 ROWS';
+        }
+
+        $sql = 'SELECT test_int FROM modify_limit_table2 T1 ORDER BY (' . $subquery . ') ASC';
+
+        $this->assertLimitResult([1, 2, 3], $sql, 10, 0);
+    }
 }
diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLServer2012PlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLServer2012PlatformTest.php
@@ -479,4 +479,14 @@ public function testModifyLimitQueryWithNewlineBeforeOrderBy(): void
         $sql         = $this->platform->modifyLimitQuery($querySql, 10);
         self::assertEquals($expectedSql, $sql);
     }
+
+    public function testModifyLimitQueryWithOrderByWithSubqueryWithOrderBy(): void
+    {
+        $subquery    = 'SELECT col2 from test ORDER BY col2 OFFSET 0 ROWS FETCH NEXT 1 ROWS ONLY';
+        $querySql    = 'SELECT col1 FROM test ORDER BY ( ' . $subquery . ' )';
+        $expectedSql = 'SELECT col1 FROM test ORDER BY ( ' . $subquery . ' )'
+            . ' OFFSET 0 ROWS FETCH NEXT 10 ROWS ONLY';
+        $sql         = $this->platform->modifyLimitQuery($querySql, 10);
+        self::assertEquals($expectedSql, $sql);
+    }
 }