[FIX] The cache store needs to be the actually store, not e.g. :memory_store

[bf4]

Bug found: The cache store needs to be the actually store, not e.g. :memory_store

Status quo in test app:

In Rails

    ActionController::Base.cache_store = :memory_store

and then AMS Railtie will look like:

  config = Rails.configuration
  ActiveModelSerializers.config.cache_store = config.action_controller.cache_store

then, in the AMS Railtie:

1. ActiveSupport.on_load(:action_controller) fires
   • ActiveModelSerializers.config.cache_store #=> nil
   • ActionController::Base.cache_store #=> #<ActiveSupport::Cache::FileStore:0x007fe319256760...]
2. After set_configs fires
   • ActiveModelSerializers.config.cache_store #+> #<ActiveSupport::Cache::FileStore:0x007fe319256760 ,
3. Tests pass, but notice that we're using the FileStore, not memory store

When we change the config to the test app:

  ActionController::Base.cache_store = :memory_store
  config = Rails.configuration
  config.action_controller.cache_store = :memory_store

then, in the AMS Railtie:

1. ActiveSupport.on_load(:action_controller) fires
   • ActiveModelSerializers.config.cache_store #=> nil
   • ActionController::Base.cache_store #=> #ActiveSupport::Cache::MemoryStore entries=0, size=0, options={}>]
2. After set_configs fires
   • ActiveModelSerializers.config.cache_store #=> :memory_store
3. And we get a lot of failures:
   • NoMethodError: undefined methodfetch' for :memory_store:Symbol`

So, we see that when we set the ActionController::Base.cache_store
directly in our test app, we could set
ActiveModelSerializers.config.cache_store in the :action_controller load
hook, but that would never use the Rails config.

To fix the Rails config, we change the config to the test app:

  config = Rails.configuration
  config.action_controller.cache_store = :memory_store

and then AMS Railtie will look like:

  ActiveModelSerializers.config.cache_store = ActiveSupport::Cache.lookup_store(config.action_controller.cache_store
  ActiveSupport.on_load(:action_controller) do
    ::ActiveModelSerializers.config.cache_store = cache_store
  end

then, in the AMS Railtie:

1. After set_configs fires
   • ActiveModelSerializers.config.cache_store #=> <#ActiveSupport::Cache::MemoryStore, object_id 70207113611740
2. ActiveSupport.on_load(:action_controller) fires
   • ActionController::Base.cache_store #=> <#ActiveSupport::Cache::MemoryStore, object_id 70207106279660
   • ActiveModelSerializers.config.cache_store #=> <#ActiveSupport::Cache::MemoryStore, object_id 70207106279660
     (notice the object_id changed)
3. And we get a failure:

  1) Failure:
  ActiveModelSerializers::CacheTest#test_associations_cache_when_updated
  [active_model_serializers/test/cache_test.rb:141]:
  --- expected
  +++ actual
  @@ -1 +1 @@
  -{:id=>"post", :title=>"New Post", :body=>"Body"}
  +{:id=>"post", :title=>"New Post", :body=>"Body", :comments=>[{:id=>2, :body=>"ZOMG A NEW COMMENT"}], :blog=>{:id=>999, :name=>"Custom blog"}, :author=>{:id=>"author", :name=>"Joao M. D. Moura"}}

If we take out the on_load(:action_controller) hook, we get a ton of
failures. So clearly, our code expects the controller cache to be the
same as the serializer cache.

So, we make sure we use an on_load(:action_controller) hook that runs
after set_configs

And look at the test and see it is filled with direct calls to ActionController::Base.cache_store

    assert_equal(new_comment_serializer.attributes, ActionController::Base.cache_store.fetch(new_comment.cache_key))
    assert_equal(@post_serializer.attributes, ActionController::Base.cache_store.fetch(@post.cache_key))

But that's not a problem in this case, since they're the same object. That just explains all the failures before when they were different objects.

For now, let's remove the :memory_store setting and use the default FileStore and that one failure goes away.

The cache log is

FileStore

MemoryStore

Cache read: post/post-20160329215156000000000 ({:expires_in=>0.1, :skip_digest=>true}) Cache generate: post/post-20160329215156000000000 ({:expires_in=>0.1, :skip_digest=>true}) Cache write: post/post-20160329215156000000000 ({:expires_in=>0.1, :skip_digest=>true}) Cache read: comment/1 ({:expires_in=>1 day, :skip_digest=>true}) Cache generate: comment/1 ({:expires_in=>1 day, :skip_digest=>true}) Cache write: comment/1 ({:expires_in=>1 day, :skip_digest=>true}) Cache read: blog/999-20160329215156000000000/272717207db26fd52c64393a49e6fc11 Cache generate: blog/999-20160329215156000000000/272717207db26fd52c64393a49e6fc11 Cache write: blog/999-20160329215156000000000/272717207db26fd52c64393a49e6fc11 Cache read: writer/author-20160329215156000000000 ({:skip_digest=>true}) Cache generate: writer/author-20160329215156000000000 ({:skip_digest=>true}) Cache write: writer/author-20160329215156000000000 ({:skip_digest=>true}) Cache read: post/post-20160329215156000000000 Cache read: comment/1 Cache read: post/post-20160329215156000000000 ({:expires_in=>0.1, :skip_digest=>true}) Cache fetch_hit: post/post-20160329215156000000000 ({:expires_in=>0.1, :skip_digest=>true}) Cache read: comment/2 ({:expires_in=>1 day, :skip_digest=>true}) Cache generate: comment/2 ({:expires_in=>1 day, :skip_digest=>true}) Cache write: comment/2 ({:expires_in=>1 day, :skip_digest=>true}) Cache read: blog/999-20160329215156000000000/272717207db26fd52c64393a49e6fc11 Cache fetch_hit: blog/999-20160329215156000000000/272717207db26fd52c64393a49e6fc11 Cache read: writer/author-20160329215156000000000 ({:skip_digest=>true}) Cache fetch_hit: writer/author-20160329215156000000000 ({:skip_digest=>true}) Cache read: comment/2 Cache read: post/post-20160329215156000000000

Cache read: post/post-20160329215156000000000 ({:expires_in=>0.1, :skip_digest=>true}) Cache generate: post/post-20160329215156000000000 ({:expires_in=>0.1, :skip_digest=>true}) Cache write: post/post-20160329215156000000000 ({:expires_in=>0.1, :skip_digest=>true}) Cache read: comment/1 ({:expires_in=>1 day, :skip_digest=>true}) Cache generate: comment/1 ({:expires_in=>1 day, :skip_digest=>true}) Cache write: comment/1 ({:expires_in=>1 day, :skip_digest=>true}) Cache read: blog/999-20160329215156000000000/272717207db26fd52c64393a49e6fc11 Cache generate: blog/999-20160329215156000000000/272717207db26fd52c64393a49e6fc11 Cache write: blog/999-20160329215156000000000/272717207db26fd52c64393a49e6fc11 Cache read: writer/author-20160329215156000000000 ({:skip_digest=>true}) Cache generate: writer/author-20160329215156000000000 ({:skip_digest=>true}) Cache write: writer/author-20160329215156000000000 ({:skip_digest=>true}) Cache read: post/post-20160329215156000000000 Cache read: comment/1 Cache read: post/post-20160329215156000000000 ({:expires_in=>0.1, :skip_digest=>true}) Cache fetch_hit: post/post-20160329215156000000000 ({:expires_in=>0.1, :skip_digest=>true}) Cache read: comment/2 ({:expires_in=>1 day, :skip_digest=>true}) Cache generate: comment/2 ({:expires_in=>1 day, :skip_digest=>true}) Cache write: comment/2 ({:expires_in=>1 day, :skip_digest=>true}) Cache read: blog/999-20160329215156000000000/272717207db26fd52c64393a49e6fc11 Cache fetch_hit: blog/999-20160329215156000000000/272717207db26fd52c64393a49e6fc11 Cache read: writer/author-20160329215156000000000 ({:skip_digest=>true}) Cache fetch_hit: writer/author-20160329215156000000000 ({:skip_digest=>true}) Cache read: comment/2 Cache read: post/post-20160329215156000000000

[bf4]

Discovered in https://github.com/rails-api/active_model_serializers/pull/1412/files#diff-8c03da6a006ecb5ab61b873ef8f5cba3R76 when I got undefined method fetch on :memory_store

[groyoh]

Looks like the right approach to me.
Could probably also be:

ActiveModelSerializers.config.cache_store = Rails.cache

I assume.

What do you think?.

[bf4]

@groyoh could be, but I'd need to check the load order of the config and whether there's any chance at init Rails.cache isn't ActionController::Base.cache

[bf4]

cc @dgynn

[dgynn]

Using ActiveSupport::Cache.lookup_store seems good. You could move that from the initializer into ActiveModel::Serializer::Configuration as a cache_store= method.

You could also move the code into the ActiveSupport.on_load(:action_controller) block and then reference ActionController::Base.cache directly.

If the main goal is to stay in sync with the way that ActionController is configured, it really would seem best to always delegate to ActionController::Base.cache rather than having a separate config and reference.

[NullVoxPopuli]

What are the advantages over the different options?

• Rails.cache
• Rails.configuration.action_controller.cache_store (this is what it used to be -- this defaults to memory_store?)
• ActiveSupport::Cache.lookup_store(Rails.configuration.action_controller.cache_store)

In my personal projects, I always use APICache https://github.com/NullVoxPopuli/api_cache
(had to fork it, cause upstream doesn't want the additional apis I added to redis.

I feel like this AMS config option should be documented (just at a minimum, what methods the store needs to implement)

[bf4]

@NullVoxPopuli @groyoh @dgynn I just updated this PR with a little code and lot in the PR description of what I've learned. It was actually a little more involved than I thought

[bf4]

this is the test that is failing w memory store

[groyoh]

Found the culprit https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model_serializers/adapter/attributes.rb#L58.

Here we modify in-place the value that is returned from the memory_store and merge in the relationships. Therefore the value within the cache is modified in place. My guess is that the file_store returns a copy/dup of the value whereas the memory_store simply returns the reference to the same object.

[bf4]

💋

[bf4]

@groyoh Amazing! #1480 (comment)