Invalid last inserted id when the table has a trigger with insert statements in SQLSrvConnection

[sarven]

Bug Report

Q	A
BC Break	yes
Version	2.9.0

Summary

I discovered a problem when the table in MSSQL has a trigger with insert statements. Id returned by method lastInsertId is invalid. It isn't id from the table in where we were inserting data, but from the table that trigger was inserting data.

Current behaviour

Like I wrote in previous paragraph, currently when we have the table with a trigger inserting some data in another table, returned last Inserted Id is related with another table, not the main one that we expect.

Expected behaviour

Returned last inserted ID should be related to the main insert statement, not that executed in a trigger.

[Ocramius]

@sarven is this even achievable with the current MSSQL clients? What extension are you using to connect to the DB?

[sarven]

@Ocramius Yes, I have current MSSQL clients and PHP 7.3. I added draft PR with a simple test for this issue, I will try to make proper implementation to fix it.

[arnegroskurth]

I ran into the same problem and as long as it is not fixed in DBAL I helped myself with a workaround:

Create a dummy table and extend your trigger to write the original id into this table at the end of the trigger execution:

CREATE TABLE [fake_last_inserted_id] ([id] INT IDENTITY(1, 1) PRIMARY KEY);

CREATE TRIGGER [my_trigger] ON [some_table] FOR INSERT AS BEGIN
    
   -- actual trigger logic...

   -- fake last inserted id
    SET IDENTITY_INSERT [fake_last_inserted_id] ON;
    DELETE FROM [fake_last_inserted_id]; -- prevent collision
    INSERT INTO [fake_last_inserted_id] ([id]) VALUES ([inserted].[id]); -- adjust id column name
    SET IDENTITY_INSERT [fake_last_inserted_id] OFF;
END;

[sarven]

@arnegroskurth I think that I created the proper solution for this problem, but it is still waiting for review. In my situation extending and modifying triggers/tables are not good practice, because I work with the database that is using by a foreign application. So currently after insert I just fetch IDENT_CURRENT. It isn't good, but works.

[mherderich]

stumbled upon this issue. i guess following change broke it: cadd79c prior to this SCOPE_IDENTITY was used as following:

;SELECT SCOPE_IDENTITY() AS LastInsertId;

now @@IDENTITY is used but there are well documented differences: https://docs.microsoft.com/de-de/sql/t-sql/functions/identity-transact-sql?view=sql-server-2017

@@IDENTITY and SCOPE_IDENTITY return the last identity value generated in any table in the current session. However, SCOPE_IDENTITY returns the value only within the current scope; @@IDENTITY is not limited to a specific scope.

[sarven]

@mherderich I checked that. It's not the problem. Without this change it will return null. Maybe it is better than returning an invalid id, but still it isn't satisfying.

[mherderich]

please see #3293 too. there is a major difference between scope_identity and @@identity.

Without this change it will return null.

i reverted this change and works for me. the actual autoincrement ID is returned. perhaps just selecting SELECT_IDENTITY() instead of @@IDENTITY might work too:

$stmt = $this->query('SELECT SCOPE_IDENTITY()');

was this break (@@IDENTITY vs SCOPE_IDENTITY()) intended?

[morozov]

IIRC, there wasn't any specific reason to introduce this change in #2617 other than fixing some existing test failures in AppVeyor. Please file a pull request with a failing test and we'll see how the change affects the existing ones.

[mherderich]

can you provide more info which test(s) failed prior to this change so we can get some background knowledge?

[morozov]

I can't.

[warslett]

@morozov @mhderderich I've been learning about this issue over the last few days. We are experiencing this issue specifically when the database is under a high load of traffic. The PR that @sarven has created deals with the erroneous call to @@IDENTITY but it deals with a further problem he is having where you have triggers that don't SET NOCOUNT ON. The reading I have been doing online indicates that it is good practice to always SET NOCOUNT ON in triggers https://dba.stackexchange.com/questions/21148/should-i-add-set-nocount-on-to-all-my-triggers All our generated replication triggers SET NOCOUNT ON. In my opinion:

• Doctrine should support MSSQL databases that have insert triggers on tables (triggers are required for replication processes)
• Doctrine should not need to deal with the scenario where a table has insert triggers that don't SET NOCOUNT ON. In this scenario the trigger should be altered to SET NOCOUNT ON as this is a safer way of using triggers in MS SQL. We should document this problem along with this recommended solution.

The testcase that should be satisfied is the following. Note SET NOCOUNT ON in the test trigger.

    /**
     * @group DBAL-3516
     */
    public function testLastInsertIdRelatedToTheMainInsertStatement() : void
    {
        if ($this->connection->getDatabasePlatform()->getName() !== 'mssql') {
            $this->markTestSkipped('Currently restricted to MSSQL');
        }

        $this->connection->query('CREATE TABLE dbal3516(id INT IDENTITY, test int);');
        $this->connection->query('CREATE TABLE dbal3516help(id INT IDENTITY, test int);');

        $this->connection->query(<<<SQL
CREATE TRIGGER dbal3516_after_insert ON dbal3516 AFTER INSERT AS
    SET NOCOUNT ON
    DECLARE @i INT = 0;
    
    WHILE @i < 3
    BEGIN
        INSERT INTO dbal3516help (test) VALUES (1);
        SET @i = @i + 1;
    END;
SQL
        );

        $this->connection->insert('dbal3516help', ['test' => 1]);
        $this->connection->lastInsertId();
        $this->connection->insert('dbal3516', ['test' => 1]);
        $this->assertEquals(1, $this->connection->lastInsertId());
    }

I have setup this test locally and reverted cadd79c and this makes the test pass. I will put this up as a PR when I get a moment and then take a look at any AppVeyor issues emerge.

[warslett]

New PR: https://github.com/doctrine/dbal/pull/3880 Here is the test that fails in AppVeyor:

There was 1 failure:
1) Doctrine\Tests\DBAL\Functional\WriteTest::testEmptyIdentityInsert
Failed asserting that '1' is greater than '1'.
C:\projects\dbal\tests\Doctrine\Tests\DBAL\Functional\WriteTest.php:285
FAILURES!
Tests: 3643, Assertions: 5807, Failures: 1, Skipped: 609, Incomplete: 6.

I will investigate what this test is doing and why it is failing.

[warslett]

also error in PHPStan. Maybe we should throw a Doctrine Exception if we can't get the ID?

$ vendor/bin/phpstan analyse
Note: Using configuration file /home/travis/build/doctrine/dbal/phpstan.neon.dist.
 459/459 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
 ------ --------------------------------------------------------------------- 
  Line   lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvConnection.php                 
 ------ --------------------------------------------------------------------- 
  103    Method Doctrine\DBAL\Driver\SQLSrv\SQLSrvConnection::lastInsertId()  
         should return string but returns string|null.                        
 ------ --------------------------------------------------------------------- 
                                                                                
 [ERROR] Found 1 error                                                          
                                                                                
The command "vendor/bin/phpstan analyse" exited with 1.
cache.2
store build cache
``

[JochemKlingeler]

EDIT: Do not use this IdentityGenerator hack, IDENT_CURRENT is not safe to use in scenario's where records are inserted in parallel. We considered using this hack with SCOPE_IDENT() instead, but sadly it gets called too late and the inserted record will be out of scope. SCOPE_IDENT() will always return null when using the IdentityGenerator method below.

A better solution is to introduce an UUID column which is generated in PHP as Doctrine then does not have to fetch the ID afterwards. Or what we decided to do, simply remove trigger and move that logic into PHP.

Original message below:

---

We recently ran into this issue and we fixed it by using a custom IdentityGenerator and implementing it on the affected entities. Hopefully this will be of use to someone encountering this issue:

use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Id\AssignedGenerator;
use Doctrine\ORM\Id\IdentityGenerator;
use Webmozart\Assert\Assert;
class TableWithInsertingTriggerIdentityGenerator extends IdentityGenerator
{
    public function generateId(EntityManagerInterface $em, object|null $entity): int
    {
        /**
         * Not sure in what cases a null gets passed, but the interface expects it.
         * The AssignedGenerator for example expects the entity to always be given, so maybe we should
         * also just assert that it needs to be there.
         * @see AssignedGenerator
         */
        Assert::object($entity);

        $metaData = $em->getClassMetadata($entity::class);
        $lastInsertId = $em->getConnection()->fetchOne('SELECT IDENT_CURRENT(:table)', ['table' => $metaData->getTableName()]);
        if (false === $lastInsertId) {
            throw NoIdentityValueException::new();
        }
        Assert::numeric($lastInsertId, 'Expected generated ID to be an integer! Got: %s');
        return (int)$lastInsertId;
    }
}

Then you can enable the custom generator in your entity by declaring it like this:

#[Id]
#[Column(name: 'ID', type: 'integer', insertable: false, updatable: false)]
#[GeneratedValue(strategy: 'CUSTOM')]
#[CustomIdGenerator(class: TableWithInsertingTriggerIdentityGenerator::class)]
private int $id;

Note the insertable and updatable properties, they are required to be set to FALSE, else Doctrine will add the id to the insert/update query.

The code above works when used with doctrine/orm 3.5.2.

[Ocramius]

@JochemKlingeler how does that SELECT work under high concurrency scenarios? Safe to use?

[JochemKlingeler]

@JochemKlingeler how does that SELECT work under high concurrency scenarios? Safe to use?

It is not, it's very much a hack and it's use should be discouraged. See edit.