Fix override API response bug in respond_with

Default responder was only using the given respond block when user
requested for HTML format, or JSON/XML format with valid resource. This
fix the responder so that it will use the given block regardless of the
validity of the resource. Note that in this case you'll have to check
for object's validity by yourself in the controller.

Fixes #4796

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