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

fix for serializers never including associations on cache hits #1276

Merged
merged 3 commits into from
Apr 1, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions lib/active_model/serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -353,13 +353,16 @@ def as_json(options={})
# Returns a hash representation of the serializable
# object without the root.
def serializable_hash
if perform_caching?
@node = if perform_caching?
cache.fetch expand_cache_key([self.class.to_s.underscore, cache_key, 'serializable-hash']) do
_serializable_hash
end
else
_serializable_hash
end

include_associations! if @object.present? && _embed
@node
end

def include_associations!
Expand Down Expand Up @@ -476,9 +479,7 @@ def scope

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change of node from attributes to _serializable_hash?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@node needs to be assigned outside of _serializable_hash, since associations can differ based on the serializer used, and the _serializable_hash method itself is cached inside serializable_hash

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks. I'm not as familiar with the 0.8 code flow.

Do you think you can get CI passing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bf4 the specs seem to fail randomly trying to call read_attribute_for_serialization - the failures are not tied to versions of rails or ruby, and can change per run. The issue appears to be in the generated faker data, but I'm not exactly sure what's going on there.

Anyway, we've used this in production without issue since I opened this PR, so it's safe to merge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kieran I'm okay to merge then, since I'm trusting you, here :) It looks like you have run the specs locally, but you say they fail intermittently for you?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, they do about 50% of the time. Something to do with the way fake objects are generated for testing.

def _serializable_hash
return nil if @object.nil?
@node = attributes
include_associations! if _embed
@node
attributes
end

def perform_caching?
Expand Down
81 changes: 81 additions & 0 deletions test/caching_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,42 @@ def read_attribute_for_serialization(name)
end
end

class Parent
def id
'parent1'
end

def name
'Kieran'
end

def children
[ Child.new ]
end

def read_attribute_for_serialization(name)
send name
end
end

class Child
def id
'child1'
end

def name
'Joshua'
end

def parent
Parent.new
end

def read_attribute_for_serialization(name)
send name
end
end

def test_serializers_have_a_cache_store
ActiveModel::Serializer.cache = NullStore.new

Expand Down Expand Up @@ -93,4 +129,49 @@ def cache_key
assert_equal instance.serializable_array, serializer.cache.read('array_serializer/cache-key/serializable-array')
assert_equal instance.to_json, serializer.cache.read('array_serializer/cache-key/to-json')
end

def test_cached_serializers_return_associations

child_serializer = Class.new(ActiveModel::Serializer) do
cached true
attributes :name

def self.to_s
'child_serializer'
end

def cache_key
object.name
end
end

parent_serializer = Class.new(ActiveModel::Serializer) do
cached true
attributes :name

has_many :children, serializer: child_serializer, embed: :ids, include: true

def self.to_s
'parent_serializer'
end

def cache_key
object.name
end
end


parent_serializer.cache = NullStore.new
child_serializer.cache = NullStore.new

instance = parent_serializer.new Parent.new, root: :parent

initial_keys = instance.as_json.keys

assert_equal(initial_keys, [:children, :parent])

cached_keys = instance.as_json.keys

assert_equal(cached_keys, [:children, :parent])
end
end