From dfca0ad50b89b11e4f8c058ea5ae3e65c898bdc7 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Fri, 18 Aug 2023 23:59:06 +0700 Subject: [PATCH 01/10] Refactor DMLQueryBuilder --- src/DMLQueryBuilder.php | 95 +++++++++---------------- tests/Provider/CommandProvider.php | 4 +- tests/Provider/QueryBuilderProvider.php | 6 ++ 3 files changed, 40 insertions(+), 65 deletions(-) diff --git a/src/DMLQueryBuilder.php b/src/DMLQueryBuilder.php index efae23a..139cb26 100644 --- a/src/DMLQueryBuilder.php +++ b/src/DMLQueryBuilder.php @@ -16,10 +16,7 @@ use Yiisoft\Db\QueryBuilder\AbstractDMLQueryBuilder; 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. @@ -40,22 +37,17 @@ public function batchInsert(string $table, array $columns, iterable|Generator $r 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> $rows */ + /** @psalm-var string[][] $rows */ foreach ($rows as $row) { $placeholders = []; - foreach ($row as $index => $value) { - if (isset($columns[$index], $mappedNames[$columns[$index]], $columnSchemas[$mappedNames[$columns[$index]]])) { + foreach ($row as $i => $value) { + if (isset($columns[$i], $columnSchemas[$columns[$i]])) { /** @var mixed $value */ - $value = $this->getTypecastValue($value, $columnSchemas[$mappedNames[$columns[$index]]]); + $value = $columnSchemas[$columns[$i]]->dbTypecast($value); } if ($value instanceof ExpressionInterface) { @@ -72,7 +64,7 @@ public function batchInsert(string $table, array $columns, iterable|Generator $r } foreach ($columns as $i => $name) { - $columns[$i] = $this->quoter->quoteColumnName($mappedNames[$name]); + $columns[$i] = $this->quoter->quoteColumnName($name); } $tableAndColumns = ' INTO ' . $this->quoter->quoteTableName($table) @@ -105,7 +97,6 @@ public function upsert( array|bool $updateColumns, array &$params = [] ): string { - $usingValues = null; $constraints = []; [$uniqueNames, $insertNames, $updateNames] = $this->prepareUpsertColumns( @@ -119,16 +110,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,60 +126,43 @@ 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"; @@ -201,28 +170,28 @@ public function upsert( 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 index bb019f9..b70e3f0 100644 --- a/tests/Provider/CommandProvider.php +++ b/tests/Provider/CommandProvider.php @@ -32,8 +32,8 @@ 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'; diff --git a/tests/Provider/QueryBuilderProvider.php b/tests/Provider/QueryBuilderProvider.php index cfa9fa8..bfcb696 100644 --- 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 => << [ 2 => ['address' => 'foo {{city}}', 'status' => 2, 'orders' => new Expression('"T_upsert"."orders" + 1')], 3 => << Date: Sat, 19 Aug 2023 18:36:54 +0700 Subject: [PATCH 02/10] Fix @psalm-var --- src/DMLQueryBuilder.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/DMLQueryBuilder.php b/src/DMLQueryBuilder.php index 139cb26..46a8ef1 100644 --- a/src/DMLQueryBuilder.php +++ b/src/DMLQueryBuilder.php @@ -41,12 +41,13 @@ public function batchInsert(string $table, array $columns, iterable|Generator $r $columns = $this->getNormalizeColumnNames($columns); $columnSchemas = $this->schema->getTableSchema($table)?->getColumns() ?? []; - /** @psalm-var string[][] $rows */ + /** @psalm-var array[] $rows */ foreach ($rows as $row) { $placeholders = []; + /** @psalm-var mixed $value */ foreach ($row as $i => $value) { if (isset($columns[$i], $columnSchemas[$columns[$i]])) { - /** @var mixed $value */ + /** @psalm-var mixed $value */ $value = $columnSchemas[$columns[$i]]->dbTypecast($value); } From ce1d958e29b7be796ba2b87729e51d42c9723062 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Sat, 19 Aug 2023 18:52:32 +0700 Subject: [PATCH 03/10] Add test for a non-primary key table --- tests/QueryBuilderTest.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/QueryBuilderTest.php b/tests/QueryBuilderTest.php index 405e961..11b44f2 100644 --- a/tests/QueryBuilderTest.php +++ b/tests/QueryBuilderTest.php @@ -633,4 +633,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', []), + ); + } } From d130cb2a1cc85e88bc1a2388196c0beef7f3f28f Mon Sep 17 00:00:00 2001 From: Tigrov Date: Sat, 19 Aug 2023 19:42:40 +0700 Subject: [PATCH 04/10] Fix yiisoft/db#61 (point 2) --- src/DMLQueryBuilder.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/DMLQueryBuilder.php b/src/DMLQueryBuilder.php index 46a8ef1..4639d12 100644 --- a/src/DMLQueryBuilder.php +++ b/src/DMLQueryBuilder.php @@ -43,9 +43,10 @@ public function batchInsert(string $table, array $columns, iterable|Generator $r /** @psalm-var array[] $rows */ foreach ($rows as $row) { + $i = 0; $placeholders = []; /** @psalm-var mixed $value */ - foreach ($row as $i => $value) { + foreach ($row as $value) { if (isset($columns[$i], $columnSchemas[$columns[$i]])) { /** @psalm-var mixed $value */ $value = $columnSchemas[$columns[$i]]->dbTypecast($value); @@ -56,6 +57,8 @@ public function batchInsert(string $table, array $columns, iterable|Generator $r } else { $placeholders[] = $this->queryBuilder->bindParam($value, $params); } + + ++$i; } $values[] = '(' . implode(', ', $placeholders) . ')'; } From a422537c4c46d1b645c8806a2c8d208bdcead0a1 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Sun, 20 Aug 2023 07:49:11 +0700 Subject: [PATCH 05/10] Fix yiisoft/db#61 (point 2) add test --- tests/Provider/QueryBuilderProvider.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Provider/QueryBuilderProvider.php b/tests/Provider/QueryBuilderProvider.php index bfcb696..86f6aa0 100644 --- a/tests/Provider/QueryBuilderProvider.php +++ b/tests/Provider/QueryBuilderProvider.php @@ -56,6 +56,7 @@ public static function batchInsert(): array DbHelper::changeSqlForOracleBatchInsert($batchInsert['bool-false, time-now()']['expected']); DbHelper::changeSqlForOracleBatchInsert($batchInsert['column table names are not checked']['expected']); + DbHelper::changeSqlForOracleBatchInsert($batchInsert['with associative values']['expected']); return $batchInsert; } From 6864203b1fe951c9cd3934076c5ea168a5426139 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Mon, 21 Aug 2023 21:36:46 +0700 Subject: [PATCH 06/10] improve test --- tests/Provider/CommandProvider.php | 3 +++ tests/Provider/QueryBuilderProvider.php | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/Provider/CommandProvider.php b/tests/Provider/CommandProvider.php index b70e3f0..65cd034 100644 --- a/tests/Provider/CommandProvider.php +++ b/tests/Provider/CommandProvider.php @@ -38,6 +38,9 @@ public static function batchInsert(): array 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 index 86f6aa0..bfcb696 100644 --- a/tests/Provider/QueryBuilderProvider.php +++ b/tests/Provider/QueryBuilderProvider.php @@ -56,7 +56,6 @@ public static function batchInsert(): array DbHelper::changeSqlForOracleBatchInsert($batchInsert['bool-false, time-now()']['expected']); DbHelper::changeSqlForOracleBatchInsert($batchInsert['column table names are not checked']['expected']); - DbHelper::changeSqlForOracleBatchInsert($batchInsert['with associative values']['expected']); return $batchInsert; } From a1af8a85054f81e375888dec2182899052b6b31a Mon Sep 17 00:00:00 2001 From: Tigrov Date: Sat, 26 Aug 2023 20:16:30 +0700 Subject: [PATCH 07/10] Add line to CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f68871..07f3c8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## 1.1.1 under development -- no changes in this release. +- Bug #233: Refactor `DMLQueryBuilder`, related with yiisoft/db#746 (@Tigrov) ## 1.1.0 July 24, 2023 From 7c7fc037cbc92434f793b5342a81533bee638503 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Wed, 4 Oct 2023 14:47:50 +0700 Subject: [PATCH 08/10] Improve performance of quoting column names up to 10% using `array_map()` --- src/DMLQueryBuilder.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/DMLQueryBuilder.php b/src/DMLQueryBuilder.php index 4639d12..6b3a82a 100644 --- a/src/DMLQueryBuilder.php +++ b/src/DMLQueryBuilder.php @@ -15,6 +15,7 @@ use Yiisoft\Db\Query\QueryInterface; use Yiisoft\Db\QueryBuilder\AbstractDMLQueryBuilder; +use function array_map; use function implode; use function count; @@ -67,9 +68,10 @@ public function batchInsert(string $table, array $columns, iterable|Generator $r return ''; } - foreach ($columns as $i => $name) { - $columns[$i] = $this->quoter->quoteColumnName($name); - } + $columns = array_map( + [$this->quoter, 'quoteColumnName'], + $columns, + ); $tableAndColumns = ' INTO ' . $this->quoter->quoteTableName($table) . ' (' . implode(', ', $columns) . ') VALUES '; From 3ee30e311a67e07924ae0dc07a928714ab1415c3 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Tue, 31 Oct 2023 09:03:20 +0700 Subject: [PATCH 09/10] Remove `Generator` --- src/DMLQueryBuilder.php | 5 ++--- tests/QueryBuilderTest.php | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/DMLQueryBuilder.php b/src/DMLQueryBuilder.php index 6b3a82a..4047a17 100644 --- 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; @@ -32,14 +31,14 @@ final class DMLQueryBuilder extends AbstractDMLQueryBuilder * @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 ''; } $values = []; - $columns = $this->getNormalizeColumnNames($columns); + $columns = $this->getNormalizeColumnNames('', $columns); $columnSchemas = $this->schema->getTableSchema($table)?->getColumns() ?? []; /** @psalm-var array[] $rows */ diff --git a/tests/QueryBuilderTest.php b/tests/QueryBuilderTest.php index edb843d..9086b4f 100644 --- 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); } From 9fe6843c7bdc1fa60ec817af717abec47162516b Mon Sep 17 00:00:00 2001 From: Tigrov Date: Tue, 31 Oct 2023 15:46:34 +0700 Subject: [PATCH 10/10] Update psalm --- psalm.xml | 3 +++ src/DMLQueryBuilder.php | 6 +----- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/psalm.xml b/psalm.xml index 23bfcce..54a52e1 100644 --- a/psalm.xml +++ b/psalm.xml @@ -14,4 +14,7 @@ + + + diff --git a/src/DMLQueryBuilder.php b/src/DMLQueryBuilder.php index 4047a17..ae03ccc 100644 --- a/src/DMLQueryBuilder.php +++ b/src/DMLQueryBuilder.php @@ -24,8 +24,6 @@ final class DMLQueryBuilder extends AbstractDMLQueryBuilder { /** - * @psalm-suppress MixedArrayOffset - * * @throws Exception * @throws InvalidArgumentException * @throws InvalidConfigException @@ -41,14 +39,12 @@ public function batchInsert(string $table, array $columns, iterable $rows, array $columns = $this->getNormalizeColumnNames('', $columns); $columnSchemas = $this->schema->getTableSchema($table)?->getColumns() ?? []; - /** @psalm-var array[] $rows */ foreach ($rows as $row) { $i = 0; $placeholders = []; - /** @psalm-var mixed $value */ + foreach ($row as $value) { if (isset($columns[$i], $columnSchemas[$columns[$i]])) { - /** @psalm-var mixed $value */ $value = $columnSchemas[$columns[$i]]->dbTypecast($value); }