Merge pull request #1650 from bf4/fix_serialization_scope

[FIX] serialization scope options

Author: bf4

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

lib/action_controller/serialization.rb

@@ -30,8 +30,8 @@ def get_serializer(resource, options = {})
         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 #=> '""'`

lib/active_model/serializer.rb

@@ -96,16 +96,18 @@ def self.get_serializer_for(klass)
       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
 

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 @@ 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
@@ -120,6 +135,10 @@ 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
@@ -158,8 +177,6 @@ def test_serialization_scope_admin
       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 +188,15 @@ def render_post_with_no_scope
         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
 
@@ -186,6 +206,10 @@ 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
@@ -194,37 +218,35 @@ def test_nil_serialization_scope_object
       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
-        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
     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
+      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