Merge pull request #1239 from beauby/fix-duplicate-jsonapi

Fix duplicate resources inside included in compound document.

CHANGELOG.md

@@ -21,6 +21,7 @@ Features:
 - [#1050](https://github.com/rails-api/active_model_serializers/pull/1050) Add support for toplevel jsonapi member (@beauby, @bf4)
 
 Fixes:
+- [#1239](https://github.com/rails-api/active_model_serializers/pull/1239) Fix duplicates in JSON API compound documents (@beauby)
 - [#1214](https://github.com/rails-api/active_model_serializers/pull/1214) retrieve the key from the reflection options when building associations (@NullVoxPopuli, @hut8)
 
 Misc:

lib/active_model/serializer/adapter/json_api.rb

@@ -81,16 +81,18 @@ module ActiveModel
 
         def serializable_hash_for_collection(options)
           hash = { data: [] }
+          included = []
           serializer.each do |s|
             result = self.class.new(s, instance_options.merge(fieldset: fieldset)).serializable_hash(options)
             hash[:data] << result[:data]
+            next unless result[:included]
 
-            if result[:included]
-              hash[:included] ||= []
-              hash[:included] |= result[:included]
-            end
+            included |= result[:included]
           end
 
+          included.delete_if { |resource| hash[:data].include?(resource) }
+          hash[:included] = included if included.any?
+
           if serializer.paginated?
             hash[:links] ||= {}
             hash[:links].update(links_for(serializer, options))
@@ -102,9 +104,10 @@ module ActiveModel
         def serializable_hash_for_single_resource
           primary_data = primary_data_for(serializer)
           relationships = relationships_for(serializer)
-          included = included_resources(@include_tree)
+          primary_data[:relationships] = relationships if relationships.any?
           hash = { data: primary_data }
-          hash[:data][:relationships] = relationships if relationships.any?
+
+          included = included_resources(@include_tree, [primary_data])
           hash[:included] = included if included.any?
 
           hash
@@ -171,31 +174,31 @@ module ActiveModel
           end
         end
 
-        def included_resources(include_tree)
+        def included_resources(include_tree, primary_data)
           included = []
 
           serializer.associations(include_tree).each do |association|
-            add_included_resources_for(association.serializer, include_tree[association.key], included)
+            add_included_resources_for(association.serializer, include_tree[association.key], primary_data, included)
           end
 
           included
         end
 
-        def add_included_resources_for(serializer, include_tree, included)
+        def add_included_resources_for(serializer, include_tree, primary_data, included)
           if serializer.respond_to?(:each)
-            serializer.each { |s| add_included_resources_for(s, include_tree, included) }
+            serializer.each { |s| add_included_resources_for(s, include_tree, primary_data, included) }
           else
             return unless serializer && serializer.object
 
-            primary_data = primary_data_for(serializer)
+            resource_object = primary_data_for(serializer)
             relationships = relationships_for(serializer)
-            primary_data[:relationships] = relationships if relationships.any?
+            resource_object[:relationships] = relationships if relationships.any?
 
-            return if included.include?(primary_data)
-            included.push(primary_data)
+            return if included.include?(resource_object) || primary_data.include?(resource_object)
+            included.push(resource_object)
 
             serializer.associations(include_tree).each do |association|
-              add_included_resources_for(association.serializer, include_tree[association.key], included)
+              add_included_resources_for(association.serializer, include_tree[association.key], primary_data, included)
             end
           end
         end

test/adapter/json_api/linked_test.rb

@@ -1,11 +1,16 @@
 require 'test_helper'
+
+NestedPost = Class.new(Model)
+class NestedPostSerializer < ActiveModel::Serializer
+  has_many :nested_posts
+end
+
 module ActiveModel
   class Serializer
     module Adapter
       class JsonApi
         class LinkedTest < Minitest::Test
           def setup
-            ActionController::Base.cache_store.clear
             @author1 = Author.new(id: 1, name: 'Steve K.')
             @author2 = Author.new(id: 2, name: 'Tenderlove')
             @bio1 = Bio.new(id: 1, content: 'AMS Contributor')
@@ -277,6 +282,112 @@ module ActiveModel
             assert_equal expected, adapter.serializable_hash
           end
         end
+
+        class NoDuplicatesTest < Minitest::Test
+          Post = Class.new(::Model)
+          Author = Class.new(::Model)
+
+          class PostSerializer < ActiveModel::Serializer
+            type 'posts'
+            belongs_to :author
+          end
+
+          class AuthorSerializer < ActiveModel::Serializer
+            type 'authors'
+            has_many :posts
+          end
+
+          def setup
+            @author = Author.new(id: 1, posts: [], roles: [], bio: nil)
+            @post1 = Post.new(id: 1, author: @author)
+            @post2 = Post.new(id: 2, author: @author)
+            @author.posts << @post1
+            @author.posts << @post2
+
+            @nestedpost1 = ::NestedPost.new(id: 1, nested_posts: [])
+            @nestedpost2 = ::NestedPost.new(id: 2, nested_posts: [])
+            @nestedpost1.nested_posts << @nestedpost1
+            @nestedpost1.nested_posts << @nestedpost2
+            @nestedpost2.nested_posts << @nestedpost1
+            @nestedpost2.nested_posts << @nestedpost2
+          end
+
+          def test_no_duplicates
+            hash = ActiveModel::SerializableResource.new(@post1, adapter: :json_api,
+                                                                 include: '*.*')
+                   .serializable_hash
+            expected = [
+              {
+                type: 'authors', id: '1',
+                relationships: {
+                  posts: {
+                    data: [
+                      { type: 'posts', id: '1' },
+                      { type: 'posts', id: '2' }
+                    ]
+                  }
+                }
+              },
+              {
+                type: 'posts', id: '2',
+                relationships: {
+                  author: {
+                    data: { type: 'authors', id: '1' }
+                  }
+                }
+              }
+            ]
+            assert_equal(expected, hash[:included])
+          end
+
+          def test_no_duplicates_collection
+            hash = ActiveModel::SerializableResource.new(
+              [@post1, @post2], adapter: :json_api,
+                                include: '*.*')
+                   .serializable_hash
+            expected = [
+              {
+                type: 'authors', id: '1',
+                relationships: {
+                  posts: {
+                    data: [
+                      { type: 'posts', id: '1' },
+                      { type: 'posts', id: '2' }
+                    ]
+                  }
+                }
+              }
+            ]
+            assert_equal(expected, hash[:included])
+          end
+
+          def test_no_duplicates_global
+            hash = ActiveModel::SerializableResource.new(
+              @nestedpost1,
+              adapter: :json_api,
+              include: '*').serializable_hash
+            expected = [
+              type: 'nested_posts', id: '2',
+              relationships: {
+                nested_posts: {
+                  data: [
+                    { type: 'nested_posts', id: '1' },
+                    { type: 'nested_posts', id: '2' }
+                  ]
+                }
+              }
+            ]
+            assert_equal(expected, hash[:included])
+          end
+
+          def test_no_duplicates_collection_global
+            hash = ActiveModel::SerializableResource.new(
+              [@nestedpost1, @nestedpost2],
+              adapter: :json_api,
+              include: '*').serializable_hash
+            assert_nil(hash[:included])
+          end
+        end
       end
     end
   end