Fix DMLQueryBuilder::insertWithReturningPks() (#287)

yiisoft · Dec 21, 2023 · 666a8e8 · 666a8e8
1 parent 5276702
commit 666a8e8
Show file tree

Hide file tree

Showing 8 changed files with 56 additions and 52 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,7 @@
 ## 1.1.1 under development
 
 - Enh #286: Change property `Schema::$typeMap` to constant `Schema::TYPE_MAP` (@Tigrov)
+- Bug #287: Fix `DMLQueryBuilder::insertWithReturningPks()` and `Command::insertWithReturningPks()` methods (@Tigrov)
 
 ## 1.1.0 November 12, 2023
 

diff --git a/src/Command.php b/src/Command.php
@@ -12,6 +12,19 @@
  */
 final class Command extends AbstractPdoCommand
 {
+    public function insertWithReturningPks(string $table, array $columns): bool|array
+    {
+        if (empty($this->db->getSchema()->getTableSchema($table)?->getPrimaryKey())) {
+            if (parent::insert($table, $columns)->execute() === 0) {
+                return false;
+            }
+
+            return [];
+        }
+
+        return parent::insertWithReturningPks($table, $columns);
+    }
+
     public function showDatabases(): array
     {
         $sql = <<<SQL

diff --git a/src/DMLQueryBuilder.php b/src/DMLQueryBuilder.php
@@ -14,6 +14,8 @@
 use Yiisoft\Db\Query\QueryInterface;
 use Yiisoft\Db\QueryBuilder\AbstractDMLQueryBuilder;
 
+use function array_flip;
+use function array_intersect_key;
 use function implode;
 use function in_array;
 
@@ -30,17 +32,18 @@ final class DMLQueryBuilder extends AbstractDMLQueryBuilder
      */
     public function insertWithReturningPks(string $table, QueryInterface|array $columns, array &$params = []): string
     {
-        [$names, $placeholders, $values, $params] = $this->prepareInsertValues($table, $columns, $params);
+        $tableSchema = $this->schema->getTableSchema($table);
+        $primaryKeys = $tableSchema?->getPrimaryKey();
+
+        if (empty($primaryKeys)) {
+            return $this->insert($table, $columns, $params);
+        }
 
         $createdCols = [];
         $insertedCols = [];
-        $returnColumns = $this->schema->getTableSchema($table)?->getColumns() ?? [];
+        $returnColumns = array_intersect_key($tableSchema?->getColumns() ?? [], array_flip($primaryKeys));
 
         foreach ($returnColumns as $returnColumn) {
-            if ($returnColumn->isComputed()) {
-                continue;
-            }
-
             $dbType = $returnColumn->getDbType();
 
             if (in_array($dbType, ['char', 'varchar', 'nchar', 'nvarchar', 'binary', 'varbinary'], true)) {
@@ -54,6 +57,8 @@ public function insertWithReturningPks(string $table, QueryInterface|array $colu
             $insertedCols[] = 'INSERTED.' . $quotedName;
         }
 
+        [$names, $placeholders, $values, $params] = $this->prepareInsertValues($table, $columns, $params);
+
         $sql = 'INSERT INTO ' . $this->quoter->quoteTableName($table)
             . (!empty($names) ? ' (' . implode(', ', $names) . ')' : '')
             . ' OUTPUT ' . implode(',', $insertedCols) . ' INTO @temporary_inserted'

diff --git a/src/Schema.php b/src/Schema.php
@@ -305,7 +305,7 @@ protected function loadTableForeignKeys(string $tableName): array
      * @throws InvalidConfigException
      * @throws Throwable
      *
-     * @return array Indexes for the given table.
+     * @return IndexConstraint[] Indexes for the given table.
      */
     protected function loadTableIndexes(string $tableName): array
     {

diff --git a/tests/CommandTest.php b/tests/CommandTest.php
@@ -127,31 +127,6 @@ public function testInsertVarbinary(mixed $expectedData, mixed $testData): void
         $this->assertSame($expectedData, $resultData['blob_col']);
     }
 
-    /**
-     * @throws Exception
-     * @throws InvalidCallException
-     * @throws InvalidConfigException
-     * @throws Throwable
-     */
-    public function testInsertWithReturningPks(): void
-    {
-        $db = $this->getConnection(true);
-
-        $command = $db->createCommand();
-
-        $this->assertSame(
-            [
-                'id' => '4',
-                'email' => 'test_1@example.com',
-                'name' => 'test_1',
-                'address' => null,
-                'status' => '0',
-                'profile_id' => null,
-            ],
-            $command->insertWithReturningPks('{{customer}}', ['name' => 'test_1', 'email' => 'test_1@example.com']),
-        );
-    }
-
     /**
      * @throws Exception
      * @throws InvalidCallException
@@ -189,9 +164,7 @@ public function testInsertWithReturningPksWithComputedColumn(): void
         $result = $command->insertWithReturningPks('{{test_trigger}}', ['stringcol' => $insertedString]);
         $transaction->commit();
 
-        $this->assertIsArray($result);
-        $this->assertSame($insertedString, $result['stringcol']);
-        $this->assertSame('1', $result['id']);
+        $this->assertSame(['id' => '1'], $result);
     }
 
     /**
@@ -213,9 +186,7 @@ public function testInsertWithReturningPksWithRowVersionColumn(): void
         $insertedString = 'test';
         $result = $command->insertWithReturningPks('{{test_trigger}}', ['stringcol' => $insertedString]);
 
-        $this->assertIsArray($result);
-        $this->assertSame($insertedString, $result['stringcol']);
-        $this->assertSame('1', $result['id']);
+        $this->assertSame(['id' => '1'], $result);
     }
 
     /**
@@ -240,9 +211,7 @@ public function testInsertWithReturningPksWithRowVersionNullColumn(): void
             ['stringcol' => $insertedString, 'RV' => new Expression('DEFAULT')],
         );
 
-        $this->assertIsArray($result);
-        $this->assertSame($insertedString, $result['stringcol']);
-        $this->assertSame('1', $result['id']);
+        $this->assertSame(['id' => '1'], $result);
     }
 
     /**

diff --git a/tests/Provider/QueryBuilderProvider.php b/tests/Provider/QueryBuilderProvider.php
@@ -113,7 +113,7 @@ public static function insertWithReturningPks(): array
                 ],
                 [],
                 <<<SQL
-                SET NOCOUNT ON;DECLARE @temporary_inserted TABLE ([id] int , [email] varchar(128) , [name] varchar(128) NULL, [address] text NULL, [status] int NULL, [profile_id] int NULL);INSERT INTO [customer] ([email], [name], [address], [is_active], [related_id]) OUTPUT INSERTED.[id],INSERTED.[email],INSERTED.[name],INSERTED.[address],INSERTED.[status],INSERTED.[profile_id] INTO @temporary_inserted VALUES (:qp0, :qp1, :qp2, :qp3, :qp4);SELECT * FROM @temporary_inserted;
+                SET NOCOUNT ON;DECLARE @temporary_inserted TABLE ([id] int );INSERT INTO [customer] ([email], [name], [address], [is_active], [related_id]) OUTPUT INSERTED.[id] INTO @temporary_inserted VALUES (:qp0, :qp1, :qp2, :qp3, :qp4);SELECT * FROM @temporary_inserted;
                 SQL,
                 [
                     ':qp0' => 'test@example.com',
@@ -128,7 +128,7 @@ public static function insertWithReturningPks(): array
                 ['{{%type}}.[[related_id]]' => null, '[[time]]' => new Expression('now()')],
                 [],
                 <<<SQL
-                SET NOCOUNT ON;DECLARE @temporary_inserted TABLE ([int_col] int , [int_col2] int NULL, [tinyint_col] tinyint NULL, [smallint_col] smallint NULL, [char_col] char(100) , [char_col2] varchar(100) NULL, [char_col3] text NULL, [float_col] decimal(4,3) , [float_col2] float NULL, [blob_col] varbinary(MAX) NULL, [numeric_col] decimal(5,2) NULL, [datetime_col] datetime , [bool_col] bit , [bool_col2] bit NULL);INSERT INTO {{%type}} ([related_id], [time]) OUTPUT INSERTED.[int_col],INSERTED.[int_col2],INSERTED.[tinyint_col],INSERTED.[smallint_col],INSERTED.[char_col],INSERTED.[char_col2],INSERTED.[char_col3],INSERTED.[float_col],INSERTED.[float_col2],INSERTED.[blob_col],INSERTED.[numeric_col],INSERTED.[datetime_col],INSERTED.[bool_col],INSERTED.[bool_col2] INTO @temporary_inserted VALUES (:qp0, now());SELECT * FROM @temporary_inserted;
+                INSERT INTO {{%type}} ([related_id], [time]) VALUES (:qp0, now())
                 SQL,
                 [':qp0' => null],
             ],
@@ -144,7 +144,7 @@ public static function insertWithReturningPks(): array
                 ],
                 [':phBar' => 'bar'],
                 <<<SQL
-                SET NOCOUNT ON;DECLARE @temporary_inserted TABLE ([id] int , [email] varchar(128) , [name] varchar(128) NULL, [address] text NULL, [status] int NULL, [profile_id] int NULL);INSERT INTO [customer] ([email], [name], [address], [is_active], [related_id], [col]) OUTPUT INSERTED.[id],INSERTED.[email],INSERTED.[name],INSERTED.[address],INSERTED.[status],INSERTED.[profile_id] INTO @temporary_inserted VALUES (:qp1, :qp2, :qp3, :qp4, :qp5, CONCAT(:phFoo, :phBar));SELECT * FROM @temporary_inserted;
+                SET NOCOUNT ON;DECLARE @temporary_inserted TABLE ([id] int );INSERT INTO [customer] ([email], [name], [address], [is_active], [related_id], [col]) OUTPUT INSERTED.[id] INTO @temporary_inserted VALUES (:qp1, :qp2, :qp3, :qp4, :qp5, CONCAT(:phFoo, :phBar));SELECT * FROM @temporary_inserted;
                 SQL,
                 [
                     ':phBar' => 'bar',
@@ -173,7 +173,7 @@ public static function insertWithReturningPks(): array
                     ),
                 [':phBar' => 'bar'],
                 <<<SQL
-                SET NOCOUNT ON;DECLARE @temporary_inserted TABLE ([id] int , [email] varchar(128) , [name] varchar(128) NULL, [address] text NULL, [status] int NULL, [profile_id] int NULL);INSERT INTO [customer] ([email], [name], [address], [is_active], [related_id]) OUTPUT INSERTED.[id],INSERTED.[email],INSERTED.[name],INSERTED.[address],INSERTED.[status],INSERTED.[profile_id] INTO @temporary_inserted SELECT [email], [name], [address], [is_active], [related_id] FROM [customer] WHERE ([email]=:qp1) AND ([name]=:qp2) AND ([address]=:qp3) AND ([is_active]=:qp4) AND ([related_id] IS NULL) AND ([col]=CONCAT(:phFoo, :phBar));SELECT * FROM @temporary_inserted;
+                SET NOCOUNT ON;DECLARE @temporary_inserted TABLE ([id] int );INSERT INTO [customer] ([email], [name], [address], [is_active], [related_id]) OUTPUT INSERTED.[id] INTO @temporary_inserted SELECT [email], [name], [address], [is_active], [related_id] FROM [customer] WHERE ([email]=:qp1) AND ([name]=:qp2) AND ([address]=:qp3) AND ([is_active]=:qp4) AND ([related_id] IS NULL) AND ([col]=CONCAT(:phFoo, :phBar));SELECT * FROM @temporary_inserted;
                 SQL,
                 [
                     ':phBar' => 'bar',
@@ -189,7 +189,7 @@ public static function insertWithReturningPks(): array
                 ['order_id' => 1, 'item_id' => 1, 'quantity' => 1, 'subtotal' => 1.0],
                 [],
                 <<<SQL
-                SET NOCOUNT ON;DECLARE @temporary_inserted TABLE ([order_id] int , [item_id] int , [quantity] int , [subtotal] decimal(10,0) );INSERT INTO {{%order_item}} ([order_id], [item_id], [quantity], [subtotal]) OUTPUT INSERTED.[order_id],INSERTED.[item_id],INSERTED.[quantity],INSERTED.[subtotal] INTO @temporary_inserted VALUES (:qp0, :qp1, :qp2, :qp3);SELECT * FROM @temporary_inserted;
+                SET NOCOUNT ON;DECLARE @temporary_inserted TABLE ([order_id] int , [item_id] int );INSERT INTO {{%order_item}} ([order_id], [item_id], [quantity], [subtotal]) OUTPUT INSERTED.[order_id],INSERTED.[item_id] INTO @temporary_inserted VALUES (:qp0, :qp1, :qp2, :qp3);SELECT * FROM @temporary_inserted;
                 SQL,
                 [':qp0' => 1, ':qp1' => 1, ':qp2' => 1, ':qp3' => 1.0,],
             ],

diff --git a/tests/SchemaTest.php b/tests/SchemaTest.php
@@ -179,4 +179,21 @@ public function testNotConnectionPDO(): void
 
         $schema->refreshTableSchema('customer');
     }
+
+    public function testNegativeDefaultValues(): void
+    {
+        $db = $this->getConnection(true);
+
+        $schema = $db->getSchema();
+        $table = $schema->getTableSchema('negative_default_values');
+
+        $this->assertNotNull($table);
+        $this->assertSame(-123, $table->getColumn('smallint_col')?->getDefaultValue());
+        $this->assertSame(-123, $table->getColumn('int_col')?->getDefaultValue());
+        $this->assertSame(-123, $table->getColumn('bigint_col')?->getDefaultValue());
+        $this->assertSame(-12345.6789, $table->getColumn('float_col')?->getDefaultValue());
+        $this->assertEquals(-33.22, $table->getColumn('numeric_col')?->getDefaultValue());
+
+        $db->close();
+    }
 }
diff --git a/tests/Support/Fixture/mssql.sql b/tests/Support/Fixture/mssql.sql
@@ -135,12 +135,11 @@ CREATE TABLE [dbo].[null_values] (
 );
 
 CREATE TABLE [dbo].[negative_default_values] (
-  [tinyint_col] [tinyint] DEFAULT '-123',
-  [smallint_col] [tinyint] DEFAULT '-123',
-  [int_col] [smallint] DEFAULT '-123',
-  [bigint_col] [int] DEFAULT '-123',
-  [float_col] [float] DEFAULT '-12345.6789',
-  [numeric_col] [decimal](5,2) DEFAULT '-33.22'
+  [smallint_col] [smallint] DEFAULT -123,
+  [int_col] [int] DEFAULT -123,
+  [bigint_col] [bigint] DEFAULT -123,
+  [float_col] [float] DEFAULT -12345.6789,
+  [numeric_col] [decimal](5,2) DEFAULT -33.22
 );
 
 CREATE TABLE [dbo].[type] (