Merge pull request #1166 from rails-api/clarify_options_variable

Prefer methods over instance variables

Author: NullVoxPopuli

lib/active_model/serializer.rb

@@ -42,16 +42,16 @@ class << self
     end
 
     def self.inherited(base)
-      base._attributes = self._attributes.try(:dup) || []
-      base._attributes_keys = self._attributes_keys.try(:dup) || {}
+      base._attributes = _attributes.try(:dup) || []
+      base._attributes_keys = _attributes_keys.try(:dup) || {}
       base._cache_digest = digest_caller_file(caller.first)
       super
     end
 
     def self.attributes(*attrs)
       attrs = attrs.first if attrs.first.class == Array
-      @_attributes.concat attrs
-      @_attributes.uniq!
+      _attributes.concat(attrs)
+      _attributes.uniq!
 
       attrs.each do |attr|
         define_method attr do
@@ -62,8 +62,8 @@ def self.attributes(*attrs)
 
     def self.attribute(attr, options = {})
       key = options.fetch(:key, attr)
-      @_attributes_keys[attr] = { key: key } if key != attr
-      @_attributes << key unless @_attributes.include?(key)
+      self._attributes_keys[attr] = { key: key } if key != attr
+      self._attributes << key unless _attributes.include?(key)
 
       ActiveModelSerializers.silence_warnings do
         define_method key do
@@ -73,16 +73,16 @@ def self.attribute(attr, options = {})
     end
 
     def self.fragmented(serializer)
-      @_fragmented = serializer
+      self._fragmented = serializer
     end
 
     # Enables a serializer to be automatically cached
     def self.cache(options = {})
-      @_cache = ActionController::Base.cache_store if Rails.configuration.action_controller.perform_caching
-      @_cache_key = options.delete(:key)
-      @_cache_only = options.delete(:only)
-      @_cache_except = options.delete(:except)
-      @_cache_options = (options.empty?) ? nil : options
+      self._cache = ActionController::Base.cache_store if Rails.configuration.action_controller.perform_caching
+      self._cache_key = options.delete(:key)
+      self._cache_only = options.delete(:only)
+      self._cache_except = options.delete(:except)
+      self._cache_options = (options.empty?) ? nil : options
     end
 
     def self.serializer_for(resource, options = {})
@@ -91,7 +91,7 @@ def self.serializer_for(resource, options = {})
       elsif resource.respond_to?(:to_ary)
         config.array_serializer
       else
-        options.fetch(:serializer, get_serializer_for(resource.class))
+        options.fetch(:serializer) { get_serializer_for(resource.class) }
       end
     end
 
@@ -104,17 +104,40 @@ def self.root_name
       name.demodulize.underscore.sub(/_serializer$/, '') if name
     end
 
+    def self.serializers_cache
+      @serializers_cache ||= ThreadSafe::Cache.new
+    end
+
+    def self.digest_caller_file(caller_line)
+      serializer_file_path = caller_line[CALLER_FILE]
+      serializer_file_contents = IO.read(serializer_file_path)
+      Digest::MD5.hexdigest(serializer_file_contents)
+    end
+
+    def self.get_serializer_for(klass)
+      serializers_cache.fetch_or_store(klass) do
+        serializer_class_name = "#{klass.name}Serializer"
+        serializer_class = serializer_class_name.safe_constantize
+
+        if serializer_class
+          serializer_class
+        elsif klass.superclass
+          get_serializer_for(klass.superclass)
+        end
+      end
+    end
+
     attr_accessor :object, :root, :meta, :meta_key, :scope
 
     def initialize(object, options = {})
-      @object = object
-      @options = options
-      @root = options[:root]
-      @meta = options[:meta]
-      @meta_key = options[:meta_key]
-      @scope = options[:scope]
-
-      scope_name = options[:scope_name]
+      self.object = object
+      self.instance_options = options
+      self.root = instance_options[:root]
+      self.meta = instance_options[:meta]
+      self.meta_key = instance_options[:meta_key]
+      self.scope = instance_options[:scope]
+
+      scope_name = instance_options[:scope_name]
       if scope_name && !respond_to?(scope_name)
         self.class.class_eval do
           define_method scope_name, lambda { scope }
@@ -123,7 +146,7 @@ def initialize(object, options = {})
     end
 
     def json_key
-      @root || object.class.model_name.to_s.underscore
+      root || object.class.model_name.to_s.underscore
     end
 
     def attributes(options = {})
@@ -143,29 +166,10 @@ def attributes(options = {})
       end
     end
 
-    def self.serializers_cache
-      @serializers_cache ||= ThreadSafe::Cache.new
-    end
-
-    def self.digest_caller_file(caller_line)
-      serializer_file_path = caller_line[CALLER_FILE]
-      serializer_file_contents = IO.read(serializer_file_path)
-      Digest::MD5.hexdigest(serializer_file_contents)
-    end
-
-    attr_reader :options
+    private # rubocop:disable Lint/UselessAccessModifier
 
-    def self.get_serializer_for(klass)
-      serializers_cache.fetch_or_store(klass) do
-        serializer_class_name = "#{klass.name}Serializer"
-        serializer_class = serializer_class_name.safe_constantize
-
-        if serializer_class
-          serializer_class
-        elsif klass.superclass
-          get_serializer_for(klass.superclass)
-        end
-      end
+    ActiveModelSerializers.silence_warnings do
+      attr_accessor :instance_options
     end
   end
 end

lib/active_model/serializer/adapter.rb

@@ -10,6 +10,7 @@ class Adapter
       autoload :JsonApi
       autoload :Null
       autoload :FlattenJson
+      autoload :CachedSerializer
 
       def self.create(resource, options = {})
         override = options.delete(:adapter)
@@ -80,11 +81,11 @@ def self.inherited(subclass)
         ActiveModel::Serializer::Adapter.register(subclass.to_s.demodulize, subclass)
       end
 
-      attr_reader :serializer
+      attr_reader :serializer, :instance_options
 
       def initialize(serializer, options = {})
         @serializer = serializer
-        @options = options
+        @instance_options = options
       end
 
       def serializable_hash(options = nil)
@@ -101,43 +102,12 @@ def fragment_cache(*args)
         raise NotImplementedError, 'This is an abstract method. Should be implemented at the concrete adapter.'
       end
 
-      private
-
       def cache_check(serializer)
-        @cached_serializer = serializer
-        @klass             = @cached_serializer.class
-        if is_cached?
-          @klass._cache.fetch(cache_key, @klass._cache_options) do
-            yield
-          end
-        elsif is_fragment_cached?
-          FragmentCache.new(self, @cached_serializer, @options).fetch
-        else
+        CachedSerializer.new(serializer).cache_check(self) do
           yield
         end
       end
 
-      def is_cached?
-        @klass._cache && !@klass._cache_only && !@klass._cache_except
-      end
-
-      def is_fragment_cached?
-        @klass._cache_only && !@klass._cache_except || !@klass._cache_only && @klass._cache_except
-      end
-
-      def cache_key
-        parts = []
-        parts << object_cache_key
-        parts << @klass._cache_digest unless @klass._cache_options && @klass._cache_options[:skip_digest]
-        parts.join('/')
-      end
-
-      def object_cache_key
-        object_time_safe = @cached_serializer.object.updated_at
-        object_time_safe = object_time_safe.strftime('%Y%m%d%H%M%S%9N') if object_time_safe.respond_to?(:strftime)
-        (@klass._cache_key) ? "#{@klass._cache_key}/#{@cached_serializer.object.id}-#{object_time_safe}" : @cached_serializer.object.cache_key
-      end
-
       def meta
         serializer.meta if serializer.respond_to?(:meta)
       end

lib/active_model/serializer/adapter/cached_serializer.rb

@@ -0,0 +1,45 @@
+module ActiveModel
+  class Serializer
+    class Adapter
+      class CachedSerializer
+        def initialize(serializer)
+          @cached_serializer = serializer
+          @klass             = @cached_serializer.class
+        end
+
+        def cache_check(adapter_instance)
+          if cached?
+            @klass._cache.fetch(cache_key, @klass._cache_options) do
+              yield
+            end
+          elsif fragment_cached?
+            FragmentCache.new(adapter_instance, @cached_serializer, adapter_instance.instance_options).fetch
+          else
+            yield
+          end
+        end
+
+        def cached?
+          @klass._cache && !@klass._cache_only && !@klass._cache_except
+        end
+
+        def fragment_cached?
+          @klass._cache_only && !@klass._cache_except || !@klass._cache_only && @klass._cache_except
+        end
+
+        def cache_key
+          parts = []
+          parts << object_cache_key
+          parts << @klass._cache_digest unless @klass._cache_options && @klass._cache_options[:skip_digest]
+          parts.join('/')
+        end
+
+        def object_cache_key
+          object_time_safe = @cached_serializer.object.updated_at
+          object_time_safe = object_time_safe.strftime('%Y%m%d%H%M%S%9N') if object_time_safe.respond_to?(:strftime)
+          (@klass._cache_key) ? "#{@klass._cache_key}/#{@cached_serializer.object.id}-#{object_time_safe}" : @cached_serializer.object.cache_key
+        end
+      end
+    end
+  end
+end

lib/active_model/serializer/adapter/fragment_cache.rb

@@ -2,7 +2,7 @@ class ActiveModel::Serializer::Adapter::FragmentCache
         attr_reader :serializer
 
         def initialize(adapter, serializer, options)
-          @options    = options
+          @instance_options = options
           @adapter    = adapter
           @serializer = serializer
         end
@@ -16,19 +16,23 @@ def fetch
           cached_serializer     = serializers[:cached].constantize.new(serializer.object)
           non_cached_serializer = serializers[:non_cached].constantize.new(serializer.object)
 
-          cached_adapter     = @adapter.class.new(cached_serializer, @options)
-          non_cached_adapter = @adapter.class.new(non_cached_serializer, @options)
+          cached_adapter     = adapter.class.new(cached_serializer, instance_options)
+          non_cached_adapter = adapter.class.new(non_cached_serializer, instance_options)
 
           # Get serializable hash from both
           cached_hash     = cached_adapter.serializable_hash
           non_cached_hash = non_cached_adapter.serializable_hash
 
           # Merge both results
-          @adapter.fragment_cache(cached_hash, non_cached_hash)
+          adapter.fragment_cache(cached_hash, non_cached_hash)
         end
 
         private
 
+        ActiveModelSerializers.silence_warnings do
+          attr_reader :instance_options, :adapter
+        end
+
         def cached_attributes(klass, serializers)
           attributes            = serializer.class._attributes
           cached_attributes     = (klass._cache_only) ? klass._cache_only : attributes.reject { |attr| klass._cache_except.include?(attr) }

lib/active_model/serializer/adapter/json.rb

@@ -15,13 +15,13 @@ def serializable_hash(options = nil)
 
             serializer.associations.each do |association|
               serializer = association.serializer
-              opts = association.options
+              association_options = association.options
 
               if serializer.respond_to?(:each)
                 array_serializer = serializer
                 hash[association.key] = array_serializer.map do |item|
                   cache_check(item) do
-                    item.attributes(opts)
+                    item.attributes(association_options)
                   end
                 end
               else
@@ -30,8 +30,8 @@ def serializable_hash(options = nil)
                     cache_check(serializer) do
                       serializer.attributes(options)
                     end
-                  elsif opts[:virtual_value]
-                    opts[:virtual_value]
+                  elsif association_options[:virtual_value]
+                    association_options[:virtual_value]
                   end
               end
             end

lib/active_model/serializer/adapter/json_api.rb

@@ -5,7 +5,7 @@ class ActiveModel::Serializer::Adapter::JsonApi < ActiveModel::Serializer::Adapt
 
         def initialize(serializer, options = {})
           super
-          @included = ActiveModel::Serializer::Utils.include_args_to_hash(@options[:include])
+          @included = ActiveModel::Serializer::Utils.include_args_to_hash(instance_options[:include])
           fields = options.delete(:fields)
           if fields
             @fieldset = ActiveModel::Serializer::Fieldset.new(fields, serializer.json_key)
@@ -24,16 +24,20 @@ def serializable_hash(options = nil)
         end
 
         def fragment_cache(cached_hash, non_cached_hash)
-          root = false if @options.include?(:include)
+          root = false if instance_options.include?(:include)
           ActiveModel::Serializer::Adapter::JsonApi::FragmentCache.new.fragment_cache(root, cached_hash, non_cached_hash)
         end
 
         private
 
+        ActiveModel.silence_warnings do
+          attr_reader :included, :fieldset
+        end
+
         def serializable_hash_for_collection(serializer, options)
           hash = { data: [] }
           serializer.each do |s|
-            result = self.class.new(s, @options.merge(fieldset: @fieldset)).serializable_hash(options)
+            result = self.class.new(s, instance_options.merge(fieldset: fieldset)).serializable_hash(options)
             hash[:data] << result[:data]
 
             if result[:included]
@@ -85,7 +89,7 @@ def resource_identifier_for(serializer)
         end
 
         def resource_object_for(serializer, options = {})
-          options[:fields] = @fieldset && @fieldset.fields_for(serializer)
+          options[:fields] = fieldset && fieldset.fields_for(serializer)
 
           cache_check(serializer) do
             result = resource_identifier_for(serializer)
@@ -120,12 +124,10 @@ def relationships_for(serializer)
         end
 
         def included_for(serializer)
-          included = @included.flat_map do |inc|
+          included.flat_map { |inc|
             association = serializer.associations.find { |assoc| assoc.key == inc.first }
             _included_for(association.serializer, inc.second) if association
-          end
-
-          included.uniq
+          }.uniq
         end
 
         def _included_for(serializer, includes)
@@ -134,7 +136,7 @@ def _included_for(serializer, includes)
           else
             return [] unless serializer && serializer.object
 
-            primary_data = primary_data_for(serializer, @options)
+            primary_data = primary_data_for(serializer, instance_options)
             relationships = relationships_for(serializer)
             primary_data[:relationships] = relationships if relationships.any?