[10.x] Make the firstOrCreate methods in relations use `createOrFir…

…st` 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 <[email protected]>
laravel · Aug 27, 2023 · 24cbb80 · 24cbb80
1 parent 27958eb
commit 24cbb80
Show file tree

Hide file tree

Showing 5 changed files with 37 additions and 3 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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();