BelongsToMany#updateOrCreate is checking the wrong table for Model existence

[Harti]

• Laravel Version: 7.25.0
• PHP Version: 7.4.7
• Database Driver & Version: MySQL 8.0.20

Description:

For Illuminate\Database\Eloquent\Relations\BelongsToMany relations only, the updateOrCreate() method is checking the pivot table instead of the related table for Model existence:

Illuminate\Database\Eloquent\Relations\BelongsToMany

    public function updateOrCreate(array $attributes, array $values = [], array $joining = [], $touch = true)
    {
        if (is_null($instance = $this->where($attributes)->first())) {
            return $this->create($values, $joining, $touch);
        }

        $instance->fill($values);

        $instance->save(['touch' => false]);

        return $instance;
    }

It appears this was an oversight because the BelongsToMany#create method however would then proceed to create an entity in $this->related.

As a side note, the HasOneOrMany#updateOrCreate method is vastly different with its tap($this->firstOrNew(...), ...) call.

Proposed fix (change first line in method body):

    public function updateOrCreate(array $attributes, array $values = [], array $joining = [], $touch = true)
    {
        if (is_null($instance = $this->related->where($attributes)->first())) {
            return $this->create($values, $joining, $touch);
        }

        $instance->fill($values);

        $instance->save(['touch' => false]);

        return $instance;
    }

Steps To Reproduce:

Example Gist with migrations, test and stacktrace (friend User-User relation): https://gist.github.com/Harti/2afb5afd524f3639045b3d57756655fd

Inline Example:

Role::create(['name' => 'existing.role', 'description' => 'foo']);
$user->roles()->updateOrCreate(['name' => 'existing.role'], ['description' => 'bar']);

parent::assertEquals(1, Role::count()); // assertion fails (or is never reached); count equals 2

A second/duplicate entry is inserted into the roles table (or a Integrity Constraint Violation is thrown, should name be unique).
Expected behavior: The existing Role is updated, and saved to the $user->roles() relation (which also only seems to happen if the instance was newly created).

Best regards,

Harti

[driesvints]

Can you please post your models/pivots?

[Harti]

@driesvints
This was entirely made up due to NDA, so I created another example with as little boilerplate as possible in a fresh Laravel installation:
https://gist.github.com/Harti/2afb5afd524f3639045b3d57756655fd

[Harti]

How can I streamline verification for you?

/edit: Didn’t mean to appear pushy, just trying to assist as best as possible.

[driesvints]

I haven't gotten to this yet sorry. Bit swamped atm.

[lk77]

Hello, is this only related to recursive relations ?

[Harti]

@lk77 no. Any many-to-many will do.

[taylorotwell]

That is kind of unexpected. It's been this way for at least 4 years and I'm surprised it has not come up before. I do worry about potentially breaking changes if people have depended on this checking the pivot table attributes.

[taylorotwell]

Looks like it was added in a PR in 2015: 955c9d7

[LeoCodes21]

I seem to remember encountering this several years ago (back in 2015, IIRC.) I thought I was just using the API incorrectly, but it still caused me a great deal of confusion. At that point, I believe the documentation reflected the intended functionality of update, not the actual (improper) function.

I think it mainly comes down to confusion between update() and sync(). I'm pretty sure changing my code to use sync solved the problem, although the application is so old that I'd have to do quite a bit of digging to find it. In any case, this should be fixed somehow. However, as @taylorotwell mentioned, that runs the risk of breaking older applications that somehow rely on this incorrect behavior.

As an alternative to modifying this part of the code and potentially breaking applications, it might be helpful to update the documentation for many-to-many relationships to clearly point out this pitfall. A warning of what not to do certainly would have helped me 5 years ago, but it's never too late to help others.

[Harti]

@taylorotwell Even so, absence of the pivot would then result in the creation of a related model, which may not include the corresponding DB column for the attribute they checked for, causing just a different kind of PDOException.

I think it could and should be treated as a bug fix for that inconsistency alone. Albeit anecdotal evidence, I haven’t found many threads when researching this issue. Didn’t seem to have ever been mentioned on the Laravel GitHub either.

Alternatively, there could be a bool flag option or something in the method signature, with the default choosing the pivot?

[lk77]

i have a similar issue too :

Integrity constraint violation: 1052 Column 'id' in where clause is ambiguous 

there is no ambiguity whatsoever, and it should work fine. When replacing $this->where($attributes)->first() by $this->related->where($attributes)->first() it works fine

dd($group->users()->updateOrCreate([
            'id' => '897'
        ], ['name' => 'test']));

[taylorotwell]

Fixed in 9.x.

[Harti]

Should any of you require the fixed behavior in your application right now, you could override the Relation type like so:

<?php

namespace App\Models\Relations;


use Log;

class BelongsToMany extends \Illuminate\Database\Eloquent\Relations\BelongsToMany
{
    private static bool $loggedNotice = false;

    /**
     * @param   array  $attributes
     * @param   array  $values
     * @param   array  $joining
     * @param   bool   $touch
     *
     * @return \Illuminate\Database\Eloquent\Builder|\Illuminate\Database\Eloquent\Model|object|null
     */
    public function updateOrCreate(array $attributes, array $values = [], array $joining = [], $touch = true)
    {
        if(!self::$loggedNotice) {
            Log::notice("Laravel's original `BelongsToMany#updateOrCreate()` method was overridden by"
                       . " `App\Models\Relationships\BelongsToMany#updateOrCreate()`");
            self::$loggedNotice = true;
        }

        if (is_null($instance = $this->related->where($attributes)->first())) {
            return $this->create($values, $joining, $touch);
        }

        $instance->fill($values);

        $instance->save(['touch' => false]);

        return $instance;
    }

}

And put this into the affected Model's class:

class Foo extends Illuminate\Database\Eloquent\Model
{

    // ... 

    /**
     * This method extends the default Laravel BelongsToMany by returning our custom BelongsToMany instance instead.
     *
     * @param   \Illuminate\Database\Eloquent\Builder  $query
     * @param   \Illuminate\Database\Eloquent\Model    $parent
     * @param   string                                 $table
     * @param   string                                 $foreignPivotKey
     * @param   string                                 $relatedPivotKey
     * @param   string                                 $parentKey
     * @param   string                                 $relatedKey
     * @param   null                                   $relationName
     *
     * @return \App\Models\Relations\BelongsToMany|\Illuminate\Database\Eloquent\Relations\BelongsToMany
     */
    protected function newBelongsToMany(Builder $query, EloquentModel $parent, $table, $foreignPivotKey, $relatedPivotKey, $relatedKey, $relationName = null) {
        return new \App\Models\Relations\BelongsToMany($query, $parent, $table, $foreignPivotKey, $relatedPivotKey, $parentKey, $relatedKey);
    }