Framework: Create method on model returning wrong primary key value (SQLSRV)

[kadevland]

• Laravel Version: 7
• PHP Version: 7.4
• Database Driver & Version: PDP_SQLSRV

Description:

When creating records of a model using create the returned record will have a wrong value for primary key if the table has a trigger on it which itself creates (inserts) a record in another table.

Steps To Reproduce:

Create a table (with an auto incrementing primary key) and add a trigger to it which inserts another record in another table). Then calling the create method on the model will return the just created record with the wrong value for the primary key field.

Note

it is the same issue as #27452, close for lack of activity but the problem is still valid

The fix staudenmeir@b4a8f81 solves the problem but is not integrated into Laravel.

[joelharkes]

@kadevland i dont think this is a laravel bug.

it seems more like a mssql bug: https://stackoverflow.com/questions/15883304/stop-trigger-changing-scope-identity

Its php Doctrine package that adds: ;SELECT SCOPE_IDENTITY() AS LastInsertId; to the query for SqlServer when query starts with 'INSERT INTO '

see class: Doctrine\DBAL\Driver\SQLSrv\SQLSrvStatement

[joelharkes]

@kadevland I would advise using Laravel Eloquent events instead of database triggers for your logic.

[kadevland]

@kadevland i dont think this is a laravel bug.

it seems more like a mssql bug: https://stackoverflow.com/questions/15883304/stop-trigger-changing-scope-identity

Its php Doctrine package that adds: ;SELECT SCOPE_IDENTITY() AS LastInsertId; to the query for SqlServer when query starts with 'INSERT INTO '

see class: Doctrine\DBAL\Driver\SQLSrv\SQLSrvStatement

You see Doctrine fix by append but laravel not append so that my purpose of issu.
It's Bug of Eloquent can not return write value of ID ( doctrine can, and if do the same in eleoquent )

You can't not replace trigger logical by php.

It possible to integrete same logical witch use in doctrice and all ready use un driver pgsql in laravel

for exemple :

Illuminate\Database\QueryGrammars\PostgresGrammar

 public function compileInsertGetId(Builder $query, $values, $sequence)
    {
        return $this->compileInsert($query, $values).' returning '.$this->wrap($sequence ?: 'id');
    }

Illuminate\Database\Query\Processors\PostgresProcessor

 public function processInsertGetId(Builder $query, $sql, $values, $sequence = null)
    {
        $result = $query->getConnection()->selectFromWriteConnection($sql, $values)[0];

        $sequence = $sequence ?: 'id';

        $id = is_object($result) ? $result->{$sequence} : $result[$sequence];

        return is_numeric($id) ? (int) $id : $id;
    }

[taylorotwell]

Anyone is welcome to PR a fix. I do not use SQL Server nor can I test it easily.

[joelharkes]

@taylorotwell Can i add a docker-compose file to test sql-server (Unit)Tests and can this be integrated in the CI build process or not possible?

[kadevland]

@joelharkes i can post DDL table and trigger, make the fixe on sqlsrvGrammar and procces
but for CI/test i'm not good at all

[joelharkes]

I Got a working docker-compose setup for SQL server unit tests.

Now I have to add the test case and discuss with Laravel team how/where to put my files.

It got:

• dockerfile to make php with SQL server drivers
• compose file
• docker entry point file to wait for host & port
• PHP script to create a database in SQL server (it starts without database)

Where do I put these files?
What do I do with phpunit tests that should only be executed in this container?

[joelharkes]

update: I've got a working unit test, working on fix this week.

[joelharkes]

Got a fix working on finalizing PR.

[kadevland]

THX.
When you finalizing PR, i will test on my project

[joelharkes]

@kadevland fixed it on master, lets see what they say :) you can upvote the PR if you like.

[driesvints]

https://github.com/laravel/framework/pull/32957 was closed pending inactivity but we welcome any help with resurrecting it.

[driesvints]

Fixed by @rodrigopedra in https://github.com/laravel/framework/pull/33430

Thanks!

[kadevland]

thx @rodrigopedra

[rodrigopedra]

You're welcome!

[driesvints]

Re-opened because we had to revert the fix. It caused failures on certain drivers. See https://github.com/laravel/framework/pull/33496

[driesvints]

When doing a new attempt we should keep in mind https://github.com/laravel/framework/issues/33485

[stevebauman]

I've dealt with this in my own production environment, and it was an issue caused by inserting another record during an INSERT trigger, since SCOPE_IDENTITY will return the lastly inserted ID in execution the scope.

https://github.com/microsoft/msphpsql/issues/288#issuecomment-429909285

Resource:

https://docs.microsoft.com/en-us/sql/t-sql/functions/scope-identity-transact-sql?view=sql-server-2017

The fix was to reset the SCOPE_IDENTITY on the INSERT trigger, which can be done like so:

-- The trigger
ALTER TRIGGER [Inventory].[OnInsertTriggerHistory] ON [Inventory].[Inventory] 
FOR INSERT AS

SET NOCOUNT ON

-- The child table insert
INSERT INTO [Inventory].[InventoryHistory] (...) VALUES (...)

-- Reset the `SCOPE_IDENTITY()` by performing an insert into a temp table.
IF (SELECT COUNT(1) from inserted) = 1 BEGIN  
    CREATE TABLE #TempIdTable (ResetIdentity int identity(1, 1))

    SET identity_insert #TempIdTable ON
        INSERT INTO #TempIdTable (ResetIdentity)
        SELECT InventoryId FROM inserted SET identity_insert #TempIdTable OFF
END

-- Drop the temp table if it exists.
IF OBJECT_ID('tempdb..#TempIdTable') IS NOT NULL DROP TABLE #TempIdTable

I'm skeptical on calling this an Laravel issue...

[rodrigopedra]

For some projects the application developer does not have accesses to change database triggers.

As noted by another comment on the thread @stevebauman linked, adding the SET NO COUNT before the INSERT, and adding the SELECT SCOPE_IDENTITY to the same clause makes PDO to return the expected last inserted ID:

References:

https://github.com/microsoft/msphpsql/issues/288#issuecomment-352672920
And https://github.com/microsoft/msphpsql/issues/288#issuecomment-352814829

I agree with @stevebauman that this is not primarily a Laravel issue. But as this can be "workaround-ed" I would add it to Laravel.

Issue is allowing any driver like the freeTDS one. If we restrict SQL Server drivers to the official one from Microsoft, fixes introduced by PR #33430 would work.

Reason I reverted that PR is because it was targeted to v6.x (LTS) and v7.x that broke some apps that used the unofficial freeTDS driver.

But I guess that for a newer version (8 maybe) we can add a requirement to only support Microsoft official drivers.

[stevebauman]

Amazing work on that PR @rodrigopedra! 🙌

Is there any possibility we could have a flag inside the developers sqlsrv configuration that enables / swaps compatibility with FreeTDS, while the MSSQL driver is supported by default?

We would of course have to create separate grammars or grammar methods to compile the insert statement. But then instead of dropping support completely, users can set the flag to true (maybe 'freetds' => true).

What are your thoughts?

Since we have access to the configuration in the SqlServerConnection we could, we could retrieve this option and apply it to the Grammar instance:

https://github.com/laravel/framework/blob/d3d36f059ef1c56e17d8e434e9fd3dfd6cbe6e53/src/Illuminate/Database/SqlServerConnection.php#L55-L63

Example:

/**
 * Get the default query grammar instance.
 *
 * @return \Illuminate\Database\Query\Grammars\SqlServerGrammar
 */
protected function getDefaultQueryGrammar()
{
    $grammar = new QueryGrammar;

    if ($this->getConfig('freetds') === true) {
        $grammar->usingFreeTds();
    }

    return $this->withTablePrefix($grammar);
}

Or, even better, if we have the ability to detect which Sqlsrv driver is being used, then can enable the Grammar option without breaking backwards compatibility. Not sure if this is doable though...

[rodrigopedra]

Thanks @stevebauman !

One of the suggestions I made in here:

https://github.com/laravel/framework/pull/33430#commitcomment-40493561

Was having some feature detection in place for that.

Laravel already does that for the ODBC driver:

https://github.com/laravel/framework/blob/97c8d87991672604e22f2a7c05e717ea2743aa0b/src/Illuminate/Database/Query/Processors/SqlServerProcessor.php#L26-L30

But I fear adding more branches to that if should be a reason to drop SQL Server support altogether.

I still think that adding the requirement to use the official driver is a better way to go, as it would simplify code and make maintenance easier. Support for alternative drivers could be added by third-party packages.

[driesvints]

How much drivers are there for SQL Server and how popular are they?

[stevebauman]

@driesvints All I know of is the official SQL Server drivers and FreeTDS, but I haven't touched FreeTDS since PHP 5.4. Unsure why anyone would use FreeTDS over the official drivers to be honest, maybe for legacy Ubuntu servers?

@rodrigopedra You're right, if it gets too complicated then it's probably best to support only the official SQL Server driver from Microsoft. However, if it's only one additional if statement to offer a fix and backwards compatibility, I'm not sure if that's a large maintenance overhead (imho).

[driesvints]

@stevebauman well, if it's really one if statement I suspect we would have resolved this issue much sooner. I also don't think it's reasonable to support every driver out there. But I want to figure out how much there are and how popular they are first.

[stevebauman]

Apologies @driesvints, I wasn't intending to oversimplify the issue. I was referring to the need to extract both the PR's logic, and the in-place working logic (that is already working with FreeTDS) into two separate method calls and adding an if to simply check if the server is using the FreeTDS driver, or the Microsoft SQL Driver -- then execute the compatible insert.

[driesvints]

No need to apologize :)

[taylorotwell]

Since this issue has only been raised by a single person and @stevebauman has since posted a fix in the trigger itself (no response from OP) I'm going to close this.