Rework model_name in derived resources

Fixes an issue where the recreated relationships might be using the wrong model_name if the model_name is changed in a resource that is derived from a non abstract resource, such as done in many of the tests.
Breaking change: Derived resources now use the model name of their base resource, if it is not abstract.
Cleans up the tests to not have the warnings about missing models.
Fixes issue in _model_class related to #952, and renames @model (class level) to @model_class to avoid confusion with instance level @model

Fixes #952, #654

(cherry picked from commit 280ccea)

Author: lgebhardt

lib/jsonapi/resource.rb

@@ -416,18 +416,15 @@ def inherited(subclass)
         subclass.immutable(false)
         subclass.caching(false)
         subclass._attributes = (_attributes || {}).dup
+
         subclass._model_hints = (_model_hints || {}).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
+        unless _model_name.empty?
+          subclass.model_name(_model_name, add_model_hint: (_model_hints && !_model_hints[_model_name].nil?) == true)
         end
 
+        subclass.rebuild_relationships(_relationships || {})
+
         subclass._allowed_filters = (_allowed_filters || Set.new).dup
 
         type = subclass.name.demodulize.sub(/Resource$/, '').underscore
@@ -438,6 +435,20 @@ def inherited(subclass)
         check_reserved_resource_name(subclass._type, subclass.name)
       end
 
+      def rebuild_relationships(relationships)
+        original_relationships = relationships.deep_dup
+
+        @_relationships = {}
+
+        if original_relationships.is_a?(Hash)
+          original_relationships.each_value do |relationship|
+            options = relationship.options.dup
+            options[:parent_resource] = self
+            _add_relationship(relationship.class, relationship.name, options)
+          end
+        end
+      end
+
       def resource_for(type)
         type = type.underscore
         type_with_module = type.include?('/') ? type : module_path + type
@@ -554,6 +565,8 @@ def model_name(model, options = {})
         @_model_name = model.to_sym
 
         model_hint(model: @_model_name, resource: self) unless options[:add_model_hint] == false
+
+        rebuild_relationships(_relationships)
       end
 
       def model_hint(model: _model_name, resource: _type)
@@ -900,10 +913,11 @@ def _model_name
         if _abstract
           return ''
         else
-          return @_model_name if defined?(@_model_name)
+          return @_model_name.to_s if defined?(@_model_name)
           class_name = self.name
           return '' if class_name.nil?
-          return @_model_name = class_name.demodulize.sub(/Resource$/, '')
+          @_model_name = class_name.demodulize.sub(/Resource$/, '')
+          return @_model_name.to_s
         end
       end
 
@@ -974,11 +988,17 @@ def attribute_caching_context(context)
       def _model_class
         return nil if _abstract
 
-        return @model if defined?(@model)
-        return nil if self.name.to_s.blank? && _model_name.to_s.blank?
-        @model = _model_name.to_s.safe_constantize
-        warn "[MODEL NOT FOUND] Model could not be found for #{self.name}. If this a base Resource declare it as abstract." if @model.nil?
-        @model
+        return @model_class if @model_class
+
+        model_name = _model_name
+        return nil if model_name.to_s.blank?
+
+        @model_class = model_name.to_s.safe_constantize
+        if @model_class.nil?
+          warn "[MODEL NOT FOUND] Model could not be found for #{self.name}. If this a base Resource declare it as abstract."
+        end
+
+        @model_class
       end
 
       def _allowed_filter?(filter)

test/fixtures/active_record.rb

@@ -961,6 +961,7 @@ class CompanyResource < JSONAPI::Resource
 end
 
 class FirmResource < CompanyResource
+  model_name "Firm"
 end
 
 class TagResource < JSONAPI::Resource

test/unit/jsonapi_request/jsonapi_request_test.rb

@@ -4,7 +4,7 @@ class CatResource < JSONAPI::Resource
   attribute :name
   attribute :breed
 
-  belongs_to :mother, class_name: 'Cat'
+  has_one :mother, class_name: 'Cat'
   has_one :father, class_name: 'Cat'
 
   filters :name

test/unit/resource/resource_test.rb

@@ -47,7 +47,9 @@ class NoMatchAbstractResource < JSONAPI::Resource
   abstract
 end
 
-class CatResource < JSONAPI::Resource
+class FelineResource < JSONAPI::Resource
+  model_name 'Cat'
+
   attribute :name
   attribute :breed
   attribute :kind, :delegate => :breed
@@ -98,6 +100,12 @@ class RelatedResource < MyModule::RelatedResource
   end
 end
 
+class PostWithReadonlyAttributesResource < JSONAPI::Resource
+  model_name 'Post'
+  attribute :title, readonly: true
+  has_one :author, readonly: true
+end
+
 class ResourceTest < ActiveSupport::TestCase
   def setup
     @post = Post.first
@@ -175,7 +183,7 @@ def test_derived_not_abstract
   def test_nil_model_class
     # ToDo:Figure out why this test does not work on Rails 4.0
     # :nocov:
-    if Rails::VERSION::MAJOR >= 4 && Rails::VERSION::MINOR >= 1
+    if (Rails::VERSION::MAJOR >= 4 && Rails::VERSION::MINOR >= 1) || (Rails::VERSION::MAJOR >= 5)
       assert_output nil, "[MODEL NOT FOUND] Model could not be found for NoMatchResource. If this a base Resource declare it as abstract.\n" do
         assert_nil NoMatchResource._model_class
       end
@@ -194,13 +202,13 @@ def test_model_alternate
   end
 
   def test_class_attributes
-    attrs = CatResource._attributes
+    attrs = FelineResource._attributes
     assert_kind_of(Hash, attrs)
     assert_equal(attrs.keys.size, 4)
   end
 
   def test_class_relationships
-    relationships = CatResource._relationships
+    relationships = FelineResource._relationships
     assert_kind_of(Hash, relationships)
     assert_equal(relationships.size, 2)
   end
@@ -214,16 +222,16 @@ def test_replace_polymorphic_to_one_link
   end
 
   def test_duplicate_relationship_name
-    assert_output nil, "[DUPLICATE RELATIONSHIP] `mother` has already been defined in CatResource.\n" do
-      CatResource.instance_eval do
+    assert_output nil, "[DUPLICATE RELATIONSHIP] `mother` has already been defined in FelineResource.\n" do
+      FelineResource.instance_eval do
         has_one :mother, class_name: 'Cat'
       end
     end
   end
 
   def test_duplicate_attribute_name
-    assert_output nil, "[DUPLICATE ATTRIBUTE] `name` has already been defined in CatResource.\n" do
-      CatResource.instance_eval do
+    assert_output nil, "[DUPLICATE ATTRIBUTE] `name` has already been defined in FelineResource.\n" do
+      FelineResource.instance_eval do
         attribute :name
       end
     end
@@ -294,7 +302,7 @@ def test_find_by_key_with_customized_base_records
   end
 
   def test_updatable_fields_does_not_include_id
-    assert(!CatResource.updatable_fields.include?(:id))
+    assert(!FelineResource.updatable_fields.include?(:id))
   end
 
   def test_filter_on_to_many_relationship_id
@@ -438,60 +446,60 @@ def apply_pagination(records, criteria, order_options)
   end
 
   def test_key_type_integer
-    CatResource.instance_eval do
+    FelineResource.instance_eval do
       key_type :integer
     end
 
-    assert CatResource.verify_key('45')
-    assert CatResource.verify_key(45)
+    assert FelineResource.verify_key('45')
+    assert FelineResource.verify_key(45)
 
     assert_raises JSONAPI::Exceptions::InvalidFieldValue do
-      CatResource.verify_key('45,345')
+      FelineResource.verify_key('45,345')
     end
 
   ensure
-    CatResource.instance_eval do
+    FelineResource.instance_eval do
       key_type nil
     end
   end
 
   def test_key_type_string
-    CatResource.instance_eval do
+    FelineResource.instance_eval do
       key_type :string
     end
 
-    assert CatResource.verify_key('45')
-    assert CatResource.verify_key(45)
+    assert FelineResource.verify_key('45')
+    assert FelineResource.verify_key(45)
 
     assert_raises JSONAPI::Exceptions::InvalidFieldValue do
-      CatResource.verify_key('45,345')
+      FelineResource.verify_key('45,345')
     end
 
   ensure
-    CatResource.instance_eval do
+    FelineResource.instance_eval do
       key_type nil
     end
   end
 
   def test_key_type_uuid
-    CatResource.instance_eval do
+    FelineResource.instance_eval do
       key_type :uuid
     end
 
-    assert CatResource.verify_key('f1a4d5f2-e77a-4d0a-acbb-ee0b98b3f6b5')
+    assert FelineResource.verify_key('f1a4d5f2-e77a-4d0a-acbb-ee0b98b3f6b5')
 
     assert_raises JSONAPI::Exceptions::InvalidFieldValue do
-      CatResource.verify_key('f1a-e77a-4d0a-acbb-ee0b98b3f6b5')
+      FelineResource.verify_key('f1a-e77a-4d0a-acbb-ee0b98b3f6b5')
     end
 
   ensure
-    CatResource.instance_eval do
+    FelineResource.instance_eval do
       key_type nil
     end
   end
 
   def test_key_type_proc
-    CatResource.instance_eval do
+    FelineResource.instance_eval do
       key_type -> (key, context) {
         return key if key.nil?
         if key.to_s.match(/^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/)
@@ -502,14 +510,14 @@ def test_key_type_proc
       }
     end
 
-    assert CatResource.verify_key('f1a4d5f2-e77a-4d0a-acbb-ee0b98b3f6b5')
+    assert FelineResource.verify_key('f1a4d5f2-e77a-4d0a-acbb-ee0b98b3f6b5')
 
     assert_raises JSONAPI::Exceptions::InvalidFieldValue do
-      CatResource.verify_key('f1a-e77a-4d0a-acbb-ee0b98b3f6b5')
+      FelineResource.verify_key('f1a-e77a-4d0a-acbb-ee0b98b3f6b5')
     end
 
   ensure
-    CatResource.instance_eval do
+    FelineResource.instance_eval do
       key_type nil
     end
   end