From 0282e754685f0ea151811352655326352eb887b5 Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Mon, 25 Sep 2023 13:13:05 +0200 Subject: [PATCH 1/5] Add test to replicate situation --- .../Database/EloquentHasManyThroughTest.php | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/Integration/Database/EloquentHasManyThroughTest.php b/tests/Integration/Database/EloquentHasManyThroughTest.php index e2395b9ee5ee..d8e7657b11a0 100644 --- a/tests/Integration/Database/EloquentHasManyThroughTest.php +++ b/tests/Integration/Database/EloquentHasManyThroughTest.php @@ -139,6 +139,37 @@ public function testHasSameParentAndThroughParentTable() $this->assertEquals([1], $categories->pluck('id')->all()); } + + public function testUpdateOrCreateWillNotUseIdFromParentModel() + { + // On Laravel 10.21.0, a bug was introduced that would update the wroong model when using `updateOrCreate()`, + // because the UPDATE statement would target a model based on the ID from the parent instead of the actual + // conditions that the `updateOrCreate()` targeted. This test replicates the case that causes this bug. + + // Manually provide IDs, keep ID 1 and 2 free for the team-mates. + $user1 = User::create(['name' => Str::random(), 'id' => 3]); + $user2 = User::create(['name' => Str::random(), 'id' => 4]); + + $team1 = Team::create(['owner_id' => $user1->id]); + $team2 = Team::create(['owner_id' => $user2->id]); + + $teamMate1 = User::create(['name' => 'John', 'team_id' => $team1->id, 'id' => 2]); + // $teamMate2->id should be the same as the $team1->id for the bug to occur. + $teamMate2 = User::create(['name' => 'Jane', 'team_id' => $team2->id, 'id' => $team1->id]); + + $user1->teamMates()->updateOrCreate([ + 'name' => 'John', + ], [ + 'name' => 'John Doe', + ]); + + // In Laravel 10.21.0, the $teamMate1 would not be updated and still have the name "John"... + $this->assertSame('John Doe', $teamMate1->fresh()->name); + // ... and the $teamMate2 would be updated and now have the name "John Doe", because this + // $teamMate has the same ID as the $user1. Even more dangerous, $teamMate2 belongs to a + // totally different team and is not related to $user1 in any way or via any relation. + $this->assertSame('Jane', $teamMate2->fresh()->name); + } } class User extends Model From f823cdcde435685e8cf02253577cc9a7052b1bd8 Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Mon, 25 Sep 2023 13:56:42 +0200 Subject: [PATCH 2/5] Failing test --- .../Database/EloquentHasManyThroughTest.php | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/tests/Integration/Database/EloquentHasManyThroughTest.php b/tests/Integration/Database/EloquentHasManyThroughTest.php index d8e7657b11a0..14a4a3fd6dab 100644 --- a/tests/Integration/Database/EloquentHasManyThroughTest.php +++ b/tests/Integration/Database/EloquentHasManyThroughTest.php @@ -8,6 +8,7 @@ use Illuminate\Support\Facades\Schema; use Illuminate\Support\Str; use Illuminate\Tests\Integration\Database\DatabaseTestCase; +use Spatie\LaravelRay\RayServiceProvider; class EloquentHasManyThroughTest extends DatabaseTestCase { @@ -153,21 +154,38 @@ public function testUpdateOrCreateWillNotUseIdFromParentModel() $team1 = Team::create(['owner_id' => $user1->id]); $team2 = Team::create(['owner_id' => $user2->id]); - $teamMate1 = User::create(['name' => 'John', 'team_id' => $team1->id, 'id' => 2]); + $teamMate1 = User::create(['name' => 'John', 'slug' => 'john-slug', 'team_id' => $team1->id, 'id' => 2]); // $teamMate2->id should be the same as the $team1->id for the bug to occur. - $teamMate2 = User::create(['name' => 'Jane', 'team_id' => $team2->id, 'id' => $team1->id]); + $teamMate2 = User::create(['name' => 'Jane', 'slug' => 'jane-slug', 'team_id' => $team2->id, 'id' => $team1->id]); + $this->assertSame(2, $teamMate1->id); + $this->assertSame(1, $teamMate2->id); + + $this->assertSame(2, $teamMate1->refresh()->id); + $this->assertSame(1, $teamMate2->refresh()->id); + + $this->assertSame('john-slug', $teamMate1->slug); + $this->assertSame('jane-slug', $teamMate2->slug); + + $this->assertSame('john-slug', $teamMate1->refresh()->slug); + $this->assertSame('jane-slug', $teamMate2->refresh()->slug); + + (new RayServiceProvider($this->app))->register(); + (new RayServiceProvider($this->app))->boot(); + + ray()->showQueries(); $user1->teamMates()->updateOrCreate([ 'name' => 'John', ], [ 'name' => 'John Doe', ]); + ray()->stopShowingQueries(); // In Laravel 10.21.0, the $teamMate1 would not be updated and still have the name "John"... $this->assertSame('John Doe', $teamMate1->fresh()->name); - // ... and the $teamMate2 would be updated and now have the name "John Doe", because this - // $teamMate has the same ID as the $user1. Even more dangerous, $teamMate2 belongs to a - // totally different team and is not related to $user1 in any way or via any relation. + // The `$teamMate2` variable would be updated and now have the name "John Doe". This is because + // `$teamMate2` has the same ID as the `$team1`. Even more dangerous, $teamMate2 belongs to a + // totally different team and is not related to `$user1` in any way or via any relationship. $this->assertSame('Jane', $teamMate2->fresh()->name); } } From 2aa0c562a1cf0b787dc44b5406dc96a8e3938ef0 Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Mon, 25 Sep 2023 14:23:59 +0200 Subject: [PATCH 3/5] Update test --- .../Database/EloquentHasManyThroughTest.php | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/tests/Integration/Database/EloquentHasManyThroughTest.php b/tests/Integration/Database/EloquentHasManyThroughTest.php index 14a4a3fd6dab..1a4eb60a642d 100644 --- a/tests/Integration/Database/EloquentHasManyThroughTest.php +++ b/tests/Integration/Database/EloquentHasManyThroughTest.php @@ -170,23 +170,22 @@ public function testUpdateOrCreateWillNotUseIdFromParentModel() $this->assertSame('john-slug', $teamMate1->refresh()->slug); $this->assertSame('jane-slug', $teamMate2->refresh()->slug); - (new RayServiceProvider($this->app))->register(); - (new RayServiceProvider($this->app))->boot(); - - ray()->showQueries(); $user1->teamMates()->updateOrCreate([ 'name' => 'John', + // The `updateOrCreate()` method tries to retrieve an existing model first like `->where($conditions)->first()`. + // In our case, that will return the model with the name `John`. However, the ID of the model with the name + // `John` is hydrated to `1` – where the actual ID should be `2` for the model with the name `John` (see + // the assertions above). If the `->where($conditions)->first()` return a model, a `->fill()->save()` + // action is executed. Because the ID is incorrectly hydrated to `1`, it will now update the Jane + // model with *all* the attributes of the `John` model, instead of updating the `John` model. ], [ - 'name' => 'John Doe', + 'slug' => 'john-doe', ]); - ray()->stopShowingQueries(); - // In Laravel 10.21.0, the $teamMate1 would not be updated and still have the name "John"... - $this->assertSame('John Doe', $teamMate1->fresh()->name); - // The `$teamMate2` variable would be updated and now have the name "John Doe". This is because - // `$teamMate2` has the same ID as the `$team1`. Even more dangerous, $teamMate2 belongs to a - // totally different team and is not related to `$user1` in any way or via any relationship. - $this->assertSame('Jane', $teamMate2->fresh()->name); + // Expect $teamMate1's slug to be updated to john-doe instead of john-old. + $this->assertSame('john-doe', $teamMate1->fresh()->slug); + // $teamMate2 should not be updated, because it belongs to a different user altogether. + $this->assertSame('jane-slug', $teamMate2->fresh()->slug); } } From aa216ed320b3ce076d0e478da3c52fa36c870ef8 Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Mon, 25 Sep 2023 14:25:21 +0200 Subject: [PATCH 4/5] WIP --- tests/Integration/Database/EloquentHasManyThroughTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Integration/Database/EloquentHasManyThroughTest.php b/tests/Integration/Database/EloquentHasManyThroughTest.php index 1a4eb60a642d..1336065b8cb8 100644 --- a/tests/Integration/Database/EloquentHasManyThroughTest.php +++ b/tests/Integration/Database/EloquentHasManyThroughTest.php @@ -143,7 +143,7 @@ public function testHasSameParentAndThroughParentTable() public function testUpdateOrCreateWillNotUseIdFromParentModel() { - // On Laravel 10.21.0, a bug was introduced that would update the wroong model when using `updateOrCreate()`, + // On Laravel 10.21.0, a bug was introduced that would update the wrong model when using `updateOrCreate()`, // because the UPDATE statement would target a model based on the ID from the parent instead of the actual // conditions that the `updateOrCreate()` targeted. This test replicates the case that causes this bug. From 8a44da05da7d59440d07540f9d7837a1ce32fb9f Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Mon, 25 Sep 2023 14:27:23 +0200 Subject: [PATCH 5/5] WIP --- .../Database/EloquentHasManyThroughTest.php | 95 +++++++++---------- 1 file changed, 47 insertions(+), 48 deletions(-) diff --git a/tests/Integration/Database/EloquentHasManyThroughTest.php b/tests/Integration/Database/EloquentHasManyThroughTest.php index 1336065b8cb8..525f6e492408 100644 --- a/tests/Integration/Database/EloquentHasManyThroughTest.php +++ b/tests/Integration/Database/EloquentHasManyThroughTest.php @@ -8,7 +8,6 @@ use Illuminate\Support\Facades\Schema; use Illuminate\Support\Str; use Illuminate\Tests\Integration\Database\DatabaseTestCase; -use Spatie\LaravelRay\RayServiceProvider; class EloquentHasManyThroughTest extends DatabaseTestCase { @@ -140,53 +139,53 @@ public function testHasSameParentAndThroughParentTable() $this->assertEquals([1], $categories->pluck('id')->all()); } - - public function testUpdateOrCreateWillNotUseIdFromParentModel() - { - // On Laravel 10.21.0, a bug was introduced that would update the wrong model when using `updateOrCreate()`, - // because the UPDATE statement would target a model based on the ID from the parent instead of the actual - // conditions that the `updateOrCreate()` targeted. This test replicates the case that causes this bug. - - // Manually provide IDs, keep ID 1 and 2 free for the team-mates. - $user1 = User::create(['name' => Str::random(), 'id' => 3]); - $user2 = User::create(['name' => Str::random(), 'id' => 4]); - - $team1 = Team::create(['owner_id' => $user1->id]); - $team2 = Team::create(['owner_id' => $user2->id]); - - $teamMate1 = User::create(['name' => 'John', 'slug' => 'john-slug', 'team_id' => $team1->id, 'id' => 2]); - // $teamMate2->id should be the same as the $team1->id for the bug to occur. - $teamMate2 = User::create(['name' => 'Jane', 'slug' => 'jane-slug', 'team_id' => $team2->id, 'id' => $team1->id]); - - $this->assertSame(2, $teamMate1->id); - $this->assertSame(1, $teamMate2->id); - - $this->assertSame(2, $teamMate1->refresh()->id); - $this->assertSame(1, $teamMate2->refresh()->id); - - $this->assertSame('john-slug', $teamMate1->slug); - $this->assertSame('jane-slug', $teamMate2->slug); - - $this->assertSame('john-slug', $teamMate1->refresh()->slug); - $this->assertSame('jane-slug', $teamMate2->refresh()->slug); - - $user1->teamMates()->updateOrCreate([ - 'name' => 'John', - // The `updateOrCreate()` method tries to retrieve an existing model first like `->where($conditions)->first()`. - // In our case, that will return the model with the name `John`. However, the ID of the model with the name - // `John` is hydrated to `1` – where the actual ID should be `2` for the model with the name `John` (see - // the assertions above). If the `->where($conditions)->first()` return a model, a `->fill()->save()` - // action is executed. Because the ID is incorrectly hydrated to `1`, it will now update the Jane - // model with *all* the attributes of the `John` model, instead of updating the `John` model. - ], [ - 'slug' => 'john-doe', - ]); - - // Expect $teamMate1's slug to be updated to john-doe instead of john-old. - $this->assertSame('john-doe', $teamMate1->fresh()->slug); - // $teamMate2 should not be updated, because it belongs to a different user altogether. - $this->assertSame('jane-slug', $teamMate2->fresh()->slug); - } + + public function testUpdateOrCreateWillNotUseIdFromParentModel() + { + // On Laravel 10.21.0, a bug was introduced that would update the wrong model when using `updateOrCreate()`, + // because the UPDATE statement would target a model based on the ID from the parent instead of the actual + // conditions that the `updateOrCreate()` targeted. This test replicates the case that causes this bug. + + // Manually provide IDs, keep ID 1 and 2 free for the team-mates. + $user1 = User::create(['name' => Str::random(), 'id' => 3]); + $user2 = User::create(['name' => Str::random(), 'id' => 4]); + + $team1 = Team::create(['owner_id' => $user1->id]); + $team2 = Team::create(['owner_id' => $user2->id]); + + $teamMate1 = User::create(['name' => 'John', 'slug' => 'john-slug', 'team_id' => $team1->id, 'id' => 2]); + // $teamMate2->id should be the same as the $team1->id for the bug to occur. + $teamMate2 = User::create(['name' => 'Jane', 'slug' => 'jane-slug', 'team_id' => $team2->id, 'id' => $team1->id]); + + $this->assertSame(2, $teamMate1->id); + $this->assertSame(1, $teamMate2->id); + + $this->assertSame(2, $teamMate1->refresh()->id); + $this->assertSame(1, $teamMate2->refresh()->id); + + $this->assertSame('john-slug', $teamMate1->slug); + $this->assertSame('jane-slug', $teamMate2->slug); + + $this->assertSame('john-slug', $teamMate1->refresh()->slug); + $this->assertSame('jane-slug', $teamMate2->refresh()->slug); + + $user1->teamMates()->updateOrCreate([ + 'name' => 'John', + // The `updateOrCreate()` method tries to retrieve an existing model first like `->where($conditions)->first()`. + // In our case, that will return the model with the name `John`. However, the ID of the model with the name + // `John` is hydrated to `1` – where the actual ID should be `2` for the model with the name `John` (see + // the assertions above). If the `->where($conditions)->first()` return a model, a `->fill()->save()` + // action is executed. Because the ID is incorrectly hydrated to `1`, it will now update the Jane + // model with *all* the attributes of the `John` model, instead of updating the `John` model. + ], [ + 'slug' => 'john-doe', + ]); + + // Expect $teamMate1's slug to be updated to john-doe instead of john-old. + $this->assertSame('john-doe', $teamMate1->fresh()->slug); + // $teamMate2 should not be updated, because it belongs to a different user altogether. + $this->assertSame('jane-slug', $teamMate2->fresh()->slug); + } } class User extends Model