[Bugfix] Ensure duplicates are not added to a has-many relation

lindyhopchris · lindyhopchris · commit 869aca9a3504 · 2018-09-21T14:48:34.000+01:00
The spec states that duplicates must not be added, but the default Laravel behaviour is to add duplicates. This commit changes the has-many relation so that it filters out duplicates before adding them on a `POST` request. Closes cloudcreativity#222
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,12 @@ resolver instance.
 ### Changed
 - Extract model sorting from the Eloquent adapter into a `SortsModels` trait.
 
+### Fixed
+- [#222](https://github.com/cloudcreativity/laravel-json-api/issues/222)
+Adding related resources to a has-many relationship now does not add duplicates. The JSON API
+spec states that duplicates must not be added, but the default Laravel behaviour does add duplicates.
+The `HasMany` relationship now takes care of this by filtering out duplicates before adding them.
+
 ### Deprecated
 - The following methods on the Eloquent adapter will be removed in `1.0.0` as they are no longer required:
   - `extractIncludePaths`
diff --git a/src/Eloquent/HasMany.php b/src/Eloquent/HasMany.php
@@ -67,6 +67,12 @@ public function replace($record, RelationshipInterface $relationship, EncodingPa
     }
 
     /**
+     * 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 RelationshipInterface $relationship
      * @param EncodingParametersInterface $parameters
@@ -75,8 +81,14 @@ public function replace($record, RelationshipInterface $relationship, EncodingPa
     public function add($record, RelationshipInterface $relationship, EncodingParametersInterface $parameters)
     {
         $related = $this->findRelated($record, $relationship);
+        $relation = $this->getRelation($record, $this->key);
+
+        $existing = $relation
+            ->getQuery()
+            ->whereKey($related->modelKeys())
+            ->get();
 
-        $this->getRelation($record, $this->key)->saveMany($related);
+        $relation->saveMany($related->diff($existing));
         $record->refresh(); // in case the relationship has been cached.
 
         return $record;
diff --git a/tests/lib/Integration/Eloquent/MorphToManyTest.php b/tests/lib/Integration/Eloquent/MorphToManyTest.php
@@ -330,6 +330,31 @@ public function testAddToRelationship()
         $this->assertTagsAre($post, $existing->merge($add));
     }
 
+    /**
+     * From the spec:
+     *
+     * > If a client makes a POST request to a URL from a relationship link,
+     * > the server MUST add the specified members to the relationship unless
+     * > they are already present. If a given type and id is already in the
+     * > relationship, the server MUST NOT add it again.
+     */
+    public function testAddToRelationshipDoesNotCreateDuplicates()
+    {
+        $post = factory(Post::class)->create();
+        $existing = factory(Tag::class, 2)->create();
+        $post->tags()->sync($existing);
+
+        $add = factory(Tag::class, 2)->create();
+        $data = $add->merge($existing)->map(function (Tag $tag) {
+            return ['type' => 'tags', 'id' => $tag->getRouteKey()];
+        })->all();
+
+        $this->doAddToRelationship($post, 'tags', $data)
+            ->assertStatus(204);
+
+        $this->assertTagsAre($post, $existing->merge($add));
+    }
+
     public function testRemoveFromRelationship()
     {
         $post = factory(Post::class)->create();
@@ -346,6 +371,29 @@ public function testRemoveFromRelationship()
         $this->assertTagsAre($post, [$tags->get(2), $tags->get(3)]);
     }
 
+    /**
+     * From the spec:
+     *
+     * > If all of the specified resources are able to be removed from,
+     * > or are already missing from, the relationship then the server
+     * > MUST return a successful response.
+     */
+    public function testRemoveWithIdsThatAreNotRelated()
+    {
+        $post = factory(Post::class)->create();
+        $tags = factory(Tag::class, 2)->create();
+        $post->tags()->sync($tags);
+
+        $data = factory(Tag::class, 2)->create()->map(function (Tag $tag) {
+            return ['type' => 'tags', 'id' => $tag->getRouteKey()];
+        })->all();
+
+        $this->doRemoveFromRelationship($post, 'tags', $data)
+            ->assertStatus(204);
+
+        $this->assertTagsAre($post, $tags);
+    }
+
     /**
      * @param $post
      * @param $tag
