From 6370e5c72a71060feeb9e16b67fd0e1a5742bedc Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Mon, 4 Apr 2016 11:01:01 -0500 Subject: [PATCH] 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 | 4 +- lib/active_model/serializer.rb | 19 +---- .../serialization_scope_name_test.rb | 23 ------ .../read_attribute_for_serialization_test.rb | 79 +++++++++++++++++++ 4 files changed, 84 insertions(+), 41 deletions(-) create mode 100644 test/serializers/read_attribute_for_serialization_test.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index f6221deb9..2de5358e5 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index c3193120d..af0b3634c 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -96,19 +96,6 @@ def self.get_serializer_for(klass) 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 @@ def initialize(object, options = {}) scope_name = instance_options[:scope_name] if scope_name && !respond_to?(scope_name) - self.class.class_eval do - define_method scope_name, lambda { scope } - end + define_singleton_method scope_name, lambda { scope } end end @@ -189,7 +174,7 @@ def json_key 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) diff --git a/test/action_controller/serialization_scope_name_test.rb b/test/action_controller/serialization_scope_name_test.rb index 1ef980583..62959455f 100644 --- a/test/action_controller/serialization_scope_name_test.rb +++ b/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 @@ def comments 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 @@ def serializer 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 @@ def new_post end tests PostViewContextTestController - teardown do - SerializationScopeTesting.undef_serializer_dynamic_scope_methods - end - def test_nil_serialization_scope assert_nil @controller._serialization_scope end diff --git a/test/serializers/read_attribute_for_serialization_test.rb b/test/serializers/read_attribute_for_serialization_test.rb new file mode 100644 index 000000000..21677e657 --- /dev/null +++ b/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