Fix read_attribute_for_serialization not seeing parent serializer methods

Fixes #1653, #1658, #1660

Define "scope_name" on instance singleton, not all instances

CHANGELOG.md

@@ -41,9 +41,11 @@ Features:
 - [#1340](https://github.com/rails-api/active_model_serializers/pull/1340) Add support for resource-level meta. (@beauby)
 
 Fixes:
+- [#1661](https://github.com/rails-api/active_model_serializers/pull/1661) Fixes `read_attribute_for_serialization` not
+  seeing methods defined in serialization superclass (#1653, #1658, #1660), introduced in #1650. (@bf4)
 - [#1651](https://github.com/rails-api/active_model_serializers/pull/1651) Fix deserialization of nil relationships. (@NullVoxPopuli)
 - [#1480](https://github.com/rails-api/active_model_serializers/pull/1480) Fix setting of cache_store from Rails configuration. (@bf4)
-  Fix uninentional mutating of value in memory cache store. (@groyoh)
+  Fix unintentional mutating of value in memory cache store. (@groyoh)
 - [#1622](https://github.com/rails-api/active_model_serializers/pull/1622) Fragment cache changed from per-record to per-serializer.
   Now, two serializers that use the same model may be separately cached. (@lserman)
 - [#1478](https://github.com/rails-api/active_model_serializers/pull/1478) Cache store will now be correctly set when serializers are

lib/active_model/serializer.rb

@@ -96,19 +96,6 @@ module ActiveModel
       end
     end
 
-    # rubocop:disable Style/ClassVars
-    def self.method_added(method_name)
-      @@_serializer_instance_methods ||= Hash.new { |h, k| h[k] = Set.new }
-      @@_serializer_instance_methods[self] << method_name
-    end
-
-    def self._serializer_instance_method_defined?(name)
-      @_serializer_instance_methods ||= (ActiveModel::Serializer.public_instance_methods - Object.public_instance_methods).to_set
-      @_serializer_instance_methods.include?(name) ||
-        @@_serializer_instance_methods[self].include?(name)
-    end
-    # rubocop:enable Style/ClassVars
-
     attr_accessor :object, :root, :scope
 
     # `scope_name` is set as :current_user by default in the controller.
@@ -122,9 +109,7 @@ module ActiveModel
 
       scope_name = instance_options[:scope_name]
       if scope_name && !respond_to?(scope_name)
-        self.class.class_eval do
+        define_singleton_method scope_name, lambda { scope }
-          define_method scope_name, lambda { scope }
-        end
       end
     end
 
@@ -189,7 +174,7 @@ module ActiveModel
     end
 
     def read_attribute_for_serialization(attr)
-      if self.class._serializer_instance_method_defined?(attr)
+      if respond_to?(attr)
         send(attr)
       elsif self.class._fragmented
         self.class._fragmented.read_attribute_for_serialization(attr)

test/action_controller/serialization_scope_name_test.rb

@@ -1,17 +1,6 @@
 require 'test_helper'
 
 module SerializationScopeTesting
-  def self.undef_serializer_dynamic_scope_methods
-    [PostSerializer, PostViewContextSerializer]. each do |serializer_class|
-      instance_method_cache = serializer_class.instance_variable_get(:@_serializer_instance_methods)
-      class_method_cache    = serializer_class.class_variable_get(:@@_serializer_instance_methods)[serializer_class]
-      [:view_context, :current_user].each do |scope_method|
-        serializer_class.send(:undef_method, scope_method) if serializer_class.method_defined?(scope_method)
-        instance_method_cache && instance_method_cache.delete(scope_method)
-        class_method_cache && class_method_cache.delete(scope_method)
-      end
-    end
-  end
   class User < ActiveModelSerializers::Model
     attr_accessor :id, :name, :admin
     def admin?
@@ -81,10 +70,6 @@ module SerializationScopeTesting
   class DefaultScopeTest < ActionController::TestCase
     tests PostTestController
 
-    teardown do
-      SerializationScopeTesting.undef_serializer_dynamic_scope_methods
-    end
-
     def test_default_serialization_scope
       assert_equal :current_user, @controller._serialization_scope
     end
@@ -135,10 +120,6 @@ module SerializationScopeTesting
     end
     tests PostViewContextTestController
 
-    teardown do
-      SerializationScopeTesting.undef_serializer_dynamic_scope_methods
-    end
-
     def test_defined_serialization_scope
       assert_equal :view_context, @controller._serialization_scope
     end
@@ -206,10 +187,6 @@ module SerializationScopeTesting
     end
     tests PostViewContextTestController
 
-    teardown do
-      SerializationScopeTesting.undef_serializer_dynamic_scope_methods
-    end
-
     def test_nil_serialization_scope
       assert_nil @controller._serialization_scope
     end

test/serializers/read_attribute_for_serialization_test.rb

@@ -0,0 +1,79 @@
+require 'test_helper'
+
+module ActiveModel
+  class Serializer
+    class ReadAttributeForSerializationTest < ActiveSupport::TestCase
+      # https://github.com/rails-api/active_model_serializers/issues/1653
+      class Parent < ActiveModelSerializers::Model
+        attr_accessor :id
+      end
+      class Child < Parent
+        attr_accessor :name
+      end
+      class ParentSerializer < ActiveModel::Serializer
+        attributes :$id
+
+        define_method(:$id) do
+          object.id
+        end
+      end
+      class ChildSerializer < ParentSerializer
+        attributes :name
+      end
+
+      def test_child_serializer_calls_dynamic_method_in_parent_serializer
+        parent = ParentSerializer.new(Parent.new(id: 5))
+        child  = ChildSerializer.new(Child.new(id: 6, name: 'Child'))
+        assert_equal 5, parent.read_attribute_for_serialization(:$id)
+        assert_equal 6, child.read_attribute_for_serialization(:$id)
+      end
+
+      # https://github.com/rails-api/active_model_serializers/issues/1658
+      class ErrorResponse < ActiveModelSerializers::Model
+        attr_accessor :error
+      end
+      class ApplicationSerializer < ActiveModel::Serializer
+        attributes :status
+
+        def status
+          object.try(:errors).blank? && object.try(:error).blank?
+        end
+      end
+      class ErrorResponseSerializer < ApplicationSerializer
+        attributes :error
+      end
+      class ErrorResponseWithSuperSerializer < ApplicationSerializer
+        attributes :error
+
+        def success
+          super
+        end
+      end
+
+      def test_child_serializer_with_error_attribute
+        error = ErrorResponse.new(error: 'i have an error')
+        serializer = ErrorResponseSerializer.new(error)
+        serializer_with_super = ErrorResponseWithSuperSerializer.new(error)
+        assert_equal false, serializer.read_attribute_for_serialization(:status)
+        assert_equal false, serializer_with_super.read_attribute_for_serialization(:status)
+      end
+
+      def test_child_serializer_with_errors
+        error = ErrorResponse.new
+        error.errors.add(:invalid, 'i am not valid')
+        serializer = ErrorResponseSerializer.new(error)
+        serializer_with_super = ErrorResponseWithSuperSerializer.new(error)
+        assert_equal false, serializer.read_attribute_for_serialization(:status)
+        assert_equal false, serializer_with_super.read_attribute_for_serialization(:status)
+      end
+
+      def test_child_serializer_no_error_attribute_or_errors
+        error = ErrorResponse.new
+        serializer = ErrorResponseSerializer.new(error)
+        serializer_with_super = ErrorResponseWithSuperSerializer.new(error)
+        assert_equal true, serializer.read_attribute_for_serialization(:status)
+        assert_equal true, serializer_with_super.read_attribute_for_serialization(:status)
+      end
+    end
+  end
+end