Merge pull request #1025 from dblock/merge-respond-to

Only merge representations onto an existing body.

Author: dblock

CHANGELOG.md

@@ -21,6 +21,7 @@
 * [#1001](https://github.com/intridea/grape/pull/1001): Fixed calling endpoint with specified format with format in its path - [@hodak](https://github.com/hodak).
 * [#1005](https://github.com/intridea/grape/pull/1005): Fixed the Grape::Middleware::Globals - [@urkle](https://github.com/urkle).
 * [#1012](https://github.com/intridea/grape/pull/1012): Fixed `allow_blank: false` with a Boolean value of `false` - [@mfunaro](https://github.com/mfunaro).
+* [#1023](https://github.com/intridea/grape/issues/1023): Fixes unexpected beahvior with `present` and an object that responds to `merge` but isn't a Hash - [@dblock](https://github.com/dblock).
 * Your contribution here.
 
 0.11.0 (2/23/2015)

UPGRADING.md

@@ -3,6 +3,12 @@ Upgrading Grape
 
 ### Upgrading to >= 0.12.0
 
+#### Changes in present
+
+Using `present` with objects that responded to `merge` would cause early evaluation of the represented object, with unexpected side-effects, such as missing parameters or environment within rendering code. Grape now only merges represented objects with a previously rendered body, usually when multiple `present` calls are made in the same route.
+
+See [grape-with-roar#5](https://github.com/dblock/grape-with-roar/issues/5) and [#1023](https://github.com/intridea/grape/issues/1023).
+
 #### Changes to regexp validator
 
 Parameters with `nil` value will now pass `regexp` validation. To disallow `nil` value for an endpoint, add `allow_blank: false`.

lib/grape/dsl/inside_route.rb

@@ -196,18 +196,17 @@ def present(*args)
         root = options.delete(:root)
 
         representation = if entity_class
-                           embeds = { env: env }
-                           embeds[:version] = env['api.version'] if env['api.version']
-                           entity_class.represent(object, embeds.merge(options))
+                           entity_representation_for(entity_class, object, options)
                          else
                            object
                          end
 
         representation = { root => representation } if root
         if key
           representation = (@body || {}).merge(key => representation)
-        elsif entity_class.present? && representation.respond_to?('merge')
-          representation = (@body || {}).merge(representation)
+        elsif entity_class.present? && @body
+          fail ArgumentError, "Representation of type #{representation.class} cannot be merged." unless representation.respond_to?('merge')
+          representation = @body.merge(representation)
         end
 
         body representation
@@ -245,6 +244,12 @@ def entity_class_for_obj(object, options)
 
         entity_class
       end
+
+      def entity_representation_for(entity_class, object, options)
+        embeds = { env: env }
+        embeds[:version] = env['api.version'] if env['api.version']
+        entity_class.represent(object, embeds.merge(options))
+      end
     end
   end
 end

spec/grape/dsl/inside_route_spec.rb

@@ -239,30 +239,51 @@ def initialize
           end
         end
 
-        describe 'with' do
-          describe 'multiple entities' do
-            let(:entity_mock1) do
-              entity_mock1 = Object.new
-              allow(entity_mock1).to receive(:represent).and_return(dummy1: 'dummy1')
-              entity_mock1
+        describe 'multiple entities' do
+          let(:entity_mock1) do
+            entity_mock1 = Object.new
+            allow(entity_mock1).to receive(:represent).and_return(dummy1: 'dummy1')
+            entity_mock1
+          end
+
+          let(:entity_mock2) do
+            entity_mock2 = Object.new
+            allow(entity_mock2).to receive(:represent).and_return(dummy2: 'dummy2')
+            entity_mock2
+          end
+
+          describe 'instance' do
+            before do
+              subject.present 'dummy1', with: entity_mock1
+              subject.present 'dummy2', with: entity_mock2
             end
 
-            let(:entity_mock2) do
-              entity_mock2 = Object.new
-              allow(entity_mock2).to receive(:represent).and_return(dummy2: 'dummy2')
-              entity_mock2
+            it 'presents both dummy objects' do
+              expect(subject.body[:dummy1]).to eq 'dummy1'
+              expect(subject.body[:dummy2]).to eq 'dummy2'
             end
+          end
+        end
 
-            describe 'instance' do
-              before do
-                subject.present 'dummy1', with: entity_mock1
-                subject.present 'dummy2', with: entity_mock2
-              end
+        describe 'non mergeable entity' do
+          let(:entity_mock1) do
+            entity_mock1 = Object.new
+            allow(entity_mock1).to receive(:represent).and_return(dummy1: 'dummy1')
+            entity_mock1
+          end
 
-              it 'presents both dummy objects' do
-                expect(subject.body[:dummy1]).to eq 'dummy1'
-                expect(subject.body[:dummy2]).to eq 'dummy2'
-              end
+          let(:entity_mock2) do
+            entity_mock2 = Object.new
+            allow(entity_mock2).to receive(:represent).and_return('not a hash')
+            entity_mock2
+          end
+
+          describe 'instance' do
+            it 'fails' do
+              subject.present 'dummy1', with: entity_mock1
+              expect {
+                subject.present 'dummy2', with: entity_mock2
+              }.to raise_error ArgumentError, 'Representation of type String cannot be merged.'
             end
           end
         end