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

Add DynamicProxyObject to fix time-consuming "getter"-methods #112

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions lib/jsonapi-serializers.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require "jsonapi-serializers/version"
require "jsonapi-serializers/attributes"
require "jsonapi-serializers/serializer"
require "jsonapi-serializers/dynamic_proxy_object"

module JSONAPI
module Serializer
Expand Down
24 changes: 24 additions & 0 deletions lib/jsonapi-serializers/dynamic_proxy_object.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
module JSONAPI
class DynamicProxyObject < BasicObject

def initialize(target)
@cache = {}
@target = target
end

def class
@target.class
end

def cache_key(name, args)
"#{name}_#{args.hash}"
end

def method_missing(name, *args, &block)
key = cache_key(name, args)
@cache[key] ||= @target.send(name, *args, &block)

Choose a reason for hiding this comment

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

This should be public_send, for maximum clarity.

Choose a reason for hiding this comment

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

I don't think ||= will be a good enough check. Different args or a block should produce different results.

@cache[key]

Choose a reason for hiding this comment

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

This line can be deleted. All ruby ...=... syntax always returns the right most expression.

end

end
end
18 changes: 14 additions & 4 deletions lib/jsonapi-serializers/serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ def relationships
end

if @_include_linkages.include?(formatted_attribute_name) || attr_data[:options][:include_data]
object = has_one_relationship(attribute_name, attr_data)
object = JSONAPI::Serializer.proxy_objects(has_one_relationship(attribute_name, attr_data))
if object.nil?
# Spec: Resource linkage MUST be represented as one of the following:
# - null for empty to-one relationships.
Expand Down Expand Up @@ -156,7 +156,7 @@ def relationships
# http://jsonapi.org/format/#document-structure-resource-relationships
if @_include_linkages.include?(formatted_attribute_name) || attr_data[:options][:include_data]
data[formatted_attribute_name]['data'] = []
objects = has_many_relationship(attribute_name, attr_data) || []
objects = JSONAPI::Serializer.proxy_objects(has_many_relationship(attribute_name, attr_data) || [])
objects.each do |obj|
related_object_serializer = JSONAPI::Serializer.find_serializer(obj, @options)
data[formatted_attribute_name]['data'] << {
Expand Down Expand Up @@ -252,7 +252,17 @@ def self.find_serializer(object, options)
find_serializer_class(object, options).new(object, options)
end

def self.proxy_objects(objects)
return nil unless objects
if objects.respond_to?(:map)
objects.map { |obj| DynamicProxyObject.new(obj) }
else

Choose a reason for hiding this comment

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

IMO respond_to?(:map) is too weak of a trait check. Any object could have the method map and not be a collection.

DynamicProxyObject.new(objects)
end
end

def self.serialize(objects, options = {})
objects = proxy_objects(objects)
# Normalize option strings to symbols.
options[:is_collection] = options.delete('is_collection') || options[:is_collection] || false
options[:include] = options.delete('include') || options[:include]
Expand Down Expand Up @@ -462,12 +472,12 @@ def self.find_recursive_relationships(root_object, root_inclusion_tree, results,
if serializer.has_one_relationships.has_key?(unformatted_attr_name)
is_valid_attr = true
attr_data = serializer.has_one_relationships[unformatted_attr_name]
object = serializer.has_one_relationship(unformatted_attr_name, attr_data)
object = proxy_objects(serializer.has_one_relationship(unformatted_attr_name, attr_data))
elsif serializer.has_many_relationships.has_key?(unformatted_attr_name)
is_valid_attr = true
is_collection = true
attr_data = serializer.has_many_relationships[unformatted_attr_name]
object = serializer.has_many_relationship(unformatted_attr_name, attr_data)
object = proxy_objects(serializer.has_many_relationship(unformatted_attr_name, attr_data))
end

if !is_valid_attr
Expand Down
13 changes: 13 additions & 0 deletions spec/dynamic_proxy_object_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
describe JSONAPI::DynamicProxyObject do
it 'should delegate methods to target and cache results' do
target = double('target')
expect(target).to receive(:foo).and_return('bar')
expect(target).to receive(:foo).with(param1: '1', param2: 2).and_return('bar2')

proxy = JSONAPI::DynamicProxyObject.new(target)
expect(proxy.foo).to eq('bar')
expect(proxy.foo).to eq('bar')
expect(proxy.foo(param1: '1', param2: 2)).to eq('bar2')
expect(proxy.foo(param1: '1', param2: 2)).to eq('bar2')
end
end