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] Revert from using createOrFirst in other *OrCreate methods #48531

Conversation

tonysm
Copy link
Contributor

@tonysm tonysm commented Sep 25, 2023

Changed

  • After the addition of createOrFirst, other *OrCreate methods were updated to use it behind the scenes. It turns out this was more problematic than we initially thought. This PR reverts using createOrFirst in other methods, but we'll keep it around for the specific scenario it was designed for: using unique constraints to determine if it should create or find the record.

I've tested the test case added in #48529 and it passes when we revert these changes here.

There was a comment here mentioning an issue, which might be related (it was also on updateOrCreate). We didn't have a way to reproduce the issue.

Reverted PRs:

@tonysm tonysm marked this pull request as ready for review September 25, 2023 14:23
@tonysm tonysm changed the title Revert from using createOrFirst in other *OrCreate methods [10.x] Revert from using createOrFirst in other *OrCreate methods Sep 25, 2023
@taylorotwell taylorotwell merged commit 408a3e3 into laravel:10.x Sep 25, 2023
20 checks passed
@tonysm
Copy link
Contributor Author

tonysm commented Sep 25, 2023

I went ahead to dig a bit more into what was going on over here and found a few things:

  1. The firstOrCreate method is missing on the HasManyThrough, which was causing the query call to be forwarded to the Eloquent Query Builder
  2. The HasManyThrough::firstOrNew differs from the one from other places. Nothing serious, but it doesn't match, so it may cause some other issues (it was already like that before the introduction of the createOrFirst method)

1. The firstOrCreate method is missing on HasManyThrough

The firstOrCreate method is missing in the HasManyThrough, so when the updateOrCreate method was calling it, the call was forwarded to the Eloquent Builder, which was the root cause. When I added the firstOrCreate method definition there, the issue was fixed.

The queries it was forming when forwarding to the query builder:

select * from "users" inner join "teams" on "teams"."id" = "users"."team_id" where "teams"."owner_id" = 3 and ("name" = "John") limit 1

update "users" set "slug" = "john-doe" where "id" = 1

The queries it was forming when I added the firstOrCreate definition to the HasManyThrough method:

select "users".*, "teams"."owner_id" as "laravel_through_key" from "users" inner join "teams" on "teams"."id" = "users"."team_id" where "teams"."owner_id" = 3 and ("name" = "John") limit 1

update "users" set "slug" = "john-doe" where "id" = 2

We can see that because it was forwarding the call to the base query builder, it was doing a select *, and when that's combined with the inner join, we were getting two IDs back, the one from the teams table and the one from the users table.

Adding the firstOrCreate method definition to the HasManyThrough fixes the issue because it does a select users.*, teams.owner_id instead of a select *.

2. The HasManyThrough::firstOrNew signature is different than in other places

I noticed that the HasManyThrough::firstOrNew method signature differs from the ones in other places. This doesn't seem to have anything to do with the issue, but it may cause other issues (I'm surprised it's like that):

grep -Rn " firstOrNew" src   
src/Illuminate/Database/Eloquent/Builder.php:552:    public function firstOrNew(array $attributes = [], array $values = [])
src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php:218:    public function firstOrNew(array $attributes = [], array $values = [])
src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php:603:    public function firstOrNew(array $attributes = [], array $values = [])
src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php:257:    public function firstOrNew(array $attributes)

As you can see, it only takes the unique attributes and doesn't even accept the values.


I suggest that we do:

  1. Add the firstOrCreate as well as the createOrFirst (this one is also missing) methods to the HasManyThrough relation class to avoid other issues if someone tries to use them and it gets forwarded to the base query builder instead
  2. Fix the firstOrNew method signature & implementation in the HasManyThrough

@tonysm
Copy link
Contributor Author

tonysm commented Sep 26, 2023

I've implemented the suggestions in #48541 and #48542.

mpyw added a commit to mpyw/framework that referenced this pull request Oct 5, 2023
taylorotwell pushed a commit that referenced this pull request Oct 12, 2023
…pdateOrCreate` improvement through `createOrFirst` + additional query tests (#48637)

* Revert "[10.x] Revert from using `createOrFirst` in other `*OrCreate` methods (#48531)"

This reverts commit 408a3e3.

* test: 💍 Add `Builder::createOrFirst()` snapshot tests

* test: 💍 Add `Builder::firstOrCreate()` snapshot tests

* test: 💍 Add `Builder::updateOrCreate()` snapshot tests

* test: 💍 Add test stubs for `DatabaseEloquentHasManyTest`

* test: 💍 Add `HasMany::createOrFirst()` snapshot tests

* test: 💍 Add `HasMany::firstOrCreate()` snapshot tests

* test: 💍 Add `HasMany::updateOrCreate()` snapshot tests

* test: 💍 prepare HasManyThrough test

* test: 💍 Add HasManyThrough::CreateOrFirst() snapshot tests

* test: 💍 Add HasManyThrough::firstOrCreate() snapshot test

* test: 💍 Add HasManyThrough::updateOrCreate() snapshot test

* refactor: 💡 use createOrFirst in firstOrCreate

* test: 💍 fix test

* style: 💄 Apply StyleCI fixes

* docs: ✏️ Add missing FIXME comments

* refactor: 💡 Omit verbose arguments

* test: 💍 Rename `DatabaseEloquentHasManyThroughTest` with fixes

* test: 💍 Add `BelongsToMany::createOrFirst/firstOrCreate` tests

* test: 💍 Extract `DatabaseEloquentHasManyTest` cases with fixes

* test: 💍 Extract `DatabaseEloquentBuilderTest` cases with fixes

* test: 💍 refactoring

* test: 💍 Add `BelongsToMany::updateOrCreate` snapshot tests

---------

Co-authored-by: fuwasegu <admin@fuwasegu.com>
timacdonald pushed a commit to timacdonald/framework that referenced this pull request Oct 24, 2023
…pdateOrCreate` improvement through `createOrFirst` + additional query tests (laravel#48637)

* Revert "[10.x] Revert from using `createOrFirst` in other `*OrCreate` methods (laravel#48531)"

This reverts commit 408a3e3.

* test: 💍 Add `Builder::createOrFirst()` snapshot tests

* test: 💍 Add `Builder::firstOrCreate()` snapshot tests

* test: 💍 Add `Builder::updateOrCreate()` snapshot tests

* test: 💍 Add test stubs for `DatabaseEloquentHasManyTest`

* test: 💍 Add `HasMany::createOrFirst()` snapshot tests

* test: 💍 Add `HasMany::firstOrCreate()` snapshot tests

* test: 💍 Add `HasMany::updateOrCreate()` snapshot tests

* test: 💍 prepare HasManyThrough test

* test: 💍 Add HasManyThrough::CreateOrFirst() snapshot tests

* test: 💍 Add HasManyThrough::firstOrCreate() snapshot test

* test: 💍 Add HasManyThrough::updateOrCreate() snapshot test

* refactor: 💡 use createOrFirst in firstOrCreate

* test: 💍 fix test

* style: 💄 Apply StyleCI fixes

* docs: ✏️ Add missing FIXME comments

* refactor: 💡 Omit verbose arguments

* test: 💍 Rename `DatabaseEloquentHasManyThroughTest` with fixes

* test: 💍 Add `BelongsToMany::createOrFirst/firstOrCreate` tests

* test: 💍 Extract `DatabaseEloquentHasManyTest` cases with fixes

* test: 💍 Extract `DatabaseEloquentBuilderTest` cases with fixes

* test: 💍 refactoring

* test: 💍 Add `BelongsToMany::updateOrCreate` snapshot tests

---------

Co-authored-by: fuwasegu <admin@fuwasegu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants