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' => '[email protected]']);

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

$this->assertEquals($user1->id, $user2->id);
$this->assertSame('[email protected]', $user2->email);
$this->assertNull($user2->name);
});
}

public function testUpdateOrCreate()
{
$user1 = EloquentTestUser::create(['email' => '[email protected]']);
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 @@ -5,9 +5,14 @@
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
use Illuminate\Tests\Integration\Database\WithEloquentIntegrationTests;

class DatabaseEloquentMySqlIntegrationTest extends MySqlTestCase
{
use WithEloquentIntegrationTests;

protected $eloquentModelClass = DatabaseEloquentMySqlIntegrationUser::class;

protected function defineDatabaseMigrationsAfterDatabaseRefreshed()
{
if (! Schema::hasTable('database_eloquent_mysql_integration_users')) {
Expand All @@ -24,39 +29,6 @@ protected function destroyDatabaseMigrations()
{
Schema::drop('database_eloquent_mysql_integration_users');
}

public function testCreateOrFirst()
{
$user1 = DatabaseEloquentMySqlIntegrationUser::createOrFirst(['email' => '[email protected]']);

$this->assertSame('[email protected]', $user1->email);
$this->assertNull($user1->name);

$user2 = DatabaseEloquentMySqlIntegrationUser::createOrFirst(
['email' => '[email protected]'],
['name' => 'Taylor Otwell']
);

$this->assertEquals($user1->id, $user2->id);
$this->assertSame('[email protected]', $user2->email);
$this->assertNull($user2->name);

$user3 = DatabaseEloquentMySqlIntegrationUser::createOrFirst(
['email' => '[email protected]'],
['name' => 'Abigail Otwell']
);

$this->assertNotEquals($user3->id, $user1->id);
$this->assertSame('[email protected]', $user3->email);
$this->assertSame('Abigail Otwell', $user3->name);

$user4 = DatabaseEloquentMySqlIntegrationUser::createOrFirst(
['name' => 'Dries Vints'],
['name' => 'Nuno Maduro', 'email' => '[email protected]']
);

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

class DatabaseEloquentMySqlIntegrationUser extends Model
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,14 @@
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
use Illuminate\Tests\Integration\Database\WithEloquentIntegrationTests;

class DatabaseEloquentPostgresIntegrationTest extends PostgresTestCase
{
use WithEloquentIntegrationTests;

protected $eloquentModelClass = DatabaseEloquentPostgresIntegrationUser::class;

protected function defineDatabaseMigrationsAfterDatabaseRefreshed()
{
if (! Schema::hasTable('database_eloquent_postgres_integration_users')) {
Expand All @@ -24,39 +29,6 @@ protected function destroyDatabaseMigrations()
{
Schema::drop('database_eloquent_postgres_integration_users');
}

public function testCreateOrFirst()
{
$user1 = DatabaseEloquentPostgresIntegrationUser::createOrFirst(['email' => '[email protected]']);

$this->assertSame('[email protected]', $user1->email);
$this->assertNull($user1->name);

$user2 = DatabaseEloquentPostgresIntegrationUser::createOrFirst(
['email' => '[email protected]'],
['name' => 'Taylor Otwell']
);

$this->assertEquals($user1->id, $user2->id);
$this->assertSame('[email protected]', $user2->email);
$this->assertNull($user2->name);

$user3 = DatabaseEloquentPostgresIntegrationUser::createOrFirst(
['email' => '[email protected]'],
['name' => 'Abigail Otwell']
);

$this->assertNotEquals($user3->id, $user1->id);
$this->assertSame('[email protected]', $user3->email);
$this->assertSame('Abigail Otwell', $user3->name);

$user4 = DatabaseEloquentPostgresIntegrationUser::createOrFirst(
['name' => 'Dries Vints'],
['name' => 'Nuno Maduro', 'email' => '[email protected]']
);

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

class DatabaseEloquentPostgresIntegrationUser extends Model
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,14 @@
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
use Illuminate\Tests\Integration\Database\WithEloquentIntegrationTests;

class DatabaseEloquentSqlServerIntegrationTest extends SqlServerTestCase
{
use WithEloquentIntegrationTests;

protected $eloquentModelClass = DatabaseEloquentSqlServerIntegrationUser::class;

protected function defineDatabaseMigrationsAfterDatabaseRefreshed()
{
if (! Schema::hasTable('database_eloquent_sql_server_integration_users')) {
Expand All @@ -24,39 +29,6 @@ protected function destroyDatabaseMigrations()
{
Schema::drop('database_eloquent_sql_server_integration_users');
}

public function testCreateOrFirst()
{
$user1 = DatabaseEloquentSqlServerIntegrationUser::createOrFirst(['email' => '[email protected]']);

$this->assertSame('[email protected]', $user1->email);
$this->assertNull($user1->name);

$user2 = DatabaseEloquentSqlServerIntegrationUser::createOrFirst(
['email' => '[email protected]'],
['name' => 'Taylor Otwell']
);

$this->assertEquals($user1->id, $user2->id);
$this->assertSame('[email protected]', $user2->email);
$this->assertNull($user2->name);

$user3 = DatabaseEloquentSqlServerIntegrationUser::createOrFirst(
['email' => '[email protected]'],
['name' => 'Abigail Otwell']
);

$this->assertNotEquals($user3->id, $user1->id);
$this->assertSame('[email protected]', $user3->email);
$this->assertSame('Abigail Otwell', $user3->name);

$user4 = DatabaseEloquentSqlServerIntegrationUser::createOrFirst(
['name' => 'Dries Vints'],
['name' => 'Nuno Maduro', 'email' => '[email protected]']
);

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

class DatabaseEloquentSqlServerIntegrationUser extends Model
Expand Down
57 changes: 57 additions & 0 deletions tests/Integration/Database/WithEloquentIntegrationTests.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php

namespace Illuminate\Tests\Integration\Database;

use Illuminate\Support\Facades\DB;

trait WithEloquentIntegrationTests
{
public function testCreateOrFirst()
{
$user1 = $this->eloquentModelClass::createOrFirst(['email' => '[email protected]']);

$this->assertSame('[email protected]', $user1->email);
$this->assertNull($user1->name);

$user2 = $this->eloquentModelClass::createOrFirst(
['email' => '[email protected]'],
['name' => 'Taylor Otwell']
);

$this->assertEquals($user1->id, $user2->id);
$this->assertSame('[email protected]', $user2->email);
$this->assertNull($user2->name);

$user3 = $this->eloquentModelClass::createOrFirst(
['email' => '[email protected]'],
['name' => 'Abigail Otwell']
);

$this->assertNotEquals($user3->id, $user1->id);
$this->assertSame('[email protected]', $user3->email);
$this->assertSame('Abigail Otwell', $user3->name);

$user4 = $this->eloquentModelClass::createOrFirst(
['name' => 'Dries Vints'],
['name' => 'Nuno Maduro', 'email' => '[email protected]']
);

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

public function testCreateOrFirstWithinTransaction()
{
$user1 = $this->eloquentModelClass::createOrFirst(['email' => '[email protected]']);

DB::transaction(function () use ($user1) {
$user2 = $this->eloquentModelClass::createOrFirst(
['email' => '[email protected]'],
['name' => 'Taylor Otwell']
);

$this->assertEquals($user1->id, $user2->id);
$this->assertSame('[email protected]', $user2->email);
$this->assertNull($user2->name);
});
}
}
Loading