Deduplicate order for MSSQL only and handle different directions

atk4 · Oct 15, 2023 · e06e0ff · e06e0ff
1 parent 6fe8b48
commit e06e0ff
Show file tree

Hide file tree

Showing 8 changed files with 93 additions and 58 deletions.
diff --git a/src/Model.php b/src/Model.php
@@ -1022,7 +1022,7 @@ public function addCteModel(string $name, self $model, bool $recursive = false)
      * Set order for model records. Multiple calls are allowed.
      *
      * @param string|array<int, string|array{string, 1?: 'asc'|'desc'}>|array<string, 'asc'|'desc'> $field
-     * @param 'asc'|'desc'                                                                          $direction
+     * @param ($field is array ? never : 'asc'|'desc')                                              $direction
      *
      * @return $this
      */

diff --git a/src/Persistence/Array_/Action.php b/src/Persistence/Array_/Action.php
@@ -128,18 +128,24 @@ protected function match(array $row, Model\Scope\AbstractScope $condition): bool
 
             return $this->evaluateIf($row[$field->shortName] ?? null, $operator, $value);
         } elseif ($condition instanceof Model\Scope) { // nested conditions
-            $matches = [];
+            $isOr = $condition->isOr();
+            $res = true;
             foreach ($condition->getNestedConditions() as $nestedCondition) {
-                $matches[] = $subMatch = $this->match($row, $nestedCondition);
+                $submatch = $this->match($row, $nestedCondition);
+
+                if ($isOr) {
+                    // do not check all conditions if any match required
+                    if ($submatch) {
+                        break;
+                    }
+                } elseif (!$submatch) {
+                    $res = false;
 
-                // do not check all conditions if any match required
-                if ($condition->isOr() && $subMatch) {
                     break;
                 }
             }
 
-            // any matches && all matches the same (if all required)
-            return array_filter($matches) && ($condition->isAnd() ? count(array_unique($matches)) === 1 : true);
+            return $res;
         }
 
         throw (new Exception('Unexpected condition type'))

diff --git a/src/Persistence/Sql/Mssql/Query.php b/src/Persistence/Sql/Mssql/Query.php
@@ -32,6 +32,19 @@ class Query extends BaseQuery
         end catch
         EOF;
 
+    protected function deduplicateRenderOrder(array $sqls): array
+    {
+        $res = [];
+        foreach ($sqls as $sql) {
+            $sqlWithoutDirection = preg_replace('~\s+(?:asc|desc)\s*$~i', '', $sql);
+            if (!isset($res[$sqlWithoutDirection])) {
+                $res[$sqlWithoutDirection] = $sql;
+            }
+        }
+
+        return array_values($res);
+    }
+
     protected function _renderLimit(): ?string
     {
         if (!isset($this->args['limit'])) {

diff --git a/src/Persistence/Sql/Query.php b/src/Persistence/Sql/Query.php
@@ -781,38 +781,6 @@ protected function _renderOption(): ?string
 
     // }}}
 
-    // {{{ Limit
-
-    /**
-     * Limit how many rows will be returned.
-     *
-     * @param int $cnt   Number of rows to return
-     * @param int $shift Offset, how many rows to skip
-     *
-     * @return $this
-     */
-    public function limit($cnt, $shift = null)
-    {
-        $this->args['limit'] = [
-            'cnt' => $cnt,
-            'shift' => $shift,
-        ];
-
-        return $this;
-    }
-
-    protected function _renderLimit(): ?string
-    {
-        if (!isset($this->args['limit'])) {
-            return null;
-        }
-
-        return ' limit ' . (int) $this->args['limit']['shift']
-            . ', ' . (int) $this->args['limit']['cnt'];
-    }
-
-    // }}}
-
     // {{{ Order
 
     /**
@@ -824,19 +792,19 @@ protected function _renderLimit(): ?string
      * $q->order(['name desc', 'id asc'])
      * $q->order('name', true);
      *
-     * @param string|Expressionable|array<int, string|Expressionable> $order order by
-     * @param string|bool                                             $desc  true to sort descending
+     * @param string|Expressionable|array<int, string|Expressionable> $order     order by
+     * @param ($order is array ? never : string|bool)                 $direction true to sort descending
      *
      * @return $this
      */
-    public function order($order, $desc = null)
+    public function order($order, $direction = null)
     {
         if (is_string($order) && str_contains($order, ',')) {
             throw new Exception('Comma-separated fields list is no longer accepted, use array instead');
         }
 
         if (is_array($order)) {
-            if ($desc !== null) {
+            if ($direction !== null) {
                 throw new Exception('If first argument is array, second argument must not be used');
             }
 
@@ -848,39 +816,84 @@ public function order($order, $desc = null)
         }
 
         // first argument may contain space, to divide field and ordering keyword
-        if ($desc === null && is_string($order) && str_contains($order, ' ')) {
+        if ($direction === null && is_string($order) && str_contains($order, ' ')) {
             $lastSpacePos = strrpos($order, ' ');
             if (in_array(strtolower(substr($order, $lastSpacePos + 1)), ['desc', 'asc'], true)) {
-                $desc = substr($order, $lastSpacePos + 1);
+                $direction = substr($order, $lastSpacePos + 1);
                 $order = substr($order, 0, $lastSpacePos);
             }
         }
 
-        if (is_bool($desc)) {
-            $desc = $desc ? 'desc' : '';
-        } elseif (strtolower($desc ?? '') === 'asc') {
-            $desc = '';
+        if (is_bool($direction)) {
+            $direction = $direction ? 'desc' : '';
+        } elseif (strtolower($direction ?? '') === 'asc') {
+            $direction = '';
         }
         // no else - allow custom order like "order by name desc nulls last" for Oracle
 
-        $this->args['order'][] = [$order, $desc];
+        $this->args['order'][] = [$order, $direction];
 
         return $this;
     }
 
+    /**
+     * @param list<string> $sqls
+     *
+     * @return list<string>
+     */
+    protected function deduplicateRenderOrder(array $sqls): array
+    {
+        return $sqls;
+    }
+
     protected function _renderOrder(): ?string
     {
         if (!isset($this->args['order'])) {
             return '';
         }
 
-        $x = [];
+        $sqls = [];
         foreach ($this->args['order'] as $tmp) {
             [$arg, $desc] = $tmp;
-            $x[] = $this->consume($arg, self::ESCAPE_IDENTIFIER_SOFT) . ($desc ? (' ' . $desc) : '');
+            $sqls[] = $this->consume($arg, self::ESCAPE_IDENTIFIER_SOFT) . ($desc ? (' ' . $desc) : '');
+        }
+
+        $sqls = array_reverse($sqls);
+        $sqlsDeduplicated = $this->deduplicateRenderOrder($sqls);
+
+        return ' order by ' . implode(', ', $sqlsDeduplicated);
+    }
+
+    // }}}
+
+    // {{{ Limit
+
+    /**
+     * Limit how many rows will be returned.
+     *
+     * @param int $cnt   Number of rows to return
+     * @param int $shift Offset, how many rows to skip
+     *
+     * @return $this
+     */
+    public function limit($cnt, $shift = null)
+    {
+        $this->args['limit'] = [
+            'cnt' => $cnt,
+            'shift' => $shift,
+        ];
+
+        return $this;
+    }
+
+    protected function _renderLimit(): ?string
+    {
+        if (!isset($this->args['limit'])) {
+            return null;
         }
 
-        return ' order by ' . implode(', ', array_unique(array_reverse($x)));
+        return ' limit ' . (int) $this->args['limit']['shift']
+            . ', ' . (int) $this->args['limit']['cnt'];
     }
 
     // }}}

diff --git a/tests/Model/Smbo/Transfer.php b/tests/Model/Smbo/Transfer.php
@@ -29,7 +29,8 @@ protected function init(): void
             if ($this->get('destination_account_id') && !$this->getId()) {
                 // in this section we test if "clone" works ok
 
-                $this->otherLegCreation = $m2 = clone $this;
+                $m2 = clone $this;
+                $this->otherLegCreation = $m2;
                 $m2->set('account_id', $m2->get('destination_account_id'));
                 $m2->set('amount', -$m2->get('amount'));
 

diff --git a/tests/ModelIteratorTest.php b/tests/ModelIteratorTest.php
@@ -19,7 +19,7 @@ public function testSetOrderArrayWithAnotherArgumentException(): void
 
         $this->expectException(Exception::class);
         $this->expectExceptionMessage('If first argument is array, second argument must not be used');
-        $m->setOrder(['name', 'salary'], 'desc');
+        $m->setOrder(['name', 'salary'], 'desc'); // @phpstan-ignore-line
     }
 
     public function testNoPersistenceTryLoadException(): void

diff --git a/tests/Persistence/Sql/QueryTest.php b/tests/Persistence/Sql/QueryTest.php
@@ -862,11 +862,11 @@ public function testOrder(): void
         );
     }
 
-    public function testOrderException1(): void
+    public function testOrderException(): void
     {
-        // if first argument is array, second argument must not be used
         $this->expectException(Exception::class);
-        $this->q('[order]')->order(['name', 'surname'], 'desc');
+        $this->expectExceptionMessage('If first argument is array, second argument must not be used');
+        $this->q('[order]')->order(['name', 'surname'], 'desc'); // @phpstan-ignore-line
     }
 
     public function testGroup(): void

diff --git a/tests/Persistence/Sql/WithDb/SelectTest.php b/tests/Persistence/Sql/WithDb/SelectTest.php
@@ -455,6 +455,8 @@ public function testOrderDuplicate(): void
     {
         $query = $this->q('employee')->field('name')
             ->order('id')
+            ->order('name', 'desc')
+            ->order('name', 'ASC')
             ->order('name')
             ->order('surname')
             ->order('name');