Refactor DMLQueryBuilder (#233)

* Refactor DMLQueryBuilder * Fix @psalm-var * Add test for a non-primary key table * Fix yiisoft/db#61 (point 2) * Fix yiisoft/db#61 (point 2) add test * improve test * Add line to CHANGELOG.md * Improve performance of quoting column names up to 10% using `array_map()` * Remove `Generator` * Update psalm
yiisoft · Nov 1, 2023 · 5ecee24 · 5ecee24
1 parent 245d3ee
commit 5ecee24
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 74 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## 1.1.1 under development
 
+- Bug #233: Refactor `DMLQueryBuilder`, related with yiisoft/db#746 (@Tigrov)
 - Enh #230: Improve column type #230 (@Tigrov)
 - Bug #240: Remove `RECURSIVE` expression from CTE queries (@Tigrov)
 

diff --git a/psalm.xml b/psalm.xml
@@ -14,4 +14,7 @@
         <directory name="vendor" />
     </ignoreFiles>
   </projectFiles>
+  <issueHandlers>
+    <MixedAssignment errorLevel="suppress" />
+  </issueHandlers>
 </psalm>
diff --git a/src/DMLQueryBuilder.php b/src/DMLQueryBuilder.php
@@ -4,7 +4,6 @@
 
 namespace Yiisoft\Db\Oracle;
 
-use Generator;
 use JsonException;
 use Yiisoft\Db\Exception\Exception;
 use Yiisoft\Db\Exception\InvalidArgumentException;
@@ -15,54 +14,47 @@
 use Yiisoft\Db\Query\QueryInterface;
 use Yiisoft\Db\QueryBuilder\AbstractDMLQueryBuilder;
 
+use function array_map;
 use function implode;
-use function ltrim;
-use function strrpos;
 use function count;
-use function reset;
 
 /**
  * Implements a DML (Data Manipulation Language) SQL statements for Oracle Server.
  */
 final class DMLQueryBuilder extends AbstractDMLQueryBuilder
 {
     /**
-     * @psalm-suppress MixedArrayOffset
-     *
      * @throws Exception
      * @throws InvalidArgumentException
      * @throws InvalidConfigException
      * @throws NotSupportedException
      */
-    public function batchInsert(string $table, array $columns, iterable|Generator $rows, array &$params = []): string
+    public function batchInsert(string $table, array $columns, iterable $rows, array &$params = []): string
     {
         if (empty($rows)) {
             return '';
         }
 
-        if (($tableSchema = $this->schema->getTableSchema($table)) !== null) {
-            $columnSchemas = $tableSchema->getColumns();
-        } else {
-            $columnSchemas = [];
-        }
-
-        $mappedNames = $this->getNormalizeColumnNames($table, $columns);
         $values = [];
+        $columns = $this->getNormalizeColumnNames('', $columns);
+        $columnSchemas = $this->schema->getTableSchema($table)?->getColumns() ?? [];
 
-        /** @psalm-var array<array-key, array<array-key, string>> $rows */
         foreach ($rows as $row) {
+            $i = 0;
             $placeholders = [];
-            foreach ($row as $index => $value) {
-                if (isset($columns[$index], $mappedNames[$columns[$index]], $columnSchemas[$mappedNames[$columns[$index]]])) {
-                    /** @var mixed $value */
-                    $value = $this->getTypecastValue($value, $columnSchemas[$mappedNames[$columns[$index]]]);
+
+            foreach ($row as $value) {
+                if (isset($columns[$i], $columnSchemas[$columns[$i]])) {
+                    $value = $columnSchemas[$columns[$i]]->dbTypecast($value);
                 }
 
                 if ($value instanceof ExpressionInterface) {
                     $placeholders[] = $this->queryBuilder->buildExpression($value, $params);
                 } else {
                     $placeholders[] = $this->queryBuilder->bindParam($value, $params);
                 }
+
+                ++$i;
             }
             $values[] = '(' . implode(', ', $placeholders) . ')';
         }
@@ -71,9 +63,10 @@ public function batchInsert(string $table, array $columns, iterable|Generator $r
             return '';
         }
 
-        foreach ($columns as $i => $name) {
-            $columns[$i] = $this->quoter->quoteColumnName($mappedNames[$name]);
-        }
+        $columns = array_map(
+            [$this->quoter, 'quoteColumnName'],
+            $columns,
+        );
 
         $tableAndColumns = ' INTO ' . $this->quoter->quoteTableName($table)
             . ' (' . implode(', ', $columns) . ') VALUES ';
@@ -105,7 +98,6 @@ public function upsert(
         array|bool $updateColumns,
         array &$params = []
     ): string {
-        $usingValues = null;
         $constraints = [];
 
         [$uniqueNames, $insertNames, $updateNames] = $this->prepareUpsertColumns(
@@ -119,16 +111,11 @@ public function upsert(
             return $this->insert($table, $insertColumns, $params);
         }
 
-        if ($updateNames === []) {
-            /** there are no columns to update */
-            $updateColumns = false;
-        }
-
         $onCondition = ['or'];
         $quotedTableName = $this->quoter->quoteTableName($table);
 
         foreach ($constraints as $constraint) {
-            $columnNames = $constraint->getColumnNames() ?? [];
+            $columnNames = (array) $constraint->getColumnNames();
             $constraintCondition = ['and'];
             /** @psalm-var string[] $columnNames */
             foreach ($columnNames as $name) {
@@ -140,89 +127,72 @@ public function upsert(
         }
 
         $on = $this->queryBuilder->buildCondition($onCondition, $params);
-        /** @psalm-var string[] $placeholders */
+
         [, $placeholders, $values, $params] = $this->prepareInsertValues($table, $insertColumns, $params);
 
         if (!empty($placeholders)) {
             $usingSelectValues = [];
-            /** @psalm-var string[] $insertNames */
+
             foreach ($insertNames as $index => $name) {
                 $usingSelectValues[$name] = new Expression($placeholders[$index]);
             }
 
-            /** @psalm-var array $params */
-            $usingValues = $this->queryBuilder->buildSelect($usingSelectValues, $params) . ' ' . $this->queryBuilder->buildFrom(['DUAL'], $params);
+            $values = $this->queryBuilder->buildSelect($usingSelectValues, $params)
+                . ' ' . $this->queryBuilder->buildFrom(['DUAL'], $params);
         }
 
         $insertValues = [];
-        $mergeSql = 'MERGE INTO '
-            . $this->quoter->quoteTableName($table)
-            . ' '
-            . 'USING (' . ($usingValues ?? ltrim((string) $values, ' '))
-            . ') "EXCLUDED" '
-            . "ON ($on)";
-
-        /** @psalm-var string[] $insertNames */
-        foreach ($insertNames as $name) {
-            $quotedName = $this->quoter->quoteColumnName($name);
-
-            if (strrpos($quotedName, '.') === false) {
-                $quotedName = '"EXCLUDED".' . $quotedName;
-            }
+        $mergeSql = 'MERGE INTO ' . $quotedTableName . ' USING (' . $values . ') "EXCLUDED" ON (' . $on . ')';
 
-            $insertValues[] = $quotedName;
+        foreach ($insertNames as $quotedName) {
+            $insertValues[] = '"EXCLUDED".' . $quotedName;
         }
 
         $insertSql = 'INSERT (' . implode(', ', $insertNames) . ')' . ' VALUES (' . implode(', ', $insertValues) . ')';
 
-        if ($updateColumns === false) {
+        if ($updateColumns === false || $updateNames === []) {
+            /** there are no columns to update */
             return "$mergeSql WHEN NOT MATCHED THEN $insertSql";
         }
 
         if ($updateColumns === true) {
             $updateColumns = [];
             /** @psalm-var string[] $updateNames */
-            foreach ($updateNames as $name) {
-                $quotedName = $this->quoter->quoteColumnName($name);
-
-                if (strrpos($quotedName, '.') === false) {
-                    $quotedName = '"EXCLUDED".' . $quotedName;
-                }
-                $updateColumns[$name] = new Expression($quotedName);
+            foreach ($updateNames as $quotedName) {
+                $updateColumns[$quotedName] = new Expression('"EXCLUDED".' . $quotedName);
             }
         }
 
-        /** @psalm-var string[] $updates */
-        [$updates, $params] = $this->prepareUpdateSets($table, $updateColumns, (array) $params);
+        [$updates, $params] = $this->prepareUpdateSets($table, $updateColumns, $params);
         $updateSql = 'UPDATE SET ' . implode(', ', $updates);
 
         return "$mergeSql WHEN MATCHED THEN $updateSql WHEN NOT MATCHED THEN $insertSql";
     }
 
     protected function prepareInsertValues(string $table, array|QueryInterface $columns, array $params = []): array
     {
-        /**
-         * @var array $names
-         * @var array $placeholders
-         */
-        [$names, $placeholders, $values, $params] = parent::prepareInsertValues($table, $columns, $params);
-
-        if (!$columns instanceof QueryInterface && empty($names)) {
+        if (empty($columns)) {
+            $names = [];
+            $placeholders = [];
             $tableSchema = $this->schema->getTableSchema($table);
 
             if ($tableSchema !== null) {
-                $tableColumns = $tableSchema->getColumns();
-                $columns = !empty($tableSchema->getPrimaryKey())
-                    ? $tableSchema->getPrimaryKey() : [reset($tableColumns)->getName()];
+                if (!empty($tableSchema->getPrimaryKey())) {
+                    $columns = $tableSchema->getPrimaryKey();
+                } else {
+                    $columns = [current($tableSchema->getColumns())->getName()];
+                }
+
                 foreach ($columns as $name) {
-                    /** @psalm-var mixed */
                     $names[] = $this->quoter->quoteColumnName($name);
                     $placeholders[] = 'DEFAULT';
                 }
             }
+
+            return [$names, $placeholders, '', $params];
         }
 
-        return [$names, $placeholders, $values, $params];
+        return parent::prepareInsertValues($table, $columns, $params);
     }
 
     public function resetSequence(string $table, int|string $value = null): string

diff --git a/tests/Provider/CommandProvider.php b/tests/Provider/CommandProvider.php
@@ -32,12 +32,15 @@ public static function batchInsert(): array
         DbHelper::changeSqlForOracleBatchInsert($batchInsert['issue11242']['expected']);
         $batchInsert['issue11242']['expectedParams'][':qp3'] = '1';
 
-        DbHelper::changeSqlForOracleBatchInsert($batchInsert['wrongBehavior']['expected']);
-        $batchInsert['wrongBehavior']['expectedParams'][':qp3'] = '0';
+        DbHelper::changeSqlForOracleBatchInsert($batchInsert['table name with column name with brackets']['expected']);
+        $batchInsert['table name with column name with brackets']['expectedParams'][':qp3'] = '0';
 
         DbHelper::changeSqlForOracleBatchInsert($batchInsert['batchInsert binds params from expression']['expected']);
         $batchInsert['batchInsert binds params from expression']['expectedParams'][':qp3'] = '0';
 
+        DbHelper::changeSqlForOracleBatchInsert($batchInsert['with associative values']['expected']);
+        $batchInsert['with associative values']['expectedParams'][':qp3'] = '1';
+
         return $batchInsert;
     }
 

diff --git a/tests/Provider/QueryBuilderProvider.php b/tests/Provider/QueryBuilderProvider.php
@@ -55,6 +55,7 @@ public static function batchInsert(): array
         SQL;
 
         DbHelper::changeSqlForOracleBatchInsert($batchInsert['bool-false, time-now()']['expected']);
+        DbHelper::changeSqlForOracleBatchInsert($batchInsert['column table names are not checked']['expected']);
 
         return $batchInsert;
     }
@@ -124,6 +125,11 @@ public static function upsert(): array
                 MERGE INTO "T_upsert" USING (SELECT :qp0 AS "email", :qp1 AS "address", :qp2 AS "status", :qp3 AS "profile_id" FROM "DUAL") "EXCLUDED" ON ("T_upsert"."email"="EXCLUDED"."email") WHEN MATCHED THEN UPDATE SET "address"="EXCLUDED"."address", "status"="EXCLUDED"."status", "profile_id"="EXCLUDED"."profile_id" WHEN NOT MATCHED THEN INSERT ("email", "address", "status", "profile_id") VALUES ("EXCLUDED"."email", "EXCLUDED"."address", "EXCLUDED"."status", "EXCLUDED"."profile_id")
                 SQL,
             ],
+            'regular values with unique at not the first position' => [
+                3 => <<<SQL
+                MERGE INTO "T_upsert" USING (SELECT :qp0 AS "address", :qp1 AS "email", :qp2 AS "status", :qp3 AS "profile_id" FROM "DUAL") "EXCLUDED" ON ("T_upsert"."email"="EXCLUDED"."email") WHEN MATCHED THEN UPDATE SET "address"="EXCLUDED"."address", "status"="EXCLUDED"."status", "profile_id"="EXCLUDED"."profile_id" WHEN NOT MATCHED THEN INSERT ("address", "email", "status", "profile_id") VALUES ("EXCLUDED"."address", "EXCLUDED"."email", "EXCLUDED"."status", "EXCLUDED"."profile_id")
+                SQL,
+            ],
             'regular values with update part' => [
                 2 => ['address' => 'foo {{city}}', 'status' => 2, 'orders' => new Expression('"T_upsert"."orders" + 1')],
                 3 => <<<SQL

diff --git a/tests/QueryBuilderTest.php b/tests/QueryBuilderTest.php
@@ -4,7 +4,6 @@
 
 namespace Yiisoft\Db\Oracle\Tests;
 
-use Generator;
 use Throwable;
 use Yiisoft\Db\Exception\Exception;
 use Yiisoft\Db\Exception\InvalidArgumentException;
@@ -125,7 +124,7 @@ public function testAlterColumn(): void
      * @throws InvalidArgumentException
      * @throws NotSupportedException
      */
-    public function testBatchInsert(string $table, array $columns, iterable|Generator $rows, string $expected): void
+    public function testBatchInsert(string $table, array $columns, iterable $rows, string $expected): void
     {
         parent::testBatchInsert($table, $columns, $rows, $expected);
     }
@@ -633,4 +632,16 @@ public function testUpsertExecute(
     ): void {
         parent::testUpsertExecute($table, $insertColumns, $updateColumns);
     }
+
+    public function testDefaultValues(): void
+    {
+        $db = $this->getConnection();
+        $queryBuilder = $db->getQueryBuilder();
+
+        // Non-primary key columns should have DEFAULT as value
+        $this->assertSame(
+            'INSERT INTO "negative_default_values" ("tinyint_col") VALUES (DEFAULT)',
+            $queryBuilder->insert('negative_default_values', []),
+        );
+    }
 }