From e9603fe802a7000dc0771751b54285f11566069b Mon Sep 17 00:00:00 2001 From: Vladimir Kochnev Date: Mon, 20 Jul 2015 18:13:42 +0300 Subject: [PATCH] Refactoring: extract delegating logic. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This one solves many problems at once: - Fixes #140. For this one I changed specs a bit. It's breaking but I think it's worth it. - Fixes #142 differently because now `respond_to?(name, true)` stuff is incapsulated in `Delegator::PlainObject`. - Old `delegate_attribute` catched `NoMethodError` and re-raised it with custom message. It's incorrect because `NoMethodError` can occur deep inside in method call — this exception simply doesn't mean that attribute doesn't exist. - Solves the problem that object is checked to `is_a?(Hash)` and `respond_to?(:fetch, true)` multiple times. - Extracts delegating logic to separate delegator classes. - Removes constructing of `valid_exposures` at all — there's no need in this method now. --- lib/grape_entity.rb | 1 + lib/grape_entity/delegator.rb | 20 ++++++++ lib/grape_entity/delegator/base.rb | 17 +++++++ .../delegator/fetchable_object.rb | 11 +++++ lib/grape_entity/delegator/hash_object.rb | 11 +++++ lib/grape_entity/delegator/plain_object.rb | 15 ++++++ lib/grape_entity/entity.rb | 47 ++++++++++--------- spec/grape_entity/entity_spec.rb | 36 +++++++++----- 8 files changed, 124 insertions(+), 34 deletions(-) create mode 100644 lib/grape_entity/delegator.rb create mode 100644 lib/grape_entity/delegator/base.rb create mode 100644 lib/grape_entity/delegator/fetchable_object.rb create mode 100644 lib/grape_entity/delegator/hash_object.rb create mode 100644 lib/grape_entity/delegator/plain_object.rb diff --git a/lib/grape_entity.rb b/lib/grape_entity.rb index da3d8da7..44815600 100644 --- a/lib/grape_entity.rb +++ b/lib/grape_entity.rb @@ -2,3 +2,4 @@ require 'active_support/core_ext' require 'grape_entity/version' require 'grape_entity/entity' +require 'grape_entity/delegator' diff --git a/lib/grape_entity/delegator.rb b/lib/grape_entity/delegator.rb new file mode 100644 index 00000000..0466bd54 --- /dev/null +++ b/lib/grape_entity/delegator.rb @@ -0,0 +1,20 @@ +require 'grape_entity/delegator/base' +require 'grape_entity/delegator/hash_object' +require 'grape_entity/delegator/fetchable_object' +require 'grape_entity/delegator/plain_object' + +module Grape + class Entity + module Delegator + def self.new(object) + if object.is_a?(Hash) || object.is_a?(OpenStruct) + HashObject.new object + elsif object.respond_to? :fetch, true + FetchableObject.new object + else + PlainObject.new object + end + end + end + end +end diff --git a/lib/grape_entity/delegator/base.rb b/lib/grape_entity/delegator/base.rb new file mode 100644 index 00000000..5422e5ab --- /dev/null +++ b/lib/grape_entity/delegator/base.rb @@ -0,0 +1,17 @@ +module Grape + class Entity + module Delegator + class Base + attr_reader :object + + def initialize(object) + @object = object + end + + def delegatable?(_attribute) + true + end + end + end + end +end diff --git a/lib/grape_entity/delegator/fetchable_object.rb b/lib/grape_entity/delegator/fetchable_object.rb new file mode 100644 index 00000000..ff3af94a --- /dev/null +++ b/lib/grape_entity/delegator/fetchable_object.rb @@ -0,0 +1,11 @@ +module Grape + class Entity + module Delegator + class FetchableObject < Base + def delegate(attribute) + object.fetch attribute + end + end + end + end +end diff --git a/lib/grape_entity/delegator/hash_object.rb b/lib/grape_entity/delegator/hash_object.rb new file mode 100644 index 00000000..aab555cc --- /dev/null +++ b/lib/grape_entity/delegator/hash_object.rb @@ -0,0 +1,11 @@ +module Grape + class Entity + module Delegator + class HashObject < Base + def delegate(attribute) + object[attribute] + end + end + end + end +end diff --git a/lib/grape_entity/delegator/plain_object.rb b/lib/grape_entity/delegator/plain_object.rb new file mode 100644 index 00000000..4d0178c0 --- /dev/null +++ b/lib/grape_entity/delegator/plain_object.rb @@ -0,0 +1,15 @@ +module Grape + class Entity + module Delegator + class PlainObject < Base + def delegate(attribute) + object.send attribute + end + + def delegatable?(attribute) + object.respond_to? attribute, true + end + end + end + end +end diff --git a/lib/grape_entity/entity.rb b/lib/grape_entity/entity.rb index 066fb957..7e67fd34 100644 --- a/lib/grape_entity/entity.rb +++ b/lib/grape_entity/entity.rb @@ -42,7 +42,7 @@ module Grape # end # end class Entity - attr_reader :object, :options + attr_reader :object, :delegator, :options # The Entity DSL allows you to mix entity functionality into # your existing classes. @@ -104,6 +104,7 @@ class << self # containing object, the values are the options that were passed into expose. # @return [Hash] of exposures attr_accessor :exposures + attr_accessor :root_exposures # Returns all formatters that are registered for this and it's ancestors # @return [Hash] of formatters attr_accessor :formatters @@ -113,6 +114,7 @@ class << self def self.inherited(subclass) subclass.exposures = exposures.try(:dup) || {} + subclass.root_exposures = root_exposures.try(:dup) || {} subclass.nested_exposures = nested_exposures.try(:dup) || {} subclass.nested_attribute_names = nested_attribute_names.try(:dup) || {} subclass.formatters = formatters.try(:dup) || {} @@ -159,7 +161,9 @@ def self.expose(*args, &block) # rubocop:disable Style/Next args.each do |attribute| - unless @nested_attributes.empty? + if @nested_attributes.empty? + root_exposures[attribute] = options + else orig_attribute = attribute.to_sym attribute = "#{@nested_attributes.last}__#{attribute}".to_sym nested_attribute_names[attribute] = orig_attribute @@ -391,6 +395,7 @@ def presented def initialize(object, options = {}) @object = object + @delegator = Delegator.new object @options = options end @@ -398,10 +403,8 @@ def exposures self.class.exposures end - def valid_exposures - exposures.select do |attribute, exposure_options| - !exposure_options[:nested] && valid_exposure?(attribute, exposure_options) - end + def root_exposures + self.class.root_exposures end def documentation @@ -424,7 +427,7 @@ def serializable_hash(runtime_options = {}) opts = options.merge(runtime_options || {}) - valid_exposures.each_with_object({}) do |(attribute, exposure_options), output| + root_exposures.each_with_object({}) do |(attribute, exposure_options), output| next unless should_return_attribute?(attribute, opts) && conditions_met?(exposure_options, opts) partial_output = value_for(attribute, opts) @@ -536,6 +539,7 @@ def nested_value_for(attribute, options) def value_for(attribute, options = {}) exposure_options = exposures[attribute.to_sym] + return unless valid_exposure?(attribute, exposure_options) if exposure_options[:using] exposure_options[:using] = exposure_options[:using].constantize if exposure_options[:using].respond_to? :constantize @@ -573,27 +577,24 @@ def delegate_attribute(attribute) name = self.class.name_for(attribute) if respond_to?(name, true) send(name) - elsif object.is_a?(Hash) - object[name] - elsif object.respond_to?(name, true) - object.send(name) - elsif object.respond_to?(:fetch, true) - object.fetch(name) else - begin - object.send(name) - rescue NoMethodError - raise NoMethodError, "#{self.class.name} missing attribute `#{name}' on #{object}" - end + delegator.delegate(name) end end def valid_exposure?(attribute, exposure_options) - (self.class.nested_exposures_for?(attribute) && self.class.nested_exposures[attribute].all? { |a, o| valid_exposure?(a, o) }) || \ - exposure_options.key?(:proc) || \ - !exposure_options[:safe] || \ - object.respond_to?(self.class.name_for(attribute)) || \ - object.is_a?(Hash) && object.key?(self.class.name_for(attribute)) + if self.class.nested_exposures_for?(attribute) + self.class.nested_exposures[attribute].all? { |a, o| valid_exposure?(a, o) } + elsif exposure_options.key?(:proc) + true + else + name = self.class.name_for(attribute) + if exposure_options[:safe] + delegator.delegatable?(name) + else + delegator.delegatable?(name) || fail(NoMethodError, "#{self.class.name} missing attribute `#{name}' on #{object}") + end + end end def conditions_met?(exposure_options, options) diff --git a/spec/grape_entity/entity_spec.rb b/spec/grape_entity/entity_spec.rb index 835e378e..f11cacd0 100644 --- a/spec/grape_entity/entity_spec.rb +++ b/spec/grape_entity/entity_spec.rb @@ -190,11 +190,14 @@ class Parent < Person subject.expose :nested end end - - valid_keys = subject.represent({}).valid_exposures.keys - - expect(valid_keys.include?(:awesome)).to be true - expect(valid_keys.include?(:not_awesome)).to be false + expect(subject.represent({}, serializable: true)).to eq( + awesome: { + nested: 'value' + }, + not_awesome: { + nested: nil + } + ) end end end @@ -848,12 +851,23 @@ class Parent < Person expect { fresh_class.new(model).serializable_hash }.not_to raise_error end - it "does not expose attributes that don't exist on the object" do + it 'exposes values of private method calls' do + some_class = Class.new do + define_method :name do + true + end + private :name + end + fresh_class.expose :name, safe: true + expect(fresh_class.new(some_class.new).serializable_hash).to eq(name: true) + end + + it "does expose attributes that don't exist on the object as nil" do fresh_class.expose :email, :nonexistent_attribute, :name, safe: true res = fresh_class.new(model).serializable_hash expect(res).to have_key :email - expect(res).not_to have_key :nonexistent_attribute + expect(res).to have_key :nonexistent_attribute expect(res).to have_key :name end @@ -864,15 +878,15 @@ class Parent < Person expect(res).to have_key :name end - it "does not expose attributes that don't exist on the object, even with criteria" do + it "does expose attributes that don't exist on the object as nil if criteria is true" do fresh_class.expose :email - fresh_class.expose :nonexistent_attribute, safe: true, if: -> { false } - fresh_class.expose :nonexistent_attribute2, safe: true, if: -> { true } + fresh_class.expose :nonexistent_attribute, safe: true, if: ->(_obj, _opts) { false } + fresh_class.expose :nonexistent_attribute2, safe: true, if: ->(_obj, _opts) { true } res = fresh_class.new(model).serializable_hash expect(res).to have_key :email expect(res).not_to have_key :nonexistent_attribute - expect(res).not_to have_key :nonexistent_attribute2 + expect(res).to have_key :nonexistent_attribute2 end end