From 24cbb80656190d218fbeeafdd9acdff1fa8cd468 Mon Sep 17 00:00:00 2001 From: Tony Messias Date: Sun, 27 Aug 2023 10:06:16 -0500 Subject: [PATCH] [10.x] Make the `firstOrCreate` methods in relations use `createOrFirst` behind the scenes (#48192) * Use createOrFirst inside firstOrCreate in BelongsToMany relations This should be the same but more robust, since it handles race conditions as well * Use createOrFirst too inside HasOneOrMany relations This should essentially be the same as before, but more robust * Fix tests using mocks * Ensure the firstOrCreate method handles unique violation when just attaching * Fix style * Update BelongsToMany.php --------- Co-authored-by: Taylor Otwell --- .../Eloquent/Relations/BelongsToMany.php | 8 ++++-- .../Eloquent/Relations/HasOneOrMany.php | 2 +- .../Database/DatabaseEloquentHasManyTest.php | 2 ++ tests/Database/DatabaseEloquentMorphTest.php | 2 ++ .../Database/EloquentHasManyTest.php | 26 +++++++++++++++++++ 5 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php b/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php index 2e1078a609aa..3f5ebec20f77 100755 --- a/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php +++ b/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php @@ -622,9 +622,13 @@ public function firstOrCreate(array $attributes = [], array $values = [], array { if (is_null($instance = (clone $this)->where($attributes)->first())) { if (is_null($instance = $this->related->where($attributes)->first())) { - $instance = $this->create(array_merge($attributes, $values), $joining, $touch); + $instance = $this->createOrFirst($attributes, $values, $joining, $touch); } else { - $this->attach($instance, $joining, $touch); + try { + $this->getQuery()->withSavepointIfNeeded(fn () => $this->attach($instance, $joining, $touch)); + } catch (UniqueConstraintViolationException $exception) { + // Nothing to do, the model was already attached... + } } } diff --git a/src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php b/src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php index c4b52db7ff3e..5ed7deb5e6ce 100755 --- a/src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php +++ b/src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php @@ -236,7 +236,7 @@ public function firstOrNew(array $attributes = [], array $values = []) public function firstOrCreate(array $attributes = [], array $values = []) { if (is_null($instance = $this->where($attributes)->first())) { - $instance = $this->create(array_merge($attributes, $values)); + $instance = $this->createOrFirst($attributes, $values); } return $instance; diff --git a/tests/Database/DatabaseEloquentHasManyTest.php b/tests/Database/DatabaseEloquentHasManyTest.php index 0bec03bc97fa..b184dfdd14c4 100755 --- a/tests/Database/DatabaseEloquentHasManyTest.php +++ b/tests/Database/DatabaseEloquentHasManyTest.php @@ -155,6 +155,7 @@ public function testFirstOrCreateMethodCreatesNewModelWithForeignKeySet() $relation = $this->getRelation(); $relation->getQuery()->shouldReceive('where')->once()->with(['foo'])->andReturn($relation->getQuery()); $relation->getQuery()->shouldReceive('first')->once()->with()->andReturn(null); + $relation->getQuery()->shouldReceive('withSavepointIfNeeded')->once()->andReturnUsing(fn ($scope) => $scope()); $model = $this->expectCreatedModel($relation, ['foo']); $this->assertEquals($model, $relation->firstOrCreate(['foo'])); @@ -165,6 +166,7 @@ public function testFirstOrCreateMethodWithValuesCreatesNewModelWithForeignKeySe $relation = $this->getRelation(); $relation->getQuery()->shouldReceive('where')->once()->with(['foo' => 'bar'])->andReturn($relation->getQuery()); $relation->getQuery()->shouldReceive('first')->once()->with()->andReturn(null); + $relation->getQuery()->shouldReceive('withSavepointIfNeeded')->once()->andReturnUsing(fn ($scope) => $scope()); $model = $this->expectCreatedModel($relation, ['foo' => 'bar', 'baz' => 'qux']); $this->assertEquals($model, $relation->firstOrCreate(['foo' => 'bar'], ['baz' => 'qux'])); diff --git a/tests/Database/DatabaseEloquentMorphTest.php b/tests/Database/DatabaseEloquentMorphTest.php index 924cd17fc97b..4870284090f2 100755 --- a/tests/Database/DatabaseEloquentMorphTest.php +++ b/tests/Database/DatabaseEloquentMorphTest.php @@ -195,6 +195,7 @@ public function testFirstOrCreateMethodCreatesNewMorphModel() $relation = $this->getOneRelation(); $relation->getQuery()->shouldReceive('where')->once()->with(['foo'])->andReturn($relation->getQuery()); $relation->getQuery()->shouldReceive('first')->once()->with()->andReturn(null); + $relation->getQuery()->shouldReceive('withSavepointIfNeeded')->once()->andReturnUsing(fn ($scope) => $scope()); $relation->getRelated()->shouldReceive('newInstance')->once()->with(['foo'])->andReturn($model = m::mock(Model::class)); $model->shouldReceive('setAttribute')->once()->with('morph_id', 1); $model->shouldReceive('setAttribute')->once()->with('morph_type', get_class($relation->getParent())); @@ -208,6 +209,7 @@ public function testFirstOrCreateMethodWithValuesCreatesNewMorphModel() $relation = $this->getOneRelation(); $relation->getQuery()->shouldReceive('where')->once()->with(['foo' => 'bar'])->andReturn($relation->getQuery()); $relation->getQuery()->shouldReceive('first')->once()->with()->andReturn(null); + $relation->getQuery()->shouldReceive('withSavepointIfNeeded')->once()->andReturnUsing(fn ($scope) => $scope()); $relation->getRelated()->shouldReceive('newInstance')->once()->with(['foo' => 'bar', 'baz' => 'qux'])->andReturn($model = m::mock(Model::class)); $model->shouldReceive('setAttribute')->once()->with('morph_id', 1); $model->shouldReceive('setAttribute')->once()->with('morph_type', get_class($relation->getParent())); diff --git a/tests/Integration/Database/EloquentHasManyTest.php b/tests/Integration/Database/EloquentHasManyTest.php index 49e1480ba9c7..ec782d770830 100644 --- a/tests/Integration/Database/EloquentHasManyTest.php +++ b/tests/Integration/Database/EloquentHasManyTest.php @@ -61,6 +61,32 @@ public function testHasOneRelationshipFromHasMany() $this->assertEquals($latestLogin->id, $user->latestLogin->id); } + public function testFirstOrCreate() + { + $user = EloquentHasManyTestUser::create(); + + $post1 = $user->posts()->create(['title' => Str::random()]); + $post2 = $user->posts()->firstOrCreate(['title' => $post1->title]); + + $this->assertTrue($post1->is($post2)); + $this->assertCount(1, $user->posts()->get()); + } + + public function testFirstOrCreateWithinTransaction() + { + $user = EloquentHasManyTestUser::create(); + + $post1 = $user->posts()->create(['title' => Str::random()]); + + DB::transaction(function () use ($user, $post1) { + $post2 = $user->posts()->firstOrCreate(['title' => $post1->title]); + + $this->assertTrue($post1->is($post2)); + }); + + $this->assertCount(1, $user->posts()->get()); + } + public function testCreateOrFirst() { $user = EloquentHasManyTestUser::create();