From a065bc28d1739631c6177c2995ba345ebcb0c7c8 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Fri, 1 Apr 2016 05:37:32 -0500 Subject: [PATCH 1/3] Fix serialization scope options --- lib/action_controller/serialization.rb | 4 +- lib/active_model/serializer.rb | 16 +++-- .../serialization_scope_name_test.rb | 64 +++++++++++-------- 3 files changed, 49 insertions(+), 35 deletions(-) diff --git a/lib/action_controller/serialization.rb b/lib/action_controller/serialization.rb index 3097cdc4..e8d5a420 100644 --- a/lib/action_controller/serialization.rb +++ b/lib/action_controller/serialization.rb @@ -30,8 +30,8 @@ module ActionController options[:adapter] = false end serializable_resource = ActiveModelSerializers::SerializableResource.new(resource, options) - serializable_resource.serialization_scope ||= serialization_scope - serializable_resource.serialization_scope_name = _serialization_scope + serializable_resource.serialization_scope ||= options.fetch(:scope) { serialization_scope } + serializable_resource.serialization_scope_name = options.fetch(:scope_name) { _serialization_scope } # For compatibility with the JSON renderer: `json.to_json(options) if json.is_a?(String)`. # Otherwise, since `serializable_resource` is not a string, the renderer would call # `to_json` on a String and given odd results, such as `"".to_json #=> '""'` diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index e8607845..c3193120 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -96,16 +96,18 @@ module ActiveModel end end - def self._serializer_instance_method_defined?(name) - _serializer_instance_methods.include?(name) + # 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 - # TODO: Fix load-order failures when different serializer instances define different - # scope methods - def self._serializer_instance_methods - @_serializer_instance_methods ||= (public_instance_methods - Object.public_instance_methods).to_set + 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 - private_class_method :_serializer_instance_methods + # rubocop:enable Style/ClassVars attr_accessor :object, :root, :scope diff --git a/test/action_controller/serialization_scope_name_test.rb b/test/action_controller/serialization_scope_name_test.rb index 5ad7251b..ca1ff22f 100644 --- a/test/action_controller/serialization_scope_name_test.rb +++ b/test/action_controller/serialization_scope_name_test.rb @@ -158,8 +158,6 @@ module SerializationScopeTesting assert_equal expected_json, @response.body end end - # FIXME: Has bugs. See comments below and - # https://github.com/rails-api/active_model_serializers/issues/1509 class NilSerializationScopeTest < ActionController::TestCase class PostViewContextTestController < ActionController::Base serialization_scope nil @@ -171,12 +169,15 @@ module SerializationScopeTesting render json: new_post, serializer: PostSerializer, adapter: :json end - # TODO: run test when - # global state in Serializer._serializer_instance_methods is fixed - # def render_post_with_passed_in_scope - # self.current_user = User.new(id: 3, name: 'Pete', admin: false) - # render json: new_post, serializer: PostSerializer, adapter: :json, scope: current_user, scope_name: :current_user - # end + def render_post_with_passed_in_scope + self.current_user = User.new(id: 3, name: 'Pete', admin: false) + render json: new_post, serializer: PostSerializer, adapter: :json, scope: current_user, scope_name: :current_user + end + + def render_post_with_passed_in_scope_without_scope_name + self.current_user = User.new(id: 3, name: 'Pete', admin: false) + render json: new_post, serializer: PostSerializer, adapter: :json, scope: current_user + end private @@ -194,8 +195,6 @@ module SerializationScopeTesting assert_nil @controller.serialization_scope end - # TODO: change to NoMethodError and match 'admin?' when the - # global state in Serializer._serializer_instance_methods is fixed def test_nil_scope if Rails.version.start_with?('4.0') exception_class = NoMethodError @@ -210,21 +209,34 @@ module SerializationScopeTesting assert_match exception_matcher, exception.message end - # TODO: run test when - # global state in Serializer._serializer_instance_methods is fixed - # def test_nil_scope_passed_in_current_user - # get :render_post_with_passed_in_scope - # expected_json = { - # post: { - # id: 4, - # title: 'Title', - # body: "The 'scope' is the 'current_user': true", - # comments: [ - # { id: 2, body: 'Scoped' } - # ] - # } - # }.to_json - # assert_equal expected_json, @response.body - # end + def test_serialization_scope_is_and_nil_scope_passed_in_current_user + get :render_post_with_passed_in_scope + expected_json = { + post: { + id: 4, + title: 'Title', + body: "The 'scope' is the 'current_user': true", + comments: [ + { id: 2, body: 'Scoped' } + ] + } + }.to_json + assert_equal expected_json, @response.body + end + + def test_serialization_scope_is_nil_and_scope_passed_in_current_user_without_scope_name + get :render_post_with_passed_in_scope_without_scope_name + expected_json = { + post: { + id: 4, + title: 'Title', + body: "The 'scope' is the 'current_user': true", + comments: [ + { id: 2, body: 'Scoped' } + ] + } + }.to_json + assert_equal expected_json, @response.body + end end end From 2edd39d2c22aec72bbd100ca3d6b64a33f8bd57f Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Fri, 1 Apr 2016 10:50:58 -0500 Subject: [PATCH 2/3] Need to teardown the dynamically added method --- .../serialization_scope_name_test.rb | 50 +++++++++++-------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/test/action_controller/serialization_scope_name_test.rb b/test/action_controller/serialization_scope_name_test.rb index ca1ff22f..1ef98058 100644 --- a/test/action_controller/serialization_scope_name_test.rb +++ b/test/action_controller/serialization_scope_name_test.rb @@ -1,6 +1,17 @@ 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? @@ -70,6 +81,10 @@ 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 @@ -120,6 +135,10 @@ 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 @@ -187,6 +206,10 @@ 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 @@ -196,14 +219,8 @@ module SerializationScopeTesting end def test_nil_scope - if Rails.version.start_with?('4.0') - exception_class = NoMethodError - exception_matcher = 'admin?' - else - exception_class = NameError - exception_matcher = /admin|current_user/ - end - exception = assert_raises(exception_class) do + exception_matcher = /current_user/ + exception = assert_raises(NameError) do get :render_post_with_no_scope end assert_match exception_matcher, exception.message @@ -225,18 +242,11 @@ module SerializationScopeTesting end def test_serialization_scope_is_nil_and_scope_passed_in_current_user_without_scope_name - get :render_post_with_passed_in_scope_without_scope_name - expected_json = { - post: { - id: 4, - title: 'Title', - body: "The 'scope' is the 'current_user': true", - comments: [ - { id: 2, body: 'Scoped' } - ] - } - }.to_json - assert_equal expected_json, @response.body + exception_matcher = /current_user/ + exception = assert_raises(NameError) do + get :render_post_with_passed_in_scope_without_scope_name + end + assert_match exception_matcher, exception.message end end end From 881edd299ba1d5189e2c42ff78392474dd234a3d Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Fri, 1 Apr 2016 12:09:06 -0500 Subject: [PATCH 3/3] Add Changelog [ci skip] --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1eb04825..84cc350e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,9 @@ Breaking changes: Features: +- [#1650](https://github.com/rails-api/active_model_serializers/pull/1650) Fix serialization scope options `scope`, `scope_name` + take precedence over `serialization_scope` in the controller. + Fix tests that required tearing down dynamic methods. (@bf4) - [#1644](https://github.com/rails-api/active_model_serializers/pull/1644) Include adapter name in cache key so that the same serializer can be cached per adapter. (@bf4 via #1346 by @kevintyll) - [#1642](https://github.com/rails-api/active_model_serializers/pull/1642) Prefer object.cache_key over the generated