Merge pull request #4869 from sikachu/master-responder-fix

Fix override API response bug in respond_with

4 files changed, 60 insertions, 8 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index df72794c31..2a5c181a3a 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,5 +1,7 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   Default responder will now always use your overridden block in `respond_with` to render your response. *Prem Sichanugrist*
+
 *   Allow `value_method` and `text_method` arguments from `collection_select` and
     `options_from_collection_for_select` to receive an object that responds to `:call`,
     such as a `proc`, to evaluate the option in the current element context. This works
diff --git a/actionpack/lib/action_controller/metal/mime_responds.rb b/actionpack/lib/action_controller/metal/mime_responds.rb
index ca383be76b..819bbb5463 100644
--- a/actionpack/lib/action_controller/metal/mime_responds.rb
+++ b/actionpack/lib/action_controller/metal/mime_responds.rb
@@ -191,7 +191,8 @@ module ActionController #:nodoc:
     def respond_to(*mimes, &block)
       raise ArgumentError, "respond_to takes either types or a block, never both" if mimes.any? && block_given?
 
-      if response = retrieve_response_from_mimes(mimes, &block)
+      if collector = retrieve_collector_from_mimes(mimes, &block)
+        response = collector.response_for(negotiated_format(collector)) || collector.default_response
         response.call(nil)
       end
     end
@@ -232,10 +233,19 @@ module ActionController #:nodoc:
       raise "In order to use respond_with, first you need to declare the formats your " <<
             "controller responds to in the class level" if self.class.mimes_for_respond_to.empty?
 
-      if response = retrieve_response_from_mimes(&block)
+      if collector = retrieve_collector_from_mimes(&block)
         options = resources.size == 1 ? {} : resources.extract_options!
-        options.merge!(:default_response => response)
-        (options.delete(:responder) || self.class.responder).call(self, resources, options)
+
+        if defined_response = collector.response_for(negotiated_format(collector))
+          if action = options.delete(:action)
+            render :action => action
+          else
+            defined_response.call(nil)
+          end
+        else
+          options.merge!(:default_response => collector.default_response)
+          (options.delete(:responder) || self.class.responder).call(self, resources, options)
+        end
       end
     end
 
@@ -263,24 +273,29 @@ module ActionController #:nodoc:
     # Collects mimes and return the response for the negotiated format. Returns
     # nil if :not_acceptable was sent to the client.
     #
-    def retrieve_response_from_mimes(mimes=nil, &block) #:nodoc:
+    def retrieve_collector_from_mimes(mimes=nil, &block) #:nodoc:
       mimes ||= collect_mimes_from_class_level
       collector = Collector.new(mimes) { |options| default_render(options || {}) }
       block.call(collector) if block_given?
 
-      if format = request.negotiate_mime(collector.order)
+      if format = negotiated_format(collector)
         self.content_type ||= format.to_s
         lookup_context.freeze_formats([format.to_sym])
-        collector.response_for(format)
+        collector
       else
         head :not_acceptable
         nil
       end
     end
 
+    def negotiated_format(collector)
+      request.negotiate_mime(collector.order)
+    end
+
     class Collector #:nodoc:
       include AbstractController::Collector
       attr_accessor :order
+      attr_reader :default_response
 
       def initialize(mimes, &block)
         @order, @responses, @default_response = [], {}, block
@@ -303,7 +318,7 @@ module ActionController #:nodoc:
       end
 
       def response_for(mime)
-        @responses[mime] || @responses[Mime::ALL] || @default_response
+        @responses[mime] || @responses[Mime::ALL]
       end
     end
   end
diff --git a/actionpack/test/controller/mime_responds_test.rb b/actionpack/test/controller/mime_responds_test.rb
index 60498822ff..69a8f4f213 100644
--- a/actionpack/test/controller/mime_responds_test.rb
+++ b/actionpack/test/controller/mime_responds_test.rb
@@ -593,6 +593,19 @@ class RenderJsonRespondWithController < RespondWithController
       format.json { render :json => RenderJsonTestException.new('boom') }
     end
   end
+
+  def create
+    resource = ValidatedCustomer.new(params[:name], 1)
+    respond_with(resource) do |format|
+      format.json do
+        if resource.errors.empty?
+          render :json => { :valid => true }
+        else
+          render :json => { :valid => false }
+        end
+      end
+    end
+  end
 end
 
 class EmptyRespondWithController < ActionController::Base
@@ -964,6 +977,18 @@ class RespondWithControllerTest < ActionController::TestCase
     assert_match(/"error":"RenderJsonTestException"/, @response.body)
   end
 
+  def test_api_response_with_valid_resource_respect_override_block
+    @controller = RenderJsonRespondWithController.new
+    post :create, :name => "sikachu", :format => :json
+    assert_equal '{"valid":true}', @response.body
+  end
+
+  def test_api_response_with_invalid_resource_respect_override_block
+    @controller = RenderJsonRespondWithController.new
+    post :create, :name => "david", :format => :json
+    assert_equal '{"valid":false}', @response.body
+  end
+
   def test_no_double_render_is_raised
     @request.accept = "text/html"
     assert_raise ActionView::MissingTemplate do
diff --git a/actionpack/test/lib/controller/fake_models.rb b/actionpack/test/lib/controller/fake_models.rb
index f2362714d7..bbb4cc5ef3 100644
--- a/actionpack/test/lib/controller/fake_models.rb
+++ b/actionpack/test/lib/controller/fake_models.rb
@@ -34,6 +34,16 @@ end
 class GoodCustomer < Customer
 end
 
+class ValidatedCustomer < Customer
+  def errors
+    if name =~ /Sikachu/i
+      []
+    else
+      [{:name => "is invalid"}]
+    end
+  end
+end
+
 module Quiz
   class Question < Struct.new(:name, :id)
     extend ActiveModel::Naming