Fix issue with relationships in derived resources

Resources derived from other resources with relationships were not correctly setting up the copied relationships.
richa · Dec 3, 2015 · 2c5dc46 · 2c5dc46
1 parent 6117675
commit 2c5dc46
Show file tree

Hide file tree

Showing 4 changed files with 110 additions and 73 deletions.
diff --git a/lib/jsonapi/relationship.rb b/lib/jsonapi/relationship.rb
@@ -1,14 +1,15 @@
 module JSONAPI
   class Relationship
     attr_reader :acts_as_set, :foreign_key, :type, :options, :name,
-                :class_name, :polymorphic, :always_include_linkage_data
+                :class_name, :polymorphic, :always_include_linkage_data,
+                :parent_resource
 
     def initialize(name, options = {})
       @name = name.to_s
       @options = options
       @acts_as_set = options.fetch(:acts_as_set, false) == true
       @foreign_key = options[:foreign_key] ? options[:foreign_key].to_sym : nil
-      @module_path = options[:module_path] || ''
+      @parent_resource = options[:parent_resource]
       @relation_name = options.fetch(:relation_name, @name)
       @polymorphic = options.fetch(:polymorphic, false) == true
       @always_include_linkage_data = options.fetch(:always_include_linkage_data, false) == true
@@ -21,7 +22,7 @@ def primary_key
     end
 
     def resource_klass
-      @resource_klass ||= Resource.resource_for(@module_path + @class_name)
+      @resource_klass = @parent_resource.resource_for(@class_name)
     end
 
     def relation_name(options)

diff --git a/lib/jsonapi/resource.rb b/lib/jsonapi/resource.rb
@@ -4,8 +4,6 @@ module JSONAPI
   class Resource
     include Callbacks
 
-    @@resource_types = {}
-
     attr_reader :context
 
     define_jsonapi_resources_callbacks :create,
@@ -281,8 +279,19 @@ def inherited(subclass)
         subclass.abstract(false)
         subclass.immutable(false)
         subclass._attributes = (_attributes || {}).dup
+        subclass.model_name(_model_name, add_model_hint: false) unless _model_name == ''
         subclass._model_hints = (_model_hints || {}).dup
-        subclass._relationships = (_relationships || {}).dup
+
+        subclass._relationships = {}
+        # Add the relationships from the base class to the subclass using the original options
+        if _relationships.is_a?(Hash)
+          _relationships.each_value do |relationship|
+            options = relationship.options.dup
+            options[:parent_resource] = subclass
+            subclass._add_relationship(relationship.class, relationship.name, options)
+          end
+        end
+
         subclass._allowed_filters = (_allowed_filters || Set.new).dup
 
         type = subclass.name.demodulize.sub(/Resource$/, '').underscore
@@ -741,49 +750,28 @@ def construct_order_options(sort_params)
         end
       end
 
-      private
-
-      def check_reserved_resource_name(type, name)
-        if [:ids, :types, :hrefs, :links].include?(type)
-          warn "[NAME COLLISION] `#{name}` is a reserved resource name."
-          return
-        end
-      end
-
-      def check_reserved_attribute_name(name)
-        # Allow :id since it can be used to specify the format. Since it is a method on the base Resource
-        # an attribute method won't be created for it.
-        if [:type].include?(name.to_sym)
-          warn "[NAME COLLISION] `#{name}` is a reserved key in #{@@resource_types[_type]}."
-        end
-      end
-
-      def check_reserved_relationship_name(name)
-        if [:id, :ids, :type, :types].include?(name.to_sym)
-          warn "[NAME COLLISION] `#{name}` is a reserved relationship name in #{@@resource_types[_type]}."
-        end
-      end
-
       def _add_relationship(klass, *attrs)
         options = attrs.extract_options!
-        options[:module_path] = module_path
+        options[:parent_resource] = self
 
         attrs.each do |attr|
-          check_reserved_relationship_name(attr)
+          relationship_name = attr.to_sym
+
+          check_reserved_relationship_name(relationship_name)
 
           # Initialize from an ActiveRecord model's properties
           if _model_class && _model_class.ancestors.collect{|ancestor| ancestor.name}.include?('ActiveRecord::Base')
-            model_association = _model_class.reflect_on_association(attr)
+            model_association = _model_class.reflect_on_association(relationship_name)
             if model_association
               options[:class_name] ||= model_association.class_name
             end
           end
 
-          @_relationships[attr] = relationship = klass.new(attr, options)
+          @_relationships[relationship_name] = relationship = klass.new(relationship_name, options)
 
           associated_records_method_name = case relationship
-                                           when JSONAPI::Relationship::ToOne then "record_for_#{attr}"
-                                           when JSONAPI::Relationship::ToMany then "records_for_#{attr}"
+                                           when JSONAPI::Relationship::ToOne then "record_for_#{relationship_name}"
+                                           when JSONAPI::Relationship::ToMany then "records_for_#{relationship_name}"
                                            end
 
           foreign_key = relationship.foreign_key
@@ -793,6 +781,7 @@ def _add_relationship(klass, *attrs)
           end unless method_defined?("#{foreign_key}=")
 
           define_method associated_records_method_name do
+            relationship = self.class._relationships[relationship_name]
             relation_name = relationship.relation_name(context: @context)
             records_for(relation_name)
           end unless method_defined?(associated_records_method_name)
@@ -803,7 +792,9 @@ def _add_relationship(klass, *attrs)
                 @model.method(foreign_key).call
               end unless method_defined?(foreign_key)
 
-              define_method attr do |options = {}|
+              define_method relationship_name do |options = {}|
+                relationship = self.class._relationships[relationship_name]
+
                 if relationship.polymorphic?
                   associated_model = public_send(associated_records_method_name)
                   resource_klass = self.class.resource_for_model(associated_model) if associated_model
@@ -815,21 +806,25 @@ def _add_relationship(klass, *attrs)
                     return associated_model ? resource_klass.new(associated_model, @context) : nil
                   end
                 end
-              end unless method_defined?(attr)
+              end unless method_defined?(relationship_name)
             else
               define_method foreign_key do
+                relationship = self.class._relationships[relationship_name]
+
                 record = public_send(associated_records_method_name)
                 return nil if record.nil?
                 record.public_send(relationship.resource_klass._primary_key)
               end unless method_defined?(foreign_key)
 
-              define_method attr do |options = {}|
+              define_method relationship_name do |options = {}|
+                relationship = self.class._relationships[relationship_name]
+
                 resource_klass = relationship.resource_klass
                 if resource_klass
                   associated_model = public_send(associated_records_method_name)
                   return associated_model ? resource_klass.new(associated_model, @context) : nil
                 end
-              end unless method_defined?(attr)
+              end unless method_defined?(relationship_name)
             end
           elsif relationship.is_a?(JSONAPI::Relationship::ToMany)
             define_method foreign_key do
@@ -838,7 +833,10 @@ def _add_relationship(klass, *attrs)
                 record.public_send(relationship.resource_klass._primary_key)
               end
             end unless method_defined?(foreign_key)
-            define_method attr do |options = {}|
+
+            define_method relationship_name do |options = {}|
+              relationship = self.class._relationships[relationship_name]
+
               resource_klass = relationship.resource_klass
               records = public_send(associated_records_method_name)
 
@@ -859,16 +857,38 @@ def _add_relationship(klass, *attrs)
               end
 
               return records.collect do |record|
-                resource_klass = self.class.resource_for_model(record)
-                if current_relationship.polymorphic?
+                if relationship.polymorphic?
                   resource_klass = self.class.resource_for_model(record)
                 end
                 resource_klass.new(record, @context)
               end
-            end unless method_defined?(attr)
+            end unless method_defined?(relationship_name)
           end
         end
       end
+
+      private
+
+      def check_reserved_resource_name(type, name)
+        if [:ids, :types, :hrefs, :links].include?(type)
+          warn "[NAME COLLISION] `#{name}` is a reserved resource name."
+          return
+        end
+      end
+
+      def check_reserved_attribute_name(name)
+        # Allow :id since it can be used to specify the format. Since it is a method on the base Resource
+        # an attribute method won't be created for it.
+        if [:type].include?(name.to_sym)
+          warn "[NAME COLLISION] `#{name}` is a reserved key in #{_resource_name_from_type(_type)}."
+        end
+      end
+
+      def check_reserved_relationship_name(name)
+        if [:id, :ids, :type, :types].include?(name.to_sym)
+          warn "[NAME COLLISION] `#{name}` is a reserved relationship name in #{_resource_name_from_type(_type)}."
+        end
+      end
     end
   end
 end
diff --git a/test/controllers/controller_test.rb b/test/controllers/controller_test.rb
@@ -3109,19 +3109,17 @@ class Api::V1::MoonsControllerTest < ActionController::TestCase
    def test_get_related_resource
       get :get_related_resource, {crater_id: 'S56D', relationship: 'moon', source: "api/v1/craters"}
       assert_response :success
-      assert_hash_equals json_response,
-                              {
-                                data: {
-                                  id: "1",
-                                  type: "moons",
-                                  links: {self: "http://test.host/moons/1"},
-                                  attributes: {name: "Titan", description: "Best known of the Saturn moons."},
-                                  relationships: {
-                                    planet: {links: {self: "http://test.host/moons/1/relationships/planet", related: "http://test.host/moons/1/planet"}},
-                                    craters: {links: {self: "http://test.host/moons/1/relationships/craters", related: "http://test.host/moons/1/craters"}}}
-                                  }
-                                }
-
+      assert_hash_equals({
+                           data: {
+                             id: "1",
+                             type: "moons",
+                             links: {self: "http://test.host/api/v1/moons/1"},
+                             attributes: {name: "Titan", description: "Best known of the Saturn moons."},
+                             relationships: {
+                               planet: {links: {self: "http://test.host/api/v1/moons/1/relationships/planet", related: "http://test.host/api/v1/moons/1/planet"}},
+                               craters: {links: {self: "http://test.host/api/v1/moons/1/relationships/craters", related: "http://test.host/api/v1/moons/1/craters"}}}
+                             }
+                           }, json_response)
    end
 
    def test_get_related_resources_with_select_some_db_columns
@@ -3150,23 +3148,24 @@ def test_show_single
   def test_get_related_resources
     get :get_related_resources, {moon_id: '1', relationship: 'craters', source: "api/v1/moons"}
     assert_response :success
-    assert_hash_equals json_response,
-                      {
-                        data: [
-                          {id:"A4D3",
-                           type:"craters",
-                           links:{self: "http://test.host/api/v1/craters/A4D3"},
-                           attributes:{code: "A4D3", description: "Small crater"},
-                           relationships:{moon: {links: {self: "http://test.host/api/v1/craters/A4D3/relationships/moon", related: "http://test.host/api/v1/craters/A4D3/moon"}}}
-                          },
-                          {id: "S56D",
-                           type: "craters",
-                           links:{self: "http://test.host/api/v1/craters/S56D"},
-                           attributes:{code: "S56D", description: "Very large crater"},
-                           relationships:{moon: {links: {self: "http://test.host/api/v1/craters/S56D/relationships/moon", related: "http://test.host/api/v1/craters/S56D/moon"}}}
-                          }
-                        ]
-                      }
+    assert_hash_equals({
+                         data: [
+                           {
+                             id:"A4D3",
+                             type:"craters",
+                             links:{self: "http://test.host/api/v1/craters/A4D3"},
+                             attributes:{code: "A4D3", description: "Small crater"},
+                             relationships:{moon: {links: {self: "http://test.host/api/v1/craters/A4D3/relationships/moon", related: "http://test.host/api/v1/craters/A4D3/moon"}}}
+                           },
+                           {
+                             id: "S56D",
+                             type: "craters",
+                             links:{self: "http://test.host/api/v1/craters/S56D"},
+                             attributes:{code: "S56D", description: "Very large crater"},
+                             relationships:{moon: {links: {self: "http://test.host/api/v1/craters/S56D/relationships/moon", related: "http://test.host/api/v1/craters/S56D/moon"}}}
+                           }
+                         ]
+                       }, json_response)
   end
 
   def test_show_relationship

diff --git a/test/unit/resource/resource_test.rb b/test/unit/resource/resource_test.rb
@@ -121,6 +121,23 @@ def test_resource_for_resource_does_not_exist_at_root
   def test_resource_for_namespaced_resource
     assert_equal(MyModule::MyNamespacedResource.resource_for('related'), MyModule::RelatedResource)
   end
+
+  def test_relationship_parent_point_to_correct_resource
+    assert_equal MyModule::MyNamespacedResource, MyModule::MyNamespacedResource._relationships[:related].parent_resource
+  end
+
+  def test_relationship_parent_option_point_to_correct_resource
+    assert_equal MyModule::MyNamespacedResource, MyModule::MyNamespacedResource._relationships[:related].options[:parent_resource]
+  end
+
+  def test_derived_resources_relationships_parent_point_to_correct_resource
+    assert_equal MyAPI::MyNamespacedResource, MyAPI::MyNamespacedResource._relationships[:related].parent_resource
+  end
+
+  def test_derived_resources_relationships_parent_options_point_to_correct_resource
+    assert_equal MyAPI::MyNamespacedResource, MyAPI::MyNamespacedResource._relationships[:related].options[:parent_resource]
+  end
+
   def test_base_resource_abstract
     assert BaseResource._abstract
   end