Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include adapter in cache key #1644

Merged

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Mar 31, 2016

Continues work in #1346, #1642

Add the Adapter type to the cache key.

This prevents incorrect results when the same object is serialized with different adapters.

Fixes #1344

Cherry-pick of
bf4@040a97b
which was a squash of
https://github.com/rails-api/active_model_serializers/commits/f89ed71058322fe7dd35d5c8b209856f8e03ad14

@bf4
Copy link
Member Author

bf4 commented Mar 31, 2016

Questions:

  • Since serializer caching caches attributes, how might an adapter cache different results? cc @kevintyll
  • Should we make including the adapter instance optional?
  • Do we need more tests?

@kevintyll
Copy link
Contributor

I think this looks good.

As to you're question about how they are cached differently, I had the same question when we started getting errors in production. Looking at the code, it looked like it parsed the cached attributes. We a monkey patch and test to confirm the issue. Here is the cached values from our test.

For the FlattenJson adapter (we are on a monkey patched version of rc3):

{:id=>1, :created_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :status=>"fail", :resource=>"resource-1", :updated_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :started_at=>Thu, 31 Mar 2016 21:36:35 UTC +00:00, :ended_at=>nil}

For JsonApi:

{:id=>"1", :type=>"alerts", :attributes=>{:created_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :status=>"fail", :resource=>"resource-1", :updated_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :started_at=>Thu, 31 Mar 2016 21:36:35 UTC +00:00, :ended_at=>nil}}

Here is our test for it:

context "#object_cache_key" do
  should 'cache different serializer types separately' do
    alert = create(:alert)
    result1 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::FlattenJson.new(::V2::AlertSerializer.new(alert)).to_json)

    assert_equal alert.status, result1['status']

    result2 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::FlattenJson.new(::V2::AlertSerializer.new(alert)).to_json)

    assert_equal result1, result2

    result1 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::JsonApi.new(::V2::AlertSerializer.new(alert)).to_json)

    assert_equal alert.status, result1['data']['attributes']['status']

    result2 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::JsonApi.new(::V2::AlertSerializer.new(alert)).to_json)

    assert_equal result1, result2
  end
end

@bf4
Copy link
Member Author

bf4 commented Mar 31, 2016

Thanks so much for that! I'll be sure to add it
On Thu, Mar 31, 2016 at 4:54 PM Kevin Tyll notifications@github.com wrote:

I think this looks good.

As to you're question about how they are cached differently, I had the
same question when we started getting errors in production. Looking at the
code, it looked like it parsed the cached attributes. We a monkey patch and
test to confirm the issue. Here is the cached values from our test.

For the FlattenJson adapter (we are on a monkey patched version of rc3):

{:id=>1, :created_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :status=>"fail", :resource=>"resource-1", :updated_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :started_at=>Thu, 31 Mar 2016 21:36:35 UTC +00:00, :ended_at=>nil}

For JsonApi:

{:id=>"1", :type=>"alerts", :attributes=>{:created_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :status=>"fail", :resource=>"resource-1", :updated_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :started_at=>Thu, 31 Mar 2016 21:36:35 UTC +00:00, :ended_at=>nil}}

Here is our test for it:

context "#object_cache_key" do
should 'cache different serializer types separately' do
alert = create(:alert)
result1 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::FlattenJson.new(::V2::AlertSerializer.new(alert)).to_json)

assert_equal alert.status, result1['status']

result2 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::FlattenJson.new(::V2::AlertSerializer.new(alert)).to_json)

assert_equal result1, result2

result1 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::JsonApi.new(::V2::AlertSerializer.new(alert)).to_json)

assert_equal alert.status, result1['data']['attributes']['status']

result2 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::JsonApi.new(::V2::AlertSerializer.new(alert)).to_json)

assert_equal result1, result2

end
end


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1644 (comment)

@bf4
Copy link
Member Author

bf4 commented Mar 31, 2016

Btw, do you have any benchmarka showing caching improves performance? I
haven't made any yet.
On Thu, Mar 31, 2016 at 5:48 PM Benjamin Fleischer bfleischer@gmail.com
wrote:

Thanks so much for that! I'll be sure to add it
On Thu, Mar 31, 2016 at 4:54 PM Kevin Tyll notifications@github.com
wrote:

I think this looks good.

As to you're question about how they are cached differently, I had the
same question when we started getting errors in production. Looking at the
code, it looked like it parsed the cached attributes. We a monkey patch and
test to confirm the issue. Here is the cached values from our test.

For the FlattenJson adapter (we are on a monkey patched version of rc3):

{:id=>1, :created_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :status=>"fail", :resource=>"resource-1", :updated_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :started_at=>Thu, 31 Mar 2016 21:36:35 UTC +00:00, :ended_at=>nil}

For JsonApi:

{:id=>"1", :type=>"alerts", :attributes=>{:created_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :status=>"fail", :resource=>"resource-1", :updated_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :started_at=>Thu, 31 Mar 2016 21:36:35 UTC +00:00, :ended_at=>nil}}

Here is our test for it:

context "#object_cache_key" do
should 'cache different serializer types separately' do
alert = create(:alert)
result1 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::FlattenJson.new(::V2::AlertSerializer.new(alert)).to_json)

assert_equal alert.status, result1['status']

result2 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::FlattenJson.new(::V2::AlertSerializer.new(alert)).to_json)

assert_equal result1, result2

result1 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::JsonApi.new(::V2::AlertSerializer.new(alert)).to_json)

assert_equal alert.status, result1['data']['attributes']['status']

result2 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::JsonApi.new(::V2::AlertSerializer.new(alert)).to_json)

assert_equal result1, result2

end
end


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1644 (comment)

@bf4
Copy link
Member Author

bf4 commented Apr 1, 2016

@kevintyll

(we are on a monkey patched version of rc3):

any way I can help you get off of that?

@bf4
Copy link
Member Author

bf4 commented Apr 1, 2016

@kevintyll What were your V2::AlertSerializer cache options?

@bf4
Copy link
Member Author

bf4 commented Apr 1, 2016

@kevintyll Here's how I wrote a test that fails on master. bf4@aee3118

It's not polished, but it gets the job done

    def test_a_serializer_rendered_by_two_adapter_returns_differently_cached_attributes
      model = Class.new(ActiveModelSerializers::Model) do
        attr_accessor :id, :status, :resource, :started_at, :ended_at, :updated_at, :created_at
      end
      Object.const_set(:Alert, model)
      serializer = Class.new(ActiveModel::Serializer) do
        cache
        attributes :id, :status, :resource, :started_at, :ended_at, :updated_at, :created_at
      end
      Object.const_set(:AlertSerializer, serializer)

      alert = Alert.new(
        id: 1,
        status: "fail",
        resource: "resource-1",
        started_at: Time.new(2016, 3, 31, 21, 36, 35, 0),
        ended_at: nil,
        updated_at: Time.new(2016, 3, 31, 21, 27, 35, 0),
        created_at: Time.new(2016, 3, 31, 21, 37, 35, 0)
      )


      serializable_alert = serializable(alert, serializer: AlertSerializer, adapter: :attributes)
      attributes_serialization1 = serializable_alert.as_json
      assert_equal alert.status, attributes_serialization1.fetch(:status)

      serializable_alert = serializable(alert, serializer: AlertSerializer, adapter: :attributes)
      attributes_serialization2 = serializable_alert.as_json
      assert_equal attributes_serialization1, attributes_serialization2

      attributes_cache_key = CachedSerializer.new(serializable_alert.adapter.serializer).cache_key
      assert_equal attributes_serialization1, cache_store.fetch(attributes_cache_key)

      serializable_alert = serializable(alert, serializer: AlertSerializer, adapter: :json_api)
      jsonapi_cache_key = CachedSerializer.new(serializable_alert.adapter.serializer).cache_key
      refute_equal attributes_cache_key, jsonapi_cache_key
      jsonapi_serialization1 = serializable_alert.as_json
      assert_equal alert.status, jsonapi_serialization1.fetch(:data).fetch(:attributes).fetch(:status)

      serializable_alert = serializable(alert, serializer: AlertSerializer, adapter: :json_api)
      jsonapi_serialization2 = serializable_alert.as_json
      assert_equal jsonapi_serialization1, jsonapi_serialization2

      cached_serialization = cache_store.fetch(jsonapi_cache_key)
      assert_equal jsonapi_serialization1, cached_serialization

      expected_jsonapi_serialization = {
        id: "1",
         type: "alerts",
         attributes: {
           created_at: 'Thu, 31 Mar 2016 21:37:35 UTC +00:00',
           status: "fail",
           resource: "resource-1",
           updated_at: 'Thu,  31 Mar 2016 21:37:35 UTC +00:00',
           started_at: 'Thu, 31 Mar 2016 21:36:35 UTC +00:00',
           ended_at: nil}
      }
      assert_equal expected_jsonapi_serialization, jsonapi_serialization1
    ensure
      Object.send(:remove_const, :Alert)
      Object.send(:remove_const, :AlertSerializer)
    end

when the cache key is the same for both adapters, after serializing with the attributes adapter, trying to serialize with the jsonapi adapter blows up since the cache value it expects is in the format of data: { etc, but it is still returning the attributes serialization.

Running bundle exec ruby -Ilib:test test/cache_test.rb -n /two/ on master with this test

ActiveModelSerializers::CacheTest#test_a_serializer_rendered_by_two_adapter_returns_differently_cached_attributes:
NoMethodError: undefined method `singularize' for nil:NilClass
active_model_serializers/lib/active_model/serializer/fieldset.rb:13:in `fields_for'
active_model_serializers/lib/active_model_serializers/adapter/json_api.rb:299:in `resource_object_for'
active_model_serializers/lib/active_model_serializers/adapter/json_api.rb:241:in `process_resource'
active_model_serializers/lib/active_model_serializers/adapter/json_api.rb:231:in `block in resource_objects_for'
active_model_serializers/lib/active_model_serializers/adapter/json_api.rb:231:in `each'
active_model_serializers/lib/active_model_serializers/adapter/json_api.rb:231:in `resource_objects_for'
active_model_serializers/lib/active_model_serializers/adapter/json_api.rb:74:in `success_document'
active_model_serializers/lib/active_model_serializers/adapter/json_api.rb:49:in `serializable_hash'
active_model_serializers/lib/active_model_serializers/adapter/base.rb:23:in `as_json'
active_model_serializers/lib/active_model_serializers/serializable_resource.rb:8:in `as_json'
active_model_serializers/lib/active_model_serializers/logging.rb:69:in `block (3 levels) in notify'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:117:in `call'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:117:in `call'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:555:in `block (2 levels) in compile'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:505:in `call'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:505:in `call'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:498:in `block (2 levels) in around'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:343:in `call'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:343:in `block (2 levels) in simple'
active_model_serializers/lib/active_model_serializers/logging.rb:22:in `call'
active_model_serializers/lib/active_model_serializers/logging.rb:22:in `block (3 levels) in instrument_rendering'
active_model_serializers/lib/active_model_serializers/logging.rb:79:in `block in notify_render'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/notifications.rb:164:in `block in instrument'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/notifications.rb:164:in `instrument'
active_model_serializers/lib/active_model_serializers/logging.rb:78:in `notify_render'
active_model_serializers/lib/active_model_serializers/logging.rb:21:in `block (2 levels) in instrument_rendering'
active_model_serializers/lib/active_model_serializers/logging.rb:97:in `tag_logger'
active_model_serializers/lib/active_model_serializers/logging.rb:20:in `block in instrument_rendering'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:441:in `instance_exec'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:441:in `block in make_lambda'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:342:in `call'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:342:in `block in simple'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:497:in `call'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:497:in `block in around'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:505:in `call'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:505:in `call'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:92:in `__run_callbacks__'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:778:in `_run_render_callbacks'
active_model_serializers/.bundle/ruby/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/callbacks.rb:81:in `run_callbacks'
active_model_serializers/lib/active_model_serializers/logging.rb:68:in `block (2 levels) in notify'
test/cache_test.rb:199:in `test_a_serializer_rendered_by_two_adapter_returns_differently_cached_attributes'

And if I

       jsonapi_cache_key = CachedSerializer.new(serializable_alert.adapter.serializer).cache_key
+      cached_serialization = cache_store.fetch(jsonapi_cache_key)
+      p [attributes_cache_key, jsonapi_cache_key, attributes_serialization2 == cached_serialization]
       refute_equal attributes_cache_key, jsonapi_cache_key 

I get confirmation the keys are the same and the values are the same.

["alert/1-20160331212735000000000/d38f40553027b9f7e8d3e1d63723be90", "alert/1-20160331212735000000000/d38f40553027b9f7e8d3e1d63723be90", true]

running on this branch with that test added I get

["alert/1-20160331212735000000000/attributes/215af6f028d68c553c79911ccc9969b5", "alert/1-20160331212735000000000/json_api/215af6f028d68c553c79911ccc9969b5", false]

(with the necessary change:

-      attributes_cache_key = CachedSerializer.new(serializable_alert.adapter.serializer).cache_key
+      attributes_cache_key = CachedSerializer.new(serializable_alert.adapter.serializer).cache_key(serializable_alert.adapter)
       assert_equal attributes_serialization1, cache_store.fetch(attributes_cache_key)

       serializable_alert = serializable(alert, serializer: AlertSerializer, adapter: :json_api)
-      jsonapi_cache_key = CachedSerializer.new(serializable_alert.adapter.serializer).cache_key
+      jsonapi_cache_key = CachedSerializer.new(serializable_alert.adapter.serializer).cache_key(serializable_alert.adapter)
       cached_serialization = cache_store.fetch(jsonapi_cache_key)
       p [attributes_cache_key, jsonapi_cache_key, attributes_serialization2 == cached_serialization]

@bf4 bf4 force-pushed the kevintyll-master-cache_serializers_by_adapter branch from 20d9076 to b3d0e90 Compare April 1, 2016 05:20
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Apr 1, 2016
Confirm caching attributes with different key json_api vs. attributes adapter

Adapted from @kevintyll's original test
rails-api#1644 (comment)
@bf4 bf4 force-pushed the kevintyll-master-cache_serializers_by_adapter branch from b3d0e90 to 16a3f93 Compare April 1, 2016 05:52
@bf4
Copy link
Member Author

bf4 commented Apr 1, 2016

Rebased off of master

@bf4 bf4 merged commit 96c5516 into rails-api:master Apr 1, 2016
@bf4 bf4 deleted the kevintyll-master-cache_serializers_by_adapter branch April 1, 2016 06:20
@kevintyll
Copy link
Contributor

I do not have any benchmarks. In fact, in 1 use case, we have a report object that can have thousands of child objects, because each object gets cached separately, it took “forever” to pull every object out and deserialize them and rebuild the object. We fell back to regular Rails cache in that instance and just serialized the entire object tree in a single shot.

On Mar 31, 2016, at 6:49 PM, Benjamin Fleischer notifications@github.com wrote:

Btw, do you have any benchmarka showing caching improves performance? I
haven't made any yet.
On Thu, Mar 31, 2016 at 5:48 PM Benjamin Fleischer bfleischer@gmail.com
wrote:

Thanks so much for that! I'll be sure to add it
On Thu, Mar 31, 2016 at 4:54 PM Kevin Tyll notifications@github.com
wrote:

I think this looks good.

As to you're question about how they are cached differently, I had the
same question when we started getting errors in production. Looking at the
code, it looked like it parsed the cached attributes. We a monkey patch and
test to confirm the issue. Here is the cached values from our test.

For the FlattenJson adapter (we are on a monkey patched version of rc3):

{:id=>1, :created_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :status=>"fail", :resource=>"resource-1", :updated_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :started_at=>Thu, 31 Mar 2016 21:36:35 UTC +00:00, :ended_at=>nil}

For JsonApi:

{:id=>"1", :type=>"alerts", :attributes=>{:created_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :status=>"fail", :resource=>"resource-1", :updated_at=>Thu, 31 Mar 2016 21:37:35 UTC +00:00, :started_at=>Thu, 31 Mar 2016 21:36:35 UTC +00:00, :ended_at=>nil}}

Here is our test for it:

context "#object_cache_key" do
should 'cache different serializer types separately' do
alert = create(:alert)
result1 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::FlattenJson.new(::V2::AlertSerializer.new(alert)).to_json)

assert_equal alert.status, result1['status']

result2 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::FlattenJson.new(::V2::AlertSerializer.new(alert)).to_json)

assert_equal result1, result2

result1 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::JsonApi.new(::V2::AlertSerializer.new(alert)).to_json)

assert_equal alert.status, result1['data']['attributes']['status']

result2 = ActiveSupport::JSON.decode(ActiveModel::Serializer::Adapter::JsonApi.new(::V2::AlertSerializer.new(alert)).to_json)

assert_equal result1, result2
end
end


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1644 (comment)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #1644 (comment)

@kevintyll
Copy link
Contributor

As far as getting off rc3, I am waiting for the internals to stabilize. Most of our monkey patching is to extend the functionality to include the related links. I created a custom adapter that inherits the JsonApi adapter, and adds the relationships sections for associations.

In order to do that, I had to override a few things and create some other new classes. While the public API remains fairly stable between release candidates, the private API I'm relying on changes quite a bit between releases. I don't want to have to redo my overrides and extensions for no net benefit.

I know you've added code that also now include the relationships section, and I assume that's in rc4, but I think I have some use cases that wouldn't be handled, so I would still need my custom adapter, at least as of the last time I looked at it months ago.

We are in production with what we have, so probably will stay on rc3 until 1.0 is released. I'll go through the pain of refactoring then.

@kevintyll
Copy link
Contributor

My V2::AlertSerializer cache options:

cache key: 'api/alerts', expires_in: 24.hours

though the key gets ignored because the alert object responds to cache_key

@bf4
Copy link
Member Author

bf4 commented Apr 1, 2016

re: #1644 (comment)

it took “forever”

Yeah, I haven't yet seen a case where using caching improves performance #1586

@bf4
Copy link
Member Author

bf4 commented Apr 1, 2016

@kevintyll Also, I'd love to help you share back any work you've done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants