-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Adding Fragment Cache to AMS #810
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
require 'active_model/serializer/adapter/fragment_cache' | ||
|
||
module ActiveModel | ||
class Serializer | ||
class Adapter | ||
|
@@ -32,8 +34,38 @@ def self.adapter_class(adapter) | |
"ActiveModel::Serializer::Adapter::#{adapter.to_s.classify}".safe_constantize | ||
end | ||
|
||
def fragment_cache(*args) | ||
raise NotImplementedError, 'This is an abstract method. Should be implemented at the concrete adapter.' | ||
end | ||
|
||
private | ||
|
||
def cache_check(serializer) | ||
@cached_serializer = serializer | ||
@klass = @cached_serializer.class | ||
if is_cached? | ||
@klass._cache.fetch(cache_key, @klass._cache_options) do | ||
yield | ||
end | ||
elsif is_fragment_cached? | ||
FragmentCache.new(self, @cached_serializer, @options, @root).fetch | ||
else | ||
yield | ||
end | ||
end | ||
|
||
def is_cached? | ||
@klass._cache && !@klass._cache_only && !@klass._cache_except | ||
end | ||
|
||
def is_fragment_cached? | ||
@klass._cache_only && !@klass._cache_except || !@klass._cache_only && @klass._cache_except | ||
end | ||
|
||
def cache_key | ||
(@klass._cache_key) ? "#{@klass._cache_key}/#{@cached_serializer.object.id}-#{@cached_serializer.object.updated_at}" : @cached_serializer.object.cache_key | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you extract all of these methods into another class?
Otherwise, it's going to be hard to be maintained. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally agree. I'll extract it to an standalone module and include it in the Adapter. If you have another suggestions just let me know |
||
|
||
def meta | ||
serializer.meta if serializer.respond_to?(:meta) | ||
end | ||
|
@@ -50,20 +82,6 @@ def include_meta(json) | |
json[meta_key] = meta if meta && root | ||
json | ||
end | ||
|
||
private | ||
|
||
def cached_object | ||
klass = serializer.class | ||
if klass._cache | ||
_cache_key = (klass._cache_key) ? "#{klass._cache_key}/#{serializer.object.id}-#{serializer.object.updated_at}" : serializer.object.cache_key | ||
klass._cache.fetch(_cache_key, klass._cache_options) do | ||
yield | ||
end | ||
else | ||
yield | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
module ActiveModel | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I commented everything that could make it easy to support. |
||
class Serializer | ||
class Adapter | ||
class FragmentCache | ||
|
||
attr_reader :serializer | ||
|
||
def initialize(adapter, serializer, options, root) | ||
@root = root | ||
@options = options | ||
@adapter = adapter | ||
@serializer = serializer | ||
end | ||
|
||
def fetch | ||
klass = serializer.class | ||
# It will split the serializer into two, one that will be cached and other wont | ||
serializers = fragment_serializer(serializer.object.class.name, klass) | ||
|
||
# Instanciate both serializers | ||
cached_serializer = serializers[:cached].constantize.new(serializer.object) | ||
non_cached_serializer = serializers[:non_cached].constantize.new(serializer.object) | ||
|
||
cached_adapter = @adapter.class.new(cached_serializer, @options) | ||
non_cached_adapter = @adapter.class.new(non_cached_serializer, @options) | ||
|
||
# Get serializable hash from both | ||
cached_hash = cached_adapter.serializable_hash | ||
non_cached_hash = non_cached_adapter.serializable_hash | ||
|
||
# Merge both results | ||
@adapter.fragment_cache(cached_hash, non_cached_hash) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The duck typing implementation that was defined at |
||
end | ||
|
||
private | ||
|
||
def cached_attributes(klass, serializers) | ||
cached_attributes = (klass._cache_only) ? klass._cache_only : serializer.attributes.keys.delete_if {|attr| klass._cache_except.include?(attr) } | ||
non_cached_attributes = serializer.attributes.keys.delete_if {|attr| cached_attributes.include?(attr) } | ||
|
||
cached_attributes.each do |attribute| | ||
options = serializer.class._attributes_keys[attribute] | ||
options ||= {} | ||
# Add cached attributes to cached Serializer | ||
serializers[:cached].constantize.attribute(attribute, options) | ||
end | ||
|
||
non_cached_attributes.each do |attribute| | ||
options = serializer.class._attributes_keys[attribute] | ||
options ||= {} | ||
# Add non-cached attributes to non-cached Serializer | ||
serializers[:non_cached].constantize.attribute(attribute, options) | ||
end | ||
end | ||
|
||
def fragment_serializer(name, klass) | ||
cached = "#{name.capitalize}CachedSerializer" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If different serializers share the same model, but has totally different attributes, would their caches collide, even if the key is different? I believe i see it on my machine. If you believe this could happen, i can present those tests to you, but i need time to set it up. maybe it should go something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeap, I think it could happen. Not the cache_key itself, because it takes the file path into account but the Class name would colide. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @joaomdmoura thanks, that would be great!
|
||
non_cached = "#{name.capitalize}NonCachedSerializer" | ||
|
||
Object.const_set cached, Class.new(ActiveModel::Serializer) unless Object.const_defined?(cached) | ||
Object.const_set non_cached, Class.new(ActiveModel::Serializer) unless Object.const_defined?(non_cached) | ||
|
||
klass._cache_options ||= {} | ||
klass._cache_options[:key] = klass._cache_key if klass._cache_key | ||
|
||
cached.constantize.cache(klass._cache_options) | ||
|
||
cached.constantize.fragmented(serializer) | ||
non_cached.constantize.fragmented(serializer) | ||
|
||
serializers = {cached: cached, non_cached: non_cached} | ||
cached_attributes(klass, serializers) | ||
serializers | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
module ActiveModel | ||
class Serializer | ||
class Adapter | ||
class Json < Adapter | ||
class FragmentCache | ||
|
||
def fragment_cache(cached_hash, non_cached_hash) | ||
non_cached_hash.merge cached_hash | ||
end | ||
|
||
end | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kurko abstracted method. Duck typing strategy.