Merge pull request #147 from marshall-lee/delegators

Refactoring: extract delegating logic.

Author: dblock

CHANGELOG.md

@@ -7,6 +7,10 @@
 * [#134](https://github.com/intridea/grape-entity/pull/134): Subclasses no longer affected in all cases by `unexpose` in parent - [@etehtsea](https://github.com/etehtsea).
 * [#135](https://github.com/intridea/grape-entity/pull/135): Added `except` option - [@dan-corneanu](https://github.com/dan-corneanu).
 * [#136](https://github.com/intridea/grape-entity/pull/136): Allow for strings in `only` and `except` options - [@bswinnerton](https://github.com/bswinnerton).
+* [#147](https://github.com/intridea/grape-entity/pull/147): Expose `safe` attributes as `nil` if they cannot be evaluated: [#140](https://github.com/intridea/grape-entity/issues/140).
+* [#147](https://github.com/intridea/grape-entity/pull/147): Fix: private method values were not exposed with `safe` option: [#142](https://github.com/intridea/grape-entity/pull/142).
+* [#147](https://github.com/intridea/grape-entity/pull/147): Remove catching of `NoMethodError` because it can occur deep inside in a method call so this exception does not mean that attribute not exist.
+* [#147](https://github.com/intridea/grape-entity/pull/147): `valid_exposures` is removed.
 * Your contribution here.
 
 0.4.5 (2015-03-10)

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'

lib/grape_entity/delegator.rb

@@ -0,0 +1,23 @@
+require 'grape_entity/delegator/base'
+require 'grape_entity/delegator/hash_object'
+require 'grape_entity/delegator/openstruct_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)
+          HashObject.new object
+        elsif defined?(OpenStruct) && object.is_a?(OpenStruct)
+          OpenStructObject.new object
+        elsif object.respond_to? :fetch, true
+          FetchableObject.new object
+        else
+          PlainObject.new object
+        end
+      end
+    end
+  end
+end

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

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

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

lib/grape_entity/delegator/openstruct_object.rb

@@ -0,0 +1,11 @@
+module Grape
+  class Entity
+    module Delegator
+      class OpenStructObject < Base
+        def delegate(attribute)
+          object.send attribute
+        end
+      end
+    end
+  end
+end

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

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,17 +395,16 @@ def presented
 
     def initialize(object, options = {})
       @object = object
+      @delegator = Delegator.new object
       @options = options
     end
 
     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)

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