From 5ecee24c342ef70be186bef457ba2aaa572bcd07 Mon Sep 17 00:00:00 2001 From: Sergei Tigrov Date: Wed, 1 Nov 2023 10:37:18 +0700 Subject: [PATCH] Refactor DMLQueryBuilder (#233) * Refactor DMLQueryBuilder * Fix @psalm-var * Add test for a non-primary key table * Fix yiisoft/db#61 (point 2) * Fix yiisoft/db#61 (point 2) add test * improve test * Add line to CHANGELOG.md * Improve performance of quoting column names up to 10% using `array_map()` * Remove `Generator` * Update psalm --- CHANGELOG.md | 1 + psalm.xml | 3 + src/DMLQueryBuilder.php | 110 +++++++++--------------- tests/Provider/CommandProvider.php | 7 +- tests/Provider/QueryBuilderProvider.php | 6 ++ tests/QueryBuilderTest.php | 15 +++- 6 files changed, 68 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aef34e3..9b94a65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## 1.1.1 under development +- Bug #233: Refactor `DMLQueryBuilder`, related with yiisoft/db#746 (@Tigrov) - Enh #230: Improve column type #230 (@Tigrov) - Bug #240: Remove `RECURSIVE` expression from CTE queries (@Tigrov) 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 efae23a..ae03ccc 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; @@ -15,11 +14,9 @@ use Yiisoft\Db\Query\QueryInterface; use Yiisoft\Db\QueryBuilder\AbstractDMLQueryBuilder; +use function array_map; 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. @@ -27,35 +24,28 @@ final class DMLQueryBuilder extends AbstractDMLQueryBuilder { /** - * @psalm-suppress MixedArrayOffset - * * @throws Exception * @throws InvalidArgumentException * @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 ''; } - 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 */ foreach ($rows as $row) { + $i = 0; $placeholders = []; - foreach ($row as $index => $value) { - if (isset($columns[$index], $mappedNames[$columns[$index]], $columnSchemas[$mappedNames[$columns[$index]]])) { - /** @var mixed $value */ - $value = $this->getTypecastValue($value, $columnSchemas[$mappedNames[$columns[$index]]]); + + foreach ($row as $value) { + if (isset($columns[$i], $columnSchemas[$columns[$i]])) { + $value = $columnSchemas[$columns[$i]]->dbTypecast($value); } if ($value instanceof ExpressionInterface) { @@ -63,6 +53,8 @@ public function batchInsert(string $table, array $columns, iterable|Generator $r } else { $placeholders[] = $this->queryBuilder->bindParam($value, $params); } + + ++$i; } $values[] = '(' . implode(', ', $placeholders) . ')'; } @@ -71,9 +63,10 @@ public function batchInsert(string $table, array $columns, iterable|Generator $r return ''; } - foreach ($columns as $i => $name) { - $columns[$i] = $this->quoter->quoteColumnName($mappedNames[$name]); - } + $columns = array_map( + [$this->quoter, 'quoteColumnName'], + $columns, + ); $tableAndColumns = ' INTO ' . $this->quoter->quoteTableName($table) . ' (' . implode(', ', $columns) . ') VALUES '; @@ -105,7 +98,6 @@ public function upsert( array|bool $updateColumns, array &$params = [] ): string { - $usingValues = null; $constraints = []; [$uniqueNames, $insertNames, $updateNames] = $this->prepareUpsertColumns( @@ -119,16 +111,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 +127,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 +171,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..65cd034 100644 --- a/tests/Provider/CommandProvider.php +++ b/tests/Provider/CommandProvider.php @@ -32,12 +32,15 @@ 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'; + 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 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 => <<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', []), + ); + } }