-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[bugfix] #58 allow inserts with primary key nulled to be rolled back #57
base: master
Are you sure you want to change the base?
[bugfix] #58 allow inserts with primary key nulled to be rolled back #57
Conversation
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
The thing is that you can have a nullable PK. So we cannot assume that the provided value should be ignored. |
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.
Well ... in general, we should, except for sqlite and some exceedingly non-standard databases perhaps.
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:
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) |
@@ -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]])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not check for "all pk values are null" instead of checking the size is 1 and the single value is null ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Alright, then we should fix this ! |
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