From 3ebda2334f5490c3e3d312a0878409c268a64812 Mon Sep 17 00:00:00 2001 From: Tony Messias Date: Mon, 25 Sep 2023 20:18:00 -0300 Subject: [PATCH 1/9] Test firstOrCreate on HasManyThrough relation --- .../Database/EloquentHasManyThroughTest.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/Integration/Database/EloquentHasManyThroughTest.php b/tests/Integration/Database/EloquentHasManyThroughTest.php index 97a2208f0a63..0ca4a3f95b16 100644 --- a/tests/Integration/Database/EloquentHasManyThroughTest.php +++ b/tests/Integration/Database/EloquentHasManyThroughTest.php @@ -140,6 +140,19 @@ public function testHasSameParentAndThroughParentTable() $this->assertEquals([1], $categories->pluck('id')->all()); } + public function testFirstOrCreateWhenModelDoesntExist() + { + $owner = User::create(['name' => 'Taylor']); + Team::create(['owner_id' => $owner->id]); + + $mate = $owner->teamMates()->firstOrCreate(['slug' => 'adam'], ['name' => 'Adam']); + + $this->assertTrue($mate->wasRecentlyCreated); + $this->assertNull($mate->team_id); + $this->assertEquals('Adam', $mate->name); + $this->assertEquals('adam', $mate->slug); + } + public function testUpdateOrCreateAffectingWrongModelsRegression() { // On Laravel 10.21.0, a bug was introduced that would update the wrong model when using `updateOrCreate()`, From 91389b875b2fd0671bcc9ef53906528d17748f87 Mon Sep 17 00:00:00 2001 From: Tony Messias Date: Mon, 25 Sep 2023 20:42:16 -0300 Subject: [PATCH 2/9] Add the firstOrCreate method in the HasManyThrough relation to avoid select * issue --- .../Eloquent/Relations/HasManyThrough.php | 16 +++++++ .../Database/EloquentHasManyThroughTest.php | 48 +++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php index ac5037185793..01ddb327dcd7 100644 --- a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php +++ b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php @@ -262,6 +262,22 @@ public function firstOrNew(array $attributes) return $instance; } + /** + * Get the first record matching the attributes. If the record is not found, create it. + * + * @param array $attributes + * @param array $values + * @return \Illuminate\Database\Eloquent\Model + */ + public function firstOrCreate(array $attributes = [], array $values = []) + { + if (is_null($instance = $this->where($attributes)->first())) { + $instance = $this->create(array_merge($attributes, $values)); + } + + return $instance; + } + /** * Create or update a related record matching the attributes, and fill it with values. * diff --git a/tests/Integration/Database/EloquentHasManyThroughTest.php b/tests/Integration/Database/EloquentHasManyThroughTest.php index 0ca4a3f95b16..9b25e1c2e654 100644 --- a/tests/Integration/Database/EloquentHasManyThroughTest.php +++ b/tests/Integration/Database/EloquentHasManyThroughTest.php @@ -153,6 +153,49 @@ public function testFirstOrCreateWhenModelDoesntExist() $this->assertEquals('adam', $mate->slug); } + public function testFirstOrCreateWhenModelExists() + { + $owner = User::create(['name' => 'Taylor']); + $team = Team::create(['owner_id' => $owner->id]); + + $team->members()->create(['slug' => 'adam', 'name' => 'Adam Wathan']); + + $mate = $owner->teamMates()->firstOrCreate(['slug' => 'adam'], ['name' => 'Adam']); + + $this->assertFalse($mate->wasRecentlyCreated); + $this->assertNotNull($mate->team_id); + $this->assertTrue($team->is($mate->team)); + $this->assertEquals('Adam Wathan', $mate->name); + $this->assertEquals('adam', $mate->slug); + } + + public function testFirstOrCreateRegressionIssue() + { + $team1 = Team::create(); + $team2 = Team::create(); + + $jane = $team2->members()->create(['name' => 'Jane', 'slug' => 'jane']); + $john = $team1->members()->create(['name' => 'John', 'slug' => 'john']); + + $taylor = User::create(['name' => 'Taylor']); + $team1->update(['owner_id' => $taylor->id]); + + $newJohn = $taylor->teamMates()->firstOrCreate( + ['slug' => 'john'], + ['name' => 'John Doe'], + ); + + $this->assertFalse($newJohn->wasRecentlyCreated); + $this->assertTrue($john->is($newJohn)); + $this->assertEquals('john', $newJohn->refresh()->slug); + $this->assertEquals('John', $newJohn->name); + + $this->assertSame('john', $john->refresh()->slug); + $this->assertSame('John', $john->name); + $this->assertSame('jane', $jane->refresh()->slug); + $this->assertSame('Jane', $jane->name); + } + public function testUpdateOrCreateAffectingWrongModelsRegression() { // On Laravel 10.21.0, a bug was introduced that would update the wrong model when using `updateOrCreate()`, @@ -241,6 +284,11 @@ public function ownedTeams() { return $this->hasMany(Team::class, 'owner_id'); } + + public function team() + { + return $this->belongsTo(Team::class); + } } class UserWithGlobalScope extends Model From f68c830c276e1ab89a010173fac92b1eed1a7944 Mon Sep 17 00:00:00 2001 From: Tony Messias Date: Mon, 25 Sep 2023 21:00:13 -0300 Subject: [PATCH 3/9] Invert the if statement on the firstOrCreate method --- .../Database/Eloquent/Relations/HasManyThrough.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php index 01ddb327dcd7..66e6b2e6af68 100644 --- a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php +++ b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php @@ -271,11 +271,11 @@ public function firstOrNew(array $attributes) */ public function firstOrCreate(array $attributes = [], array $values = []) { - if (is_null($instance = $this->where($attributes)->first())) { - $instance = $this->create(array_merge($attributes, $values)); + if ($instance = $this->where($attributes)->first()) { + return $instance; } - return $instance; + return $this->create(array_merge($attributes, $values)); } /** From cb60560d410e7fe186ae3a50d81379a3f9492278 Mon Sep 17 00:00:00 2001 From: Tony Messias Date: Mon, 25 Sep 2023 21:00:41 -0300 Subject: [PATCH 4/9] Adds tests for the firstOrCreate when model doesn't exist --- .../Database/EloquentHasManyThroughTest.php | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/Integration/Database/EloquentHasManyThroughTest.php b/tests/Integration/Database/EloquentHasManyThroughTest.php index 9b25e1c2e654..24226de5393a 100644 --- a/tests/Integration/Database/EloquentHasManyThroughTest.php +++ b/tests/Integration/Database/EloquentHasManyThroughTest.php @@ -36,6 +36,14 @@ protected function defineDatabaseMigrationsAfterDatabaseRefreshed() $table->increments('id'); $table->integer('category_id'); }); + + Schema::create('articles', function (Blueprint $table) { + $table->id(); + $table->foreignId('user_id'); + $table->string('slug')->unique(); + $table->string('title')->nullable(); + $table->timestamps(); + }); } public function testBasicCreateAndRetrieve() @@ -196,6 +204,22 @@ public function testFirstOrCreateRegressionIssue() $this->assertSame('Jane', $jane->name); } + public function testCreateOrFirstWhenRecordDoesntExist() + { + $team = Team::create(); + $tony = $team->members()->create(['name' => 'Tony']); + + $article = $team->articles()->createOrFirst( + ['user_id' => $tony->id, 'slug' => 'laravel-forever'], + ['title' => 'Laravel Forever'], + ); + + $this->assertTrue($article->wasRecentlyCreated); + $this->assertEquals('laravel-forever', $article->slug); + $this->assertEquals('Laravel Forever', $article->title); + $this->assertTrue($tony->is($article->user)); + } + public function testUpdateOrCreateAffectingWrongModelsRegression() { // On Laravel 10.21.0, a bug was introduced that would update the wrong model when using `updateOrCreate()`, @@ -289,6 +313,11 @@ public function team() { return $this->belongsTo(Team::class); } + + public function articles() + { + return $this->hasMany(Article::class); + } } class UserWithGlobalScope extends Model @@ -322,6 +351,11 @@ public function membersWithGlobalScope() { return $this->hasMany(UserWithGlobalScope::class, 'team_id'); } + + public function articles() + { + return $this->hasManyThrough(Article::class, User::class); + } } class Category extends Model @@ -342,3 +376,13 @@ class Product extends Model public $timestamps = false; protected $guarded = []; } + +class Article extends Model +{ + protected $guarded = []; + + public function user() + { + return $this->belongsTo(User::class); + } +} From b192426fed9221332a579b8080faedb5190e6144 Mon Sep 17 00:00:00 2001 From: Tony Messias Date: Mon, 25 Sep 2023 22:30:54 -0300 Subject: [PATCH 5/9] Test createOrFirst on HasManyThrough relations with existing models --- .../Database/EloquentHasManyThroughTest.php | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/tests/Integration/Database/EloquentHasManyThroughTest.php b/tests/Integration/Database/EloquentHasManyThroughTest.php index 24226de5393a..862aea950f9b 100644 --- a/tests/Integration/Database/EloquentHasManyThroughTest.php +++ b/tests/Integration/Database/EloquentHasManyThroughTest.php @@ -40,8 +40,7 @@ protected function defineDatabaseMigrationsAfterDatabaseRefreshed() Schema::create('articles', function (Blueprint $table) { $table->id(); $table->foreignId('user_id'); - $table->string('slug')->unique(); - $table->string('title')->nullable(); + $table->string('title')->unique(); $table->timestamps(); }); } @@ -210,16 +209,36 @@ public function testCreateOrFirstWhenRecordDoesntExist() $tony = $team->members()->create(['name' => 'Tony']); $article = $team->articles()->createOrFirst( - ['user_id' => $tony->id, 'slug' => 'laravel-forever'], ['title' => 'Laravel Forever'], + ['user_id' => $tony->id], ); $this->assertTrue($article->wasRecentlyCreated); - $this->assertEquals('laravel-forever', $article->slug); $this->assertEquals('Laravel Forever', $article->title); $this->assertTrue($tony->is($article->user)); } + public function testCreateOrFirstWhenRecordExists() + { + $team = Team::create(); + $taylor = $team->members()->create(['name' => 'Taylor']); + $tony = $team->members()->create(['name' => 'Tony']); + + $existingArticle = $taylor->articles()->create([ + 'title' => 'Laravel Forever', + ]); + + $newArticle = $team->articles()->createOrFirst( + ['title' => 'Laravel Forever'], + ['user_id' => $tony->id], + ); + + $this->assertFalse($newArticle->wasRecentlyCreated); + $this->assertEquals('Laravel Forever', $newArticle->title); + $this->assertTrue($taylor->is($newArticle->user)); + $this->assertTrue($existingArticle->is($newArticle)); + } + public function testUpdateOrCreateAffectingWrongModelsRegression() { // On Laravel 10.21.0, a bug was introduced that would update the wrong model when using `updateOrCreate()`, From d3041d17c4c22bc144073881905b3c3b0aa44c52 Mon Sep 17 00:00:00 2001 From: Tony Messias Date: Mon, 25 Sep 2023 22:41:47 -0300 Subject: [PATCH 6/9] Fix the createOrFirst also happening the regression test from the updateOrCreate --- .../Eloquent/Relations/HasManyThrough.php | 19 ++++++++++++++ .../Database/EloquentHasManyThroughTest.php | 25 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php index 66e6b2e6af68..e9e31c7bd1eb 100644 --- a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php +++ b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php @@ -10,6 +10,7 @@ use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Database\Eloquent\Relations\Concerns\InteractsWithDictionary; use Illuminate\Database\Eloquent\SoftDeletes; +use Illuminate\Database\UniqueConstraintViolationException; class HasManyThrough extends Relation { @@ -278,6 +279,24 @@ public function firstOrCreate(array $attributes = [], array $values = []) return $this->create(array_merge($attributes, $values)); } + /** + * Get the first record matching the attributes. If the record is not found, create it. + * Attempt to create the record with the given attribute and values. If a unique contraint + * violation is triggered, + * + * @param array $attributes + * @param array $values + * @return \Illuminate\Database\Eloquent\Model + */ + public function createOrFirst(array $attributes = [], array $values = []) + { + try { + return $this->create(array_merge($attributes, $values)); + } catch (UniqueConstraintViolationException $exception) { + return $this->where($attributes)->first() ?? throw $exception; + } + } + /** * Create or update a related record matching the attributes, and fill it with values. * diff --git a/tests/Integration/Database/EloquentHasManyThroughTest.php b/tests/Integration/Database/EloquentHasManyThroughTest.php index 862aea950f9b..5e13c6287032 100644 --- a/tests/Integration/Database/EloquentHasManyThroughTest.php +++ b/tests/Integration/Database/EloquentHasManyThroughTest.php @@ -239,6 +239,31 @@ public function testCreateOrFirstWhenRecordExists() $this->assertTrue($existingArticle->is($newArticle)); } + public function testCreateOrFirstRegressionIssue() + { + $team1 = Team::create(); + + $taylor = $team1->members()->create(['name' => 'Taylor']); + $tony = $team1->members()->create(['name' => 'Tony']); + + $existingTonyArticle = $tony->articles()->create(['title' => 'The New createOrFirst Method']); + $existingTaylorArticle = $taylor->articles()->create(['title' => 'Laravel Forever']); + + $newArticle = $team1->articles()->createOrFirst( + ['title' => 'Laravel Forever'], + ['user_id' => $tony->id], + ); + + $this->assertFalse($newArticle->wasRecentlyCreated); + $this->assertTrue($existingTaylorArticle->is($newArticle)); + $this->assertEquals('Laravel Forever', $newArticle->refresh()->title); + $this->assertTrue($taylor->is($newArticle->user)); + + $this->assertSame('Laravel Forever', $existingTaylorArticle->refresh()->title); + $this->assertSame('The New createOrFirst Method', $existingTonyArticle->refresh()->title); + $this->assertTrue($tony->is($existingTonyArticle->user)); + } + public function testUpdateOrCreateAffectingWrongModelsRegression() { // On Laravel 10.21.0, a bug was introduced that would update the wrong model when using `updateOrCreate()`, From d09870905a5cd4809b4a0c415c685db12139ccc2 Mon Sep 17 00:00:00 2001 From: Tony Messias Date: Mon, 25 Sep 2023 22:48:52 -0300 Subject: [PATCH 7/9] Wraps the create part of the createOrFirst in a savepoint if needed --- .../Eloquent/Relations/HasManyThrough.php | 2 +- .../Database/EloquentHasManyThroughTest.php | 22 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php index e9e31c7bd1eb..67caa574ae93 100644 --- a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php +++ b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php @@ -291,7 +291,7 @@ public function firstOrCreate(array $attributes = [], array $values = []) public function createOrFirst(array $attributes = [], array $values = []) { try { - return $this->create(array_merge($attributes, $values)); + return $this->getQuery()->withSavepointIfNeeded(fn () => $this->create(array_merge($attributes, $values))); } catch (UniqueConstraintViolationException $exception) { return $this->where($attributes)->first() ?? throw $exception; } diff --git a/tests/Integration/Database/EloquentHasManyThroughTest.php b/tests/Integration/Database/EloquentHasManyThroughTest.php index 5e13c6287032..0f5c38508fb0 100644 --- a/tests/Integration/Database/EloquentHasManyThroughTest.php +++ b/tests/Integration/Database/EloquentHasManyThroughTest.php @@ -5,6 +5,7 @@ use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\SoftDeletes; use Illuminate\Database\Schema\Blueprint; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Schema; use Illuminate\Support\Str; use Illuminate\Tests\Integration\Database\DatabaseTestCase; @@ -239,6 +240,27 @@ public function testCreateOrFirstWhenRecordExists() $this->assertTrue($existingArticle->is($newArticle)); } + public function testCreateOrFirstWhenRecordExistsInTransaction() + { + $team = Team::create(); + $taylor = $team->members()->create(['name' => 'Taylor']); + $tony = $team->members()->create(['name' => 'Tony']); + + $existingArticle = $taylor->articles()->create([ + 'title' => 'Laravel Forever', + ]); + + $newArticle = DB::transaction(fn () => $team->articles()->createOrFirst( + ['title' => 'Laravel Forever'], + ['user_id' => $tony->id], + )); + + $this->assertFalse($newArticle->wasRecentlyCreated); + $this->assertEquals('Laravel Forever', $newArticle->title); + $this->assertTrue($taylor->is($newArticle->user)); + $this->assertTrue($existingArticle->is($newArticle)); + } + public function testCreateOrFirstRegressionIssue() { $team1 = Team::create(); From d44a82ea5c8fbda081ee33cc725c10737fe710f4 Mon Sep 17 00:00:00 2001 From: Tony Messias Date: Mon, 25 Sep 2023 23:02:00 -0300 Subject: [PATCH 8/9] Fix comment --- src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php index 67caa574ae93..dbf8f194b58b 100644 --- a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php +++ b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php @@ -280,9 +280,7 @@ public function firstOrCreate(array $attributes = [], array $values = []) } /** - * Get the first record matching the attributes. If the record is not found, create it. - * Attempt to create the record with the given attribute and values. If a unique contraint - * violation is triggered, + * Attempt to create the record. If a unique constraint violation occurs, attempt to find the matching record. * * @param array $attributes * @param array $values From 0efc6f437b3ae8a4839c5d4acd138004754972c1 Mon Sep 17 00:00:00 2001 From: Tony Messias Date: Mon, 25 Sep 2023 23:33:50 -0300 Subject: [PATCH 9/9] Adds the not is_null in the guard if statement --- src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php index dbf8f194b58b..079bdd8b3bde 100644 --- a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php +++ b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php @@ -272,7 +272,7 @@ public function firstOrNew(array $attributes) */ public function firstOrCreate(array $attributes = [], array $values = []) { - if ($instance = $this->where($attributes)->first()) { + if (! is_null($instance = $this->where($attributes)->first())) { return $instance; }