From 9ce36904cfade4b158c8e1a497d908b62737a1a9 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Wed, 16 Mar 2016 23:51:53 -0500 Subject: [PATCH 1/6] Allow devs to opt out of test warnings --- Rakefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Rakefile b/Rakefile index 1425d24a..58faa5d6 100644 --- a/Rakefile +++ b/Rakefile @@ -43,7 +43,8 @@ Rake::TestTask.new do |t| t.libs << 'test' t.libs << 'lib' t.test_files = FileList['test/**/*_test.rb'] - t.ruby_opts = ['-w -r./test/test_helper.rb'] + t.ruby_opts = ['-r./test/test_helper.rb'] + t.ruby_opts << ' -w' unless ENV['NO_WARN'] == 'true' t.verbose = true end From 27b514a63be8e1cf6c5dc61c2bd4042982d21df0 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Wed, 16 Mar 2016 23:52:31 -0500 Subject: [PATCH 2/6] Add missing object context needed for tests to be run alone --- test/action_controller/serialization_test.rb | 2 +- test/active_model_serializers/adapter_for_test.rb | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/test/action_controller/serialization_test.rb b/test/action_controller/serialization_test.rb index 3870c25a..aa7375f2 100644 --- a/test/action_controller/serialization_test.rb +++ b/test/action_controller/serialization_test.rb @@ -454,7 +454,7 @@ module ActionController end def test_render_event_is_emmited - ActiveSupport::Notifications.subscribe('render.active_model_serializers') do |name| + ::ActiveSupport::Notifications.subscribe('render.active_model_serializers') do |name| @name = name end diff --git a/test/active_model_serializers/adapter_for_test.rb b/test/active_model_serializers/adapter_for_test.rb index 2707fc8e..1439b987 100644 --- a/test/active_model_serializers/adapter_for_test.rb +++ b/test/active_model_serializers/adapter_for_test.rb @@ -1,5 +1,7 @@ +require 'test_helper' + module ActiveModelSerializers - class AdapterForTest < ActiveSupport::TestCase + class AdapterForTest < ::ActiveSupport::TestCase UnknownAdapterError = ::ActiveModelSerializers::Adapter::UnknownAdapterError def test_serializer_adapter_returns_configured_adapter From 9953d7abe09d0e7a8aa64c7b0c105ebe7f8b2827 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Thu, 28 Jan 2016 15:57:13 -0600 Subject: [PATCH 3/6] Trigger callback to set serializer#_cache when controller loaded --- lib/active_model/serializer/caching.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/active_model/serializer/caching.rb b/lib/active_model/serializer/caching.rb index 3ebb4bae..4d015acb 100644 --- a/lib/active_model/serializer/caching.rb +++ b/lib/active_model/serializer/caching.rb @@ -89,6 +89,10 @@ module ActiveModel # https://github.com/rails-api/active_model_serializers/pull/1249#issuecomment-146567837 def cache(options = {}) self._cache = ActiveModelSerializers.config.cache_store if ActiveModelSerializers.config.perform_caching + serializer = self + ActiveSupport.on_load(:action_controller) do + serializer._cache = ActiveModelSerializers.config.cache_store if ActiveModelSerializers.config.perform_caching + end self._cache_key = options.delete(:key) self._cache_only = options.delete(:only) self._cache_except = options.delete(:except) From c3c69a607aac0698b51db2e9cc9cba0c585d291a Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Wed, 16 Mar 2016 22:25:44 -0500 Subject: [PATCH 4/6] Separate enabling of caching and setting the cache store --- lib/active_model/serializer/caching.rb | 71 +++++++- .../cached_serializer.rb | 4 +- test/cache_test.rb | 13 ++ .../caching_configuration_test_isolated.rb | 168 ++++++++++++++++++ 4 files changed, 248 insertions(+), 8 deletions(-) create mode 100644 test/serializers/caching_configuration_test_isolated.rb diff --git a/lib/active_model/serializer/caching.rb b/lib/active_model/serializer/caching.rb index 4d015acb..8a51ec77 100644 --- a/lib/active_model/serializer/caching.rb +++ b/lib/active_model/serializer/caching.rb @@ -5,7 +5,7 @@ module ActiveModel included do with_options instance_writer: false, instance_reader: false do |serializer| - serializer.class_attribute :_cache # @api private : the cache object + serializer.class_attribute :_cache # @api private : the cache store serializer.class_attribute :_fragmented # @api private : @see ::fragmented serializer.class_attribute :_cache_key # @api private : when present, is first item in cache_key serializer.class_attribute :_cache_only # @api private : when fragment caching, whitelists cached_attributes. Cannot combine with except @@ -71,6 +71,7 @@ module ActiveModel # when Rails.configuration.action_controller.perform_caching # # @params options [Hash] with valid keys: + # cache_store : @see ::_cache # key : @see ::_cache_key # only : @see ::_cache_only # except : @see ::_cache_except @@ -88,16 +89,74 @@ module ActiveModel # @todo require less code comments. See # https://github.com/rails-api/active_model_serializers/pull/1249#issuecomment-146567837 def cache(options = {}) - self._cache = ActiveModelSerializers.config.cache_store if ActiveModelSerializers.config.perform_caching - serializer = self - ActiveSupport.on_load(:action_controller) do - serializer._cache = ActiveModelSerializers.config.cache_store if ActiveModelSerializers.config.perform_caching - end + self._cache = + options.delete(:cache_store) || + ActiveModelSerializers.config.cache_store || + ActiveSupport::Cache.lookup_store(:null_store) 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 + + # @return [true, false] + # We're using class variables here because it is a class attribute + # that is globally true for the `ActiveModel::Serializer` class; i.e. neither per subclass nor inherited. + # + # We're not using a class_attribute because of the special behavior in + # `perform_caching` setting itself to `ActiveModelSerializers.config.perform_caching` + # when first called if it wasn't first set. + # + # This is to allow us to have a global config that can be set any time before + # `perform_caching` is called. + # + # One downside of this, is that subsequent setting of the global config will not change + # `ActiveModel::Serializer.perform_caching`, but that should be an edge case that + # is easily handled. + # + # If you, reading this, can figure out how to have ActiveModel::Serializer always delegate + # `perform_caching` and `perform_caching=` to the global config, that would make a nice PR. + # rubocop:disable Style/ClassVars + def perform_caching + return @@perform_caching if defined?(@@perform_caching) + self.perform_caching = ActiveModelSerializers.config.perform_caching + end + alias perform_caching? perform_caching + + # @param [true, false] + def perform_caching=(perform_caching) + @@perform_caching = perform_caching + end + # rubocop:enable Style/ClassVars + + # The canonical method for getting the cache store for the serializer. + # + # @return [nil] when _cache is not set (i.e. when `cache` has not been called) + # @return [._cache] when _cache is not the NullStore + # @return [ActiveModelSerializers.config.cache_store] when _cache is the NullStore. + # This is so we can use `cache` being called to mean the serializer should be cached + # even if ActiveModelSerializers.config.cache_store has not yet been set. + # That means that when _cache is the NullStore and ActiveModelSerializers.config.cache_store + # is configured, `cache_store` becomes `ActiveModelSerializers.config.cache_store`. + # @return [nil] when _cache is the NullStore and ActiveModelSerializers.config.cache_store is nil. + def cache_store + return nil if _cache.nil? + return _cache if _cache.class != ActiveSupport::Cache::NullStore + if ActiveModelSerializers.config.cache_store + self._cache = ActiveModelSerializers.config.cache_store + else + nil + end + end + + def cache_enabled? + perform_caching? && cache_store && !_cache_only && !_cache_except + end + + def fragment_cache_enabled? + perform_caching? && cache_store && + (_cache_only && !_cache_except || !_cache_only && _cache_except) + end end end end diff --git a/lib/active_model_serializers/cached_serializer.rb b/lib/active_model_serializers/cached_serializer.rb index 8bc85600..11a6259d 100644 --- a/lib/active_model_serializers/cached_serializer.rb +++ b/lib/active_model_serializers/cached_serializer.rb @@ -18,11 +18,11 @@ module ActiveModelSerializers end def cached? - @klass._cache && !@klass._cache_only && !@klass._cache_except + @klass.cache_enabled? end def fragment_cached? - @klass._cache && (@klass._cache_only && !@klass._cache_except || !@klass._cache_only && @klass._cache_except) + @klass.fragment_cache_enabled? end def cache_key diff --git a/test/cache_test.rb b/test/cache_test.rb index 72b486e3..46d3a973 100644 --- a/test/cache_test.rb +++ b/test/cache_test.rb @@ -34,6 +34,19 @@ module ActiveModelSerializers @blog_serializer = BlogSerializer.new(@blog) end + def test_explicit_cache_store + default_store = Class.new(ActiveModel::Serializer) do + cache + end + explicit_store = Class.new(ActiveModel::Serializer) do + cache cache_store: ActiveSupport::Cache::FileStore + end + + assert ActiveSupport::Cache::MemoryStore, ActiveModelSerializers.config.cache_store + assert ActiveSupport::Cache::MemoryStore, default_store.cache_store + assert ActiveSupport::Cache::FileStore, explicit_store.cache_store + end + def test_inherited_cache_configuration inherited_serializer = Class.new(PostSerializer) diff --git a/test/serializers/caching_configuration_test_isolated.rb b/test/serializers/caching_configuration_test_isolated.rb new file mode 100644 index 00000000..1b6d5541 --- /dev/null +++ b/test/serializers/caching_configuration_test_isolated.rb @@ -0,0 +1,168 @@ +# Execute this test in isolation +require 'support/isolated_unit' + +class CachingConfigurationTest < ActiveSupport::TestCase + include ActiveSupport::Testing::Isolation + + setup do + require 'rails' + # AMS needs to be required before Rails.application is initialized for + # Railtie's to fire in Rails.application.initialize! + # (and make_basic_app initializes the app) + require 'active_model_serializers' + # Create serializers before Rails.application.initialize! + # To ensure we're testing that the cache settings depend on + # the Railtie firing, not on the ActionController being loaded. + create_serializers + end + + def create_serializers + @cached_serializer = Class.new(ActiveModel::Serializer) do + cache skip_digest: true + attributes :id, :name, :title + end + @fragment_cached_serializer = Class.new(ActiveModel::Serializer) do + cache only: :id + attributes :id, :name, :title + end + @non_cached_serializer = Class.new(ActiveModel::Serializer) do + attributes :id, :name, :title + end + end + + class PerformCachingTrue < CachingConfigurationTest + setup do + # Let's make that Rails app and initialize it! + make_basic_app do |app| + app.config.action_controller.perform_caching = true + app.config.action_controller.cache_store = ActiveSupport::Cache.lookup_store(:memory_store) + end + end + + test 'it sets perform_caching to true on AMS.config and serializers' do + assert Rails.configuration.action_controller.perform_caching + assert ActiveModelSerializers.config.perform_caching + assert ActiveModel::Serializer.perform_caching? + assert @cached_serializer.perform_caching? + assert @non_cached_serializer.perform_caching? + assert @fragment_cached_serializer.perform_caching? + end + + test 'it sets the AMS.config.cache_store to the controller cache_store' do + assert_equal controller_cache_store, ActiveSupport::Cache::MemoryStore + assert_equal controller_cache_store, ActiveModelSerializers.config.cache_store.class + end + + test 'it sets the cached serializer cache_store to the ActionController::Base.cache_store' do + assert_equal ActiveSupport::Cache::NullStore, @cached_serializer._cache.class + assert_equal controller_cache_store, @cached_serializer.cache_store.class + assert_equal ActiveSupport::Cache::MemoryStore, @cached_serializer._cache.class + end + + test 'the cached serializer has cache_enabled?' do + assert @cached_serializer.cache_enabled? + end + + test 'the cached serializer does not have fragment_cache_enabled?' do + refute @cached_serializer.fragment_cache_enabled? + end + + test 'the non-cached serializer cache_store is nil' do + assert_equal nil, @non_cached_serializer._cache + assert_equal nil, @non_cached_serializer.cache_store + assert_equal nil, @non_cached_serializer._cache + end + + test 'the non-cached serializer does not have cache_enabled?' do + refute @non_cached_serializer.cache_enabled? + end + + test 'the non-cached serializer does not have fragment_cache_enabled?' do + refute @non_cached_serializer.fragment_cache_enabled? + end + + test 'it sets the fragment cached serializer cache_store to the ActionController::Base.cache_store' do + assert_equal ActiveSupport::Cache::NullStore, @fragment_cached_serializer._cache.class + assert_equal controller_cache_store, @fragment_cached_serializer.cache_store.class + assert_equal ActiveSupport::Cache::MemoryStore, @fragment_cached_serializer._cache.class + end + + test 'the fragment cached serializer does not have cache_enabled?' do + refute @fragment_cached_serializer.cache_enabled? + end + + test 'the fragment cached serializer has fragment_cache_enabled?' do + assert @fragment_cached_serializer.fragment_cache_enabled? + end + end + + class PerformCachingFalse < CachingConfigurationTest + setup do + # Let's make that Rails app and initialize it! + make_basic_app do |app| + app.config.action_controller.perform_caching = false + app.config.action_controller.cache_store = ActiveSupport::Cache.lookup_store(:memory_store) + end + end + + test 'it sets perform_caching to false on AMS.config and serializers' do + refute Rails.configuration.action_controller.perform_caching + refute ActiveModelSerializers.config.perform_caching + refute ActiveModel::Serializer.perform_caching? + refute @cached_serializer.perform_caching? + refute @non_cached_serializer.perform_caching? + refute @fragment_cached_serializer.perform_caching? + end + + test 'it sets the AMS.config.cache_store to the controller cache_store' do + assert_equal controller_cache_store, ActiveSupport::Cache::MemoryStore + assert_equal controller_cache_store, ActiveModelSerializers.config.cache_store.class + end + + test 'it sets the cached serializer cache_store to the ActionController::Base.cache_store' do + assert_equal ActiveSupport::Cache::NullStore, @cached_serializer._cache.class + assert_equal controller_cache_store, @cached_serializer.cache_store.class + assert_equal ActiveSupport::Cache::MemoryStore, @cached_serializer._cache.class + end + + test 'the cached serializer does not have cache_enabled?' do + refute @cached_serializer.cache_enabled? + end + + test 'the cached serializer does not have fragment_cache_enabled?' do + refute @cached_serializer.fragment_cache_enabled? + end + + test 'the non-cached serializer cache_store is nil' do + assert_equal nil, @non_cached_serializer._cache + assert_equal nil, @non_cached_serializer.cache_store + assert_equal nil, @non_cached_serializer._cache + end + + test 'the non-cached serializer does not have cache_enabled?' do + refute @non_cached_serializer.cache_enabled? + end + + test 'the non-cached serializer does not have fragment_cache_enabled?' do + refute @non_cached_serializer.fragment_cache_enabled? + end + + test 'it sets the fragment cached serializer cache_store to the ActionController::Base.cache_store' do + assert_equal ActiveSupport::Cache::NullStore, @fragment_cached_serializer._cache.class + assert_equal controller_cache_store, @fragment_cached_serializer.cache_store.class + assert_equal ActiveSupport::Cache::MemoryStore, @fragment_cached_serializer._cache.class + end + + test 'the fragment cached serializer does not have cache_enabled?' do + refute @fragment_cached_serializer.cache_enabled? + end + + test 'the fragment cached serializer does not have fragment_cache_enabled?' do + refute @fragment_cached_serializer.fragment_cache_enabled? + end + end + + def controller_cache_store + ActionController::Base.cache_store.class + end +end From 1230dd95ba9b76792ea687dc97ea96045744eb6c Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Thu, 24 Mar 2016 21:34:10 -0500 Subject: [PATCH 5/6] Add CHANGELOG [ci skip] --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a37fbc75..06534b3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ Features: - [#1340](https://github.com/rails-api/active_model_serializers/pull/1340) Add support for resource-level meta. (@beauby) Fixes: +- [#1478](https://github.com/rails-api/active_model_serializers/pull/1478) Cache store will now be correctly set when serializers are + loaded *before* Rails initializes. (@bf4) - [#1570](https://github.com/rails-api/active_model_serializers/pull/1570) Fixed pagination issue with last page size. (@bmorrall) - [#1516](https://github.com/rails-api/active_model_serializers/pull/1516) No longer return a nil href when only adding meta to a relationship link. (@groyoh) From dd60a371ae3a9ef204f79e326c475acfb0f0bd03 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Thu, 24 Mar 2016 21:15:23 -0500 Subject: [PATCH 6/6] Simplify caching of value of config.perform_caching --- lib/active_model/serializer/caching.rb | 30 ++++++-------------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/lib/active_model/serializer/caching.rb b/lib/active_model/serializer/caching.rb index 8a51ec77..85a395d3 100644 --- a/lib/active_model/serializer/caching.rb +++ b/lib/active_model/serializer/caching.rb @@ -99,34 +99,18 @@ module ActiveModel self._cache_options = options.empty? ? nil : options end + # Value is from ActiveModelSerializers.config.perform_caching. Is used to + # globally enable or disable all serializer caching, just like + # Rails.configuration.action_controller.perform_caching, which is its + # default value in a Rails application. # @return [true, false] - # We're using class variables here because it is a class attribute - # that is globally true for the `ActiveModel::Serializer` class; i.e. neither per subclass nor inherited. - # - # We're not using a class_attribute because of the special behavior in - # `perform_caching` setting itself to `ActiveModelSerializers.config.perform_caching` - # when first called if it wasn't first set. - # - # This is to allow us to have a global config that can be set any time before - # `perform_caching` is called. - # - # One downside of this, is that subsequent setting of the global config will not change - # `ActiveModel::Serializer.perform_caching`, but that should be an edge case that - # is easily handled. - # - # If you, reading this, can figure out how to have ActiveModel::Serializer always delegate - # `perform_caching` and `perform_caching=` to the global config, that would make a nice PR. + # Memoizes value of config first time it is called with a non-nil value. # rubocop:disable Style/ClassVars def perform_caching - return @@perform_caching if defined?(@@perform_caching) - self.perform_caching = ActiveModelSerializers.config.perform_caching + return @@perform_caching if defined?(@@perform_caching) && !@@perform_caching.nil? + @@perform_caching = ActiveModelSerializers.config.perform_caching end alias perform_caching? perform_caching - - # @param [true, false] - def perform_caching=(perform_caching) - @@perform_caching = perform_caching - end # rubocop:enable Style/ClassVars # The canonical method for getting the cache store for the serializer.