From f28e486d4a645c2f4d93e4d55d4060c8b3668afe Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Wed, 1 Jun 2016 01:32:14 -0500 Subject: [PATCH 1/6] Remove Attributes adapter recursion --- .../adapter/attributes.rb | 48 +++++++++++-------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/lib/active_model_serializers/adapter/attributes.rb b/lib/active_model_serializers/adapter/attributes.rb index 56a155c3..9a15f616 100644 --- a/lib/active_model_serializers/adapter/attributes.rb +++ b/lib/active_model_serializers/adapter/attributes.rb @@ -3,44 +3,51 @@ module ActiveModelSerializers class Attributes < Base def initialize(serializer, options = {}) super - @include_directive = - if options[:include_directive] - options[:include_directive] - elsif options[:include] - JSONAPI::IncludeDirective.new(options[:include], allow_wildcard: true) - else - ActiveModelSerializers.default_include_directive - end end def serializable_hash(options = nil) options = serialization_options(options) if serializer.respond_to?(:each) - serializable_hash_for_collection(options) + serializable_hash_for_collection(serializer, options) else - serializable_hash_for_single_resource(options) + serializable_hash_for_single_resource(serializer, instance_options, options) end end private - def serializable_hash_for_collection(options) - instance_options[:cached_attributes] ||= ActiveModel::Serializer.cache_read_multi(serializer, self, @include_directive) - opts = instance_options.merge(include_directive: @include_directive) - serializer.map { |s| Attributes.new(s, opts).serializable_hash(options) } + def include_directive_from_options(options) + if options[:include_directive] + options[:include_directive] + elsif options[:include] + JSONAPI::IncludeDirective.new(options[:include], allow_wildcard: true) + else + ActiveModelSerializers.default_include_directive + end end - def serializable_hash_for_single_resource(options) - cached_attributes = instance_options[:cached_attributes] || {} + def serializable_hash_for_collection(serializers, options) + include_directive = include_directive_from_options(instance_options) + instance_options[:cached_attributes] ||= ActiveModel::Serializer.cache_read_multi(serializers, self, include_directive) + instance_opts = instance_options.merge(include_directive: include_directive) + serializers.map do |serializer| + serializable_hash_for_single_resource(serializer, instance_opts, options) + end + end + + def serializable_hash_for_single_resource(serializer, instance_options, options) + options[:include_directive] ||= include_directive_from_options(instance_options) + cached_attributes = instance_options[:cached_attributes] ||= {} resource = serializer.cached_attributes(options[:fields], cached_attributes, self) - relationships = resource_relationships(options) + relationships = resource_relationships(serializer, options) resource.merge(relationships) end - def resource_relationships(options) + def resource_relationships(serializer, options) relationships = {} - serializer.associations(@include_directive).each do |association| + include_directive = options.fetch(:include_directive) + serializer.associations(include_directive).each do |association| relationships[association.key] ||= relationship_value_for(association, options) end @@ -51,7 +58,8 @@ module ActiveModelSerializers return association.options[:virtual_value] if association.options[:virtual_value] return unless association.serializer && association.serializer.object - opts = instance_options.merge(include_directive: @include_directive[association.key]) + include_directive = options.fetch(:include_directive) + opts = instance_options.merge(include_directive: include_directive[association.key]) relationship_value = Attributes.new(association.serializer, opts).serializable_hash(options) if association.options[:polymorphic] && relationship_value From 41575e36f772e2c96b086cd3f163212a99421abb Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Wed, 1 Jun 2016 01:39:13 -0500 Subject: [PATCH 2/6] Moving Attributes#serializable_hash_for_single_resource to Serializer --- lib/active_model/serializer.rb | 43 ++++++++++++++++++ .../adapter/attributes.rb | 45 ++----------------- 2 files changed, 46 insertions(+), 42 deletions(-) diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index 50ac2bbd..cee2f8d0 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -98,6 +98,16 @@ module ActiveModel end end + def self.include_directive_from_options(options) + if options[:include_directive] + options[:include_directive] + elsif options[:include] + JSONAPI::IncludeDirective.new(options[:include], allow_wildcard: true) + else + ActiveModelSerializers.default_include_directive + end + end + attr_accessor :object, :root, :scope # `scope_name` is set as :current_user by default in the controller. @@ -185,6 +195,39 @@ module ActiveModel end end + def serializable_hash_for_single_resource(adapter_options, options, adapter_instance) + cached_attributes = adapter_options[:cached_attributes] ||= {} + resource = cached_attributes(options[:fields], cached_attributes, adapter_instance) + relationships = resource_relationships(options) + resource.merge(relationships) + end + + def resource_relationships(options) + relationships = {} + include_directive = options.fetch(:include_directive) + associations(include_directive).each do |association| + relationships[association.key] ||= relationship_value_for(association, options) + end + + relationships + end + + def relationship_value_for(association, options) + return association.options[:virtual_value] if association.options[:virtual_value] + return unless association.serializer && association.serializer.object + + include_directive = options.fetch(:include_directive) + opts = instance_options.merge(include_directive: include_directive[association.key]) + relationship_value = ActiveModelSerializers::Adapter::Attributes.new(association.serializer, opts).serializable_hash(options) + + if association.options[:polymorphic] && relationship_value + polymorphic_type = association.serializer.object.class.name.underscore + relationship_value = { type: polymorphic_type, polymorphic_type.to_sym => relationship_value } + end + + relationship_value + end + protected attr_accessor :instance_options diff --git a/lib/active_model_serializers/adapter/attributes.rb b/lib/active_model_serializers/adapter/attributes.rb index 9a15f616..b32169db 100644 --- a/lib/active_model_serializers/adapter/attributes.rb +++ b/lib/active_model_serializers/adapter/attributes.rb @@ -17,18 +17,8 @@ module ActiveModelSerializers private - def include_directive_from_options(options) - if options[:include_directive] - options[:include_directive] - elsif options[:include] - JSONAPI::IncludeDirective.new(options[:include], allow_wildcard: true) - else - ActiveModelSerializers.default_include_directive - end - end - def serializable_hash_for_collection(serializers, options) - include_directive = include_directive_from_options(instance_options) + include_directive = ActiveModel::Serializer.include_directive_from_options(instance_options) instance_options[:cached_attributes] ||= ActiveModel::Serializer.cache_read_multi(serializers, self, include_directive) instance_opts = instance_options.merge(include_directive: include_directive) serializers.map do |serializer| @@ -37,37 +27,8 @@ module ActiveModelSerializers end def serializable_hash_for_single_resource(serializer, instance_options, options) - options[:include_directive] ||= include_directive_from_options(instance_options) - cached_attributes = instance_options[:cached_attributes] ||= {} - resource = serializer.cached_attributes(options[:fields], cached_attributes, self) - relationships = resource_relationships(serializer, options) - resource.merge(relationships) - end - - def resource_relationships(serializer, options) - relationships = {} - include_directive = options.fetch(:include_directive) - serializer.associations(include_directive).each do |association| - relationships[association.key] ||= relationship_value_for(association, options) - end - - relationships - end - - def relationship_value_for(association, options) - return association.options[:virtual_value] if association.options[:virtual_value] - return unless association.serializer && association.serializer.object - - include_directive = options.fetch(:include_directive) - opts = instance_options.merge(include_directive: include_directive[association.key]) - relationship_value = Attributes.new(association.serializer, opts).serializable_hash(options) - - if association.options[:polymorphic] && relationship_value - polymorphic_type = association.serializer.object.class.name.underscore - relationship_value = { type: polymorphic_type, polymorphic_type.to_sym => relationship_value } - end - - relationship_value + options[:include_directive] ||= ActiveModel::Serializer.include_directive_from_options(instance_options) + serializer.serializable_hash_for_single_resource(instance_options, options, self) end end end From 516e7da8ff18714aff5aa8d5d08ca60ef76e7ebe Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Wed, 1 Jun 2016 01:49:04 -0500 Subject: [PATCH 3/6] Move serialization logic into Serializer and CollectionSerializer --- lib/active_model/serializer.rb | 32 +++++++++++-------- .../serializer/collection_serializer.rb | 10 ++++++ .../adapter/attributes.rb | 27 +--------------- 3 files changed, 30 insertions(+), 39 deletions(-) diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index cee2f8d0..6b376b9c 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -98,6 +98,7 @@ module ActiveModel end end + # @api private def self.include_directive_from_options(options) if options[:include_directive] options[:include_directive] @@ -161,9 +162,9 @@ module ActiveModel # serializer.as_json(include: { posts: { include: { comments: { only: :body } }, only: :title } }) def serializable_hash(adapter_opts = nil) adapter_opts ||= {} - adapter_opts = { include: '*', adapter: :attributes }.merge!(adapter_opts) - adapter = ActiveModelSerializers::Adapter.create(self, adapter_opts) - adapter.serializable_hash(adapter_opts) + adapter_opts = { include: '*' }.merge!(adapter_opts) + adapter_instance = ActiveModelSerializers::Adapter::Attributes.new(self, adapter_opts) + serialize(adapter_opts, {}, adapter_instance) end alias to_hash serializable_hash alias to_h serializable_hash @@ -195,33 +196,38 @@ module ActiveModel end end - def serializable_hash_for_single_resource(adapter_options, options, adapter_instance) + # @api private + def serialize(adapter_options, options, adapter_instance) + 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) - relationships = resource_relationships(options) + relationships = resource_relationships(adapter_options, options, adapter_instance) resource.merge(relationships) end - def resource_relationships(options) + # @api private + def resource_relationships(adapter_options, options, adapter_instance) relationships = {} include_directive = options.fetch(:include_directive) associations(include_directive).each do |association| - relationships[association.key] ||= relationship_value_for(association, options) + adapter_opts = adapter_options.merge(include_directive: include_directive[association.key]) + relationships[association.key] ||= relationship_value_for(association, adapter_opts, adapter_instance) end relationships end - def relationship_value_for(association, options) + # @api private + def relationship_value_for(association, adapter_options, adapter_instance) return association.options[:virtual_value] if association.options[:virtual_value] - return unless association.serializer && association.serializer.object + association_serializer = association.serializer + association_object = association_serializer && association_serializer.object + return unless association_object - include_directive = options.fetch(:include_directive) - opts = instance_options.merge(include_directive: include_directive[association.key]) - relationship_value = ActiveModelSerializers::Adapter::Attributes.new(association.serializer, opts).serializable_hash(options) + relationship_value = association_serializer.serialize(adapter_options, {}, adapter_instance) if association.options[:polymorphic] && relationship_value - polymorphic_type = association.serializer.object.class.name.underscore + polymorphic_type = association_object.class.name.underscore relationship_value = { type: polymorphic_type, polymorphic_type.to_sym => relationship_value } end diff --git a/lib/active_model/serializer/collection_serializer.rb b/lib/active_model/serializer/collection_serializer.rb index f026ebfe..a2367195 100644 --- a/lib/active_model/serializer/collection_serializer.rb +++ b/lib/active_model/serializer/collection_serializer.rb @@ -56,6 +56,16 @@ module ActiveModel object.respond_to?(:size) end + # @api private + def serialize(adapter_options, options, adapter_instance) + include_directive = ActiveModel::Serializer.include_directive_from_options(adapter_options) + adapter_options[:cached_attributes] ||= ActiveModel::Serializer.cache_read_multi(self, adapter_instance, include_directive) + adapter_opts = adapter_options.merge(include_directive: include_directive) + serializers.map do |serializer| + serializer.serialize(adapter_opts, options, adapter_instance) + end + end + protected attr_reader :serializers, :options diff --git a/lib/active_model_serializers/adapter/attributes.rb b/lib/active_model_serializers/adapter/attributes.rb index b32169db..3d2e7b52 100644 --- a/lib/active_model_serializers/adapter/attributes.rb +++ b/lib/active_model_serializers/adapter/attributes.rb @@ -1,34 +1,9 @@ module ActiveModelSerializers module Adapter class Attributes < Base - def initialize(serializer, options = {}) - super - end - def serializable_hash(options = nil) options = serialization_options(options) - - if serializer.respond_to?(:each) - serializable_hash_for_collection(serializer, options) - else - serializable_hash_for_single_resource(serializer, instance_options, options) - end - end - - private - - def serializable_hash_for_collection(serializers, options) - include_directive = ActiveModel::Serializer.include_directive_from_options(instance_options) - instance_options[:cached_attributes] ||= ActiveModel::Serializer.cache_read_multi(serializers, self, include_directive) - instance_opts = instance_options.merge(include_directive: include_directive) - serializers.map do |serializer| - serializable_hash_for_single_resource(serializer, instance_opts, options) - end - end - - def serializable_hash_for_single_resource(serializer, instance_options, options) - options[:include_directive] ||= ActiveModel::Serializer.include_directive_from_options(instance_options) - serializer.serializable_hash_for_single_resource(instance_options, options, self) + serializer.serialize(instance_options, options, self) end end end From 913f396bb17823bf88f57afbb1abcee8bad60f32 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sat, 4 Jun 2016 14:37:51 -0500 Subject: [PATCH 4/6] Move adapter cache properties to class level (where they belong). --- lib/active_model/serializer.rb | 9 ++- lib/active_model/serializer/caching.rb | 2 +- lib/active_model_serializers/adapter.rb | 4 ++ lib/active_model_serializers/adapter/base.rb | 72 ++++++++++--------- .../adapter/json_api.rb | 36 +++++----- test/cache_test.rb | 28 ++++---- 6 files changed, 84 insertions(+), 67 deletions(-) diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index 6b376b9c..b2884b9c 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -109,6 +109,10 @@ module ActiveModel end end + def self.serialization_adapter_instance + @serialization_adapter_instance ||= ActiveModelSerializers::Adapter::Attributes + end + attr_accessor :object, :root, :scope # `scope_name` is set as :current_user by default in the controller. @@ -163,8 +167,7 @@ module ActiveModel def serializable_hash(adapter_opts = nil) adapter_opts ||= {} adapter_opts = { include: '*' }.merge!(adapter_opts) - adapter_instance = ActiveModelSerializers::Adapter::Attributes.new(self, adapter_opts) - serialize(adapter_opts, {}, adapter_instance) + serialize(adapter_opts) end alias to_hash serializable_hash alias to_h serializable_hash @@ -197,7 +200,7 @@ module ActiveModel end # @api private - def serialize(adapter_options, options, adapter_instance) + def serialize(adapter_options, options = {}, adapter_instance = self.class.serialization_adapter_instance) 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) diff --git a/lib/active_model/serializer/caching.rb b/lib/active_model/serializer/caching.rb index 0f1fffb2..5adf4ee3 100644 --- a/lib/active_model/serializer/caching.rb +++ b/lib/active_model/serializer/caching.rb @@ -318,7 +318,7 @@ module ActiveModel parts = [] parts << object_cache_key - parts << adapter_instance.cached_name + parts << adapter_instance.cache_key parts << self.class._cache_digest unless self.class._skip_digest? @cache_key = parts.join('/') end diff --git a/lib/active_model_serializers/adapter.rb b/lib/active_model_serializers/adapter.rb index 44c10db1..93e4c044 100644 --- a/lib/active_model_serializers/adapter.rb +++ b/lib/active_model_serializers/adapter.rb @@ -51,6 +51,10 @@ module ActiveModelSerializers self end + def registered_name(adapter_class) + ADAPTER_MAP.key adapter_class + end + # @param adapter [String, Symbol, Class] name to fetch adapter by # @return [ActiveModelSerializers::Adapter] subclass of Adapter # @raise [UnknownAdapterError] diff --git a/lib/active_model_serializers/adapter/base.rb b/lib/active_model_serializers/adapter/base.rb index 8c5e73d3..7b60db70 100644 --- a/lib/active_model_serializers/adapter/base.rb +++ b/lib/active_model_serializers/adapter/base.rb @@ -8,6 +8,40 @@ module ActiveModelSerializers ActiveModelSerializers::Adapter.register(subclass) end + # Sets the default transform for the adapter. + # + # @return [Symbol] the default transform for the adapter + def self.default_key_transform + :unaltered + end + + # Determines the transform to use in order of precedence: + # adapter option, global config, adapter default. + # + # @param options [Object] + # @return [Symbol] the transform to use + def self.transform(options) + return options[:key_transform] if options && options[:key_transform] + ActiveModelSerializers.config.key_transform || default_key_transform + end + + # Transforms the casing of the supplied value. + # + # @param value [Object] the value to be transformed + # @param options [Object] serializable resource options + # @return [Symbol] the default transform for the adapter + def self.transform_key_casing!(value, options) + KeyTransform.send(transform(options), value) + end + + def self.cache_key + @cache_key ||= ActiveModelSerializers::Adapter.registered_name(self) + end + + def self.fragment_cache(cached_hash, non_cached_hash) + non_cached_hash.merge cached_hash + end + attr_reader :serializer, :instance_options def initialize(serializer, options = {}) @@ -15,10 +49,6 @@ module ActiveModelSerializers @instance_options = options end - def cached_name - @cached_name ||= self.class.name.demodulize.underscore - end - # Subclasses that implement this method must first call # options = serialization_options(options) def serializable_hash(_options = nil) @@ -29,8 +59,12 @@ module ActiveModelSerializers serializable_hash(options) end + def cache_key + self.class.cache_key + end + def fragment_cache(cached_hash, non_cached_hash) - non_cached_hash.merge cached_hash + self.class.fragment_cache(cached_hash, non_cached_hash) end private @@ -44,34 +78,6 @@ module ActiveModelSerializers def root serializer.json_key.to_sym if serializer.json_key end - - class << self - # Sets the default transform for the adapter. - # - # @return [Symbol] the default transform for the adapter - def default_key_transform - :unaltered - end - - # Determines the transform to use in order of precedence: - # adapter option, global config, adapter default. - # - # @param options [Object] - # @return [Symbol] the transform to use - def transform(options) - return options[:key_transform] if options && options[:key_transform] - ActiveModelSerializers.config.key_transform || default_key_transform - end - - # Transforms the casing of the supplied value. - # - # @param value [Object] the value to be transformed - # @param options [Object] serializable resource options - # @return [Symbol] the default transform for the adapter - def transform_key_casing!(value, options) - KeyTransform.send(transform(options), value) - end - end end end end diff --git a/lib/active_model_serializers/adapter/json_api.rb b/lib/active_model_serializers/adapter/json_api.rb index eb7f39b7..5293078c 100644 --- a/lib/active_model_serializers/adapter/json_api.rb +++ b/lib/active_model_serializers/adapter/json_api.rb @@ -31,16 +31,27 @@ module ActiveModelSerializers autoload :Error autoload :Deserialization + def self.default_key_transform + :dash + end + + def self.fragment_cache(cached_hash, non_cached_hash, root = true) + core_cached = cached_hash.first + core_non_cached = non_cached_hash.first + no_root_cache = cached_hash.delete_if { |key, _value| key == core_cached[0] } + no_root_non_cache = non_cached_hash.delete_if { |key, _value| key == core_non_cached[0] } + cached_resource = (core_cached[1]) ? core_cached[1].deep_merge(core_non_cached[1]) : core_non_cached[1] + hash = root ? { root => cached_resource } : cached_resource + + hash.deep_merge no_root_non_cache.deep_merge no_root_cache + end + def initialize(serializer, options = {}) super @include_directive = JSONAPI::IncludeDirective.new(options[:include], allow_wildcard: true) @fieldset = options[:fieldset] || ActiveModel::Serializer::Fieldset.new(options.delete(:fields)) end - def self.default_key_transform - :dash - end - # {http://jsonapi.org/format/#crud Requests are transactional, i.e. success or failure} # {http://jsonapi.org/format/#document-top-level data and errors MUST NOT coexist in the same document.} def serializable_hash(*) @@ -52,6 +63,11 @@ module ActiveModelSerializers self.class.transform_key_casing!(document, instance_options) end + def fragment_cache(cached_hash, non_cached_hash) + root = !instance_options.include?(:include) + self.class.fragment_cache(cached_hash, non_cached_hash, root) + end + # {http://jsonapi.org/format/#document-top-level Primary data} # definition: # ☐ toplevel_data (required) @@ -174,18 +190,6 @@ module ActiveModelSerializers hash end - def fragment_cache(cached_hash, non_cached_hash) - root = false if instance_options.include?(:include) - core_cached = cached_hash.first - core_non_cached = non_cached_hash.first - no_root_cache = cached_hash.delete_if { |key, _value| key == core_cached[0] } - no_root_non_cache = non_cached_hash.delete_if { |key, _value| key == core_non_cached[0] } - cached_resource = (core_cached[1]) ? core_cached[1].deep_merge(core_non_cached[1]) : core_non_cached[1] - hash = root ? { root => cached_resource } : cached_resource - - hash.deep_merge no_root_non_cache.deep_merge no_root_cache - end - protected attr_reader :fieldset diff --git a/test/cache_test.rb b/test/cache_test.rb index ce208ddf..9dba8d70 100644 --- a/test/cache_test.rb +++ b/test/cache_test.rb @@ -102,13 +102,13 @@ module ActiveModelSerializers render_object_with_cache(uncached_author) key = "#{uncached_author_serializer.class._cache_key}/#{uncached_author_serializer.object.id}-#{uncached_author_serializer.object.updated_at.strftime("%Y%m%d%H%M%S%9N")}" - key = "#{key}/#{adapter.cached_name}" + key = "#{key}/#{adapter.cache_key}" assert_equal(uncached_author_serializer.attributes.to_json, cache_store.fetch(key).to_json) end def test_default_cache_key_fallback render_object_with_cache(@comment) - key = "#{@comment.cache_key}/#{adapter.cached_name}" + key = "#{@comment.cache_key}/#{adapter.cache_key}" assert_equal(@comment_serializer.attributes.to_json, cache_store.fetch(key).to_json) end @@ -139,9 +139,9 @@ module ActiveModelSerializers Timecop.freeze(Time.current) do render_object_with_cache(@post) - key = "#{@post.cache_key}/#{adapter.cached_name}" + key = "#{@post.cache_key}/#{adapter.cache_key}" assert_equal(@post_serializer.attributes, cache_store.fetch(key)) - key = "#{@comment.cache_key}/#{adapter.cached_name}" + key = "#{@comment.cache_key}/#{adapter.cache_key}" assert_equal(@comment_serializer.attributes, cache_store.fetch(key)) end end @@ -152,9 +152,9 @@ module ActiveModelSerializers render_object_with_cache(@post) # Check if it cached the objects separately - key = "#{@post.cache_key}/#{adapter.cached_name}" + key = "#{@post.cache_key}/#{adapter.cache_key}" assert_equal(@post_serializer.attributes, cache_store.fetch(key)) - key = "#{@comment.cache_key}/#{adapter.cached_name}" + key = "#{@comment.cache_key}/#{adapter.cache_key}" assert_equal(@comment_serializer.attributes, cache_store.fetch(key)) # Simulating update on comments relationship with Post @@ -166,9 +166,9 @@ module ActiveModelSerializers render_object_with_cache(@post) # Check if the the new comment was cached - key = "#{new_comment.cache_key}/#{adapter.cached_name}" + key = "#{new_comment.cache_key}/#{adapter.cache_key}" assert_equal(new_comment_serializer.attributes, cache_store.fetch(key)) - key = "#{@post.cache_key}/#{adapter.cached_name}" + key = "#{@post.cache_key}/#{adapter.cache_key}" assert_equal(@post_serializer.attributes, cache_store.fetch(key)) end end @@ -184,7 +184,7 @@ module ActiveModelSerializers hash = render_object_with_cache(@location) assert_equal(hash, expected_result) - key = "#{@location.cache_key}/#{adapter.cached_name}" + key = "#{@location.cache_key}/#{adapter.cache_key}" assert_equal({ place: 'Nowhere' }, cache_store.fetch(key)) end @@ -276,7 +276,7 @@ module ActiveModelSerializers def test_uses_file_digest_in_cache_key render_object_with_cache(@blog) - key = "#{@blog.cache_key}/#{adapter.cached_name}/#{::Model::FILE_DIGEST}" + key = "#{@blog.cache_key}/#{adapter.cache_key}/#{::Model::FILE_DIGEST}" assert_equal(@blog_serializer.attributes, cache_store.fetch(key)) end @@ -291,7 +291,7 @@ module ActiveModelSerializers actual = ActiveModel::Serializer.object_cache_keys(serializable.adapter.serializer, serializable.adapter, include_directive) assert_equal 3, actual.size - assert actual.any? { |key| key == "comment/1/#{serializable.adapter.cached_name}" } + assert actual.any? { |key| key == "comment/1/#{serializable.adapter.cache_key}" } assert actual.any? { |key| key =~ %r{post/post-\d+} } assert actual.any? { |key| key =~ %r{author/author-\d+} } end @@ -306,13 +306,13 @@ module ActiveModelSerializers include_directive = ActiveModelSerializers.default_include_directive cached_attributes = ActiveModel::Serializer.cache_read_multi(serializer, attributes, include_directive) - assert_equal cached_attributes["#{@comment.cache_key}/#{attributes.cached_name}"], Comment.new(id: 1, body: 'ZOMG A COMMENT').attributes - assert_equal cached_attributes["#{@comment.post.cache_key}/#{attributes.cached_name}"], Post.new(id: 'post', title: 'New Post', body: 'Body').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 writer = @comment.post.blog.writer writer_cache_key = writer.cache_key - assert_equal cached_attributes["#{writer_cache_key}/#{attributes.cached_name}"], Author.new(id: 'author', name: 'Joao M. D. Moura').attributes + assert_equal cached_attributes["#{writer_cache_key}/#{attributes.cache_key}"], Author.new(id: 'author', name: 'Joao M. D. Moura').attributes end end From caf4910b6e4e3af1bd5cac1116bc9aedd8cd2b1f Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sat, 4 Jun 2016 14:47:22 -0500 Subject: [PATCH 5/6] Remove controller :assigns warnings/shim --- test/action_controller/adapter_selector_test.rb | 2 +- test/action_controller/explicit_serializer_test.rb | 4 ++-- test/action_controller/serialization_test.rb | 6 +++--- test/support/rails_app.rb | 9 --------- 4 files changed, 6 insertions(+), 15 deletions(-) diff --git a/test/action_controller/adapter_selector_test.rb b/test/action_controller/adapter_selector_test.rb index aa3539f7..f392b4a4 100644 --- a/test/action_controller/adapter_selector_test.rb +++ b/test/action_controller/adapter_selector_test.rb @@ -32,7 +32,7 @@ module ActionController expected = { data: { - id: assigns(:profile).id.to_s, + id: @controller.instance_variable_get(:@profile).id.to_s, type: 'profiles', attributes: { name: 'Name 1', diff --git a/test/action_controller/explicit_serializer_test.rb b/test/action_controller/explicit_serializer_test.rb index 00b4db81..8886d07d 100644 --- a/test/action_controller/explicit_serializer_test.rb +++ b/test/action_controller/explicit_serializer_test.rb @@ -103,9 +103,9 @@ module ActionController { 'title' => 'New Post', 'body' => 'Body', - 'id' => assigns(:post).id, + 'id' => @controller.instance_variable_get(:@post).id, 'comments' => [{ 'id' => 1 }, { 'id' => 2 }], - 'author' => { 'id' => assigns(:author).id } + 'author' => { 'id' => @controller.instance_variable_get(:@author).id } } ] diff --git a/test/action_controller/serialization_test.rb b/test/action_controller/serialization_test.rb index 196d8493..8ffdc100 100644 --- a/test/action_controller/serialization_test.rb +++ b/test/action_controller/serialization_test.rb @@ -163,7 +163,7 @@ module ActionController end expected = { data: { - id: assigns(:profile).id.to_s, + id: @controller.instance_variable_get(:@profile).id.to_s, type: 'profiles', attributes: { name: 'Name 1', @@ -246,7 +246,7 @@ module ActionController expected = { data: [ { - id: assigns(:profiles).first.id.to_s, + id: @controller.instance_variable_get(:@profiles).first.id.to_s, type: 'profiles', attributes: { name: 'Name 1', @@ -269,7 +269,7 @@ module ActionController expected = { data: [ { - id: assigns(:profiles).first.id.to_s, + id: @controller.instance_variable_get(:@profiles).first.id.to_s, type: 'profiles', attributes: { name: 'Name 1', diff --git a/test/support/rails_app.rb b/test/support/rails_app.rb index 16e776c7..0bbae1fe 100644 --- a/test/support/rails_app.rb +++ b/test/support/rails_app.rb @@ -22,15 +22,6 @@ ActionController::TestCase.class_eval do def setup @routes = Routes end - - # For Rails5 - # https://github.com/rails/rails/commit/ca83436d1b3b6cedd1eca2259f65661e69b01909#diff-b9bbf56e85d3fe1999f16317f2751e76L17 - def assigns(key = nil) - warn "DEPRECATION: Calling 'assigns(#{key})' from #{caller[0]}" - assigns = {}.with_indifferent_access - @controller.view_assigns.each { |k, v| assigns.regular_writer(k, v) } - key.nil? ? assigns : assigns[key] - end end # ActiveRecord::Migrator.migrations_paths = [File.expand_path("../../test/dummy/db/migrate", __FILE__)] From 7254d34c9082c8a1d6b22224d87d4e81ba291467 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 5 Jun 2016 23:01:21 -0500 Subject: [PATCH 6/6] Move Serializer#serialize into Serializer#serializable_hash --- lib/active_model/serializer.rb | 27 ++++------ .../serializer/collection_serializer.rb | 50 +++++++++++-------- .../adapter/attributes.rb | 2 +- 3 files changed, 41 insertions(+), 38 deletions(-) diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index b2884b9c..570073c7 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -109,6 +109,7 @@ module ActiveModel end end + # @api private def self.serialization_adapter_instance @serialization_adapter_instance ||= ActiveModelSerializers::Adapter::Attributes end @@ -138,9 +139,7 @@ module ActiveModel # associations, similar to how ActiveModel::Serializers::JSON is used # in ActiveRecord::Base. # - # TODO: Move to here the Attributes adapter logic for - # +serializable_hash_for_single_resource(options)+ - # and include ActiveModel::Serializers::JSON. + # TODO: Include ActiveModel::Serializers::JSON. # So that the below is true: # @param options [nil, Hash] The same valid options passed to `serializable_hash` # (:only, :except, :methods, and :include). @@ -164,10 +163,13 @@ module ActiveModel # serializer.as_json(include: :posts) # # Second level and higher order associations work as well: # serializer.as_json(include: { posts: { include: { comments: { only: :body } }, only: :title } }) - def serializable_hash(adapter_opts = nil) - adapter_opts ||= {} - adapter_opts = { include: '*' }.merge!(adapter_opts) - serialize(adapter_opts) + def serializable_hash(adapter_options = nil, options = {}, adapter_instance = self.class.serialization_adapter_instance) + 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) + relationships = resource_relationships(adapter_options, options, adapter_instance) + resource.merge(relationships) end alias to_hash serializable_hash alias to_h serializable_hash @@ -199,15 +201,6 @@ module ActiveModel end end - # @api private - def serialize(adapter_options, options = {}, adapter_instance = self.class.serialization_adapter_instance) - 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) - relationships = resource_relationships(adapter_options, options, adapter_instance) - resource.merge(relationships) - end - # @api private def resource_relationships(adapter_options, options, adapter_instance) relationships = {} @@ -227,7 +220,7 @@ module ActiveModel association_object = association_serializer && association_serializer.object return unless association_object - relationship_value = association_serializer.serialize(adapter_options, {}, adapter_instance) + relationship_value = association_serializer.serializable_hash(adapter_options, {}, adapter_instance) if association.options[:polymorphic] && relationship_value polymorphic_type = association_object.class.name.underscore diff --git a/lib/active_model/serializer/collection_serializer.rb b/lib/active_model/serializer/collection_serializer.rb index a2367195..bb84644c 100644 --- a/lib/active_model/serializer/collection_serializer.rb +++ b/lib/active_model/serializer/collection_serializer.rb @@ -11,22 +11,23 @@ module ActiveModel @object = resources @options = options @root = options[:root] - serializer_context_class = options.fetch(:serializer_context_class, ActiveModel::Serializer) - @serializers = resources.map do |resource| - serializer_class = options.fetch(:serializer) { serializer_context_class.serializer_for(resource) } - - if serializer_class.nil? # rubocop:disable Style/GuardClause - fail NoSerializerError, "No serializer found for resource: #{resource.inspect}" - else - serializer_class.new(resource, options.except(:serializer)) - end - end + @serializers = serializers_from_resources end def success? true end + # @api private + def serializable_hash(adapter_options, options, adapter_instance) + include_directive = ActiveModel::Serializer.include_directive_from_options(adapter_options) + adapter_options[:cached_attributes] ||= ActiveModel::Serializer.cache_read_multi(self, adapter_instance, include_directive) + adapter_opts = adapter_options.merge(include_directive: include_directive) + serializers.map do |serializer| + serializer.serializable_hash(adapter_opts, options, adapter_instance) + end + end + # TODO: unify naming of root, json_key, and _type. Right now, a serializer's # json_key comes from the root option or the object's model name, by default. # But, if a dev defines a custom `json_key` method with an explicit value, @@ -56,19 +57,28 @@ module ActiveModel object.respond_to?(:size) end - # @api private - def serialize(adapter_options, options, adapter_instance) - include_directive = ActiveModel::Serializer.include_directive_from_options(adapter_options) - adapter_options[:cached_attributes] ||= ActiveModel::Serializer.cache_read_multi(self, adapter_instance, include_directive) - adapter_opts = adapter_options.merge(include_directive: include_directive) - serializers.map do |serializer| - serializer.serialize(adapter_opts, options, adapter_instance) - end - end - protected attr_reader :serializers, :options + + private + + def serializers_from_resources + serializer_context_class = options.fetch(:serializer_context_class, ActiveModel::Serializer) + object.map do |resource| + serializer_from_resource(resource, serializer_context_class, options) + end + end + + def serializer_from_resource(resource, serializer_context_class, options) + serializer_class = options.fetch(:serializer) { serializer_context_class.serializer_for(resource) } + + if serializer_class.nil? # rubocop:disable Style/GuardClause + fail NoSerializerError, "No serializer found for resource: #{resource.inspect}" + else + serializer_class.new(resource, options.except(:serializer)) + end + end end end end diff --git a/lib/active_model_serializers/adapter/attributes.rb b/lib/active_model_serializers/adapter/attributes.rb index 3d2e7b52..f28295c0 100644 --- a/lib/active_model_serializers/adapter/attributes.rb +++ b/lib/active_model_serializers/adapter/attributes.rb @@ -3,7 +3,7 @@ module ActiveModelSerializers class Attributes < Base def serializable_hash(options = nil) options = serialization_options(options) - serializer.serialize(instance_options, options, self) + serializer.serializable_hash(instance_options, options, self) end end end