[bugfix] #58 allow inserts with primary key nulled to be rolled back #57

jtopenpetition · 2023-06-14T14:15:27Z

fixes #58

when the row to be inserted contains null for its primary key column, it's most likely, that it's an auto-increment or otherwise generated primary key, so the $row value is not reliable.

in that particular case, prefer the last insert id over the row value

remark: I wanted to add tests, but I couldn't get the docker container to run for a while, and when I did, I couldn't get the tests to run successfully, so I gave up. Theoretically, the groups table has an auto increment primary key, with which this could be tested, but i didn't make it that far to actually be able to write a test.

this bug was introduced in #44 to fix another bug

when the row to be inserted contains `null` for its primary key column, it's most likely, that it's an auto-increment or otherwise generated primary key, so the row value is not reliable. in that particular case, prefer the last insert id over the row value

JesusTheHun · 2023-06-15T17:40:51Z

The thing is that you can have a nullable PK. So we cannot assume that the provided value should be ignored.
However this is a breaking change indeed, so maybe we should check if the PK is nullable, if not, then use the last inserted id during teardown.
Can you update your PR in that direction ?

jtopenpetition · 2023-06-16T08:24:45Z

The thing is that you can have a nullable PK.

Actually, as far as I know, only sqlite allows this, and only due to a "historic oversight". SQL standard (at least since SQL-92 section 4.10.2) explicitly forbids nullable columns in primary keys in general (!!!). That is both single-column and multi-column primary keys.

So we cannot assume that the provided value should be ignored.

Well ... in general, we should, except for sqlite and some exceedingly non-standard databases perhaps.

However this is a breaking change indeed, so maybe we should check if the PK is nullable, if not, then use the last inserted id during teardown. Can you update your PR in that direction ?

To be frank, I would much prefer to just roll back #44, since it was the breaking change to begin with. It was the reason why we didn't upgrade to module-db 2.x.

I would rather have a flag (or a check for the sqlite driver being used) to allow this non-standard behavior, than adding an essentially unnecessary nullable-check for every insert (because schemas may change between inserts, caching is hard, etc.) - also, checks for column definitions/properties isn't implemented in module-db, so this would be from scratch.

side note: Relying on a nullable primary key is - to be honest - madness. I mean, sqlite even allows it to have multiple rows with null as primary key:

sqlite> .nullvalue NULL
sqlite> create table null_madness (id int primary key);
sqlite> insert into null_madness (id) values (null), (null);
sqlite> select * from null_madness;
NULL
NULL
sqlite> select * from null_madness where id = null;
sqlite> select * from null_madness where id is null;
NULL
NULL

why would anyone want this? Why wouldn't you have a nullable unique key instead? Because it's technically not a primary key, since you can't use it to identify every single row, if two rows have the same pk value. flip table

(side side note: interestingly enough ... int is supposed to be an alias for integer, however, the same code produces different results with integer instead of int, since "integer primary key" is meaning autoincrement and is also strict on the datatype and doesn't allow null)

JesusTheHun · 2023-06-16T10:57:30Z

src/Codeception/Module/Db.php

@@ -808,7 +808,9 @@ private function addInsertedRow(string $table, array $row, $id): void
 {
 $primaryKey = $this->_getDriver()->getPrimaryKey($table);
 $primary = [];
- if ($primaryKey !== []) {
+ if (count($primaryKey) == 1 && is_null($row[$primaryKey[0]])) {


Why not check for "all pk values are null" instead of checking the size is 1 and the single value is null ?

tbh, I'm actually not certain if auto-increment is even possible in a composite primary key. this might be database specific, again. I would shy away from setting anything, if the primary key had more than one (nulled) column. Someone with broader knowledge than me might know this ...

only size 1 and only if it's null seemed a very safe bet ;o)

addendum: this was also the behaviour before, but ... inverted, if there was an $id, it would be set on the primary key, which - as you pointed out - is actually dangerous, since the last insert id can return a non-primary auto-increment value and would override a proper primary key value.

JesusTheHun · 2023-06-16T10:57:44Z

Actually, as far as I know, only sqlite allows this

Alright, then we should fix this !

jtopenpetition changed the title ~~allow inserts with primary key nulled to be rolled back~~ [bugfix] #58 allow inserts with primary key nulled to be rolled back Jun 14, 2023

JesusTheHun reviewed Jun 16, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] #58 allow inserts with primary key nulled to be rolled back #57

[bugfix] #58 allow inserts with primary key nulled to be rolled back #57

jtopenpetition commented Jun 14, 2023 •

edited

JesusTheHun commented Jun 15, 2023

jtopenpetition commented Jun 16, 2023

JesusTheHun Jun 16, 2023

jtopenpetition Jun 16, 2023 •

edited

JesusTheHun commented Jun 16, 2023 •

edited

[bugfix] #58 allow inserts with primary key nulled to be rolled back #57

Are you sure you want to change the base?

[bugfix] #58 allow inserts with primary key nulled to be rolled back #57

Conversation

jtopenpetition commented Jun 14, 2023 • edited

JesusTheHun commented Jun 15, 2023

jtopenpetition commented Jun 16, 2023

JesusTheHun Jun 16, 2023

Choose a reason for hiding this comment

jtopenpetition Jun 16, 2023 • edited

Choose a reason for hiding this comment

JesusTheHun commented Jun 16, 2023 • edited

jtopenpetition commented Jun 14, 2023 •

edited

jtopenpetition Jun 16, 2023 •

edited

JesusTheHun commented Jun 16, 2023 •

edited