Skip to content
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

Create method on model returning wrong primary key value #27452

Closed
amestsantim opened this issue Feb 8, 2019 · 21 comments
Closed

Create method on model returning wrong primary key value #27452

amestsantim opened this issue Feb 8, 2019 · 21 comments

Comments

@amestsantim
Copy link

  • Laravel Version: 5.7.20
  • PHP Version: 7.2.10
  • Database Driver & Version:
    DriverName => "libmsodbcsql-17.2.so.0.1",
    DriverODBCVer => "03.52",
    DriverVer => "17.02.0000",
    ExtensionVer => "5.3.0",

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.
In effect, the returned primary key value is the value of the record inserted by the trigger and not of the just created record of the model. Same behavior is observed if fresh() method is chained on the create method.

I suspect this is happening because the driver is using @@IDENTITY or SCOPE_IDENTITY whereas IDENT_CURRENT( 'table_name' ) should have been used to get the LastInsertId after the create step.
Here is the relevant section from the SQL Server 2017 documentation (https://docs.microsoft.com/en-us/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.
IDENT_CURRENT is not limited by scope and session; it is limited to a specified table. IDENT_CURRENT returns the identity value generated for a specific table in any session and any scope. "

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.

@amestsantim
Copy link
Author

I believe the relevant code in the source is on line 46 of Illuminate/Database/Query/Processors/SqlServerProcessor.php

'SELECT CAST(COALESCE(SCOPE_IDENTITY(), @@IDENTITY) AS int) AS insertid'

@staudenmeir
Copy link
Contributor

staudenmeir commented Feb 8, 2019

I can reproduce the issue, but I don't fully understand it.

When I test it directly in the database, SCOPE_IDENTITY() returns the correct primary key. In Laravel however, SCOPE_IDENTITY() returns null and so I receive the incorrect result of @@IDENTITY.

The issue with IDENT_CURRENT() is that it returns the last identity value generated in any session. So theoretically, a different connection could insert a new row between the INSERT and the SELECT IDENT_CURRENT() query. Then you would also get an incorrect value.

@darksaboteur You added the code in #13423, can you take a look at this?

@darksaboteur
Copy link
Contributor

I have encountered this issue too when using Merge Replication.
I actually forgot I hadn't submitted my patches for this already.

I've thrown together a branch darksaboteur@0c5a0e2 with my fixes copied to it for you to look at. I don't have time to test it now, so there may be some syntax errors or other trivial issues.

Please test it and let me know if it fixes triggers too so I can create a Merge Request for it.

@staudenmeir
Copy link
Contributor

Here is a shorter version of @darksaboteur's fix: staudenmeir@b4a8f81

@amestsantim Does it work for you?

@darksaboteur Now that SCOPE_IDENTITY() works, do we still need COALESCE(SCOPE_IDENTITY(), @@IDENTITY)? Shouldn't SCOPE_IDENTITY() cover all cases?

@driesvints
Copy link
Member

Closing pending author inactivity. Feel free to reply if this is still an issue.

@amestsantim
Copy link
Author

I'm not sure I understand why this issue is being closed. Has the bug been fixed in laravel/framework?
If so, great but if not, by closing the issue, we're just kicking the ball down to the next person who will create a new issue for it.

@driesvints
Copy link
Member

@amestsantim I closed it because you didn't reply anymore. Can you please check the answers by @darksaboteur and @staudenmeir and answer their questions?

@driesvints driesvints reopened this Mar 29, 2019
@amestsantim
Copy link
Author

Alright, give me a few days to patch my installation with @staudenmeir 's fix and test it.

@driesvints
Copy link
Member

Closing this issue because it's inactive, already solved, old or not relevant anymore. Feel free to reply if you're still experiencing this issue.

@skalero01
Copy link

skalero01 commented Aug 4, 2019

I am experience the same issue. Using laravel 5.8 and php 7.3 @driesvints

To reproduce

Create a table

CREATE TABLE `seasons` (
  `season_id` int(4) NOT NULL,
  `name` varchar(30) DEFAULT NULL,
  `deleted_at` timestamp NULL DEFAULT NULL,
  `created_at` timestamp NULL DEFAULT NULL,
  `updated_at` timestamp NULL DEFAULT NULL,
  PRIMARY KEY (`season_id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

Create a model Season.php

namespace App;

use Illuminate\Database\Eloquent\Model;

class Season extends Model
{
    protected $table = 'seasons';
    protected $primaryKey = 'season_id';
    protected $guarded = [];
}

On tinker execute App\Season::create(['season_id'=>2018, 'name' => 'Season 2018']).

That returns the next message

App\Season {#4086
     season_id: 0,
     name: "Season 2018",
     updated_at: "2019-08-03 20:48:40",
     created_at: "2019-08-03 20:48:40",
   }

My question is.. Why isn't returning the season_id correctly? On the database the season_id was saved correctly.

@staudenmeir
Copy link
Contributor

@skalero01 That's a different issue. You need to set public $incrementing = false; in your model.

@skalero01
Copy link

Thanks @staudenmeir! it worked :)

@humblecoder
Copy link

@staudenmeir @skalero01 I need to make use of id immediately after create(). Setting $incrementing = false breaks that ability. For example:

$u = User::create(...);
$c = Car::create([
 'name' => 'blah',
 'user_id' => $u->id
]);

In this example, $u->id is actually null -- not even $u->fresh() works. However, if I leave $incrementing = true then I begin to receive incorrect values for $u->id. This is borne out by trying to use DB::getPdo()->lastInsertId() which also reports the wrong value and eventually becomes more and more incorrect.

NOTE: I'm sure there is a "cause" for the value getting off, but I have no earthly idea what it is.
Currently, I mitigate the problem by re-querying for the object based on identifiable traits, but this obviously isn't the fix.

@skalero01
Copy link

@humblecoder have you tried using the fresh function?

$u = User::create(...);
$u = $u->fresh();
$c = Car::create([
 'name' => 'blah',
 'user_id' => $u->id
]);

@humblecoder
Copy link

@skalero01 I have. It returns null. My guess is due to the fact that it attempts a lookup based on a bad id.

I need to do some digging into what makes the PDO tick. Is it possible to reset or "re-calibrate" the results of DB::getPdo()->lastInsertId()? 🤔 It's very strange that it gets "off track" and never recovers.

@staudenmeir
Copy link
Contributor

I begin to receive incorrect values for $u->id

@humblecoder What does "incorrect" mean? Are you also using SQL Server?

@humblecoder
Copy link

humblecoder commented Nov 7, 2019

@staudenmeir "incorrect" means the number slowly creeps off track. It's as if it has its own auto-increment mechanism. It has gotten as high as +4 off and seems it would keep climbing if I didn't manually address the issue.

Yes, I'm also using SQL Server.

@Serhioromano
Copy link

Serhioromano commented Feb 28, 2020

I have the same issue. I had tables and inserted thee some users but I had problems with constraint. I created a dump and deleted all the tables. Then I edited dump (MySQL) and change all AUTOINCREMENT to 1 so that tables start ёшвё from 1. Created DB again.

Now when I insert user it inserts it into DB with id 1 but User::create returns id 17.

Update

After research, I found out that I use listen to log all my SQL queries. Most probably lastInsertId returns Id of log entry.

How to avoid this?

@kadevland
Copy link

kadevland commented May 18, 2020

Can reopen this issu.
I have the same problem
I have many table with trigger so model don't take the good primary key value.
I use SQL server

@peter-olom
Copy link

This is still an issue in 2021

$post = Post::create([ 'author_id' => $request->author, 'series_id' => $request->series, 'content' => $request->content, ]);
After calling create method, $post->id will have a wrong value.

@rodrigopedra
Copy link
Contributor

Hey @peter-olom please see these comments on issue #32883 where you posted the same comment above:

#32883 (comment)

#32883 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants