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

Duplicate entries on updateOrCreate #19372

Closed
chimit opened this issue May 28, 2017 · 47 comments
Closed

Duplicate entries on updateOrCreate #19372

chimit opened this issue May 28, 2017 · 47 comments

Comments

@chimit
Copy link
Contributor

chimit commented May 28, 2017

  • Laravel Version: 5.4
  • PHP Version: 7.1
  • Database Driver & Version: MySQL 5.7

Description:

I want to save mobile devices information in the database using their UUIDs. The uuid field has a unique index. The code is:

$device = Device::updateOrCreate(
    ['uuid' => $request->input('uuid')],
    [
        'info' => $request->input('info'),
    ]
);

But I'm getting a lot of errors in logs:

Next Doctrine\DBAL\Driver\PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'aa18269c-cf32-4c6b-b450-e84a5dba0a0c' for key 'devices_device_uuid_unique' in /var/www/api/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOStatement.php:93
Stack trace:

I also tried to use firstOrNew:

$device = Device::firstOrNew(['uuid' => $request->input('uuid')]);
$device->info = $request->input('info');
$device->save();

But it's equal I guess.

If I remove the unique index from the uuid field the problem is gone but I get duplicate entries. That's strange because I thought updateOrCreate method guarantees uniqueness of the entries.

Is it a bug of the updateOrCreate method?

@Kyslik
Copy link
Contributor

Kyslik commented May 28, 2017

Whats your database structure, share migration for the table.

@srmklive
Copy link
Contributor

Can you share your database structure or migration file for the table?

@chimit
Copy link
Contributor Author

chimit commented May 29, 2017

Schema::create('devices', function (Blueprint $table) {
    $table->increments('id');
    $table->integer('user_id')->unsigned()->nullable();
    $table->string('uuid')->unique();
    $table->timestamps();
});

@themsaid
Copy link
Member

themsaid commented May 29, 2017

Since it happens on random occasions I believe it might be a race condition with the record is being created via another request in between the check and insert inside updateOrCreate().

@Reaper45
Copy link

Reaper45 commented Jun 6, 2017

Well. I think this is a Laravel mass assignment Issue.
When using the Eloquent create method, you will need to specify either a fillable or guarded attribute on the model, as all Eloquent models protect against mass-assignment by default. In your case @chimit: add this to your device.php model

public $fillable = ['uuid', 'info'];

I put the solution code here.

@chimit
Copy link
Contributor Author

chimit commented Jun 7, 2017

Hi @Reaper45!
I already have it in my Device model otherwise I would get Mass Assignment exceptions. The problem is that updateOrCreate method doesn't really guarantee what it was created for. Maybe this method should be improved, for example, using transactions.

@themsaid
Copy link
Member

updateOrCreate method doesn't really guarantee what it was created for. Maybe this method should be improved, for example, using transactions.

Feel free to open a PR if you have a better idea, meanwhile I'm closing this since it's not actually a bug.

@alessandrolattao
Copy link

alessandrolattao commented Jul 24, 2017

I am having the exact same problem.
If at least two requests happen simultaneously, updateOrCreate does not behave correctly.
You can easily reproduce the problem by using ab (apache bench) by using this command: ab -n 2 -c 2 http://your.url.here (I used a GET request for testing purpose).
This generates two simultaneous requests to the "update url" and lead eloquent to generate two insert queries.

@lk77
Copy link

lk77 commented Jul 24, 2017

i think you could use lock to prevent simultaneous calls.


Device::where(['uuid'` => $request->input('uuid')]->lock(true);
//your code
Device::where(['uuid' => $request->input('uuid')]->lock(false); 

or something like that i've not tested it.

@alessandrolattao
Copy link

alessandrolattao commented Jul 24, 2017

Yes, I could.
But that would force me to add locks everywhere to handle updateOrCreate problems.
I think the function should be atomic. :)

@rajjanorkar
Copy link

this is still problem.

@didioh
Copy link

didioh commented Apr 23, 2018

I solved this problem by overriding Illuminate\Database\Eloquent\Model and creating a custom firstOrCreate method that returns the model if there is a duplicate error exception. This should work for mySQL connections.

class SystemModel extends Illuminate\Database\Eloquent\Model
{
	/**
	* Check if Illuminate\Database\QueryException is Duplicate Entry Exception.
	*
	* @param  array  $attributes
	* @return \Illuminate\Database\Eloquent\Model
	*/
	protected function isDuplicateEntryException(Illuminate\Database\QueryException $e){
		$errorCode  = $e->errorInfo[1];
		if ($errorCode === 1062) { // Duplicate Entry error code
			return true;
		}
		return false;
	}

	/**
	* Get the first record matching the attributes or create it.
	*
	* @param  array  $attributes
	* @param  array  $values
	* @return \Illuminate\Database\Eloquent\Model
	*/
	public static function firstOrCreate(array $attributes, array $values = [])
	{
		try {    		
			$static = (new static);
			return $static->create($attributes + $values);
		} catch (Illuminate\Database\QueryException $e){
			if($static->isDuplicateEntryException($e)) {
				return $static->where($attributes)->first();
			}
		}
	}
}

@paulrrogers
Copy link

Please reopen this ticket. I'd agree with other commenters that this is an actual bug since it is surprising behavior for any moderately busy service where collisions are possible. While the implementation does not concern me the syntax implies it is somehow atomic.

This could be achieved in a variety of ways:

  • upsert (syntax varies by DBMS)
  • pessimistic locks ("SELECT ... FOR UPDATE")
  • database transactions with at least one auto-retry
  • partial mitigation with updated_at columns (typically needs sub-second precision).

In my opinion firstOrCreate also has this weakness and likely any ...OrCreate method.

All that said, I can see why the ...OrNew methods cannot be atomic since they only return unsaved, dirty objects. The others do not have that restriction.

@cg-rsands
Copy link

@themsaid this really is a bug. Race conditions are a big problem with updateOrCreate. We are fighting this at present, upsert is a safe method with mysql

@alessandro-aglietti
Copy link

Manage race conditions manually when using updateOrCreate with an ORM in 21st century?? C'mon!

@themsaid
Copy link
Member

themsaid commented May 7, 2018

Guys, please open a PR if you have a different approach that works on all supported database engines :) PRs are welcomed.

@aligajani
Copy link

This is still a problem, albeit occurs very rarely.

@msdavid1296
Copy link

msdavid1296 commented Jul 30, 2018

Still, I am getting the same issue.

(3/3) QueryException
SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '344b879b-279d-f8e2-01e2-59083ac6f538' for key 'employee_id_unique' (SQL: insert into employee (id, title, first_name, last_name, office_phone, department, mobile, fax, primary_address, city, state, country, postal_code, email, back_account, citizenship, dob, doe, gender, salary, type_of_employeement, employee_no, is_sync, updated_at, created_at) values (344b879b-279d-f8e2-01e2-59083ac6f538, , Vitali, Berby, , , , , , , , , , , , , , , male, , , , 1, 2018-07-30 03:48:35, 2018-07-30 03:48:35))

@flexgrip
Copy link

flexgrip commented Aug 9, 2018

I too am having this issue. For now, the only solution seems to be the old long form method of checking if there's an instance of the model, if not then create a new one, fill it, save, etc. Super inconvenient.

I'm not even sure how this is happening on my end. I don't think the calls are happening close enough together to cause the duplicates. Anyway, just hoping there's a better solution soon. Thanks.

timcortesi added a commit to EscherLabs/Graphene that referenced this issue Aug 19, 2018
@mst101
Copy link

mst101 commented Aug 22, 2018

@didioh 's approach works for me, as does adding a trait to the affected Model to override the updateOrCreate and firstOrCreate methods, as described here.

I agree with others that this should be default behaviour.

@whande1992
Copy link

I have a problem that may be the cause of your problem.

My table has an id column, language and description, I'm running the method for validating the id and language. To insert everything occurs perfectly.

ID_|_ LANGUAGE_ |_ DESCRIPTION
1 _____1________language description 1
1 _____2________language description 2
1 _____3________language description 3

The problem occurs when it is updated, when sending to update
id= 1
language= 3
description= updating language 3

description :: updateOrCreate (['id' => 1, 'language' => 3],              ['description' => 'updating language 3'] );

it will be like this

ID_ |_ LANGUAGE _ |_ DESCRIPTION
1_____1________ updating language 3
1_____2________ lupdating language 3
1_____3________ updating language 3

Laravel 5.6

@Gogtsu
Copy link

Gogtsu commented Aug 28, 2018

39164774_1652775314847989_5528931938674409472_n

@ghost
Copy link

ghost commented Oct 20, 2018

I had the same issue. I found a situation in which $added_products would contain records with the same primary key as records in DB.

$order->order_products()->createMany($added_products);

So (because there isn't updateOrCreateMany()) I changed this to:

foreach($added_products as $added_product) {
    $order->order_products()->updateOrCreate($added_product);
}

But that didn't work. Then I figured out that I hadn't specified the $primaryKey in the model file. After I added protected $primaryKey = ['order_id', 'product_id']; to the model file, it still didn't work because apparently Laravel supporting composite primary keys would make Eloquent too complex.

My solution, with composite primary key support

foreach($added_products as $added_product) {
    // $order->order_products()->updateOrCreate($added_product);
    \Helper::updateOrCreate($order->order_products(), $added_product);
}

And in the helper class:

public static function updateOrCreate($object, $attributes) {
    $primaryKey = [];
    $modelPrimaryKey = (array)$object->getRelated()->getKeyName();
    
    $attributes_withParentKey = $attributes;
    $attributes_withParentKey[$object->getForeignKeyName()] = $object->getParentKey();

    $values = $attributes; // without primary key

    foreach($modelPrimaryKey as $field) {
        $primaryKey[$field] = $attributes_withParentKey[$field];
        unset($values[$field]);
    }

    DB::transaction(function() use(&$object, $primaryKey, $values, $attributes) {
        // if record exists
        if (DB::table($object->getRelated()->getTable())->where($primaryKey)->first()) {
            // can't do $object->update($attributes) because one part of the PK would be in $attributes
            DB::table($object->getRelated()->getTable())->where($primaryKey)->update($values);
        } else {
            $object->create($attributes);
        }
    }, 5);
}

Also, Query\Builder::updateOrInsert() seems vulnerable to race conditions:

public function updateOrInsert(array $attributes, array $values = [])
{
    if (! $this->where($attributes)->exists()) {
        return $this->insert(array_merge($attributes, $values));
    }
    return (bool) $this->take(1)->update($values);
}

Shouldn't the method use DB::transaction? What if the record get deleted between ! $this->where($attributes)->exists() and $this->insert(array_merge($attributes, $values))?

@amenk
Copy link
Contributor

amenk commented Oct 28, 2018

This might be of help: Maybe we should make a pullrequest for this trait?

https://gist.github.com/troatie/def0fba42fcfb70f873b7f033fbe255f

@Yismen
Copy link

Yismen commented Nov 25, 2018

Just remove your foreign key from the $fillable array property and it will work. Foreign key should not be mass assigned. It worked for me.

@sweebee
Copy link

sweebee commented Apr 11, 2019

I've got (sometimes) the same issue:

			    Product::updateOrCreate(
				    ['catalog_id' => $row['catalog_id'], 'product_code' => $row['product_code']],
				    $row
			    );

only the ID is guarded but sometimes it adds duplicates.

@ludo237
Copy link

ludo237 commented Apr 11, 2019

It's a race condition you should leverage the lock logic.

@tonviet712
Copy link

I've got the same issue (sometimes):
SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry

@litlife
Copy link

litlife commented May 20, 2019

I've also got the same rarely random issue when use updateOrCreate
SQLSTATE[23505]: Unique violation: 7 ERROR: duplicate key value violates unique constraint
Laravel Version: 5.8
PHP Version: 7.3.5
Database version: PostgreSQL 10.7 (Ubuntu 10.7-1.pgdg14.04+1)

@staudenmeir
Copy link
Contributor

I've created an UPSERT package for all databases: https://github.com/staudenmeir/laravel-upsert

@gbwashleybrown
Copy link

Yeah, this is definitely still a problem. Whats the point in an updateORcreate method if it does the exact opposite

@msztorc
Copy link

msztorc commented Jul 16, 2019

Methods firstOrCreate and updateOrCreate are totally unusable for real application which allows simultaneous requests. Solve this or rename to firstOrDuplicate. 😃

@eduardodallmann
Copy link

Same problem

@bhuvidya
Copy link

I must admit I'm kinda stunned this hasn't been fixed natively in the Eloquent codebase. I've reverted to using a trait on my model classes that that wraps a "create advisory lock" mysql call around updateOrCreate() and firstOrCreate() - but it's a hack and will only work for MySQL.

@bhuvidya
Copy link

At the very least the docs should be explicit that these methods are not implemented using native database SQL queries, and are therefore not "transaction safe".

@duq
Copy link

duq commented Jul 17, 2019

Guys, please open a PR if you have a different approach that works on all supported database engines :) PRs are welcomed.

Is it possible to have different fixes for different database engines with the same outcome?

We should at least update the docs with a warning about using firstOrCreate/updateOrCreate methods:
https://laravel.com/docs/5.8/eloquent#inserting-and-updating-models

@aligajani
Copy link

aligajani commented Jul 17, 2019 via email

@novaknole
Copy link

I think, if you use index and have updateorcreate method, and put that inside try catch block, all is good.

Imagine two threads both enter the if statement. So both of them are gonna write into the database. when one will start inserting it, the table will be locked for the other one. So first one gets inserted, and another one waits. when first is done and second one starts inserting, it's gonna throw exception of duplicate event entries. YOu got the catch block right? so you catch it and nothing is a problem.

@hxgdzyuyi
Copy link

Is it possible to use INSERT ... ON DUPLICATE KEY UPDATE Syntax ?

@staudenmeir
Copy link
Contributor

@hxgdzyuyi Please see laravel/ideas#1090 (comment) why that is not a universal solution for updateOrCreate().

@waleedakhlaq
Copy link

how to avoid a duplicate entry in a db row?

@idezigns
Copy link

idezigns commented Nov 5, 2019

Just remove your foreign key from the $fillable array property and it will work. Foreign key should not be mass assigned. It worked for me.

This worked for me, my primary 'id' key is the only unique key in my table, once I removed this from fillable assignment in model it worked.

@kcassam
Copy link

kcassam commented Feb 11, 2020

Is this solution valid ?:

try {                
    $program = Program::updateOrCreate(['key' => $key], [...]);
} catch (PDOException $e) {
    // race condition, let's retry
    $program = Program::updateOrCreate(['key' => $key], [...]);
}

@comtelindodev
Copy link

is this fixed ?
cause I got the same problem, Laravel 6

@amirasyraf
Copy link

Problem still exists. Laravel 7.5.2

@simonmaass
Copy link

can confirm issue still present

@driesvints
Copy link
Member

Please open a new issue with steps to reproduce.

@laravel laravel locked and limited conversation to collaborators Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests