Refactor DMLQueryBuilder (#275)

* Refactor DMLQueryBuilder * Improve test * Add line to CHANGELOG.md
yiisoft · Nov 1, 2023 · 997ae8b · 997ae8b
1 parent 1be5ddb
commit 997ae8b
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 61 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## 1.0.2 under development
 
+- Bug #275: Refactor `DMLQueryBuilder`, related with yiisoft/db#746 (@Tigrov)
 - Bug #278: Remove `RECURSIVE` expression from CTE queries (@Tigrov)
 - Bug #280: Fix type boolean (@terabytesoftw)
 

diff --git a/src/DMLQueryBuilder.php b/src/DMLQueryBuilder.php
@@ -11,16 +11,11 @@
 use Yiisoft\Db\Exception\InvalidConfigException;
 use Yiisoft\Db\Exception\NotSupportedException;
 use Yiisoft\Db\Expression\Expression;
-use Yiisoft\Db\Expression\ExpressionInterface;
 use Yiisoft\Db\Query\QueryInterface;
 use Yiisoft\Db\QueryBuilder\AbstractDMLQueryBuilder;
-use Yiisoft\Db\Schema\SchemaInterface;
 
 use function implode;
 use function in_array;
-use function ltrim;
-use function strrpos;
-use function is_array;
 
 /**
  * Implements a DML (Data Manipulation Language) SQL statements for MSSQL Server.
@@ -35,16 +30,12 @@ final class DMLQueryBuilder extends AbstractDMLQueryBuilder
      */
     public function insertWithReturningPks(string $table, QueryInterface|array $columns, array &$params = []): string
     {
-        /**
-         * @psalm-var string[] $names
-         * @psalm-var string[] $placeholders
-         */
         [$names, $placeholders, $values, $params] = $this->prepareInsertValues($table, $columns, $params);
 
+        $createdCols = [];
+        $insertedCols = [];
+        $returnColumns = $this->schema->getTableSchema($table)?->getColumns() ?? [];
 
-        $createdCols = $insertedCols = [];
-        $tableSchema = $this->schema->getTableSchema($table);
-        $returnColumns = $tableSchema?->getColumns() ?? [];
         foreach ($returnColumns as $returnColumn) {
             if ($returnColumn->isComputed()) {
                 continue;
@@ -54,9 +45,7 @@ public function insertWithReturningPks(string $table, QueryInterface|array $colu
 
             if (in_array($dbType, ['char', 'varchar', 'nchar', 'nvarchar', 'binary', 'varbinary'], true)) {
                 $dbType .= '(MAX)';
-            }
-
-            if ($returnColumn->getDbType() === SchemaInterface::TYPE_TIMESTAMP) {
+            } elseif ($dbType === 'timestamp') {
                 $dbType = $returnColumn->isAllowNull() ? 'varbinary(8)' : 'binary(8)';
             }
 
@@ -65,12 +54,10 @@ public function insertWithReturningPks(string $table, QueryInterface|array $colu
             $insertedCols[] = 'INSERTED.' . $quotedName;
         }
 
-        $sql = 'INSERT INTO '
-            . $this->quoter->quoteTableName($table)
+        $sql = 'INSERT INTO ' . $this->quoter->quoteTableName($table)
             . (!empty($names) ? ' (' . implode(', ', $names) . ')' : '')
             . ' OUTPUT ' . implode(',', $insertedCols) . ' INTO @temporary_inserted'
-            . (!empty($placeholders) ? ' VALUES (' . implode(', ', $placeholders) . ')' : (string) $values);
-
+            . (!empty($placeholders) ? ' VALUES (' . implode(', ', $placeholders) . ')' : ' ' . $values);
 
         return 'SET NOCOUNT ON;DECLARE @temporary_inserted TABLE (' . implode(', ', $createdCols) . ');'
             . $sql . ';SELECT * FROM @temporary_inserted;';
@@ -118,7 +105,6 @@ public function upsert(
         /** @psalm-var Constraint[] $constraints */
         $constraints = [];
 
-        /** @psalm-var string[] $insertNames */
         [$uniqueNames, $insertNames, $updateNames] = $this->prepareUpsertColumns(
             $table,
             $insertColumns,
@@ -135,74 +121,50 @@ public function upsert(
 
         foreach ($constraints as $constraint) {
             $constraintCondition = ['and'];
+            $columnNames = (array) $constraint->getColumnNames();
 
-            $columnNames = $constraint->getColumnNames() ?? [];
-
-            if (is_array($columnNames)) {
-                /** @psalm-var string[] $columnNames */
-                foreach ($columnNames as $name) {
-                    $quotedName = $this->quoter->quoteColumnName($name);
-                    $constraintCondition[] = "$quotedTableName.$quotedName=[EXCLUDED].$quotedName";
-                }
+            /** @psalm-var string[] $columnNames */
+            foreach ($columnNames as $name) {
+                $quotedName = $this->quoter->quoteColumnName($name);
+                $constraintCondition[] = "$quotedTableName.$quotedName=[EXCLUDED].$quotedName";
             }
 
             $onCondition[] = $constraintCondition;
         }
 
         $on = $this->queryBuilder->buildCondition($onCondition, $params);
 
-        /** @psalm-var string[] $placeholders */
         [, $placeholders, $values, $params] = $this->prepareInsertValues($table, $insertColumns, $params);
-        $mergeSql = 'MERGE ' . $this->quoter->quoteTableName($table) . ' WITH (HOLDLOCK) '
-            . 'USING (' . (!empty($placeholders)
-            ? 'VALUES (' . implode(', ', $placeholders) . ')'
-            : ltrim((string) $values, ' ')) . ') AS [EXCLUDED] (' . implode(', ', $insertNames) . ') ' . "ON ($on)";
-        $insertValues = [];
 
-        foreach ($insertNames as $name) {
-            $quotedName = $this->quoter->quoteColumnName($name);
+        $mergeSql = 'MERGE ' . $quotedTableName . ' WITH (HOLDLOCK) USING ('
+            . (!empty($placeholders) ? 'VALUES (' . implode(', ', $placeholders) . ')' : $values)
+            . ') AS [EXCLUDED] (' . implode(', ', $insertNames) . ') ' . "ON ($on)";
 
-            if (strrpos($quotedName, '.') === false) {
-                $quotedName = '[EXCLUDED].' . $quotedName;
-            }
+        $insertValues = [];
 
-            $insertValues[] = $quotedName;
+        foreach ($insertNames as $quotedName) {
+            $insertValues[] = '[EXCLUDED].' . $quotedName;
         }
 
-        $insertSql = 'INSERT (' . implode(', ', $insertNames) . ')' . ' VALUES (' . implode(', ', $insertValues) . ')';
+        $insertSql = 'INSERT (' . implode(', ', $insertNames) . ') VALUES (' . implode(', ', $insertValues) . ')';
 
-        if ($updateNames === []) {
+        if ($updateColumns === false || $updateNames === []) {
             /** there are no columns to update */
-            $updateColumns = false;
-        }
-
-        if ($updateColumns === false) {
             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);
             }
         }
 
-        /**
-         * @var array $params
-         *
-         * @psalm-var string[] $updates
-         * @psalm-var array<string, ExpressionInterface|string> $updateColumns
-         */
         [$updates, $params] = $this->prepareUpdateSets($table, $updateColumns, $params);
-        $updateSql = 'UPDATE SET ' . implode(', ', $updates);
 
-        return "$mergeSql WHEN MATCHED THEN $updateSql WHEN NOT MATCHED THEN $insertSql;";
+        return "$mergeSql WHEN MATCHED THEN UPDATE SET " . implode(', ', $updates)
+            . " WHEN NOT MATCHED THEN $insertSql;";
     }
 }
diff --git a/tests/Provider/CommandProvider.php b/tests/Provider/CommandProvider.php
@@ -18,6 +18,23 @@ final class CommandProvider extends \Yiisoft\Db\Tests\Provider\CommandProvider
 
     protected static string $driverName = 'sqlsrv';
 
+    public static function batchInsert(): array
+    {
+        $batchInsert = parent::batchInsert();
+
+        $batchInsert['multirow']['expectedParams'][':qp3'] = 1;
+        $batchInsert['multirow']['expectedParams'][':qp7'] = 0;
+
+        $batchInsert['issue11242']['expectedParams'][':qp3'] = 1;
+
+        $batchInsert['table name with column name with brackets']['expectedParams'][':qp3'] = 0;
+
+        $batchInsert['batchInsert binds params from expression']['expectedParams'][':qp3'] = 0;
+        $batchInsert['with associative values']['expectedParams'][':qp3'] = 1;
+
+        return $batchInsert;
+    }
+
     /**
      * @throws JsonException
      */

diff --git a/tests/Provider/QueryBuilderProvider.php b/tests/Provider/QueryBuilderProvider.php
@@ -218,6 +218,14 @@ public static function upsert(): array
                     '[EXCLUDED].[address], [EXCLUDED].[status], [EXCLUDED].[profile_id]);',
             ],
 
+            'regular values with unique at not the first position' => [
+                3 => 'MERGE [T_upsert] WITH (HOLDLOCK) USING (VALUES (:qp0, :qp1, :qp2, :qp3)) AS [EXCLUDED] ' .
+                    '([address], [email], [status], [profile_id]) 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]);',
+            ],
+
             'regular values with update part' => [
                 3 => 'MERGE [T_upsert] WITH (HOLDLOCK) USING (VALUES (:qp0, :qp1, :qp2, :qp3)) AS [EXCLUDED] ' .
                     '([email], [address], [status], [profile_id]) ON ([T_upsert].[email]=[EXCLUDED].[email]) ' .