diff --git a/system/Database/Postgre/Builder.php b/system/Database/Postgre/Builder.php index 4e5245bca994..5367ae2a0fc3 100644 --- a/system/Database/Postgre/Builder.php +++ b/system/Database/Postgre/Builder.php @@ -395,14 +395,20 @@ static function ($key, $value) use ($table, $alias, $that): RawSql|string { if (isset($this->QBOptions['setQueryAsData'])) { $data = $this->QBOptions['setQueryAsData']; } else { + $updateFields = $this->QBOptions['updateFields'] ?? []; + $data = implode( " UNION ALL\n", array_map( - static fn ($value): string => 'SELECT ' . implode(', ', array_map( - static fn ($key, $index): string => $index . ' ' . $key, + fn (array $value): string => sprintf('SELECT %s', implode(', ', array_map( + fn (string $key, float|int|string $index): string => sprintf( + '%s %s', + $this->castValue((string) $index, $this->getSourceType($table, $key, $updateFields)), + $key, + ), $keys, $value, - )), + ))), $values, ), ) . "\n"; @@ -411,6 +417,38 @@ static function ($key, $value) use ($table, $alias, $that): RawSql|string { return str_replace('{:_table_:}', $data, $sql); } + /** + * Returns the type shared by every destination column fed from the source key, or null when they differ. + * + * @param array $map Destination column to source key. + */ + private function getSourceType(string $table, string $key, array $map): ?string + { + $columns = array_filter(array_keys($map, $key, true), is_string(...)); + + if ($columns === []) { + return $this->getFieldType($table, $key); + } + + $types = array_unique(array_map(fn (string $column): ?string => $this->getFieldType($table, $column), $columns)); + + return count($types) === 1 ? reset($types) : null; + } + + /** + * Returns the literal cast to the column type, except a numeric literal for a numeric column, which stays as is so a non-integral value never rounds into a match. + */ + private function castValue(string $value, ?string $type): string + { + $isNumericType = in_array(strtolower((string) $type), ['smallint', 'integer', 'bigint', 'numeric', 'real', 'double precision'], true); + + if ($isNumericType && is_numeric($value)) { + return $value; + } + + return $this->cast($value, $type); + } + /** * Returns cast expression. * @@ -437,11 +475,15 @@ private function getFieldType(string $table, string $fieldName): ?string foreach ($this->db->getFieldData($table) as $field) { $type = $field->type; - // If `character` (or `char`) lacks a specifier, it is equivalent - // to `character(1)`. - // See https://www.postgresql.org/docs/current/datatype-character.html - if ($field->type === 'character') { - $type = $field->type . '(' . $field->max_length . ')'; + // information_schema reports enum, domain and array columns as `USER-DEFINED` or `ARRAY`, which are not castable names. + if ($type === 'USER-DEFINED' || $type === 'ARRAY') { + continue; + } + + // `character` without a length is `character(1)` and `character(n)` truncates on cast, + // while `bpchar` keeps the full value. + if ($type === 'character') { + $type = 'bpchar'; } $this->QBOptions['fieldTypes'][$table][$field->name] = $type; @@ -613,14 +655,20 @@ static function ($key, $value) use ($table, $alias, $that): RawSql|string { if (isset($this->QBOptions['setQueryAsData'])) { $data = $this->QBOptions['setQueryAsData']; } else { + $constraints = $this->QBOptions['constraints'] ?? []; + $data = implode( " UNION ALL\n", array_map( - static fn ($value): string => 'SELECT ' . implode(', ', array_map( - static fn ($key, $index): string => $index . ' ' . $key, + fn (array $value): string => sprintf('SELECT %s', implode(', ', array_map( + fn (string $key, float|int|string $index): string => sprintf( + '%s %s', + $this->castValue((string) $index, $this->getSourceType($table, $key, $constraints)), + $key, + ), $keys, $value, - )), + ))), $values, ), ) . "\n"; diff --git a/tests/system/Database/Builder/DeleteTest.php b/tests/system/Database/Builder/DeleteTest.php index 200eb9ca5b4a..0866cac724a4 100644 --- a/tests/system/Database/Builder/DeleteTest.php +++ b/tests/system/Database/Builder/DeleteTest.php @@ -14,6 +14,7 @@ namespace CodeIgniter\Database\Builder; use CodeIgniter\Database\BaseBuilder; +use CodeIgniter\Database\Postgre\Builder as PostgreBuilder; use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\Mock\MockConnection; use CodeIgniter\Test\Mock\MockQuery; @@ -169,4 +170,50 @@ public function testDeleteBatchEscapesWhereInBinds(): void $this->assertStringContainsString("IN ('anything'' OR ''1''=''1','Ahmadinejad')", $query->getQuery()); $this->assertStringNotContainsString("IN anything' OR '1'='1", $query->getQuery()); } + + public function testDeleteBatchPostgreCastsPositionalAndMappedConstraints(): void + { + $db = new class (['DBDriver' => 'Postgre']) extends MockConnection { + protected function _fieldData(string $table): array + { + return [ + (object) ['name' => 'id', 'type' => 'integer', 'max_length' => 32], + (object) ['name' => 'name', 'type' => 'text', 'max_length' => null], + ]; + } + }; + + $positional = (new PostgreBuilder('jobs', $db))->testMode() + ->setData([['id' => 1], ['id' => '2'], ['id' => 1.4]], null, 'data') + ->onConstraint('id') + ->deleteBatch(); + + $expected = <<<'EOF' + DELETE FROM "jobs" + USING ( + SELECT 1 "id" UNION ALL + SELECT CAST('2' AS INTEGER) "id" UNION ALL + SELECT 1.4 "id" + ) "data" + WHERE "jobs"."id" = "data"."id" + EOF; + + $this->assertSame($expected, rtrim($positional[0])); + + $mapped = (new PostgreBuilder('jobs', $db))->testMode() + ->setData([['value' => 'x'], ['value' => 5]], null, 'data') + ->onConstraint(['name' => 'value']) + ->deleteBatch(); + + $expected = <<<'EOF' + DELETE FROM "jobs" + USING ( + SELECT CAST('x' AS TEXT) "value" UNION ALL + SELECT CAST(5 AS TEXT) "value" + ) "data" + WHERE "jobs"."name" = CAST("data"."value" AS TEXT) + EOF; + + $this->assertSame($expected, rtrim($mapped[0])); + } } diff --git a/tests/system/Database/Builder/UpdateTest.php b/tests/system/Database/Builder/UpdateTest.php index 5a4ccd0be9d3..91b220b0ba1b 100644 --- a/tests/system/Database/Builder/UpdateTest.php +++ b/tests/system/Database/Builder/UpdateTest.php @@ -15,6 +15,7 @@ use CodeIgniter\Database\BaseBuilder; use CodeIgniter\Database\Exceptions\DatabaseException; +use CodeIgniter\Database\Postgre\Builder as PostgreBuilder; use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\Mock\MockConnection; use CodeIgniter\Test\Mock\MockQuery; @@ -469,4 +470,72 @@ public function testSetWithAndWithoutEscape(): void $this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledUpdate())); $this->assertSame($expectedBinds, $builder->getBinds()); } + + public function testUpdateBatchPostgreDoesNotCastEnumAndArrayColumns(): void + { + $db = new class (['DBDriver' => 'Postgre']) extends MockConnection { + protected function _fieldData(string $table): array + { + return [ + (object) ['name' => 'id', 'type' => 'integer', 'max_length' => 32], + (object) ['name' => 'mood', 'type' => 'USER-DEFINED', 'max_length' => null], + (object) ['name' => 'tags', 'type' => 'ARRAY', 'max_length' => null], + ]; + } + }; + + $builder = new PostgreBuilder('jobs', $db); + $sql = $builder->testMode()->updateBatch([ + ['id' => 1, 'mood' => 'happy', 'tags' => '{a}'], + ['id' => 2, 'mood' => 'sad', 'tags' => '{b}'], + ], 'id'); + + $expected = <<<'EOF' + UPDATE "jobs" + SET + "mood" = _u."mood", + "tags" = _u."tags" + FROM ( + SELECT 1 "id", 'happy' "mood", '{a}' "tags" UNION ALL + SELECT 2 "id", 'sad' "mood", '{b}' "tags" + ) _u + WHERE "jobs"."id" = CAST(_u."id" AS INTEGER) + EOF; + + $this->assertSame([$expected], $sql); + } + + public function testUpdateBatchPostgreDoesNotCastSourceSharedByColumnsOfDifferentTypes(): void + { + $db = new class (['DBDriver' => 'Postgre']) extends MockConnection { + protected function _fieldData(string $table): array + { + return [ + (object) ['name' => 'id', 'type' => 'integer', 'max_length' => 32], + (object) ['name' => 'number', 'type' => 'integer', 'max_length' => 32], + (object) ['name' => 'label', 'type' => 'text', 'max_length' => null], + ]; + } + }; + + $builder = new PostgreBuilder('jobs', $db); + $sql = $builder->testMode() + ->setData([['id' => 1, 'value' => 1.4]], null, 'data') + ->updateFields(['number' => 'value', 'label' => 'value']) + ->onConstraint('id') + ->updateBatch(); + + $expected = <<<'EOF' + UPDATE "jobs" + SET + "number" = CAST("data"."value" AS INTEGER), + "label" = CAST("data"."value" AS TEXT) + FROM ( + SELECT 1 "id", 1.4 "value" + ) "data" + WHERE "jobs"."id" = CAST("data"."id" AS INTEGER) + EOF; + + $this->assertSame([$expected], $sql); + } } diff --git a/tests/system/Database/Live/DeleteTest.php b/tests/system/Database/Live/DeleteTest.php index 651dff251a7f..daef35068386 100644 --- a/tests/system/Database/Live/DeleteTest.php +++ b/tests/system/Database/Live/DeleteTest.php @@ -119,6 +119,81 @@ public function testDeleteBatchPreventsSQLInjectionInWhere(): void $this->seeInDatabase('user', ['email' => 'derek@world.com', 'name' => 'Derek Jones']); } + public function testDeleteBatchWithMixedConstraintValueTypesInTextColumn(): void + { + if ($this->db->DBDriver === 'SQLSRV') { + $this->markTestSkipped('SQL Server cannot compare `text` columns with `=`.'); + } + + if ($this->db->DBDriver === 'OCI8') { + $this->markTestSkipped('TODO: the OCI8 builder does not cast mixed `UNION ALL` values yet. Remove this skip once it does.'); + } + + $table = 'type_test'; + + $builder = $this->db->table($table); + $builder->truncate(); + + foreach (['Example', '587', 'kept'] as $i => $text) { + $builder->insert([ + 'type_varchar' => 'test' . $i, + 'type_char' => 'char', + 'type_text' => $text, + 'type_smallint' => 32767, + 'type_integer' => 2_147_483_647, + 'type_bigint' => 9_223_372_036_854_775_807, + 'type_float' => 10.1, + 'type_numeric' => 123.23, + 'type_date' => '2023-12-0' . ($i + 1), + 'type_datetime' => '2023-12-21 12:00:00', + ]); + } + + $this->db->table($table) + ->setData([['text' => 'Example'], ['text' => 587]], null, 'data') + ->onConstraint(['type_text' => 'text']) + ->deleteBatch(); + + $this->dontSeeInDatabase($table, ['type_varchar' => 'test0']); + $this->dontSeeInDatabase($table, ['type_varchar' => 'test1']); + $this->seeInDatabase($table, ['type_varchar' => 'test2']); + } + + public function testDeleteBatchDoesNotTruncateConstraintValueForCharColumn(): void + { + if ($this->db->DBDriver === 'OCI8') { + $this->markTestSkipped('TODO: Oracle resolves a `UNION ALL` of `CHAR` literals with different lengths to `VARCHAR2`, so the OCI8 builder must cast them. Remove this skip once it does.'); + } + + $table = 'type_test'; + + $builder = $this->db->table($table); + $builder->truncate(); + + for ($i = 0; $i < 2; $i++) { + $builder->insert([ + 'type_varchar' => 'test' . $i, + 'type_char' => 'char' . $i, + 'type_text' => 'text', + 'type_smallint' => 32767, + 'type_integer' => 2_147_483_647, + 'type_bigint' => 9_223_372_036_854_775_807, + 'type_float' => 10.1, + 'type_numeric' => 123.23, + 'type_date' => '2023-12-0' . ($i + 1), + 'type_datetime' => '2023-12-21 12:00:00', + ]); + } + + $this->db->table($table) + ->setData([['char' => 'char0 X'], ['char' => 'char1']], null, 'data') + ->onConstraint(['type_char' => 'char']) + ->deleteBatch(); + + $this->seeInDatabase($table, ['type_varchar' => 'test0']); + $this->dontSeeInDatabase($table, ['type_varchar' => 'test1']); + } + public function testDeleteBatchConstraintsDate(): void { $table = 'type_test'; diff --git a/tests/system/Database/Live/UpdateTest.php b/tests/system/Database/Live/UpdateTest.php index 8961e7b05664..4e3944a6c6c2 100644 --- a/tests/system/Database/Live/UpdateTest.php +++ b/tests/system/Database/Live/UpdateTest.php @@ -278,6 +278,156 @@ public static function provideUpdateBatch(): iterable ]; } + public function testUpdateBatchWithMixedValueTypesInTextColumn(): void + { + if ($this->db->DBDriver === 'SQLSRV') { + $this->markTestSkipped('SQL Server cannot compare `text` columns with `=`.'); + } + + if ($this->db->DBDriver === 'OCI8') { + $this->markTestSkipped('TODO: the OCI8 builder does not cast mixed `UNION ALL` values yet. Remove this skip once it does.'); + } + + $table = 'type_test'; + + $builder = $this->db->table($table); + $builder->truncate(); + + for ($i = 1; $i < 3; $i++) { + $builder->insert([ + 'type_varchar' => 'test' . $i, + 'type_char' => 'char' . $i, + 'type_text' => 'text', + 'type_smallint' => 32767, + 'type_integer' => 2_147_483_647, + 'type_bigint' => 9_223_372_036_854_775_807, + 'type_float' => 10.1, + 'type_numeric' => 123.23, + 'type_date' => '2023-12-0' . $i, + 'type_datetime' => '2023-12-21 12:00:00', + ]); + } + + $this->db->table($table)->updateBatch([ + ['type_varchar' => 'test1', 'type_text' => 'Example'], + ['type_varchar' => 'test2', 'type_text' => 587], + ], 'type_varchar'); + + $this->seeInDatabase($table, ['type_varchar' => 'test1', 'type_text' => 'Example']); + $this->seeInDatabase($table, ['type_varchar' => 'test2', 'type_text' => '587']); + } + + public function testUpdateBatchDoesNotTruncateConstraintValueForCharColumn(): void + { + if ($this->db->DBDriver === 'OCI8') { + $this->markTestSkipped('TODO: Oracle resolves a `UNION ALL` of `CHAR` literals with different lengths to `VARCHAR2`, so the OCI8 builder must cast them. Remove this skip once it does.'); + } + + $table = 'type_test'; + + $builder = $this->db->table($table); + $builder->truncate(); + + for ($i = 1; $i < 3; $i++) { + $builder->insert([ + 'type_varchar' => 'test' . $i, + 'type_char' => 'char' . $i, + 'type_text' => 'text', + 'type_smallint' => 32767, + 'type_integer' => 2_147_483_647, + 'type_bigint' => 9_223_372_036_854_775_807, + 'type_float' => 10.1, + 'type_numeric' => 123.23, + 'type_date' => '2023-12-0' . $i, + 'type_datetime' => '2023-12-21 12:00:00', + ]); + } + + $this->db->table($table)->updateBatch([ + ['type_char' => 'char1 X', 'type_text' => 'changed'], + ['type_char' => 'char2', 'type_text' => 'changed'], + ], 'type_char'); + + $this->seeInDatabase($table, ['type_varchar' => 'test1', 'type_text' => 'text']); + $this->seeInDatabase($table, ['type_varchar' => 'test2', 'type_text' => 'changed']); + } + + public function testUpdateBatchWithMappedUpdateFieldsAndMixedValueTypes(): void + { + if ($this->db->DBDriver === 'SQLSRV') { + $this->markTestSkipped('SQL Server cannot compare `text` columns with `=`.'); + } + + if ($this->db->DBDriver === 'OCI8') { + $this->markTestSkipped('TODO: the OCI8 builder does not cast mixed `UNION ALL` values yet. Remove this skip once it does.'); + } + + $table = 'type_test'; + + $builder = $this->db->table($table); + $builder->truncate(); + + for ($i = 1; $i < 3; $i++) { + $builder->insert([ + 'type_varchar' => 'test' . $i, + 'type_char' => 'char' . $i, + 'type_text' => 'text', + 'type_smallint' => 32767, + 'type_integer' => 2_147_483_647, + 'type_bigint' => 9_223_372_036_854_775_807, + 'type_float' => 10.1, + 'type_numeric' => 123.23, + 'type_date' => '2023-12-0' . $i, + 'type_datetime' => '2023-12-21 12:00:00', + ]); + } + + $this->db->table($table) + ->setData([ + ['type_varchar' => 'test1', 'text' => 'Example'], + ['type_varchar' => 'test2', 'text' => 587], + ], null, 'data') + ->updateFields(['type_text' => 'text']) + ->onConstraint('type_varchar') + ->updateBatch(); + + $this->seeInDatabase($table, ['type_varchar' => 'test1', 'type_text' => 'Example']); + $this->seeInDatabase($table, ['type_varchar' => 'test2', 'type_text' => '587']); + } + + public function testUpdateBatchKeepsSourceValueForColumnsOfDifferentTypes(): void + { + if ($this->db->DBDriver === 'SQLSRV') { + $this->markTestSkipped('SQL Server cannot compare `text` columns with `=`.'); + } + + $table = 'type_test'; + + $builder = $this->db->table($table); + $builder->truncate(); + + $builder->insert([ + 'type_varchar' => 'test1', + 'type_char' => 'char1', + 'type_text' => 'text', + 'type_smallint' => 32767, + 'type_integer' => 2_147_483_647, + 'type_bigint' => 9_223_372_036_854_775_807, + 'type_float' => 10.1, + 'type_numeric' => 123.23, + 'type_date' => '2023-12-01', + 'type_datetime' => '2023-12-21 12:00:00', + ]); + + $this->db->table($table) + ->setData([['type_varchar' => 'test1', 'value' => 1.4]], null, 'data') + ->updateFields(['type_integer' => 'value', 'type_text' => 'value']) + ->onConstraint('type_varchar') + ->updateBatch(); + + $this->seeInDatabase($table, ['type_varchar' => 'test1', 'type_text' => '1.4']); + } + public function testUpdateWithWhereSameColumn(): void { $this->db->table('user')->update(['country' => 'CA'], ['country' => 'US']); diff --git a/user_guide_src/source/changelogs/v4.7.5.rst b/user_guide_src/source/changelogs/v4.7.5.rst index 34dc4dcf41eb..45416eef17da 100644 --- a/user_guide_src/source/changelogs/v4.7.5.rst +++ b/user_guide_src/source/changelogs/v4.7.5.rst @@ -55,6 +55,7 @@ Bugs Fixed - **Database:** Fixed a bug where rebuilding a SQLite3 table (e.g., ``Forge::dropColumn()``, ``Forge::modifyColumn()``, ``Forge::dropForeignKey()`` and ``Forge::dropPrimaryKey()``) corrupted the table names referenced by its foreign keys when ``DBPrefix`` was set. - **Database:** Fixed a bug where Postgre query failures were silently ignored when ``DBDebug`` was enabled and PHP warnings were disabled. A ``DatabaseException`` is now thrown. - **Database:** Fixed a bug where ``getFieldData()`` failed for a schema-qualified table name on Postgre, SQLSRV and SQLite3, and where ``protectIdentifiers()`` prefixed an already protected segment of a dotted identifier a second time. +- **Database:** Fixed a bug where ``updateBatch()`` and ``deleteBatch()`` failed on PostgreSQL when the rows supplied different PHP types (e.g., a string and an integer) for the same column, because the ``UNION ALL`` subquery values were not cast to the column type. - **Debug:** Fixed a bug where ``Timer::start()`` treated ``0.0`` as an empty value and substituted the current time. - **Files:** Fixed a bug where ``File::move()`` and ``UploadedFile::move()`` set executable and overly permissive file permissions (``0777 & ~umask()`` instead of ``0666 & ~umask()``), and ``UploadedFile::move()`` targeted the parent directory instead of the destination file for ``chmod()``. - **Helpers:** Fixed a bug where ``get_dir_file_info()`` returned incomplete entries for subdirectories and missing files instead of omitting them.