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

[jtopenpetition]

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

[JesusTheHun]

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]

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]

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

Alright, then we should fix this !