fix: [Postgre] QueryBuilder::updateBatch() does not work (No API change) #8439

kenjis · 2024-01-21T02:07:41Z

Description
Supersedes #8426
Fixes #7387
See https://forum.codeigniter.com/showthread.php?tid=88871

$data = [
    [
        'type_text'     => 'updated',
        'type_bigint'   => 9_999_999,
        'type_date'     => '2023-12-01', // Key
        'type_datetime' => '2024-01-01 09:00:00',
    ],
    [
        'type_text'     => 'updated',
        'type_bigint'   => 9_999_999,
        'type_date'     => '2023-12-02', // Key
        'type_datetime' => '2024-01-01 09:00:00',
    ],
];
$this->db->table($table)->updateBatch($data, 'type_date');

UPDATE "db_type_test"
SET
"type_bigint" = CAST(_u."type_bigint" AS BIGINT),
"type_datetime" = CAST(_u."type_datetime" AS TIMESTAMP WITHOUT TIME ZONE),
"type_text" = CAST(_u."type_text" AS TEXT)
FROM (
SELECT 9999999 "type_bigint", '2023-12-01' "type_date", '2024-01-01 09:00:00' "type_datetime", 'updated' "type_text" UNION ALL
SELECT 9999999 "type_bigint", '2023-12-02' "type_date", '2024-01-01 09:00:00' "type_datetime", 'updated' "type_text"
) _u
WHERE "db_type_test"."type_date" = CAST(_u."type_date" AS DATE)

References:

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2024-01-21T02:31:15Z

Starting mssql service container
  /usr/bin/docker pull mcr.microsoft.com/mssql/server:2022-latest
  Error response from daemon: received unexpected HTTP status: 503 Service Unavailable
  Warning: Docker pull failed with exit code 1, back off 8.668 seconds before retry.
  /usr/bin/docker pull mcr.microsoft.com/mssql/server:2022-latest
  Error response from daemon: received unexpected HTTP status: 503 Service Unavailable
  Warning: Docker pull failed with exit code 1, back off 4.351 seconds before retry.
  /usr/bin/docker pull mcr.microsoft.com/mssql/server:2022-latest
  Error response from daemon: Head "https://mcr.microsoft.com/v2/mssql/server/manifests/2022-latest": no basic auth credentials
  Error: Docker pull failed with exit code 1

https://github.com/codeigniter4/CodeIgniter4/actions/runs/7598402465/job/20694371974?pr=8439

kenjis · 2024-01-21T06:59:46Z

SQLSRV

There was 1 error:

1) CodeIgniter\Database\Live\UpdateTest::testUpdateBatch
CodeIgniter\Database\Exceptions\DatabaseException: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]The data types text and varchar are incompatible in the equal to operator.

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseConnection.php:647
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseBuilder.php:1697
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/Constraints/SeeInDatabase.php:55
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/DatabaseTestTrait.php:280
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Database/Live/UpdateTest.php:156
phpvfscomposer:///home/runner/work/CodeIgniter4/CodeIgniter4/vendor/phpunit/phpunit/phpunit:106

https://github.com/codeigniter4/CodeIgniter4/actions/runs/7598402465/job/20695798884?pr=8439

It seems we cannot compare text and varchar with = .
The following code causes the error.

        $this->seeInDatabase($table, [
            'type_varchar'  => 'test1',
            'type_text'     => 'updated',
            'type_bigint'   => 9_999_999,
            'type_date'     => '2024-01-01',
            'type_datetime' => '2024-01-01 09:00:00',
        ]);

SELECT COUNT(*) AS "numrows"
FROM "test"."dbo"."db_type_test"
WHERE "type_varchar" = 'test1'
AND "type_text" = 'updated'
AND "type_bigint" = 9999999
AND "type_date" = '2024-01-01'
AND "type_datetime" = '2024-01-01 09:00:00'

Data type text, ntext, image are deprecated in SQL Server 2016
Use varchar(max), nvarchar(max), and varbinary(max) data types.
https://learn.microsoft.com/en-us/sql/database-engine/deprecated-database-engine-features-in-sql-server-2016?view=sql-server-ver16#features-deprecated-in-a-future-version-of-sql-server

kenjis · 2024-01-21T07:59:19Z

OCI8

There was 1 error:

1) CodeIgniter\Database\Live\UpdateTest::testUpdateBatch
CodeIgniter\Database\Exceptions\DatabaseException: oci_execute(): ORA-01400: cannot insert NULL into ("ORACLE"."db_type_test"."type_boolean")

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseConnection.php:647
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseBuilder.php:2302
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Database/Live/UpdateTest.php:133
phpvfscomposer:///home/runner/work/CodeIgniter4/CodeIgniter4/vendor/phpunit/phpunit/phpunit:106

https://github.com/codeigniter4/CodeIgniter4/actions/runs/7598402465/job/20695798698

It seems OCI8 Forge maps 'type_boolean' => ['type' => 'BOOLEAN', 'null' => true], to "type_boolean" NUMBER(1) not null.
But not null is not needed. Is this a bug?
→ I sent PR #8440

sclubricants · 2024-01-21T19:17:02Z

Might run into problems in the where part as well

WHERE "db_type_test"."type_varchar" = _u."type_varchar"

It probably works here because its text to varchar.

We could cast everything in the select part but it adds a lot of redundancy.

Maybe try the query where the constraint is timestamp and see what it does.

sclubricants · 2024-01-21T22:11:31Z

Looks like we should take a look at sqsrv->forge->_attributeType(array &$attributes)

Maybe text should be mapped to nvarchar(max) and enum as well.

This could be problematic changing these though I suppose.

kenjis · 2024-01-22T00:25:49Z

Might run into problems in the where part as well
WHERE "db_type_test"."type_varchar" = _u."type_varchar"

Yes, fixed in c4ecd69

sclubricants · 2024-01-22T01:39:02Z

Maybe uppercase ::text to ::TEXT. This is part of SQL and might be easier to read.

Could do this in getFieldTypes()

kenjis · 2024-01-22T02:47:55Z

We could cast everything in the select part but it adds a lot of redundancy.
Maybe try the query where the constraint is timestamp and see what it does.

I'm not sure what types are safe to remove.

If we remove ::bigint, the following query fails:

UPDATE "db_type_test"
SET
"type_bigint" = _u."type_bigint",
"type_integer" = _u."type_integer"
FROM (
SELECT '2448114396435166946' "type_bigint", '9999999' "type_integer", 'test1' "type_varchar" UNION ALL
SELECT '2448114396435166946' "type_bigint", '9999999' "type_integer", 'test2' "type_varchar"
) _u
WHERE "db_type_test"."type_varchar" = _u."type_varchar"

ErrorException: pg_query(): Query failed: ERROR:  column "type_bigint" is of type bigint but expression is of type text
LINE 3: "type_bigint" = _u."type_bigint",
                        ^
HINT:  You will need to rewrite or cast the expression.

In this case, the value '2448114396435166946' is wrong, but this may happen, because all data returned by PosrgreSQL are PHP strings.
https://forum.codeigniter.com/showthread.php?tid=88871&pid=414027#pid414027

sclubricants · 2024-01-22T18:53:04Z

tests/_support/Database/Migrations/20160428212500_Create_test_tables.php

+            // FLOAT should not be used on MySQL.
+            //   CREATE TABLE t (f FLOAT, d DOUBLE);
+            //   INSERT INTO t VALUES(99.9, 99.9);
+            //   SELECT * FROM t WHERE f=99.9; // Empty set


Float is an approximation of a number. The following would work:

SELECT * FROM t WHERE f > 99.89 AND f < 99.91;

Yes, and we cannot use seeInDatabase().

sclubricants · 2024-01-22T22:33:57Z

system/Database/Postgre/Builder.php

+    private function castValue(string $fieldName, $value): string
+    {
+        if (! isset($this->QBOptions['fieldTypes'])) {
+            throw new LogicException(


I'm not sure I like throwing an exception here. I think rather than an exception if fieldTypes aren't defined then getFieldTypes() should be called. This would require the additional parameter $table though. The way it is the method is dependent on another method being called to get some data. If you don't want to add $table as another parameter then you could add $fieldTypes.

You also would not need to pass $value to the method as you could simply append ::TEXT or empty string.

$value . $that->getFieldType($fieldName, $fieldTypes); $value . $that->getFieldType($fieldName, $table);

On second thought might should include value because with other DBMS as well as Postgre the format is:

CAST ('100' AS INTEGER)

So:

$that->castValue($table, $fieldName, $value);

private function castValue(string $table, string $fieldName, $value): string { if (! isset($this->QBOptions['fieldTypes'])) { $this->QBOptions['fieldTypes'] = $this->getFieldTypes($table); } $type = $this->QBOptions['fieldTypes'][$fieldName] ?? null; return ($type === null) ? $value : $value . '::' . strtoupper($type); // return ($type === null) ? $value : 'CAST(' . $value . ' AS ' . strtoupper($type) . ')'; }

$that->castValue($table, $fieldName, "'2024-01-01'"); // CAST('2024-01-01' AS DATE) $that->castValue($table, $fieldName, $alias . $fieldName); // CAST(_u."created_date" AS DATE)

You could still use the ::DATE format but in case this was ever implemented with other DBMS then it would work with the CAST(expression AS type) format.

If you were using it in the SELECT part then you would be using values but in the SET part you are using field names.

call it expression:

$that->castExpression($table, $fieldName, $expression); $that->cast($table, $fieldName, $expression); $that->castExpression($table, $fieldName, '1!=2'); // CAST(1!=2 AS BOOLEAN)

$this->cast($expression, $type); // CAST(expression AS type) $this->cast($expression, $this->getFieldType($table, $fieldName)); private function getFieldType(string $table, string $fieldName): string { if (! isset($this->QBOptions['fieldTypes'][$table])) { $this->QBOptions['fieldTypes'][$table] = []; foreach ($this->db->getFieldData($table) as $field) { $this->QBOptions['fieldTypes'][$table][$field->name] = $field->type; } } return $this->QBOptions['fieldTypes'][$table][$fieldName] ?? null; } // BaseBuilder protected function cast($expression, $type): string // possible future public method { return $this->_cast($expression, $type); } protected function _cast($expression, $type): string { //return ($type === null) ? $expression : $expression . '::' . strtoupper($type); return ($type === null) ? $expression : 'CAST(' . $expression . ' AS ' . strtoupper($type) . ')'; // covers most all dbms }

Changed. 6d4ce25

I made some updates to last post.. see what ya think

I don't like castValue() because it might not always be a value but rather an expression. Most DBMS document as CAST(expression AS type)

cast($expression, $type) makes sense. But we can't add API in a patch version.

Changed. 92cc84e

See https://dev.mysql.com/doc/refman/8.0/en/floating-point-types.html

kenjis · 2024-01-23T02:55:21Z

@codeigniter4/database-team Review, please.

michalsn

Looks great - thank you @kenjis and @sclubricants!

sclubricants · 2024-01-23T19:26:10Z

system/Database/Postgre/Builder.php

+                array_map(
+                    static fn ($key, $value) => $key . ($value instanceof RawSql ?
+                            ' = ' . $value :
+                            ' = ' . $alias . '.' . $that->cast($value, $that->getFieldType($table, $key))),


Suggested change

' = ' . $alias . '.' . $that->cast($value, $that->getFieldType($table, $key))),

' = ' . $that->cast($alias . '.' . $value, $that->getFieldType($table, $key))),

$alias . '.' . $value is the full part of the expression. I would like to see us use the more universal format of CAST(expression AS type). If we use this then you definitely have to include the alias as part of expression. The expression here is "table"."column"

sclubricants · 2024-01-23T19:27:42Z

system/Database/Postgre/Builder.php

+                            return $value;
+                        }
+
+                        return $table . '.' . $value . ' = ' . $alias . '.' . $that->cast($value, $that->getFieldType($table, $value));


Suggested change

return $table . '.' . $value . ' = ' . $alias . '.' . $that->cast($value, $that->getFieldType($table, $value));

return $table . '.' . $value . ' = ' . $that->cast($alias . '.' . $value, $that->getFieldType($table, $value));

sclubricants · 2024-01-23T19:39:47Z

system/Database/Postgre/Builder.php

+     */
+    private function cast($expression, ?string $type): string
+    {
+        return ($type === null) ? $expression : $expression . '::' . strtoupper($type);


Suggested change

return ($type === null) ? $expression : $expression . '::' . strtoupper($type);

return ($type === null) ? $expression : 'CAST(' . $expression . ' AS ' . strtoupper($type) . ')';

The expression::TYPE format is a shorthand format postgre supports. However Postgre and all other DBMS support the format CAST(expression AS type). We should go ahead and use this format so that if this method is moved to BaseBuilder it will generate code compatible with all DBMS.

https://www.postgresqltutorial.com/postgresql-tutorial/postgresql-cast/
https://docs.oracle.com/javadb/10.8.3.0/ref/rrefsqlj33562.html
https://dev.mysql.com/doc/refman/8.0/en/cast-functions.html#function_cast
https://learn.microsoft.com/en-us/sql/t-sql/functions/cast-and-convert-transact-sql?view=sql-server-ver16
https://www.sqlite.org/lang_expr.html#castexpr

sclubricants · 2024-01-23T21:09:08Z

This looks good to me.

We should probably check upsertBatch() and deleteBatch() as well. I think insertBatch() should be ok.

kenjis · 2024-01-24T23:45:25Z

Thank you! @sclubricants @michalsn

kenjis · 2024-01-25T02:44:44Z

@sclubricants

We should probably check upsertBatch() and deleteBatch() as well. I think insertBatch() should be ok.

See

kenjis added bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer labels Jan 21, 2024

kenjis mentioned this pull request Jan 21, 2024

fix: [Postgre] QueryBuilder::updateBatch() does not work <column_name>::<type> #8426

Closed

5 tasks

kenjis force-pushed the fix-postgre-updateBatch-2 branch from 2b4ddc7 to ef13c8e Compare January 21, 2024 08:49

kenjis mentioned this pull request Jan 21, 2024

fix: OCI8 Forge always sets NOT NULL when BOOLEAN is specified #8440

Merged

5 tasks

kenjis force-pushed the fix-postgre-updateBatch-2 branch from 5c01940 to b008b0d Compare January 22, 2024 02:51

sclubricants reviewed Jan 22, 2024

View reviewed changes

kenjis added 14 commits January 23, 2024 09:24

test: update comment

02de356

test: add test for type date and datetime

b796f0b

refactor: copy _updateBatch() to override

a71aa3c

refactor: make if condition more strict

f1b4a6c

fix: updateBatch() Postgre type error

8832601

docs: add sub section titles and tweaks

1d96753

docs: add changelog

9ae66a2

refactory: by rector

560e745

test: fix assertion for SQLSRV

1bffa76

test: add test for updateBatch() with constrants DATE

38fb5b2

fix: Postgre updateBatch() SQL type error in WHERE part

3336010

test: add test for more types

f86c6d2

refactor: add castValue() and use it

fc0a858

test: refactor with dataProvider

ec11284

kenjis added 4 commits January 23, 2024 09:24

refactor: make type uppercase in SQL string for readablitiy

d4f3e92

test: add test for int as string

5ca6dfa

test: FLOAT on MySQL cannot be used as WHERE = key

64cd791

See https://dev.mysql.com/doc/refman/8.0/en/floating-point-types.html

refactor: change castValue() API

6d4ce25

kenjis force-pushed the fix-postgre-updateBatch-2 branch from e18caaf to 6d4ce25 Compare January 23, 2024 00:58

kenjis added 2 commits January 23, 2024 11:32

docs: add @param

cd644df

refactor: change castValue(),getFieldTypes() to cast(),getFieldType()

92cc84e

kenjis force-pushed the fix-postgre-updateBatch-2 branch from e5cfe9e to 92cc84e Compare January 23, 2024 02:32

refactor: by rector

fa2e073

michalsn approved these changes Jan 23, 2024

View reviewed changes

sclubricants reviewed Jan 23, 2024

View reviewed changes

refactor: use CAST(expression AS type) instead of ::type

fbacb30

kenjis requested a review from sclubricants January 23, 2024 20:50

sclubricants approved these changes Jan 23, 2024

View reviewed changes

kenjis mentioned this pull request Jan 24, 2024

Bug: [Postgres] QueryBuilder::updateBatch() pg_affected_rows(): Argument #1 ($result) must be of type PgSql\Result, bool given #7387

Closed

docs: update PHPDoc

baca36e

kenjis merged commit c7cfba4 into codeigniter4:develop Jan 24, 2024
63 checks passed

kenjis deleted the fix-postgre-updateBatch-2 branch January 24, 2024 23:45

kenjis mentioned this pull request Jan 25, 2024

fix: [Postgre] QueryBuilder::deleteBatch() does not work #8451

Merged

5 tasks

kenjis mentioned this pull request Feb 7, 2024

fix: [Postgre] updateBatch() breaks char type data #8524

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [Postgre] QueryBuilder::updateBatch() does not work (No API change) #8439

fix: [Postgre] QueryBuilder::updateBatch() does not work (No API change) #8439

kenjis commented Jan 21, 2024 •

edited

Loading

kenjis commented Jan 21, 2024

kenjis commented Jan 21, 2024 •

edited

Loading

kenjis commented Jan 21, 2024 •

edited

Loading

sclubricants commented Jan 21, 2024

sclubricants commented Jan 21, 2024

kenjis commented Jan 22, 2024

sclubricants commented Jan 22, 2024 •

edited

Loading

kenjis commented Jan 22, 2024

sclubricants Jan 22, 2024

kenjis Jan 23, 2024

sclubricants Jan 22, 2024 •

edited

Loading

sclubricants Jan 22, 2024

sclubricants Jan 22, 2024

sclubricants Jan 22, 2024 •

edited

Loading

sclubricants Jan 22, 2024 •

edited

Loading

sclubricants Jan 23, 2024 •

edited

Loading

sclubricants Jan 23, 2024 •

edited

Loading

kenjis Jan 23, 2024

sclubricants Jan 23, 2024 •

edited

Loading

kenjis Jan 23, 2024

kenjis commented Jan 23, 2024

michalsn left a comment

sclubricants Jan 23, 2024 •

edited by kenjis

Loading

kenjis Jan 23, 2024

sclubricants Jan 23, 2024

kenjis Jan 23, 2024

sclubricants Jan 23, 2024

kenjis Jan 23, 2024

sclubricants commented Jan 23, 2024

kenjis commented Jan 24, 2024

kenjis commented Jan 25, 2024

	' = ' . $alias . '.' . $that->cast($value, $that->getFieldType($table, $key))),
	' = ' . $that->cast($alias . '.' . $value, $that->getFieldType($table, $key))),

	return $table . '.' . $value . ' = ' . $alias . '.' . $that->cast($value, $that->getFieldType($table, $value));
	return $table . '.' . $value . ' = ' . $that->cast($alias . '.' . $value, $that->getFieldType($table, $value));

	return ($type === null) ? $expression : $expression . '::' . strtoupper($type);
	return ($type === null) ? $expression : 'CAST(' . $expression . ' AS ' . strtoupper($type) . ')';

fix: [Postgre] QueryBuilder::updateBatch() does not work (No API change) #8439

fix: [Postgre] QueryBuilder::updateBatch() does not work (No API change) #8439

Conversation

kenjis commented Jan 21, 2024 • edited Loading

kenjis commented Jan 21, 2024

kenjis commented Jan 21, 2024 • edited Loading

kenjis commented Jan 21, 2024 • edited Loading

sclubricants commented Jan 21, 2024

sclubricants commented Jan 21, 2024

kenjis commented Jan 22, 2024

sclubricants commented Jan 22, 2024 • edited Loading

kenjis commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sclubricants Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sclubricants Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

sclubricants Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

sclubricants Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

sclubricants Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sclubricants Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenjis commented Jan 23, 2024

michalsn left a comment

Choose a reason for hiding this comment

sclubricants Jan 23, 2024 • edited by kenjis Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sclubricants commented Jan 23, 2024

kenjis commented Jan 24, 2024

kenjis commented Jan 25, 2024

kenjis commented Jan 21, 2024 •

edited

Loading

kenjis commented Jan 21, 2024 •

edited

Loading

kenjis commented Jan 21, 2024 •

edited

Loading

sclubricants commented Jan 22, 2024 •

edited

Loading

sclubricants Jan 22, 2024 •

edited

Loading

sclubricants Jan 22, 2024 •

edited

Loading

sclubricants Jan 22, 2024 •

edited

Loading

sclubricants Jan 23, 2024 •

edited

Loading

sclubricants Jan 23, 2024 •

edited

Loading

sclubricants Jan 23, 2024 •

edited

Loading

sclubricants Jan 23, 2024 •

edited by kenjis

Loading