Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Fix createOrFirst on transactions #48144

Merged
merged 13 commits into from
Aug 23, 2023
19 changes: 18 additions & 1 deletion src/Illuminate/Database/Eloquent/Builder.php
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,9 @@ public function firstOrCreate(array $attributes = [], array $values = [])
public function createOrFirst(array $attributes = [], array $values = [])
{
try {
return $this->create(array_merge($attributes, $values));
return $this->withSavepointIfNeeded(function () use ($attributes, $values) {
Copy link
Contributor Author

@tonysm tonysm Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really liked how it read before: createOrFirst was essentially calling create() then first(), now it has more noise. I thought about pushing the savepoint to the create() method, which would keep this part as it was, but then it would be a bigger change.

return $this->create(array_merge($attributes, $values));
});
} catch (UniqueConstraintViolationException $exception) {
return $this->where($attributes)->first();
}
Expand Down Expand Up @@ -2029,4 +2031,19 @@ public function __clone()
{
$this->query = clone $this->query;
}

/**
* Wraps the given Closure with a transaction savepoint if needed.
*
* @template TModelValue
*
* @param \Closure(): TModelValue $scope
* @return TModelValue
*/
public function withSavepointIfNeeded(Closure $scope): mixed
Copy link
Contributor Author

@tonysm tonysm Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about adding this public method. I thought about just wrapping the insert part in a DB::transaction(fn), but not sure what y'all would think of that. I initially had it as protected, but I needed the same logic in the relationship part of the feature.

Let me know what y'all think. Open to suggestions.

Copy link
Contributor

@mpyw mpyw Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For MySQL/SQLServer/SQLite, this is not necessary; Postgres just needs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I figured it wouldn't be a problem to wrap in savepoint transactions for the other drivers as well. It doesn't seem to hurt.

{
return $this->getQuery()->getConnection()->transactionLevel() > 0
? $this->getQuery()->getConnection()->transaction($scope)
: $scope();
}
}
8 changes: 6 additions & 2 deletions src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php
Original file line number Diff line number Diff line change
Expand Up @@ -643,14 +643,18 @@ public function firstOrCreate(array $attributes = [], array $values = [], array
public function createOrFirst(array $attributes = [], array $values = [], array $joining = [], $touch = true)
{
try {
return $this->create(array_merge($attributes, $values), $joining, $touch);
return $this->getQuery()->withSavePointIfNeeded(function () use ($attributes, $values, $joining, $touch) {
return $this->create(array_merge($attributes, $values), $joining, $touch);
});
} catch (UniqueConstraintViolationException $exception) {
// ...
}

try {
return tap($this->related->where($attributes)->first(), function ($instance) use ($joining, $touch) {
$this->attach($instance, $joining, $touch);
$this->getQuery()->withSavepointIfNeeded(function () use ($instance, $joining, $touch) {
$this->attach($instance, $joining, $touch);
});
});
} catch (UniqueConstraintViolationException $exception) {
return (clone $this)->where($attributes)->first();
Expand Down
4 changes: 3 additions & 1 deletion src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,9 @@ 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(function () use ($attributes, $values) {
return $this->create(array_merge($attributes, $values));
});
} catch (UniqueConstraintViolationException $exception) {
return $this->where($attributes)->first();
}
Expand Down
16 changes: 16 additions & 0 deletions tests/Database/DatabaseEloquentIntegrationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,22 @@ public function testCreateOrFirst()
$this->assertSame('Nuno Maduro', $user4->name);
}

public function testCreateOrFirstWithinTransaction()
{
$user1 = EloquentTestUniqueUser::create(['email' => 'taylorotwell@gmail.com']);

DB::transaction(function () use ($user1) {
$user2 = EloquentTestUniqueUser::createOrFirst(
['email' => 'taylorotwell@gmail.com'],
['name' => 'Taylor Otwell']
);

$this->assertEquals($user1->id, $user2->id);
$this->assertSame('taylorotwell@gmail.com', $user2->email);
$this->assertNull($user2->name);
});
}

public function testUpdateOrCreate()
{
$user1 = EloquentTestUser::create(['email' => 'taylorotwell@gmail.com']);
Expand Down
13 changes: 13 additions & 0 deletions tests/Integration/Database/EloquentBelongsToManyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,19 @@ public function testCreateOrFirstUnrelatedExisting()
$this->assertTrue($tag->is($post->tagsUnique()->first()));
}

public function testCreateOrFirstWithinTransaction()
{
$post = Post::create(['title' => Str::random()]);

$tag = UniqueTag::create(['name' => Str::random()]);

$post->tagsUnique()->attach(UniqueTag::all());

DB::transaction(function () use ($tag, $post) {
$this->assertEquals($tag->id, $post->tagsUnique()->createOrFirst(['name' => $tag->name])->id);
});
}

public function testFirstOrNewMethodWithValues()
{
$post = Post::create(['title' => Str::random()]);
Expand Down
16 changes: 16 additions & 0 deletions tests/Integration/Database/EloquentHasManyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Illuminate\Database\Eloquent\Relations\HasOne;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Schema;
use Illuminate\Support\Str;

Expand Down Expand Up @@ -70,6 +71,21 @@ public function testCreateOrFirst()
$this->assertTrue($post1->is($post2));
$this->assertCount(1, $user->posts()->get());
}

public function testCreateOrFirstWithinTransaction()
{
$user = EloquentHasManyTestUser::create();

$post1 = $user->posts()->create(['title' => Str::random()]);

DB::transaction(function () use ($user, $post1) {
$post2 = $user->posts()->createOrFirst(['title' => $post1->title]);

$this->assertTrue($post1->is($post2));
});

$this->assertCount(1, $user->posts()->get());
}
}

class EloquentHasManyTestUser extends Model
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Schema;

class DatabaseEloquentMySqlIntegrationTest extends MySqlTestCase
Expand Down Expand Up @@ -57,6 +58,22 @@ public function testCreateOrFirst()

$this->assertSame('Nuno Maduro', $user4->name);
}

public function testCreateOrFirstWithinTransaction()
{
$user1 = DatabaseEloquentMySqlIntegrationUser::createOrFirst(['email' => 'taylor@laravel.com']);

DB::transaction(function () use ($user1) {
$user2 = DatabaseEloquentMySqlIntegrationUser::createOrFirst(
['email' => 'taylor@laravel.com'],
['name' => 'Taylor Otwell']
);

$this->assertEquals($user1->id, $user2->id);
$this->assertSame('taylor@laravel.com', $user2->email);
$this->assertNull($user2->name);
});
}
}

class DatabaseEloquentMySqlIntegrationUser extends Model
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Schema;

class DatabaseEloquentPostgresIntegrationTest extends PostgresTestCase
Expand Down Expand Up @@ -57,6 +58,22 @@ public function testCreateOrFirst()

$this->assertSame('Nuno Maduro', $user4->name);
}

public function testCreateOrFirstWithinTransaction()
{
$user1 = DatabaseEloquentPostgresIntegrationUser::create(['email' => 'taylor@laravel.com']);

DB::transaction(function () use ($user1) {
$user2 = DatabaseEloquentPostgresIntegrationUser::createOrFirst(
['email' => 'taylor@laravel.com'],
['name' => 'Taylor Otwell']
);

$this->assertEquals($user1->id, $user2->id);
$this->assertSame('taylor@laravel.com', $user2->email);
$this->assertNull($user2->name);
});
}
}

class DatabaseEloquentPostgresIntegrationUser extends Model
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Schema;

class DatabaseEloquentSqlServerIntegrationTest extends SqlServerTestCase
Expand Down Expand Up @@ -57,6 +58,22 @@ public function testCreateOrFirst()

$this->assertSame('Nuno Maduro', $user4->name);
}

public function testCreateOrFirstWithinTransaction()
{
$user1 = DatabaseEloquentSqlServerIntegrationUser::createOrFirst(['email' => 'taylor@laravel.com']);

DB::transaction(function () use ($user1) {
$user2 = DatabaseEloquentSqlServerIntegrationUser::createOrFirst(
['email' => 'taylor@laravel.com'],
['name' => 'Taylor Otwell']
);

$this->assertEquals($user1->id, $user2->id);
$this->assertSame('taylor@laravel.com', $user2->email);
$this->assertNull($user2->name);
});
}
}

class DatabaseEloquentSqlServerIntegrationUser extends Model
Expand Down