From cdd40b8dfb3a2f29a72fb2e07ca0e9718f15494c Mon Sep 17 00:00:00 2001 From: Alexander Kartavenko Date: Wed, 21 Aug 2019 11:56:20 +0300 Subject: [PATCH] Fix #17504: Fix upsert when `$updateColumns = true` but there are no columns to update in the table --- framework/CHANGELOG.md | 1 + framework/db/cubrid/QueryBuilder.php | 4 ++++ framework/db/mssql/QueryBuilder.php | 4 ++++ framework/db/mysql/QueryBuilder.php | 4 ++++ framework/db/oci/QueryBuilder.php | 4 ++++ framework/db/pgsql/QueryBuilder.php | 8 ++++++++ framework/db/sqlite/QueryBuilder.php | 4 ++++ tests/data/mssql.sql | 7 +++++++ tests/data/mysql.sql | 6 ++++++ tests/data/postgres.sql | 6 ++++++ tests/data/sqlite.sql | 6 ++++++ tests/framework/db/QueryBuilderTest.php | 13 +++++++++++++ tests/framework/db/cubrid/QueryBuilderTest.php | 4 ++++ tests/framework/db/mssql/QueryBuilderTest.php | 3 +++ tests/framework/db/mysql/QueryBuilderTest.php | 3 +++ tests/framework/db/oci/QueryBuilderTest.php | 4 ++++ tests/framework/db/pgsql/QueryBuilderTest.php | 6 ++++++ tests/framework/db/sqlite/QueryBuilderTest.php | 3 +++ 18 files changed, 90 insertions(+) diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index 4a1d08c5fb..7c4e1dfa62 100644 --- a/framework/CHANGELOG.md +++ b/framework/CHANGELOG.md @@ -4,6 +4,7 @@ Yii Framework 2 Change Log 2.0.26 under development ------------------------ +- Bug #17504: Fix upsert when `$updateColumns = true` but there are no columns to update in the table (alexkart) - Bug #17511: Fixed IPv6 subnets matching in `IpHelper::inRange()` (kamarton) diff --git a/framework/db/cubrid/QueryBuilder.php b/framework/db/cubrid/QueryBuilder.php index 1ad9825fe0..6ebbc5845c 100644 --- a/framework/db/cubrid/QueryBuilder.php +++ b/framework/db/cubrid/QueryBuilder.php @@ -70,6 +70,10 @@ class QueryBuilder extends \yii\db\QueryBuilder if (empty($uniqueNames)) { return $this->insert($table, $insertColumns, $params); } + if ($updateNames === []) { + // there are no columns to update + $updateColumns = false; + } $onCondition = ['or']; $quotedTableName = $this->db->quoteTableName($table); diff --git a/framework/db/mssql/QueryBuilder.php b/framework/db/mssql/QueryBuilder.php index eaf023d21c..350a9c8729 100644 --- a/framework/db/mssql/QueryBuilder.php +++ b/framework/db/mssql/QueryBuilder.php @@ -454,6 +454,10 @@ class QueryBuilder extends \yii\db\QueryBuilder if (empty($uniqueNames)) { return $this->insert($table, $insertColumns, $params); } + if ($updateNames === []) { + // there are no columns to update + $updateColumns = false; + } $onCondition = ['or']; $quotedTableName = $this->db->quoteTableName($table); diff --git a/framework/db/mysql/QueryBuilder.php b/framework/db/mysql/QueryBuilder.php index a210760535..e41a120a2d 100644 --- a/framework/db/mysql/QueryBuilder.php +++ b/framework/db/mysql/QueryBuilder.php @@ -278,6 +278,10 @@ class QueryBuilder extends \yii\db\QueryBuilder if (empty($uniqueNames)) { return $insertSql; } + if ($updateNames === []) { + // there are no columns to update + $updateColumns = false; + } if ($updateColumns === true) { $updateColumns = []; diff --git a/framework/db/oci/QueryBuilder.php b/framework/db/oci/QueryBuilder.php index 5f380b13e0..f4e6b3db70 100644 --- a/framework/db/oci/QueryBuilder.php +++ b/framework/db/oci/QueryBuilder.php @@ -218,6 +218,10 @@ EOD; if (empty($uniqueNames)) { return $this->insert($table, $insertColumns, $params); } + if ($updateNames === []) { + // there are no columns to update + $updateColumns = false; + } $onCondition = ['or']; $quotedTableName = $this->db->quoteTableName($table); diff --git a/framework/db/pgsql/QueryBuilder.php b/framework/db/pgsql/QueryBuilder.php index 4b79b49944..2f0720033b 100644 --- a/framework/db/pgsql/QueryBuilder.php +++ b/framework/db/pgsql/QueryBuilder.php @@ -338,6 +338,10 @@ class QueryBuilder extends \yii\db\QueryBuilder if (empty($uniqueNames)) { return $insertSql; } + if ($updateNames === []) { + // there are no columns to update + $updateColumns = false; + } if ($updateColumns === false) { return "$insertSql ON CONFLICT DO NOTHING"; @@ -368,6 +372,10 @@ class QueryBuilder extends \yii\db\QueryBuilder if (empty($uniqueNames)) { return $this->insert($table, $insertColumns, $params); } + if ($updateNames === []) { + // there are no columns to update + $updateColumns = false; + } /** @var Schema $schema */ $schema = $this->db->getSchema(); diff --git a/framework/db/sqlite/QueryBuilder.php b/framework/db/sqlite/QueryBuilder.php index 5f9cb5de41..38e0fa9273 100644 --- a/framework/db/sqlite/QueryBuilder.php +++ b/framework/db/sqlite/QueryBuilder.php @@ -74,6 +74,10 @@ class QueryBuilder extends \yii\db\QueryBuilder if (empty($uniqueNames)) { return $this->insert($table, $insertColumns, $params); } + if ($updateNames === []) { + // there are no columns to update + $updateColumns = false; + } list(, $placeholders, $values, $params) = $this->prepareInsertValues($table, $insertColumns, $params); $insertSql = 'INSERT OR IGNORE INTO ' . $this->db->quoteTableName($table) diff --git a/tests/data/mssql.sql b/tests/data/mssql.sql index c44c1be744..7207c18b46 100644 --- a/tests/data/mssql.sql +++ b/tests/data/mssql.sql @@ -22,6 +22,7 @@ IF OBJECT_ID('[T_constraints_3]', 'U') IS NOT NULL DROP TABLE [T_constraints_3]; IF OBJECT_ID('[T_constraints_2]', 'U') IS NOT NULL DROP TABLE [T_constraints_2]; IF OBJECT_ID('[T_constraints_1]', 'U') IS NOT NULL DROP TABLE [T_constraints_1]; IF OBJECT_ID('[T_upsert]', 'U') IS NOT NULL DROP TABLE [T_upsert]; +IF OBJECT_ID('[T_upsert_1]', 'U') IS NOT NULL DROP TABLE [T_upsert_1]; IF OBJECT_ID('[table.with.special.characters]', 'U') IS NOT NULL DROP TABLE [table.with.special.characters]; IF OBJECT_ID('[stranger ''table]', 'U') IS NOT NULL DROP TABLE [stranger 'table]; @@ -329,6 +330,12 @@ CREATE TABLE [T_upsert] UNIQUE ([email], [recovery_email]) ); +CREATE TABLE [T_upsert_1] +( + [a] INT NOT NULL, + UNIQUE ([a]) +); + CREATE TABLE [dbo].[table.with.special.characters] ( [id] [int] ); diff --git a/tests/data/mysql.sql b/tests/data/mysql.sql index 84d12ade50..a509a568f3 100644 --- a/tests/data/mysql.sql +++ b/tests/data/mysql.sql @@ -32,6 +32,7 @@ DROP TABLE IF EXISTS `T_constraints_3` CASCADE; DROP TABLE IF EXISTS `T_constraints_2` CASCADE; DROP TABLE IF EXISTS `T_constraints_1` CASCADE; DROP TABLE IF EXISTS `T_upsert` CASCADE; +DROP TABLE IF EXISTS `T_upsert_1`; CREATE TABLE `constraints` ( @@ -396,3 +397,8 @@ CREATE TABLE `T_upsert` UNIQUE (`email`, `recovery_email`) ) ENGINE = 'InnoDB' DEFAULT CHARSET = 'utf8'; + +CREATE TABLE `T_upsert_1` ( + `a` int(11) NOT NULL, + PRIMARY KEY (`a`) +) ENGINE=InnoDB DEFAULT CHARSET=utf8; diff --git a/tests/data/postgres.sql b/tests/data/postgres.sql index bc61454811..1950339c71 100644 --- a/tests/data/postgres.sql +++ b/tests/data/postgres.sql @@ -33,6 +33,7 @@ DROP TABLE IF EXISTS "T_constraints_3"; DROP TABLE IF EXISTS "T_constraints_2"; DROP TABLE IF EXISTS "T_constraints_1"; DROP TABLE IF EXISTS "T_upsert"; +DROP TABLE IF EXISTS "T_upsert_1"; DROP SCHEMA IF EXISTS "schema1" CASCADE; DROP SCHEMA IF EXISTS "schema2" CASCADE; @@ -416,3 +417,8 @@ CREATE TABLE "T_upsert" "profile_id" INT NULL, UNIQUE ("email", "recovery_email") ); + +CREATE TABLE "T_upsert_1" +( + "a" INT NOT NULL PRIMARY KEY +); diff --git a/tests/data/sqlite.sql b/tests/data/sqlite.sql index 8cb1663ca0..b39cc7a8ec 100644 --- a/tests/data/sqlite.sql +++ b/tests/data/sqlite.sql @@ -29,6 +29,7 @@ DROP TABLE IF EXISTS "T_constraints_3"; DROP TABLE IF EXISTS "T_constraints_2"; DROP TABLE IF EXISTS "T_constraints_1"; DROP TABLE IF EXISTS "T_upsert"; +DROP TABLE IF EXISTS "T_upsert_1"; CREATE TABLE "profile" ( id INTEGER NOT NULL, @@ -354,3 +355,8 @@ CREATE TABLE "T_upsert" "profile_id" INT NULL, UNIQUE ("email", "recovery_email") ); + +CREATE TABLE "T_upsert_1" +( + "a" INTEGER NOT NULL PRIMARY KEY +); diff --git a/tests/framework/db/QueryBuilderTest.php b/tests/framework/db/QueryBuilderTest.php index 44b49e0ac2..8feafa133d 100644 --- a/tests/framework/db/QueryBuilderTest.php +++ b/tests/framework/db/QueryBuilderTest.php @@ -2171,6 +2171,17 @@ abstract class QueryBuilderTest extends DatabaseTestCase ':qp1' => 0, ], ], + 'no columns to update' => [ + 'T_upsert_1', + [ + 'a' => 1, + ], + true, + null, + [ + ':qp0' => 1, + ], + ], ]; } @@ -2182,6 +2193,8 @@ abstract class QueryBuilderTest extends DatabaseTestCase * @param array|null $updateColumns * @param string|string[] $expectedSQL * @param array $expectedParams + * @throws \yii\base\NotSupportedException + * @throws \Exception */ public function testUpsert($table, $insertColumns, $updateColumns, $expectedSQL, $expectedParams) { diff --git a/tests/framework/db/cubrid/QueryBuilderTest.php b/tests/framework/db/cubrid/QueryBuilderTest.php index aef5b954c0..33679a34f3 100644 --- a/tests/framework/db/cubrid/QueryBuilderTest.php +++ b/tests/framework/db/cubrid/QueryBuilderTest.php @@ -107,6 +107,10 @@ class QueryBuilderTest extends \yiiunit\framework\db\QueryBuilderTest foreach ($concreteData as $testName => $data) { $newData[$testName] = array_replace($newData[$testName], $data); } + + // skip test + unset($newData['no columns to update']); + return $newData; } } diff --git a/tests/framework/db/mssql/QueryBuilderTest.php b/tests/framework/db/mssql/QueryBuilderTest.php index 62b6049b4d..464af406f5 100644 --- a/tests/framework/db/mssql/QueryBuilderTest.php +++ b/tests/framework/db/mssql/QueryBuilderTest.php @@ -311,6 +311,9 @@ class QueryBuilderTest extends \yiiunit\framework\db\QueryBuilderTest 'query, values and expressions without update part' => [ 3 => 'MERGE {{%T_upsert}} WITH (HOLDLOCK) USING (SELECT :phEmail AS [email], now() AS [[time]]) AS [EXCLUDED] ([email], [[time]]) ON ({{%T_upsert}}.[email]=[EXCLUDED].[email]) WHEN MATCHED THEN UPDATE SET [ts]=:qp1, [[orders]]=T_upsert.orders + 1 WHEN NOT MATCHED THEN INSERT ([email], [[time]]) VALUES ([EXCLUDED].[email], [EXCLUDED].[[time]]);', ], + 'no columns to update' => [ + 3 => 'MERGE [T_upsert_1] WITH (HOLDLOCK) USING (VALUES (:qp0)) AS [EXCLUDED] ([a]) ON ([T_upsert_1].[a]=[EXCLUDED].[a]) WHEN NOT MATCHED THEN INSERT ([a]) VALUES ([EXCLUDED].[a]);', + ], ]; $newData = parent::upsertProvider(); foreach ($concreteData as $testName => $data) { diff --git a/tests/framework/db/mysql/QueryBuilderTest.php b/tests/framework/db/mysql/QueryBuilderTest.php index a47fab9fd3..93f341d1b9 100644 --- a/tests/framework/db/mysql/QueryBuilderTest.php +++ b/tests/framework/db/mysql/QueryBuilderTest.php @@ -256,6 +256,9 @@ class QueryBuilderTest extends \yiiunit\framework\db\QueryBuilderTest 'query, values and expressions without update part' => [ 3 => 'INSERT INTO {{%T_upsert}} (`email`, [[time]]) SELECT :phEmail AS `email`, now() AS [[time]] ON DUPLICATE KEY UPDATE `ts`=:qp1, [[orders]]=T_upsert.orders + 1', ], + 'no columns to update' => [ + 3 => 'INSERT INTO `T_upsert_1` (`a`) VALUES (:qp0) ON DUPLICATE KEY UPDATE `a`=`T_upsert_1`.`a`', + ], ]; $newData = parent::upsertProvider(); foreach ($concreteData as $testName => $data) { diff --git a/tests/framework/db/oci/QueryBuilderTest.php b/tests/framework/db/oci/QueryBuilderTest.php index 9a036d7323..10e1f16af4 100644 --- a/tests/framework/db/oci/QueryBuilderTest.php +++ b/tests/framework/db/oci/QueryBuilderTest.php @@ -239,6 +239,10 @@ WHERE rownum <= 1) "EXCLUDED" ON ("T_upsert"."email"="EXCLUDED"."email") WHEN NO foreach ($concreteData as $testName => $data) { $newData[$testName] = array_replace($newData[$testName], $data); } + + // skip test + unset($newData['no columns to update']); + return $newData; } } diff --git a/tests/framework/db/pgsql/QueryBuilderTest.php b/tests/framework/db/pgsql/QueryBuilderTest.php index e4b78bdb06..14b2eaf183 100644 --- a/tests/framework/db/pgsql/QueryBuilderTest.php +++ b/tests/framework/db/pgsql/QueryBuilderTest.php @@ -347,6 +347,12 @@ class QueryBuilderTest extends \yiiunit\framework\db\QueryBuilderTest 'INSERT INTO {{%T_upsert}} ("email", [[time]]) SELECT :phEmail AS "email", now() AS [[time]] ON CONFLICT ("email") DO UPDATE SET "ts"=:qp1, [[orders]]=T_upsert.orders + 1', ], ], + 'no columns to update' => [ + 3 => [ + 'WITH "EXCLUDED" ("a") AS (VALUES (CAST(:qp0 AS int2))) INSERT INTO "T_upsert_1" ("a") SELECT "a" FROM "EXCLUDED" WHERE NOT EXISTS (SELECT 1 FROM "T_upsert_1" WHERE (("T_upsert_1"."a"="EXCLUDED"."a")))', + 'INSERT INTO "T_upsert_1" ("a") VALUES (:qp0) ON CONFLICT DO NOTHING', + ], + ], ]; $newData = parent::upsertProvider(); foreach ($concreteData as $testName => $data) { diff --git a/tests/framework/db/sqlite/QueryBuilderTest.php b/tests/framework/db/sqlite/QueryBuilderTest.php index f0122dfc11..e0dd672207 100644 --- a/tests/framework/db/sqlite/QueryBuilderTest.php +++ b/tests/framework/db/sqlite/QueryBuilderTest.php @@ -192,6 +192,9 @@ class QueryBuilderTest extends \yiiunit\framework\db\QueryBuilderTest 'query, values and expressions without update part' => [ 3 => 'WITH "EXCLUDED" (`email`, [[time]]) AS (SELECT :phEmail AS `email`, now() AS [[time]]) UPDATE {{%T_upsert}} SET `ts`=:qp1, [[orders]]=T_upsert.orders + 1 WHERE {{%T_upsert}}.`email`=(SELECT `email` FROM `EXCLUDED`); INSERT OR IGNORE INTO {{%T_upsert}} (`email`, [[time]]) SELECT :phEmail AS `email`, now() AS [[time]];', ], + 'no columns to update' => [ + 3 => 'INSERT OR IGNORE INTO `T_upsert_1` (`a`) VALUES (:qp0)', + ], ]; $newData = parent::upsertProvider(); foreach ($concreteData as $testName => $data) {