Fix last insert ID consistency across drivers by morozov · Pull Request #3074 · doctrine/dbal

morozov · 2018-03-31T18:35:04Z

Reopened #2765 from the personal repo (replaces #2648).

TODO

Why should the ID be a string, not an int? Because on a 32-bit system, a numeric ID will be represented as a string by the driver (e.g. mysqli). In a general case, an extremely large ID will not fit the int range anyways.
Simplify the test to have a better score on Scrutinizer.
Why should we return a zero when the last inserted ID is unavailable, e.g. why not NULL? Because some drivers return 0 when there's no ID. Besides that, for the caller, there's no difference between 0 and NULL, they both mean "none".
Why do we need to suppress errors? Let's suppress them for BC. Error reporting will be improved in Statement objects should throw exception in exceptional cases #3005.
Address code review concerns.

morozov · 2018-04-02T01:42:27Z

Looks like we have a connection leak in general and especially with PDO drivers. Here's the number of MySQL connections obtained from SHOW PROCESSLIST on each test teardown:

MySQLi	PDO MySQL
3	4
4	6
…	…
41	77

morozov · 2018-04-02T02:28:49Z

Almost there. Down to one failure on SQL Server.

tests/Doctrine/Tests/DBAL/Functional/LastInsertIdTest.php

…rever holes have been forgotten

morozov · 2018-04-03T07:09:50Z

It's green now, @Ocramius, @Majkl578.

Majkl578

I am not sure about the possible BC impact especially around those type changes (making everything string). :|

Majkl578 · 2018-04-03T14:38:57Z

lib/Doctrine/DBAL/Driver/LastInsertId.php

+        return $this->value;
+    }
+
+    public function register(string $value) : void


I'm not a fan of having this crate mutable.

We can make it immutable. Would it make more sense to rename this to something like with() : self in this case? What about get()? Should it be something like toValue() instead?

@Majkl578, sadly, the immutable approach will not just work since MysqliConnection and MysqliStatement share the same instance of LastInsertId. It's probably done like this to mitigate:

MySQL drivers reset the last insert ID to 0 after each non-insert query

It could be worked around by keeping the reference to the connection from the statement. But this approach stinks too (also implemented in PDO Statement/Connection pair).

Majkl578 · 2018-04-03T14:42:40Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

 class PDOConnection extends PDO implements Connection, ServerInfoAwareConnection
 {
+    /** @var string */
+    private $lastInsertId = '0';


This default value + doc block don't seem valid since you assign LastInsertId below.

Majkl578 · 2018-04-03T14:47:19Z

lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvStatement.php

-            $this->sql .= self::LAST_INSERT_ID_SQL;
-            $this->lastInsertId = $lastInsertId;
-        }
+        $this->lastInsertId = $lastInsertId;


Shouldn't this use Driver\LastInsertId too instead of Driver\SQLSrv\LastInsertId? For BC, Driver\SQLSrv\LastInsertId can exend Driver\LastInsertId and be deprecated.
[same for SQLSrvConnection]

Majkl578 · 2018-04-03T14:47:40Z

lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvStatement.php

+
+    private function trackLastInsertId() : void
+    {
+        if (! $this->lastInsertId) {


Majkl578 · 2018-04-03T14:48:28Z

lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvConnection.php

        }

-        return $stmt->fetchColumn();
+        return $stmt->fetchColumn() ?: '0';


I don't like?: here, it hides possible error values (would that be false, '0' or 0 here?) - is it possible to use !== false for example?

In the original proposal, lastInsertId() suppresses errors everywhere by design (see the OCI implementation specifically). Not sure I understand and agree with it. We definitely could improve here.

Majkl578 · 2018-04-03T14:52:04Z

lib/Doctrine/DBAL/Driver/SQLAnywhere/SQLAnywhereConnection.php

    {
        if (null === $name) {
-            return sasql_insert_id($this->connection);
+            return (string) sasql_insert_id($this->connection);


Looks BC break-ish, what returned false before would now return ''.

Majkl578 · 2018-04-03T14:52:19Z

lib/Doctrine/DBAL/Driver/SQLAnywhere/SQLAnywhereConnection.php

        }

-        return $this->query('SELECT ' . $name . '.CURRVAL')->fetchColumn();
+        return (string) $this->query('SELECT ' . $name . '.CURRVAL')->fetchColumn();


Looks BC break-ish, what returned false before would now return ''.

Majkl578 · 2018-04-03T14:58:30Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

+     *
+     * @throws \PDOException
+     */
+    public function trackLastInsertId() : void


The whole design decision behind this method is a bit shady - making subclasses override/disable this behavior looks a bit weird. Would love to see this extracted into some InsertIdStrategyTracking with multiple providers (no-op/identity/...).

Majkl578 · 2018-04-03T15:00:36Z

lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php

    {
        if ($name === null) {
-            return false;
+            return '0';


Looks BC break-ish, what returned false before would now return '0'.

Majkl578 · 2018-04-03T15:00:46Z

lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php

        }

-        return (int) $result;
+        return (string) $stmt->fetchColumn();


Looks BC break-ish, what returned 0 before would now return '0'.

…rever holes have been forgotten

morozov · 2018-04-03T17:36:23Z

Given the amount of remaining work which is entirely about the API and BC-related concerns, I propose to move it out of v2.7.0. The pressure of this ticket being a release blocker is demotivating and provocative to taking shortcuts.

morozov · 2018-04-03T17:53:42Z

lib/Doctrine/DBAL/Driver/Connection.php

     * Returns the ID of the last inserted row or sequence value.
     *
+     * If no sequence name is given, "0" is returned if there currently is no last insert ID available
+     * in the current session or in case of an error or if the driver does not support this operation.


@Majkl578 this is where the error suppression is explicitly defined. It looks dangerous to me that the return value may be different from what's expected because of a runtime issue.

Ocramius · 2018-04-03T17:56:09Z

@morozov agreed, pushing this back into 3.0.

…ed API

… with a hardcoded value

…handles per test)

…rever holes have been forgotten

morozov · 2018-04-18T22:31:38Z

I'm closing this as "won't fix" and not a bug. Below is the explanation of what exactly was attempted to get fixed and why it shouldn't.

Inconsistency in when the last inserted ID is available across platforms

This is what the largest part of the code change is about including the introduction of the LastInsertID class: MySQL "forgets" the ID right after the next query following the INSERT one, PostgreSQL "forgets" it after dropping the table, etc.

This issue is being fixed at the cost of creating either unnecessary coupling between the Statement and Connection objects (see MySQL and PDO for example), or code duplication (see SQL Server). At the same time, it allows the caller to be careless about capturing the last inserted ID any time and counting on it always being available. At the same time, if the caller is careless enough, there may be another INSERT happened between the original one and the moment of the capture, so this approach is error-prone by definition.

Instead of fixing this issue, we can explicitly recommend fetching the last insert ID immediately after INSERT. Ironically, Oracle which doesn't support identity columns, provides even better way of fetching the ID using the INSERT ... RETURNING id INTO :id syntax.

Inconsistency in what the last inserted ID looks like

Yes, there is an inconsistency between what an empty ID looks like (all kinds of empty values) and the non-empty ones (integer or a string). But it's natural for a weakly-typed language like PHP.

If the ID is empty, it can be reliably checked as if (!$lastInsertedID) without any knowledge of its type. Furthermore, without the proper error reporting (Statement objects should throw exception in exceptional cases #3005) the puristic approach of if (!$lastInsertedID === '0') is dangerous in the way that it doesn't account for potentially existing bug in the DBAL or underlying drivers (e.g. $lastInsertedID = false as a result of an error). Technically, any empty value means "none" regardless of its type.
If it's not empty, the most common use case is to use it in subsequent INSERT/UPDATE statements. At this point, the type of the ID doesn't matter as long as it can be bound to the statement.

Depite the fact that IDs are mostly integers in the DB internals, we cannot represent them as such due to potential overflow issues. At the same time, declaring them strings on the API level is misleading in the way that they are not just strings (e.g. "xyz"), they are strings representing integers, and PHP is fine with that.

Technically, we can re-iterate on the type consistency later, after #3005 is resolved but besides being theoretically correct, I don't see a practical value in it.

morozov mentioned this pull request Mar 31, 2018

#2648 fix last insert id consistency #2765

Closed

morozov force-pushed the fix-last-insert-id-consistency branch from 71b1239 to 547f921 Compare March 31, 2018 18:39

morozov self-assigned this Mar 31, 2018

morozov added this to the 2.7.0 milestone Mar 31, 2018

morozov added Bug Improvement Drivers labels Mar 31, 2018

morozov force-pushed the fix-last-insert-id-consistency branch from 547f921 to f9cf17a Compare April 2, 2018 01:35

morozov force-pushed the fix-last-insert-id-consistency branch from d335808 to 6fb67c9 Compare April 2, 2018 02:01

Ocramius reviewed Apr 2, 2018

View reviewed changes

tests/Doctrine/Tests/DBAL/Functional/LastInsertIdTest.php Show resolved Hide resolved

Ocramius added a commit to Ocramius/dbal that referenced this pull request Apr 2, 2018

doctrine#3074 doctrine#2765 Applying CS and adding lacking types whe…

e9daa7b

…rever holes have been forgotten

Ocramius mentioned this pull request Apr 2, 2018

#3074 #2765 Applying CS and adding lacking types wherever holes have been forgotten morozov/dbal#1

Merged

morozov force-pushed the fix-last-insert-id-consistency branch 4 times, most recently from 5746e9f to 8d1ebf7 Compare April 3, 2018 06:36

morozov requested review from Majkl578 and Ocramius April 3, 2018 07:09

Majkl578 reviewed Apr 3, 2018

View reviewed changes

Ocramius added a commit to Ocramius/dbal that referenced this pull request Apr 3, 2018

doctrine#3074 doctrine#2765 Applying CS and adding lacking types whe…

b05053c

…rever holes have been forgotten

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#3074 doctrine#2765 Applying CS and adding lacking types whe…

eb85408

…rever holes have been forgotten

morozov commented Apr 3, 2018

View reviewed changes

Ocramius modified the milestones: 2.7.0, 3.0.0 Apr 3, 2018

Ocramius unassigned morozov Apr 3, 2018

deeky666 and others added 15 commits April 17, 2018 09:28

use LastInsertId container object in PDO connection

a9ca3f8

refactor last insert ID extension points in PDO connection

a2f28aa

fix code style

b1c5757

[DBAL-2648] adding return hints and parameter hints to newly introduc…

ff7bdfd

…ed API

[DBAL-2648] adding type hints to the LastInsertId class

7952542

doctrine#2648 deprecated LAST_INSERT_ID_SQL class constant

5e089d2

doctrine#2648 removed nullability of the lastInsertId private property

0a51b6e

doctrine#2648 wrap ERRMODE_EXCEPTION restoring in a finally block

1acac6d

doctrine#2648 restoring original error mode instead of overwriting it…

1db5d66

… with a hardcoded value

[DBAL-2648] Renamed set() to register()

65ba2ab

[DBAL-2648] Replaced duplicated code with variadic argument

959e73f

[DBAL-2648] Fixed failures on PDO drivers

04d481e

[DBAL-2648] Close temporary connection to eliminate the leak (1 of 2 …

5b415be

…handles per test)

doctrine#3074 doctrine#2765 Applying CS and adding lacking types whe…

46371ea

…rever holes have been forgotten

[DBAL-2648] Trying to decrease complexity of the test

0ea7162

morozov force-pushed the fix-last-insert-id-consistency branch 2 times, most recently from 9fa28d2 to 007c327 Compare April 17, 2018 17:06

morozov added 2 commits April 17, 2018 21:18

[DBAL-2648] Generalized the test

2f51416

[DBAL-2648] Addressed some Scrutinizer issues

6894505

morozov force-pushed the fix-last-insert-id-consistency branch from 007c327 to 6894505 Compare April 18, 2018 04:34

morozov closed this Apr 18, 2018

morozov added Won't Fix and removed Bug labels Apr 18, 2018

morozov deleted the fix-last-insert-id-consistency branch May 27, 2018 18:54

morozov mentioned this pull request Nov 26, 2018

DBAL 3.0: suggestions to improve the Driver\Connection interface #3335

Closed

morozov mentioned this pull request Dec 5, 2019

Extract getSequenceNumber() from lastInsertId() #3759

Closed

morozov mentioned this pull request Oct 6, 2020

Throw exception from lastInsertId() if there is no identity value #4324

Merged

morozov removed this from the 4.0.0 milestone Nov 17, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jul 26, 2022

Uh oh!

Conversation

morozov commented Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

morozov commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morozov commented Apr 2, 2018

Uh oh!

Uh oh!

morozov commented Apr 3, 2018

Uh oh!

Majkl578 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ocramius commented Apr 3, 2018

Uh oh!

morozov commented Apr 18, 2018

Inconsistency in when the last inserted ID is available across platforms

Inconsistency in what the last inserted ID looks like

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

morozov commented Mar 31, 2018 •

edited

Loading

morozov commented Apr 2, 2018 •

edited

Loading