From 5c0c7c53e5f0601e630fe5631844e459128f337c Mon Sep 17 00:00:00 2001 From: Tigrov Date: Thu, 22 May 2025 10:11:17 +0700 Subject: [PATCH 1/7] Replace `upsertWithReturningPks()` with `upsertWithReturning()` in `DMLQueryBuilder` class --- src/DMLQueryBuilder.php | 46 ++++++++++++++++++++----- tests/Provider/QueryBuilderProvider.php | 44 ++++++++++++++++++----- tests/QueryBuilderTest.php | 7 ++-- 3 files changed, 77 insertions(+), 20 deletions(-) diff --git a/src/DMLQueryBuilder.php b/src/DMLQueryBuilder.php index cdeb8730..76d7593a 100644 --- a/src/DMLQueryBuilder.php +++ b/src/DMLQueryBuilder.php @@ -80,27 +80,55 @@ public function upsert( } } - [$updates, $params] = $this->prepareUpdateSets($table, $updateColumns, $params); + $quotedUniqueNames = array_map($this->quoter->quoteColumnName(...), $uniqueNames); + $updates = $this->prepareUpdateSets($table, $updateColumns, $params); return $insertSql - . ' ON CONFLICT (' . implode(', ', $uniqueNames) . ') DO UPDATE SET ' . implode(', ', $updates); + . ' ON CONFLICT (' . implode(', ', $quotedUniqueNames) . ')' + . ' DO UPDATE SET ' . implode(', ', $updates); } - public function upsertWithReturningPks( + public function upsertWithReturning( string $table, array|QueryInterface $insertColumns, array|bool $updateColumns = true, + array|null $returnColumns = null, array &$params = [], ): string { - $sql = $this->upsert($table, $insertColumns, $updateColumns, $params); - $returnColumns = $this->schema->getTableSchema($table)?->getPrimaryKey(); + $upsertSql = $this->upsert($table, $insertColumns, $updateColumns, $params); - if (!empty($returnColumns)) { - $returnColumns = array_map($this->quoter->quoteColumnName(...), $returnColumns); + $returnColumns ??= $this->schema->getTableSchema($table)?->getColumnNames(); - $sql .= ' RETURNING ' . implode(', ', $returnColumns); + if (empty($returnColumns)) { + return $upsertSql; } - return $sql; + $returnColumns = array_map($this->quoter->quoteColumnName(...), $returnColumns); + + if (str_ends_with($upsertSql, ' ON CONFLICT DO NOTHING')) { + $dummyColumn = $this->getDummyColumn($table); + + $upsertSql = substr($upsertSql, 0, -10) . "DO UPDATE SET $dummyColumn = $dummyColumn"; + } + + return $upsertSql . ' RETURNING ' . implode(', ', $returnColumns); + } + + private function getDummyColumn(string $table): string + { + /** @psalm-suppress PossiblyNullReference */ + $columns = $this->schema->getTableSchema($table)->getColumns(); + + foreach ($columns as $column) { + if ($column->isPrimaryKey() || $column->isUnique()) { + continue; + } + + /** @psalm-suppress PossiblyNullArgument */ + return $this->quoter->quoteColumnName($column->getName()); + } + + /** @psalm-suppress PossiblyNullArgument, PossiblyFalseReference */ + return $this->quoter->quoteColumnName(end($columns)->getName()); } } diff --git a/tests/Provider/QueryBuilderProvider.php b/tests/Provider/QueryBuilderProvider.php index 4b00f52f..ca8f941a 100644 --- a/tests/Provider/QueryBuilderProvider.php +++ b/tests/Provider/QueryBuilderProvider.php @@ -13,6 +13,7 @@ use Yiisoft\Db\Tests\Support\TraversableObject; use function array_replace; +use function array_splice; final class QueryBuilderProvider extends \Yiisoft\Db\Tests\Provider\QueryBuilderProvider { @@ -160,7 +161,7 @@ public static function upsert(): array ], 'values and expressions without update part' => [ 3 => << [ @@ -170,7 +171,7 @@ public static function upsert(): array ], 'query, values and expressions without update part' => [ 3 => << [ @@ -180,7 +181,7 @@ public static function upsert(): array ], 'no columns to update with unique' => [ 3 => << [ @@ -199,15 +200,29 @@ public static function upsert(): array return $upsert; } - public static function upsertWithReturningPks(): array + public static function upsertWithReturning(): array { $upsert = self::upsert(); - foreach ($upsert as &$data) { - $data[3] .= ' RETURNING `id`'; + $withoutUpdate = [ + 'regular values without update part', + 'query without update part', + 'values and expressions without update part', + 'query, values and expressions without update part', + 'no columns to update with unique', + ]; + + foreach ($upsert as $name => &$data) { + array_splice($data, 3, 0, [['id']]); + if (in_array($name, $withoutUpdate, true)) { + $data[4] = substr($data[4], 0, -10) . 'DO UPDATE SET `ts` = `ts`'; + } + + $data[4] .= ' RETURNING `id`'; } - $upsert['no columns to update'][3] = 'INSERT INTO `T_upsert_1` (`a`) VALUES (:qp0) ON CONFLICT DO NOTHING RETURNING `a`'; + $upsert['no columns to update'][3] = ['a']; + $upsert['no columns to update'][4] = 'INSERT INTO `T_upsert_1` (`a`) VALUES (:qp0) ON CONFLICT DO UPDATE SET `a` = `a` RETURNING `a`'; return [ ...$upsert, @@ -215,17 +230,30 @@ public static function upsertWithReturningPks(): array 'notauto_pk', ['id_1' => 1, 'id_2' => 2.5, 'type' => 'Test'], true, + ['id_1', 'id_2'], 'INSERT INTO `notauto_pk` (`id_1`, `id_2`, `type`) VALUES (:qp0, :qp1, :qp2)' . ' ON CONFLICT (`id_1`, `id_2`) DO UPDATE SET `type`=EXCLUDED.`type` RETURNING `id_1`, `id_2`', [':qp0' => 1, ':qp1' => 2.5, ':qp2' => 'Test'], ], - 'no primary key' => [ + 'no return columns' => [ 'type', ['int_col' => 3, 'char_col' => 'a', 'float_col' => 1.2, 'bool_col' => true], true, + [], 'INSERT INTO `type` (`int_col`, `char_col`, `float_col`, `bool_col`) VALUES (:qp0, :qp1, :qp2, :qp3)', [':qp0' => 3, ':qp1' => 'a', ':qp2' => 1.2, ':qp3' => true], ], + 'return all columns' => [ + 'T_upsert', + ['email' => 'test@example.com', 'address' => 'test address', 'status' => 1, 'profile_id' => 1], + true, + null, + 'INSERT INTO `T_upsert` (`email`, `address`, `status`, `profile_id`) VALUES (:qp0, :qp1, :qp2, :qp3)' + . ' ON CONFLICT (`email`) DO UPDATE SET' + . ' `address`=EXCLUDED.`address`, `status`=EXCLUDED.`status`, `profile_id`=EXCLUDED.`profile_id`' + . ' RETURNING `id`, `ts`, `email`, `recovery_email`, `address`, `status`, `orders`, `profile_id`', + [':qp0' => 'test@example.com', ':qp1' => 'test address', ':qp2' => 1, ':qp3' => 1], + ], ]; } diff --git a/tests/QueryBuilderTest.php b/tests/QueryBuilderTest.php index f9819412..610c045f 100644 --- a/tests/QueryBuilderTest.php +++ b/tests/QueryBuilderTest.php @@ -623,15 +623,16 @@ public function testUpsert( parent::testUpsert($table, $insertColumns, $updateColumns, $expectedSql, $expectedParams); } - #[DataProviderExternal(QueryBuilderProvider::class, 'upsertWithReturningPks')] - public function testUpsertWithReturningPks( + #[DataProviderExternal(QueryBuilderProvider::class, 'upsertWithReturning')] + public function testUpsertWithReturning( string $table, array|QueryInterface $insertColumns, array|bool $updateColumns, + array|null $returnColumns, string $expectedSql, array $expectedParams ): void { - parent::testUpsertWithReturningPks($table, $insertColumns, $updateColumns, $expectedSql, $expectedParams); + parent::testUpsertWithReturning($table, $insertColumns, $updateColumns, $returnColumns, $expectedSql, $expectedParams); } #[DataProviderExternal(QueryBuilderProvider::class, 'selectScalar')] From 5ba9dc17ce5fa2837c43f47a8f71bf7104732c5b Mon Sep 17 00:00:00 2001 From: Tigrov Date: Sat, 24 May 2025 13:18:14 +0700 Subject: [PATCH 2/7] Update phpunit.xml.dist --- phpunit.xml.dist | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/phpunit.xml.dist b/phpunit.xml.dist index c65978bb..81ae00f5 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -1,5 +1,12 @@ - + From 4d45676b3ab825d7983ff8ed45dfa3238b5d4d1b Mon Sep 17 00:00:00 2001 From: Tigrov Date: Sat, 24 May 2025 14:36:36 +0700 Subject: [PATCH 3/7] Update CHANGELOG.md [skip ci] --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dcee8393..1fb91079 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,8 +38,8 @@ - New #348: Realize `Schema::loadResultColumn()` method (@Tigrov) - New #354: Add `FOR` clause to query (@vjik) - New #355: Use `DateTimeColumn` class for datetime column types (@Tigrov) -- New #356: Implement `DMLQueryBuilder::upsertWithReturningPks()` method (@Tigrov) -- Enh #356: Refactor `Command::insertWithReturningPks()` and `DMLQueryBuilder::upsert()` methods (@Tigrov) +- New #356, #357: Implement `DMLQueryBuilder::upsertWithReturning()` method (@Tigrov) +- Enh #356, #357: Refactor `Command::insertWithReturningPks()` and `DMLQueryBuilder::upsert()` methods (@Tigrov) ## 1.2.0 March 21, 2024 From efc5cc6146914a50110f8ffefe7c2501c6c82d1e Mon Sep 17 00:00:00 2001 From: Tigrov Date: Sat, 24 May 2025 15:42:43 +0700 Subject: [PATCH 4/7] Improve --- src/Command.php | 39 ++++++++++++++++++++++++++------------- tests/CommandTest.php | 16 ++++++++++++++++ 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/src/Command.php b/src/Command.php index 5efe7b16..1e388bd2 100644 --- a/src/Command.php +++ b/src/Command.php @@ -27,31 +27,44 @@ final class Command extends AbstractPdoCommand { public function insertWithReturningPks(string $table, array|QueryInterface $columns): array|false { + $tableSchema = $this->db->getSchema()->getTableSchema($table); + $primaryKeys = $tableSchema?->getPrimaryKey() ?? []; + $tableColumns = $tableSchema?->getColumns() ?? []; + + foreach ($primaryKeys as $name) { + /** @var ColumnInterface $column */ + $column = $tableColumns[$name]; + + if ($column->isAutoIncrement()) { + continue; + } + + if ($columns instanceof QueryInterface) { + throw new NotSupportedException( + __METHOD__ . '() is not supported by Sqlite for tables without auto increment when inserting sub-query.' + ); + } + + break; + } + $params = []; - $sql = $this->db->getQueryBuilder()->insert($table, $columns, $params); - $this->setSql($sql)->bindValues($params); + $insertSql = $this->db->getQueryBuilder()->insert($table, $columns, $params); + $this->setSql($insertSql)->bindValues($params); if ($this->execute() === 0) { return false; } - $tableSchema = $this->db->getSchema()->getTableSchema($table); - $tablePrimaryKeys = $tableSchema?->getPrimaryKey() ?? []; - - if (empty($tablePrimaryKeys)) { + if (empty($primaryKeys)) { return []; } - if ($columns instanceof QueryInterface) { - throw new NotSupportedException(__METHOD__ . '() not supported for QueryInterface by SQLite.'); - } - $result = []; - /** @var TableSchema $tableSchema */ - foreach ($tablePrimaryKeys as $name) { + foreach ($primaryKeys as $name) { /** @var ColumnInterface $column */ - $column = $tableSchema->getColumn($name); + $column = $tableColumns[$name]; if ($column->isAutoIncrement()) { $value = $this->db->getLastInsertId(); diff --git a/tests/CommandTest.php b/tests/CommandTest.php index 1ad74381..2d3fdbaf 100644 --- a/tests/CommandTest.php +++ b/tests/CommandTest.php @@ -10,6 +10,7 @@ use Yiisoft\Db\Exception\Exception; use Yiisoft\Db\Exception\InvalidConfigException; use Yiisoft\Db\Exception\NotSupportedException; +use Yiisoft\Db\Query\Query; use Yiisoft\Db\Sqlite\Tests\Support\TestTrait; use Yiisoft\Db\Tests\Common\CommonCommandTest; @@ -326,6 +327,21 @@ public function testGetRawSql(string $sql, array $params, string $expectedRawSql parent::testGetRawSql($sql, $params, $expectedRawSql); } + public function testInsertWithReturningPksWithSubqueryAndNoAutoincrement(): void + { + $db = $this->getConnection(true); + $command = $db->createCommand(); + + $query = (new Query($db))->select(['order_id' => 1, 'item_id' => 2, 'quantity' => 3, 'subtotal' => 4]); + + $this->expectException(NotSupportedException::class); + $this->expectExceptionMessage( + 'Yiisoft\Db\Sqlite\Command::insertWithReturningPks() is not supported by Sqlite for tables without auto increment when inserting sub-query.' + ); + + $command->insertWithReturningPks('order_item', $query); + } + /** * @throws Throwable * @throws InvalidConfigException From e39c310f4b3059439189a0c34bc697dbad2a2e8d Mon Sep 17 00:00:00 2001 From: Tigrov Date: Sat, 24 May 2025 15:57:11 +0700 Subject: [PATCH 5/7] Fix psalm --- src/Command.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Command.php b/src/Command.php index 1e388bd2..694b335f 100644 --- a/src/Command.php +++ b/src/Command.php @@ -69,6 +69,7 @@ public function insertWithReturningPks(string $table, array|QueryInterface $colu if ($column->isAutoIncrement()) { $value = $this->db->getLastInsertId(); } else { + /** @var array $columns */ $value = $columns[$name] ?? $column->getDefaultValue(); } From 693bc16aee66ace36de6b75ac67253a3324cc822 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Tue, 27 May 2025 15:19:28 +0700 Subject: [PATCH 6/7] Rename `upsertWithReturning()` to `upsertReturning()` --- src/DMLQueryBuilder.php | 2 +- tests/Provider/QueryBuilderProvider.php | 2 +- tests/QueryBuilderTest.php | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/DMLQueryBuilder.php b/src/DMLQueryBuilder.php index 76d7593a..cb6ad60d 100644 --- a/src/DMLQueryBuilder.php +++ b/src/DMLQueryBuilder.php @@ -88,7 +88,7 @@ public function upsert( . ' DO UPDATE SET ' . implode(', ', $updates); } - public function upsertWithReturning( + public function upsertReturning( string $table, array|QueryInterface $insertColumns, array|bool $updateColumns = true, diff --git a/tests/Provider/QueryBuilderProvider.php b/tests/Provider/QueryBuilderProvider.php index ca8f941a..6b1aaf58 100644 --- a/tests/Provider/QueryBuilderProvider.php +++ b/tests/Provider/QueryBuilderProvider.php @@ -200,7 +200,7 @@ public static function upsert(): array return $upsert; } - public static function upsertWithReturning(): array + public static function upsertReturning(): array { $upsert = self::upsert(); diff --git a/tests/QueryBuilderTest.php b/tests/QueryBuilderTest.php index 610c045f..8f9105e1 100644 --- a/tests/QueryBuilderTest.php +++ b/tests/QueryBuilderTest.php @@ -623,8 +623,8 @@ public function testUpsert( parent::testUpsert($table, $insertColumns, $updateColumns, $expectedSql, $expectedParams); } - #[DataProviderExternal(QueryBuilderProvider::class, 'upsertWithReturning')] - public function testUpsertWithReturning( + #[DataProviderExternal(QueryBuilderProvider::class, 'upsertReturning')] + public function testUpsertReturning( string $table, array|QueryInterface $insertColumns, array|bool $updateColumns, @@ -632,7 +632,7 @@ public function testUpsertWithReturning( string $expectedSql, array $expectedParams ): void { - parent::testUpsertWithReturning($table, $insertColumns, $updateColumns, $returnColumns, $expectedSql, $expectedParams); + parent::testUpsertReturning($table, $insertColumns, $updateColumns, $returnColumns, $expectedSql, $expectedParams); } #[DataProviderExternal(QueryBuilderProvider::class, 'selectScalar')] From 758a9b38693cf6f45cf8f29b15702afbc1e57a97 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Wed, 28 May 2025 17:21:00 +0700 Subject: [PATCH 7/7] Update CHANGELOG.md [skip ci] --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fb91079..fcf6c8ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,7 +38,7 @@ - New #348: Realize `Schema::loadResultColumn()` method (@Tigrov) - New #354: Add `FOR` clause to query (@vjik) - New #355: Use `DateTimeColumn` class for datetime column types (@Tigrov) -- New #356, #357: Implement `DMLQueryBuilder::upsertWithReturning()` method (@Tigrov) +- New #356, #357: Implement `DMLQueryBuilder::upsertReturning()` method (@Tigrov) - Enh #356, #357: Refactor `Command::insertWithReturningPks()` and `DMLQueryBuilder::upsert()` methods (@Tigrov) ## 1.2.0 March 21, 2024