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

sarven commented 5 years ago

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 commented 5 years ago

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

sarven commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

morozov commented 5 years ago

I can't.

warslett commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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
``

doctrine / dbal