fix: Model handling of Entity $primaryKey casting by kenjis · Pull Request #8282 · codeigniter4/CodeIgniter4

kenjis · 2023-12-01T08:48:38Z

Description
Fixes#7797
Supersedes #8271

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

SQLite3::escapeString() is not binary safe. https://www.php.net/manual/en/sqlite3.escapestring.php

kenjis · 2023-12-02T01:10:48Z

batchUpdate() does not work because the following SQL cannot update the columns:

UPDATE`db_uuid`INNER JOIN (
SELECT'<binary_uuid_value_1>'`id`, 'update1'`value`UNION ALLSELECT'<binary_uuid_value_2>'`id`, 'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

The following patch makes the test pass:

--- a/system/Database/MySQLi/Builder.php+++ b/system/Database/MySQLi/Builder.php@@ -131,7 +131,7 @@ class Builder extends BaseBuilder
" UNION ALL\n",
array_map(
static fn ($value) => 'SELECT ' . implode(', ', array_map(
- static fn ($key, $index) => $index . ' ' . $key,+ static fn ($key, $index) => (is_string($index) ? '_binary' . $index : $index) . ' ' . $key,
$keys,
$value
)),

This makes the following SQL:

UPDATE`db_uuid`INNER JOIN (
SELECT _binary'<binary_uuid_value_1>'`id`, _binary'update1'`value`UNION ALLSELECT _binary'<binary_uuid_value_2>'`id`, _binary'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

See https://dev.mysql.com/doc/refman/8.0/en/string-literals.html

This is the same data type issue in batchUpdate() as the following case in PostgreSQL:
https://forum.codeigniter.com/showthread.php?tid=88871

kenjis · 2023-12-04T21:30:28Z

@codeigniter4/database-team Review, please.

kenjis · 2023-12-01T08:48:38Z

Description
Fixes#7797
Supersedes #8271

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

SQLite3::escapeString() is not binary safe. https://www.php.net/manual/en/sqlite3.escapestring.php

kenjis · 2023-12-02T01:10:48Z

batchUpdate() does not work because the following SQL cannot update the columns:

UPDATE`db_uuid`INNER JOIN (
SELECT'<binary_uuid_value_1>'`id`, 'update1'`value`UNION ALLSELECT'<binary_uuid_value_2>'`id`, 'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

The following patch makes the test pass:

--- a/system/Database/MySQLi/Builder.php+++ b/system/Database/MySQLi/Builder.php@@ -131,7 +131,7 @@ class Builder extends BaseBuilder
" UNION ALL\n",
array_map(
static fn ($value) => 'SELECT ' . implode(', ', array_map(
- static fn ($key, $index) => $index . ' ' . $key,+ static fn ($key, $index) => (is_string($index) ? '_binary' . $index : $index) . ' ' . $key,
$keys,
$value
)),

This makes the following SQL:

UPDATE`db_uuid`INNER JOIN (
SELECT _binary'<binary_uuid_value_1>'`id`, _binary'update1'`value`UNION ALLSELECT _binary'<binary_uuid_value_2>'`id`, _binary'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

See https://dev.mysql.com/doc/refman/8.0/en/string-literals.html

This is the same data type issue in batchUpdate() as the following case in PostgreSQL:
https://forum.codeigniter.com/showthread.php?tid=88871

kenjis · 2023-12-04T21:30:28Z

@codeigniter4/database-team Review, please.

kenjis · 2023-12-01T08:48:38Z

Description
Fixes#7797
Supersedes #8271

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

SQLite3::escapeString() is not binary safe. https://www.php.net/manual/en/sqlite3.escapestring.php

kenjis · 2023-12-02T01:10:48Z

batchUpdate() does not work because the following SQL cannot update the columns:

UPDATE`db_uuid`INNER JOIN (
SELECT'<binary_uuid_value_1>'`id`, 'update1'`value`UNION ALLSELECT'<binary_uuid_value_2>'`id`, 'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

The following patch makes the test pass:

--- a/system/Database/MySQLi/Builder.php+++ b/system/Database/MySQLi/Builder.php@@ -131,7 +131,7 @@ class Builder extends BaseBuilder
" UNION ALL\n",
array_map(
static fn ($value) => 'SELECT ' . implode(', ', array_map(
- static fn ($key, $index) => $index . ' ' . $key,+ static fn ($key, $index) => (is_string($index) ? '_binary' . $index : $index) . ' ' . $key,
$keys,
$value
)),

This makes the following SQL:

UPDATE`db_uuid`INNER JOIN (
SELECT _binary'<binary_uuid_value_1>'`id`, _binary'update1'`value`UNION ALLSELECT _binary'<binary_uuid_value_2>'`id`, _binary'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

See https://dev.mysql.com/doc/refman/8.0/en/string-literals.html

This is the same data type issue in batchUpdate() as the following case in PostgreSQL:
https://forum.codeigniter.com/showthread.php?tid=88871

kenjis · 2023-12-04T21:30:28Z

@codeigniter4/database-team Review, please.

kenjis · 2023-12-01T08:48:38Z

Description
Fixes#7797
Supersedes #8271

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

SQLite3::escapeString() is not binary safe. https://www.php.net/manual/en/sqlite3.escapestring.php

kenjis · 2023-12-02T01:10:48Z

batchUpdate() does not work because the following SQL cannot update the columns:

UPDATE`db_uuid`INNER JOIN (
SELECT'<binary_uuid_value_1>'`id`, 'update1'`value`UNION ALLSELECT'<binary_uuid_value_2>'`id`, 'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

The following patch makes the test pass:

--- a/system/Database/MySQLi/Builder.php+++ b/system/Database/MySQLi/Builder.php@@ -131,7 +131,7 @@ class Builder extends BaseBuilder
" UNION ALL\n",
array_map(
static fn ($value) => 'SELECT ' . implode(', ', array_map(
- static fn ($key, $index) => $index . ' ' . $key,+ static fn ($key, $index) => (is_string($index) ? '_binary' . $index : $index) . ' ' . $key,
$keys,
$value
)),

This makes the following SQL:

UPDATE`db_uuid`INNER JOIN (
SELECT _binary'<binary_uuid_value_1>'`id`, _binary'update1'`value`UNION ALLSELECT _binary'<binary_uuid_value_2>'`id`, _binary'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

See https://dev.mysql.com/doc/refman/8.0/en/string-literals.html

This is the same data type issue in batchUpdate() as the following case in PostgreSQL:
https://forum.codeigniter.com/showthread.php?tid=88871

kenjis · 2023-12-04T21:30:28Z

@codeigniter4/database-team Review, please.

kenjis · 2023-12-01T08:48:38Z

Description
Fixes#7797
Supersedes #8271

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

SQLite3::escapeString() is not binary safe. https://www.php.net/manual/en/sqlite3.escapestring.php

kenjis · 2023-12-02T01:10:48Z

batchUpdate() does not work because the following SQL cannot update the columns:

UPDATE`db_uuid`INNER JOIN (
SELECT'<binary_uuid_value_1>'`id`, 'update1'`value`UNION ALLSELECT'<binary_uuid_value_2>'`id`, 'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

The following patch makes the test pass:

--- a/system/Database/MySQLi/Builder.php+++ b/system/Database/MySQLi/Builder.php@@ -131,7 +131,7 @@ class Builder extends BaseBuilder
" UNION ALL\n",
array_map(
static fn ($value) => 'SELECT ' . implode(', ', array_map(
- static fn ($key, $index) => $index . ' ' . $key,+ static fn ($key, $index) => (is_string($index) ? '_binary' . $index : $index) . ' ' . $key,
$keys,
$value
)),

This makes the following SQL:

UPDATE`db_uuid`INNER JOIN (
SELECT _binary'<binary_uuid_value_1>'`id`, _binary'update1'`value`UNION ALLSELECT _binary'<binary_uuid_value_2>'`id`, _binary'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

See https://dev.mysql.com/doc/refman/8.0/en/string-literals.html

This is the same data type issue in batchUpdate() as the following case in PostgreSQL:
https://forum.codeigniter.com/showthread.php?tid=88871

kenjis · 2023-12-04T21:30:28Z

@codeigniter4/database-team Review, please.

kenjis · 2023-12-01T08:48:38Z

Description
Fixes#7797
Supersedes #8271

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

SQLite3::escapeString() is not binary safe. https://www.php.net/manual/en/sqlite3.escapestring.php

kenjis · 2023-12-02T01:10:48Z

batchUpdate() does not work because the following SQL cannot update the columns:

UPDATE`db_uuid`INNER JOIN (
SELECT'<binary_uuid_value_1>'`id`, 'update1'`value`UNION ALLSELECT'<binary_uuid_value_2>'`id`, 'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

The following patch makes the test pass:

--- a/system/Database/MySQLi/Builder.php+++ b/system/Database/MySQLi/Builder.php@@ -131,7 +131,7 @@ class Builder extends BaseBuilder
" UNION ALL\n",
array_map(
static fn ($value) => 'SELECT ' . implode(', ', array_map(
- static fn ($key, $index) => $index . ' ' . $key,+ static fn ($key, $index) => (is_string($index) ? '_binary' . $index : $index) . ' ' . $key,
$keys,
$value
)),

This makes the following SQL:

UPDATE`db_uuid`INNER JOIN (
SELECT _binary'<binary_uuid_value_1>'`id`, _binary'update1'`value`UNION ALLSELECT _binary'<binary_uuid_value_2>'`id`, _binary'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

See https://dev.mysql.com/doc/refman/8.0/en/string-literals.html

This is the same data type issue in batchUpdate() as the following case in PostgreSQL:
https://forum.codeigniter.com/showthread.php?tid=88871

kenjis · 2023-12-04T21:30:28Z

@codeigniter4/database-team Review, please.

kenjis · 2023-12-01T08:48:38Z

Description
Fixes#7797
Supersedes #8271

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

SQLite3::escapeString() is not binary safe. https://www.php.net/manual/en/sqlite3.escapestring.php

kenjis · 2023-12-02T01:10:48Z

batchUpdate() does not work because the following SQL cannot update the columns:

UPDATE`db_uuid`INNER JOIN (
SELECT'<binary_uuid_value_1>'`id`, 'update1'`value`UNION ALLSELECT'<binary_uuid_value_2>'`id`, 'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

The following patch makes the test pass:

--- a/system/Database/MySQLi/Builder.php+++ b/system/Database/MySQLi/Builder.php@@ -131,7 +131,7 @@ class Builder extends BaseBuilder
" UNION ALL\n",
array_map(
static fn ($value) => 'SELECT ' . implode(', ', array_map(
- static fn ($key, $index) => $index . ' ' . $key,+ static fn ($key, $index) => (is_string($index) ? '_binary' . $index : $index) . ' ' . $key,
$keys,
$value
)),

This makes the following SQL:

UPDATE`db_uuid`INNER JOIN (
SELECT _binary'<binary_uuid_value_1>'`id`, _binary'update1'`value`UNION ALLSELECT _binary'<binary_uuid_value_2>'`id`, _binary'update2'`value`
) `_u`ON`db_uuid`.`id`=`_u`.`id`SET`db_uuid`.`value`=`_u`.`value`

See https://dev.mysql.com/doc/refman/8.0/en/string-literals.html

This is the same data type issue in batchUpdate() as the following case in PostgreSQL:
https://forum.codeigniter.com/showthread.php?tid=88871

kenjis · 2023-12-04T21:30:28Z

@codeigniter4/database-team Review, please.

kenjis marked this pull request as draft December 1, 2023 08:48

kenjis force-pushed the fix-model-pk-with-cast branch from 6f7c53d to 4ed4743Compare December 1, 2023 08:52

kenjis changed the title ~~fix: Model handles cast $primaryKey value when using Entity~~fix: Model handling of Entity $primaryKey casting Dec 1, 2023

test: add test for Model and Entity with cast primaryKey
2ee4979

kenjis force-pushed the fix-model-pk-with-cast branch from 4ed4743 to 2ee4979Compare December 1, 2023 09:55

fix: processing in Model of Entity for which primary key is cast
b8330e9

kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Dec 1, 2023

kenjis added 2 commits December 1, 2023 19:05

chore: update phpstan-baseline
7f7be9a

test: skip test on OCI8, Postgre, SQLSRV
c0a86fd

kenjis mentioned this pull request Dec 1, 2023
Bug: Model inserts the casted $primaryKey value when using an entity and casts. #7797
Closed

kenjis marked this pull request as ready for review December 1, 2023 22:30

kenjis mentioned this pull request Dec 1, 2023
fix: [Model] update does not work when using custom entity with toRawArray() #8271
Closed
5 tasks

test: skip SQLite3
87a706c
SQLite3::escapeString() is not binary safe. https://www.php.net/manual/en/sqlite3.escapestring.php

kenjis force-pushed the fix-model-pk-with-cast branch from f89e7dc to 87a706cCompare December 1, 2023 22:34

test: skip test for batchUpdate() that does not work
c357e4c

kenjis added the database Issues or pull requests that affect the database layer label Dec 2, 2023

michalsn reviewed Dec 2, 2023
View reviewed changes

Comment threadsystem/Model.php
Comment threadtests/_support/Entity/Cast/CastUUID.php Outdated

test: fix test Entity name
6df976b

kenjis mentioned this pull request Dec 2, 2023
Bug: [Postgres] QueryBuilder::updateBatch() pg_affected_rows(): Argument #1 ($result) must be of type PgSql\Result, bool given #7387
Closed

michalsn approved these changes Dec 6, 2023
View reviewed changes

kenjis merged commit fc2bff7 into codeigniter4:developDec 6, 2023

kenjis deleted the fix-model-pk-with-cast branch December 6, 2023 23:54

kenjis mentioned this pull request Jan 18, 2024
fix: [Postgre] QueryBuilder::updateBatch() does not work <column_name>::<type>#8426
Closed
5 tasks