Skip to content

Commit

Permalink
[10.x] Fix duplicate conditions on retrying SELECT calls under `cre…
Browse files Browse the repository at this point in the history
…ateOrFirst()` (#48725)

* fix: duplicate conditions with `firstOrCreate` and `createOrFirst`

* fix: missing mocks due to additional code
  • Loading branch information
KentarouTakeda authored Oct 16, 2023
1 parent 1bae4cd commit 2f88a30
Show file tree
Hide file tree
Showing 8 changed files with 17 additions and 19 deletions.
2 changes: 1 addition & 1 deletion src/Illuminate/Database/Eloquent/Builder.php
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ public function firstOrNew(array $attributes = [], array $values = [])
*/
public function firstOrCreate(array $attributes = [], array $values = [])
{
if (! is_null($instance = $this->where($attributes)->first())) {
if (! is_null($instance = (clone $this)->where($attributes)->first())) {
return $instance;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ public function firstOrNew(array $attributes = [], array $values = [])
*/
public function firstOrCreate(array $attributes = [], array $values = [])
{
if (! is_null($instance = $this->where($attributes)->first())) {
if (! is_null($instance = (clone $this)->where($attributes)->first())) {
return $instance;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ public function firstOrNew(array $attributes = [], array $values = [])
*/
public function firstOrCreate(array $attributes = [], array $values = [])
{
if (is_null($instance = $this->where($attributes)->first())) {
if (is_null($instance = (clone $this)->where($attributes)->first())) {
$instance = $this->createOrFirst($attributes, $values);
}

Expand Down
6 changes: 2 additions & 4 deletions tests/Database/DatabaseEloquentBuilderCreateOrFirstTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,7 @@ public function testFirstOrCreateMethodRetrievesRecordCreatedJustNow(): void

$model->getConnection()
->expects('select')
// FIXME: duplicate conditions
->with('select * from "table" where ("attr" = ?) and ("attr" = ?) limit 1', ['foo', 'foo'], false)
->with('select * from "table" where ("attr" = ?) limit 1', ['foo'], false)
->andReturn([[
'id' => 123,
'attr' => 'foo',
Expand Down Expand Up @@ -273,8 +272,7 @@ public function testUpdateOrCreateMethodUpdatesRecordCreatedJustNow(): void

$model->getConnection()
->expects('select')
// FIXME: duplicate conditions
->with('select * from "table" where ("attr" = ?) and ("attr" = ?) limit 1', ['foo', 'foo'], false)
->with('select * from "table" where ("attr" = ?) limit 1', ['foo'], false)
->andReturn([[
'id' => 123,
'attr' => 'foo',
Expand Down
6 changes: 2 additions & 4 deletions tests/Database/DatabaseEloquentHasManyCreateOrFirstTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -177,8 +177,7 @@ public function testFirstOrCreateMethodRetrievesRecordCreatedJustNow(): void

$model->getConnection()
->expects('select')
// FIXME: duplicate conditions
->with('select * from "child_table" where "child_table"."parent_id" = ? and "child_table"."parent_id" is not null and ("attr" = ?) and ("attr" = ?) limit 1', [123, 'foo', 'foo'], false)
->with('select * from "child_table" where "child_table"."parent_id" = ? and "child_table"."parent_id" is not null and ("attr" = ?) limit 1', [123, 'foo'], false)
->andReturn([[
'id' => 456,
'parent_id' => 123,
Expand Down Expand Up @@ -290,8 +289,7 @@ public function testUpdateOrCreateMethodUpdatesRecordCreatedJustNow(): void

$model->getConnection()
->expects('select')
// FIXME: duplicate conditions
->with('select * from "child_table" where "child_table"."parent_id" = ? and "child_table"."parent_id" is not null and ("attr" = ?) and ("attr" = ?) limit 1', [123, 'foo', 'foo'], false)
->with('select * from "child_table" where "child_table"."parent_id" = ? and "child_table"."parent_id" is not null and ("attr" = ?) limit 1', [123, 'foo'], false)
->andReturn([[
'id' => 456,
'parent_id' => 123,
Expand Down
4 changes: 3 additions & 1 deletion tests/Database/DatabaseEloquentHasManyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Illuminate\Database\Query\Builder as QueryBuilder;
use Illuminate\Database\UniqueConstraintViolationException;
use Mockery as m;
use PHPUnit\Framework\TestCase;
Expand Down Expand Up @@ -343,7 +344,8 @@ public function testCreateManyCreatesARelatedModelForEachRecord()

protected function getRelation()
{
$builder = m::mock(Builder::class);
$queryBuilder = m::mock(QueryBuilder::class);
$builder = m::mock(Builder::class, [$queryBuilder]);
$builder->shouldReceive('whereNotNull')->with('table.foreign_key');
$builder->shouldReceive('where')->with('table.foreign_key', '=', 1);
$related = m::mock(Model::class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,10 +196,9 @@ public function testFirstOrCreateMethodRetrievesRecordCreatedJustNow(): void

$parent->getConnection()
->expects('select')
// FIXME: duplicate conditions
->with(
'select "child".*, "pivot"."parent_id" as "laravel_through_key" from "child" inner join "pivot" on "pivot"."id" = "child"."pivot_id" where "pivot"."parent_id" = ? and ("attr" = ?) and ("attr" = ? and "val" = ?) limit 1',
[123, 'foo', 'foo', 'bar'],
'select "child".*, "pivot"."parent_id" as "laravel_through_key" from "child" inner join "pivot" on "pivot"."id" = "child"."pivot_id" where "pivot"."parent_id" = ? and ("attr" = ? and "val" = ?) limit 1',
[123, 'foo', 'bar'],
true,
)
->andReturn([[
Expand Down Expand Up @@ -334,10 +333,9 @@ public function testUpdateOrCreateMethodUpdatesRecordCreatedJustNow(): void

$parent->getConnection()
->expects('select')
// FIXME: duplicate conditions
->with(
'select "child".*, "pivot"."parent_id" as "laravel_through_key" from "child" inner join "pivot" on "pivot"."id" = "child"."pivot_id" where "pivot"."parent_id" = ? and ("attr" = ?) and ("attr" = ? and "val" = ?) limit 1',
[123, 'foo', 'foo', 'bar'],
'select "child".*, "pivot"."parent_id" as "laravel_through_key" from "child" inner join "pivot" on "pivot"."id" = "child"."pivot_id" where "pivot"."parent_id" = ? and ("attr" = ? and "val" = ?) limit 1',
[123, 'foo', 'bar'],
true,
)
->andReturn([[
Expand Down
4 changes: 3 additions & 1 deletion tests/Database/DatabaseEloquentMorphTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Illuminate\Database\Eloquent\Relations\MorphMany;
use Illuminate\Database\Eloquent\Relations\MorphOne;
use Illuminate\Database\Eloquent\Relations\Relation;
use Illuminate\Database\Query\Builder as QueryBuilder;
use Illuminate\Database\UniqueConstraintViolationException;
use Mockery as m;
use PHPUnit\Framework\TestCase;
Expand Down Expand Up @@ -443,7 +444,8 @@ public function testIsNotModelWithAnotherConnection()

protected function getOneRelation()
{
$builder = m::mock(Builder::class);
$queryBuilder = m::mock(QueryBuilder::class);
$builder = m::mock(Builder::class, [$queryBuilder]);
$builder->shouldReceive('whereNotNull')->once()->with('table.morph_id');
$builder->shouldReceive('where')->once()->with('table.morph_id', '=', 1);
$related = m::mock(Model::class);
Expand Down

0 comments on commit 2f88a30

Please sign in to comment.