cloudcreativity / laravel-json-api

JSON API (jsonapi.org) package for Laravel applications.
http://laravel-json-api.readthedocs.io/en/latest/
Apache License 2.0
780 stars 109 forks source link

Adding existing many to many relationship with incrementing pivot #580

Closed bbprojectnet closed 3 years ago

bbprojectnet commented 3 years ago

Hi,

When i try to add again same relation to many-to-many relationship, the relation is not added - it's ok.

But, in the same case, when i have pivot model with $incrementing = true, it works weird. For first record in pivot table is works as excpected, duplicates are not added, but for any other record (ID != 1) they are - what causes the error if we have unique constraint in database for related columns.

lindyhopchris commented 3 years ago

Hi! Does it work when you use the regular eloquent methods on the relationship? I.e. if you don't do it via the JSON API package and use the Eloquent relationship methods directly?

bbprojectnet commented 3 years ago

Hello,

Yes, it's "works", but eloquent reletionships don't care about duplicates, so in incremental primary key case, when i don't have unique constraint new record will be added each time (it's ok), in other case database throw error when record currently exists (it's also ok).

As i can see in HasMany relation for adapter:

    /**
     * Add records to the relationship.
     *
     * Note that the spec says that duplicates MUST NOT be added. The default Laravel
     * behaviour is to add duplicates, therefore we need to do some work to ensure
     * that we only add the records that are not already in the relationship.
     *
     * @param Model $record
     * @param array $relationship
     * @param EncodingParametersInterface $parameters
     * @return Model
     */
    public function add($record, array $relationship, EncodingParametersInterface $parameters)
    {
        $related = $this->findRelated($record, $relationship);
        $relation = $this->getRelation($record, $this->key);

        $existing = $relation
            ->getQuery()
            ->whereKey($related->modelKeys())
            ->get();

        $relation->saveMany($related->diff($existing));
        $record->refresh(); // in case the relationship has been cached.

        return $record;
    }

json-api should care this, and no add duplicates, and it's works fine when pivot table don't have incremental primary key column, in other case not (only for existsing ID=1 it's ok). I think there is a problem in query in this method, pivot ID overlap ID in main result and trick whereKey($related->modelKeys()) condition. Probably FQCM should solve the problem.

lindyhopchris commented 3 years ago

@bbprojectnet sorry for slow reply - I seem to have missed the notification for your reply!

Have had a look through this and I can't see any tests for an Eloquent belongsToMany() relation - which is a bit of an omission! I'll add some tests and see if I can reproduce the problem you encountered.

The code you've pasted does imply that it should be working as it only adds the diff between the values submitted by the client and the existing models. So I'll see whether it passes or not in tests.

lindyhopchris commented 3 years ago

As suspected, this line:

$relation->saveMany($related->diff($existing));

Means that duplicates are not added. I've added tests that show the code is working. So at the moment, I can't see any bug to fix.

You'll need to debug in your application to see what's going on. If you are sure that this is a bug in this package, then I need a PR with a failing test to show the bug can be reproduced.

Closing this issue for now, but please reopen if you can reproduce the bug in the tests.