[10.x] Revival of the reverted changes in 10.25.0: firstOrCreate updateOrCreate improvement through createOrFirst + additional query tests

[mpyw]

Prior Discussion

• [10.x] Revival of the reverted changes in 10.25.0: `firstOrCreate` `updateOrCreate` improvement through `createOrFirst` + additional query tests #48567

What Changes?

This PR unreverts #48531 and revives firstOrCreate updateOrCreate enhancement under createOrFirst. They were unreliable in previous versions. In high-load applications, unique key constraint errors would occur even when checking for the existence of a record beforehand. The purpose of refactoring with createOrFirst internally is to catch this and retry automatically.

Actual Implementation Changes

• Revert "[10.x] Revert from using createOrFirst in other *OrCreate methods"
  • Reinstate previously reverted changes.
• refactor: 💡 use createOrFirst in firstOrCreate
  • Fixed HasManyThrough::firstOrCreate to use createOrFirst. This was a previously overlooked point.

Coverages

• Builder::createOrFirst by @mpyw
• Builder::firstOrCreate by @mpyw
• Builder::updateOrCreate by @mpyw
• HasMany::createOrFirst by @mpyw
• HasMany::firstOrCreate by @mpyw
• HasMany::updateOrCreate by @mpyw
• HasManyThrough::createOrFirst by @fuwasegu + @mpyw
• HasManyThrough::firstOrCreate by @fuwasegu + @mpyw
• HasManyThrough::updateOrCreate by @fuwasegu + @mpyw
• BelongsToMany::createOrFirst by @mpyw
• BelongsToMany::firstOrCreate by @mpyw
• BelongsToMany::updateOrCreate by @mpyw

Notes

Some FIXME comments are remaining.

$model->getConnection()
    ->expects('select')
    // FIXME: duplicate conditions
    ->with('select * from "table" where ("attr" = ?) and ("attr" = ?) limit 1', ['foo', 'foo'], false)
    ->andReturn([[
        'id' => 123,
        'attr' => 'foo',
        'val' => 'bar',
        'created_at' => '2023-01-01 00:00:00',
        'updated_at' => '2023-01-01 00:00:00',
    ]]);

These indicate a problem with duplicate and useless conditions being given when retrying the SELECT, but do not significantly bother with the operation. Attempting to fix this using clone may yield some unexpected effects due to the Builder mutableness. Therefore, we'd like to recognize this as a known issue and leave it out of the scope of this PR.

@tonysm If you think you can fix this problem, could you please submit another PR? Thank you in advance.

[driesvints]

I honestly suggest to just leave things be. I also don't know what this is fixing again?

[mpyw]

@driesvints If you have any comments, please see #48567. I personally emailed @taylorotwell and asked him about the policy and he agreed that as long as we add a test it should be fine.

---

firstOrCreate updateOrCreate was unreliable in previous Laravel versions. In high-load applications, unique key constraint errors would occur even when checking for the existence of a record beforehand. The purpose of refactoring with createOrFirst internally is to catch this and retry automatically.

[mpyw]

@taylorotwell @crynobone @driesvints @tonysm @ralphjsmit @ryanldy @trevorgehman @SakiTakamachi @fuwasegu Please review the changes. Thank you in advance.

[morloderex]

@mpyw if we rely on a unique constraint and the model's attributes doesn't not include the primary key of the table. And the attributes used let's call it name in this example. Doesn't have a unique constraint on the table wouldn't this new behaviour of relying on a unique constraint violation not be a major breaking change, since the unique constraint would never actually occur from the database provider?

[mpyw]

@morloderex This is not a breaking change. It works as follows.

• Before
  • firstOrCreate() tries to retrieve through first() and calls create() if there is no result
  • updateOrCreate() tries to retrieve through first() and calls create() if there is no result, update() if there is a result.
• After
  • firstOrCreate() tries to retrieve through first() and calls createOrFirst() if there is no result
  • updateOrCreate() tries to retrieve through first() and calls createOrFirst() if there is no result, update() if there is a result.

The create() is just changed to createOrFirst(), not that firstOrCreate() updateOrCreate() will not work without unique constraints in the future. It only means that firstOrCreate() and updateOrCreate() will work more reliably with unique constraints.

[trevorgehman]

Doesn't have a unique constraint on the table wouldn't this new behaviour of relying on a unique constraint violation not be a major breaking change, since the unique constraint would never actually occur from the database provider?

Without a unique constraint on the table firstOrCreate() will work the same way as it does now.

[trevorgehman]

I also don't know what this is fixing again?

@driesvints Essentially it solves race conditions. In high concurrency environments we can't rely on firstOrCreate() in its current form because between the first() and the create() calls, another process may have already created the record. So the create() call will throw a UniqueConstraintViolationException in that case.

We have for a long time implemented a workaround like this by extending the Builder:

public function firstOrCreate(array $attributes = [], array $values = [])
    {
        try {
            return parent::firstOrCreate($attributes, $values);
        } catch (UniqueConstraintViolationException $e) {
            return parent::first($attributes) ?? throw $e;
        }
    }

This strategy of catching UniqueConstraintViolationException is what createOrFirst() is already doing. So rather than having the same try/catch block in firstOrCreate() @mpyw is just replacing the create() call with createOrFirst():

framework/src/Illuminate/Database/Eloquent/Builder.php

Lines 580 to 587 in fcceb52

	public function createOrFirst(array $attributes = [], array $values = [])
	{
	try {
	return $this->withSavepointIfNeeded(fn () => $this->create(array_merge($attributes, $values)));
	} catch (UniqueConstraintViolationException $e) {
	return $this->useWritePdo()->where($attributes)->first() ?? throw $e;
	}
	}

[deleugpn]

I don't envy Taylor on this PR at all 😅

[✗] This is a significant change to one of Laravel's most important modules (Eloquent).
[✔️] But it seems like this was only reverted because Eloquent already had existing bugs that were somewhat "invisible". So in the long run the introduction of this change made Eloquent better.
[✔️] It is a convenient addition to the framework .
[✔️] There are many tests added which covers a large set of functionailty. It may be some duplicate tests, but having extra tests often have no downsides.
[✗] But ultimately as an end-user, if you need this, how hard is it to search/replace firstOrCreate() with createOrFirst() on your codebase?

[SakiTakamachi]

@deleugpn

But ultimately as an end-user, if you need this, how hard is it to search/replace firstOrCreate() with createOrFirst() on your codebase?

I don't understand why it needs to be replaced.

[mpyw]

@deleugpn You do not need to replace them.

1. Many existing usages of firstOrCreate in Laravel likely occur in scenarios where occasionally something is not created, but most are already created. It operates by SELECT from a replica and then INSERT into the primary if it doesn’t exist. The only additional behavior is re-SELECT from the primary if there’s a collision' afterward. In prescribed situations, continuing to use firstOrCreate is entirely acceptable.
2. Conversely, createOrFirst operates very efficiently in cases where most are not created, but some are. If you use this in scenario 1, not only does the attempt to INSERT unnecessarily burden the database, but it also negatively consumes the auto-increment number space.

Generally, continuing to use firstOrCreate should pose no issues. Therefore, you rarely need to – and likely should not – rewrite something previously written with firstOrCreate to use createOrFirst instead.

[trevorgehman]

[✗] But ultimately as an end-user, if you need this, how hard is it to search/replace firstOrCreate() with createOrFirst() on your codebase?

They have different use-cases. If 95% of the time the record already exists, then firstOrCreate() makes more sense and will be more performant.

[mpyw]

@trevorgehman Exactly the same opinion with you 😄

[mpyw]

We also hope that this change will be covered in Laravel News. It is not a BC, but I think it is significant to let people know.

[mpyw]

19f8b61: Some of afterCommit-related tests are fixed, so we merged the latest 10.x branch just in case to confirm all tests pass.

❯ git merge upstream/10.x
Merge made by the 'ort' strategy.
 src/Illuminate/Console/GeneratorCommand.php                            |   9 ++++--
 src/Illuminate/Database/Concerns/BuildsQueries.php                     |  35 ++++++++++++++++++++++-
 src/Illuminate/Database/Concerns/ManagesTransactions.php               |  11 ++++----
 src/Illuminate/Database/DatabaseTransactionsManager.php                |  30 +-------------------
 src/Illuminate/Database/Eloquent/Model.php                             |   1 +
 src/Illuminate/Database/Query/Grammars/Grammar.php                     |   3 +-
 src/Illuminate/Foundation/Application.php                              |   2 +-
 src/Illuminate/Foundation/Testing/DatabaseTransactionsManager.php      |   2 +-
 src/Illuminate/Mail/Transport/SesTransport.php                         |   4 +--
 src/Illuminate/Mail/Transport/SesV2Transport.php                       |   4 +--
 src/Illuminate/Support/Str.php                                         |  19 ++++++++++++-
 src/Illuminate/View/Compilers/BladeCompiler.php                        |   4 +--
 tests/Database/DatabaseConnectionTest.php                              |  15 ++++++++++
 tests/Database/DatabaseEloquentModelTest.php                           | 150 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/Database/DatabaseQueryBuilderTest.php                            |  20 +++++++++++++
 tests/Database/DatabaseQueryGrammarTest.php                            |  44 +++++++++++++++++++++++++++++
 tests/Integration/Console/GeneratorCommandTest.php                     |  28 ++++++++++++++++++
 tests/Integration/Database/EloquentTransactionWithAfterCommitTests.php |  48 +++++++++++++++++++++++++++++++
 tests/Integration/Database/QueryBuilderTest.php                        |  38 +++++++++++++++++++++++++
 tests/Integration/Queue/ThrottlesExceptionsWithRedisTest.php           |  15 +++++-----
 tests/Mail/MailSesTransportTest.php                                    |  20 +++++++++++++
 tests/Mail/MailSesV2TransportTest.php                                  |  20 +++++++++++++
 tests/Support/SupportStrTest.php                                       |   2 ++
 23 files changed, 467 insertions(+), 57 deletions(-)
 create mode 100644 tests/Database/DatabaseQueryGrammarTest.php
 create mode 100644 tests/Integration/Console/GeneratorCommandTest.php

[akr4m]

Impressive refinement of firstOrCreate and updateOrCreate to handle high-load scenarios. Extensive testing bolsters confidence. Great work @mpyw! 👍

[taylorotwell]

Thanks - this is probably our last attempt at this. 😬

[tonysm]

Great work on this @mpyw and @fuwasegu. It's looking good!

[nunomaduro]

@mpyw Can you check this bug report? #49514.

[mpyw]

@nunomaduro Okay, I'll check it out