From 35a7c81034dabb9b4b02c13291ece954412abb9a Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Mon, 6 Jun 2016 21:10:16 -0500 Subject: [PATCH] Fix up caching, especially fragment_cache --- lib/active_model/serializer.rb | 4 +- lib/active_model/serializer/caching.rb | 89 +++++++------------ .../adapter/json_api.rb | 2 +- .../json_api/resource_identifier_test.rb | 2 +- test/cache_test.rb | 40 +++++---- 5 files changed, 62 insertions(+), 75 deletions(-) diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index 570073c7..f94ba556 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -167,7 +167,7 @@ module ActiveModel adapter_options ||= {} options[:include_directive] ||= ActiveModel::Serializer.include_directive_from_options(adapter_options) cached_attributes = adapter_options[:cached_attributes] ||= {} - resource = cached_attributes(options[:fields], cached_attributes, adapter_instance) + resource = fetch_attributes(options[:fields], cached_attributes, adapter_instance) relationships = resource_relationships(adapter_options, options, adapter_instance) resource.merge(relationships) end @@ -195,6 +195,8 @@ module ActiveModel if respond_to?(attr) send(attr) elsif self.class._fragmented + # Attribute method wasn't available on this (fragment cached) serializer, + # so read it from the original serializer it was based on. self.class._fragmented.read_attribute_for_serialization(attr) else object.read_attribute_for_serialization(attr) diff --git a/lib/active_model/serializer/caching.rb b/lib/active_model/serializer/caching.rb index 5adf4ee3..0897ea01 100644 --- a/lib/active_model/serializer/caching.rb +++ b/lib/active_model/serializer/caching.rb @@ -1,5 +1,3 @@ -# TODO(BF): refactor file to be smaller -# rubocop:disable Metrics/ModuleLength module ActiveModel class Serializer UndefinedCacheKey = Class.new(StandardError) @@ -9,10 +7,10 @@ module ActiveModel included do with_options instance_writer: false, instance_reader: false do |serializer| serializer.class_attribute :_cache # @api private : the cache store - serializer.class_attribute :_fragmented # @api private : @see ::fragmented + serializer.class_attribute :_fragmented # @api private : Used ONLY by FragmentedSerializer to reference original serializer serializer.class_attribute :_cache_key # @api private : when present, is first item in cache_key. Ignored if the serializable object defines #cache_key. - serializer.class_attribute :_cache_only # @api private : when fragment caching, whitelists cached_attributes. Cannot combine with except - serializer.class_attribute :_cache_except # @api private : when fragment caching, blacklists cached_attributes. Cannot combine with only + serializer.class_attribute :_cache_only # @api private : when fragment caching, whitelists fetch_attributes. Cannot combine with except + serializer.class_attribute :_cache_except # @api private : when fragment caching, blacklists fetch_attributes. Cannot combine with only serializer.class_attribute :_cache_options # @api private : used by CachedSerializer, passed to _cache.fetch # _cache_options include: # expires_in @@ -79,13 +77,6 @@ module ActiveModel _attributes - cached_attributes end - # @api private - # Used by FragmentCache on the CachedSerializer - # to call attribute methods on the fragmented cached serializer. - def fragmented(serializer) - self._fragmented = serializer - end - # Enables a serializer to be automatically cached # # Sets +::_cache+ object to ActionController::Base.cache_store @@ -208,46 +199,44 @@ module ActiveModel end end - def cached_attributes(fields, cached_attributes, adapter_instance) + ### INSTANCE METHODS + def fetch_attributes(fields, cached_attributes, adapter_instance) if self.class.cache_enabled? key = cache_key(adapter_instance) cached_attributes.fetch(key) do - cache_check(adapter_instance) do + self.class.cache_store.fetch(key, self.class._cache_options) do attributes(fields) end end + elsif self.class.fragment_cache_enabled? + fetch_fragment_cache(adapter_instance) else - cache_check(adapter_instance) do - attributes(fields) - end + attributes(fields) end end - def cache_check(adapter_instance) - if self.class.cache_enabled? - self.class.cache_store.fetch(cache_key(adapter_instance), self.class._cache_options) do + def fetch(adapter_instance, cache_options = self.class._cache_options) + if self.class.cache_store + self.class.cache_store.fetch(cache_key(adapter_instance), cache_options) do yield end - elsif self.class.fragment_cache_enabled? - fetch_fragment_cache(adapter_instance) else yield end end - # 1. Create a CachedSerializer and NonCachedSerializer from the serializer class + # 1. Create a CachedSerializer from the serializer class # 2. Serialize the above two with the given adapter # 3. Pass their serializations to the adapter +::fragment_cache+ # # It will split the serializer into two, one that will be cached and one that will not # # Given a resource name - # 1. Dynamically creates a CachedSerializer and NonCachedSerializer + # 1. Dynamically creates a CachedSerializer # for a given class 'name' # 2. Call # CachedSerializer.cache(serializer._cache_options) - # CachedSerializer.fragmented(serializer) - # NonCachedSerializer.cache(serializer._cache_options) + # CachedSerializer._fragmented = serializer # 3. Build a hash keyed to the +cached+ and +non_cached+ serializers # 4. Call +cached_attributes+ on the serializer class and the above hash # 5. Return the hash @@ -256,40 +245,47 @@ module ActiveModel # When +name+ is User::Admin # creates the Serializer classes (if they don't exist). # CachedUser_AdminSerializer - # NonCachedUser_AdminSerializer # # Given a hash of its cached and non-cached serializers # 1. Determine cached attributes from serializer class options # 2. Add cached attributes to cached Serializer # 3. Add non-cached attributes to non-cached Serializer def fetch_fragment_cache(adapter_instance) - serializer_class_name = self.class.name.gsub('::'.freeze, '_'.freeze) self.class._cache_options ||= {} self.class._cache_options[:key] = self.class._cache_key if self.class._cache_key - cached_serializer = _get_or_create_fragment_cached_serializer(serializer_class_name) + cached_serializer = _get_or_create_fragment_cached_serializer cached_hash = ActiveModelSerializers::SerializableResource.new( object, serializer: cached_serializer, adapter: adapter_instance.class ).serializable_hash - non_cached_serializer = _get_or_create_fragment_non_cached_serializer(serializer_class_name) - non_cached_hash = ActiveModelSerializers::SerializableResource.new( - object, - serializer: non_cached_serializer, - adapter: adapter_instance.class - ).serializable_hash + fields = self.class.non_cached_attributes + non_cached_hash = attributes(fields, true) # Merge both results adapter_instance.fragment_cache(cached_hash, non_cached_hash) end - def _get_or_create_fragment_cached_serializer(serializer_class_name) - cached_serializer = _get_or_create_fragment_serializer "Cached#{serializer_class_name}" + def _get_or_create_fragment_cached_serializer + serializer_class_name = self.class.name.gsub('::'.freeze, '_'.freeze) + cached_serializer_name = "Cached#{serializer_class_name}" + if Object.const_defined?(cached_serializer_name) + cached_serializer = Object.const_get(cached_serializer_name) + # HACK: Test concern in production code :( + # But it's better than running all the cached fragment serializer + # code multiple times. + if Rails.env == 'test'.freeze + Object.send(:remove_const, cached_serializer_name) + return _get_or_create_fragment_cached_serializer + end + return cached_serializer + end + cached_serializer = Object.const_set(cached_serializer_name, Class.new(ActiveModel::Serializer)) cached_serializer.cache(self.class._cache_options) cached_serializer.type(self.class._type) - cached_serializer.fragmented(self) + cached_serializer._fragmented = self self.class.cached_attributes.each do |attribute| options = self.class._attributes_keys[attribute] || {} cached_serializer.attribute(attribute, options) @@ -297,22 +293,6 @@ module ActiveModel cached_serializer end - def _get_or_create_fragment_non_cached_serializer(serializer_class_name) - non_cached_serializer = _get_or_create_fragment_serializer "NonCached#{serializer_class_name}" - non_cached_serializer.type(self.class._type) - non_cached_serializer.fragmented(self) - self.class.non_cached_attributes.each do |attribute| - options = self.class._attributes_keys[attribute] || {} - non_cached_serializer.attribute(attribute, options) - end - non_cached_serializer - end - - def _get_or_create_fragment_serializer(name) - return Object.const_get(name) if Object.const_defined?(name) - Object.const_set(name, Class.new(ActiveModel::Serializer)) - end - def cache_key(adapter_instance) return @cache_key if defined?(@cache_key) @@ -339,4 +319,3 @@ module ActiveModel end end end -# rubocop:enable Metrics/ModuleLength diff --git a/lib/active_model_serializers/adapter/json_api.rb b/lib/active_model_serializers/adapter/json_api.rb index 5293078c..6232086b 100644 --- a/lib/active_model_serializers/adapter/json_api.rb +++ b/lib/active_model_serializers/adapter/json_api.rb @@ -294,7 +294,7 @@ module ActiveModelSerializers # {http://jsonapi.org/format/#document-resource-objects Document Resource Objects} def resource_object_for(serializer) - resource_object = serializer.cache_check(self) do + resource_object = serializer.fetch(self) do resource_object = ResourceIdentifier.new(serializer, instance_options).as_json requested_fields = fieldset && fieldset.fields_for(resource_object[:type]) diff --git a/test/adapter/json_api/resource_identifier_test.rb b/test/adapter/json_api/resource_identifier_test.rb index a6f312c4..7624d92b 100644 --- a/test/adapter/json_api/resource_identifier_test.rb +++ b/test/adapter/json_api/resource_identifier_test.rb @@ -42,7 +42,7 @@ module ActiveModelSerializers end def test_id_defined_on_fragmented - FragmentedSerializer.fragmented(WithDefinedIdSerializer.new(@model)) + FragmentedSerializer._fragmented = WithDefinedIdSerializer.new(@model) test_id(FragmentedSerializer, 'special_id') end diff --git a/test/cache_test.rb b/test/cache_test.rb index 9dba8d70..e804eb5b 100644 --- a/test/cache_test.rb +++ b/test/cache_test.rb @@ -204,7 +204,7 @@ module ActiveModelSerializers # Based on original failing test by @kevintyll # rubocop:disable Metrics/AbcSize - def test_a_serializer_rendered_by_two_adapter_returns_differently_cached_attributes + def test_a_serializer_rendered_by_two_adapter_returns_differently_fetch_attributes Object.const_set(:Alert, Class.new(ActiveModelSerializers::Model) do attr_accessor :id, :status, :resource, :started_at, :ended_at, :updated_at, :created_at end) @@ -225,7 +225,7 @@ module ActiveModelSerializers created_at: Time.new(2016, 3, 31, 21, 37, 35, 0) ) - expected_cached_attributes = { + expected_fetch_attributes = { id: 1, status: 'fail', resource: 'resource-1', @@ -250,7 +250,7 @@ module ActiveModelSerializers # Assert attributes are serialized correctly serializable_alert = serializable(alert, serializer: AlertSerializer, adapter: :attributes) attributes_serialization = serializable_alert.as_json - assert_equal expected_cached_attributes, alert.attributes + assert_equal expected_fetch_attributes, alert.attributes assert_equal alert.attributes, attributes_serialization attributes_cache_key = serializable_alert.adapter.serializer.cache_key(serializable_alert.adapter) assert_equal attributes_serialization, cache_store.fetch(attributes_cache_key) @@ -296,23 +296,28 @@ module ActiveModelSerializers assert actual.any? { |key| key =~ %r{author/author-\d+} } end - def test_cached_attributes - serializer = ActiveModel::Serializer::CollectionSerializer.new([@comment, @comment]) + def test_fetch_attributes_from_cache + serializers = ActiveModel::Serializer::CollectionSerializer.new([@comment, @comment]) Timecop.freeze(Time.current) do render_object_with_cache(@comment) - attributes = Adapter::Attributes.new(serializer) - include_directive = ActiveModelSerializers.default_include_directive - cached_attributes = ActiveModel::Serializer.cache_read_multi(serializer, attributes, include_directive) + options = {} + adapter_options = {} + adapter_instance = ActiveModelSerializers::Adapter::Attributes.new(serializers, adapter_options) + serializers.serializable_hash(adapter_options, options, adapter_instance) + cached_attributes = adapter_options.fetch(:cached_attributes) - assert_equal cached_attributes["#{@comment.cache_key}/#{attributes.cache_key}"], Comment.new(id: 1, body: 'ZOMG A COMMENT').attributes - assert_equal cached_attributes["#{@comment.post.cache_key}/#{attributes.cache_key}"], Post.new(id: 'post', title: 'New Post', body: 'Body').attributes + include_directive = ActiveModelSerializers.default_include_directive + manual_cached_attributes = ActiveModel::Serializer.cache_read_multi(serializers, adapter_instance, include_directive) + assert_equal manual_cached_attributes, cached_attributes + + assert_equal cached_attributes["#{@comment.cache_key}/#{adapter_instance.cache_key}"], Comment.new(id: 1, body: 'ZOMG A COMMENT').attributes + assert_equal cached_attributes["#{@comment.post.cache_key}/#{adapter_instance.cache_key}"], Post.new(id: 'post', title: 'New Post', body: 'Body').attributes writer = @comment.post.blog.writer writer_cache_key = writer.cache_key - - assert_equal cached_attributes["#{writer_cache_key}/#{attributes.cache_key}"], Author.new(id: 'author', name: 'Joao M. D. Moura').attributes + assert_equal cached_attributes["#{writer_cache_key}/#{adapter_instance.cache_key}"], Author.new(id: 'author', name: 'Joao M. D. Moura').attributes end end @@ -442,6 +447,9 @@ module ActiveModelSerializers name: @role.name } assert_equal(@role_hash, expected_result) + ensure + fragmented_serializer = @role_serializer + Object.send(:remove_const, fragmented_serializer._get_or_create_fragment_cached_serializer.name) end def test_fragment_fetch_with_namespaced_object @@ -452,6 +460,9 @@ module ActiveModelSerializers id: @spam.id } assert_equal(@spam_hash, expected_result) + ensure + fragmented_serializer = @spam_serializer + Object.send(:remove_const, fragmented_serializer._get_or_create_fragment_cached_serializer.name) end private @@ -476,10 +487,5 @@ module ActiveModelSerializers def adapter @serializable_resource.adapter end - - def cached_serialization(serializer) - cache_key = serializer.cache_key(adapter) - cache_store.fetch(cache_key) - end end end