Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

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
  • Loading branch information...
commit 567ac65b423c30c24aa6c0c0522858e3c240eb26 1 parent 4ca633e
Prem Sichanugrist authored February 03, 2012
2  actionpack/CHANGELOG.md
Source Rendered
... ...
@@ -1,5 +1,7 @@
1 1
 ## Rails 3.2.2 (unreleased) ##
2 2
 
  3
+*   Default responder will now always use your overridden block in `respond_with` to render your response. *Prem Sichanugrist*
  4
+
3 5
 *   check_box helper with :disabled => true will generate a disabled hidden field to conform with the HTML convention where disabled fields are not submitted with the form.
4 6
     This is a behavior change, previously the hidden tag had a value of the disabled checkbox.
5 7
     *Tadas Tamosauskas*
31  actionpack/lib/action_controller/metal/mime_responds.rb
@@ -191,7 +191,8 @@ def clear_respond_to
191 191
     def respond_to(*mimes, &block)
192 192
       raise ArgumentError, "respond_to takes either types or a block, never both" if mimes.any? && block_given?
193 193
 
194  
-      if response = retrieve_response_from_mimes(mimes, &block)
  194
+      if collector = retrieve_collector_from_mimes(mimes, &block)
  195
+        response = collector.response_for(negotiated_format(collector)) || collector.default_response
195 196
         response.call(nil)
196 197
       end
197 198
     end
@@ -232,10 +233,19 @@ def respond_with(*resources, &block)
232 233
       raise "In order to use respond_with, first you need to declare the formats your " <<
233 234
             "controller responds to in the class level" if self.class.mimes_for_respond_to.empty?
234 235
 
235  
-      if response = retrieve_response_from_mimes(&block)
  236
+      if collector = retrieve_collector_from_mimes(&block)
236 237
         options = resources.size == 1 ? {} : resources.extract_options!
237  
-        options.merge!(:default_response => response)
238  
-        (options.delete(:responder) || self.class.responder).call(self, resources, options)
  238
+
  239
+        if defined_response = collector.response_for(negotiated_format(collector))
  240
+          if action = options.delete(:action)
  241
+            render :action => action
  242
+          else
  243
+            defined_response.call(nil)
  244
+          end
  245
+        else
  246
+          options.merge!(:default_response => collector.default_response)
  247
+          (options.delete(:responder) || self.class.responder).call(self, resources, options)
  248
+        end
239 249
       end
240 250
     end
241 251
 
@@ -263,24 +273,29 @@ def collect_mimes_from_class_level #:nodoc:
263 273
     # Collects mimes and return the response for the negotiated format. Returns
264 274
     # nil if :not_acceptable was sent to the client.
265 275
     #
266  
-    def retrieve_response_from_mimes(mimes=nil, &block) #:nodoc:
  276
+    def retrieve_collector_from_mimes(mimes=nil, &block) #:nodoc:
267 277
       mimes ||= collect_mimes_from_class_level
268 278
       collector = Collector.new(mimes) { |options| default_render(options || {}) }
269 279
       block.call(collector) if block_given?
270 280
 
271  
-      if format = request.negotiate_mime(collector.order)
  281
+      if format = negotiated_format(collector)
272 282
         self.content_type ||= format.to_s
273 283
         lookup_context.freeze_formats([format.to_sym])
274  
-        collector.response_for(format)
  284
+        collector
275 285
       else
276 286
         head :not_acceptable
277 287
         nil
278 288
       end
279 289
     end
280 290
 
  291
+    def negotiated_format(collector)
  292
+      request.negotiate_mime(collector.order)
  293
+    end
  294
+
281 295
     class Collector #:nodoc:
282 296
       include AbstractController::Collector
283 297
       attr_accessor :order
  298
+      attr_reader :default_response
284 299
 
285 300
       def initialize(mimes, &block)
286 301
         @order, @responses, @default_response = [], {}, block
@@ -303,7 +318,7 @@ def custom(mime_type, &block)
303 318
       end
304 319
 
305 320
       def response_for(mime)
306  
-        @responses[mime] || @responses[Mime::ALL] || @default_response
  321
+        @responses[mime] || @responses[Mime::ALL]
307 322
       end
308 323
     end
309 324
   end
25  actionpack/test/controller/mime_responds_test.rb
@@ -597,6 +597,19 @@ def index
597 597
       format.json { render :json => RenderJsonTestException.new('boom') }
598 598
     end
599 599
   end
  600
+
  601
+  def create
  602
+    resource = ValidatedCustomer.new(params[:name], 1)
  603
+    respond_with(resource) do |format|
  604
+      format.json do
  605
+        if resource.errors.empty?
  606
+          render :json => { :valid => true }
  607
+        else
  608
+          render :json => { :valid => false }
  609
+        end
  610
+      end
  611
+    end
  612
+  end
600 613
 end
601 614
 
602 615
 class EmptyRespondWithController < ActionController::Base
@@ -968,6 +981,18 @@ def test_render_json_object_responds_to_str_still_produce_json
968 981
     assert_match(/"error":"RenderJsonTestException"/, @response.body)
969 982
   end
970 983
 
  984
+  def test_api_response_with_valid_resource_respect_override_block
  985
+    @controller = RenderJsonRespondWithController.new
  986
+    post :create, :name => "sikachu", :format => :json
  987
+    assert_equal '{"valid":true}', @response.body
  988
+  end
  989
+
  990
+  def test_api_response_with_invalid_resource_respect_override_block
  991
+    @controller = RenderJsonRespondWithController.new
  992
+    post :create, :name => "david", :format => :json
  993
+    assert_equal '{"valid":false}', @response.body
  994
+  end
  995
+
971 996
   def test_no_double_render_is_raised
972 997
     @request.accept = "text/html"
973 998
     assert_raise ActionView::MissingTemplate do
10  actionpack/test/lib/controller/fake_models.rb
@@ -34,6 +34,16 @@ class BadCustomer < Customer
34 34
 class GoodCustomer < Customer
35 35
 end
36 36
 
  37
+class ValidatedCustomer < Customer
  38
+  def errors
  39
+    if name =~ /Sikachu/i
  40
+      []
  41
+    else
  42
+      [{:name => "is invalid"}]
  43
+    end
  44
+  end
  45
+end
  46
+
37 47
 module Quiz
38 48
   class Question < Struct.new(:name, :id)
39 49
     extend ActiveModel::Naming

0 notes on commit 567ac65

Please sign in to comment.
Something went wrong with that request. Please try again.