From 567ac65b423c30c24aa6c0c0522858e3c240eb26 Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Fri, 3 Feb 2012 11:47:47 -0500 Subject: [PATCH] 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 --- actionpack/CHANGELOG.md | 2 ++ .../action_controller/metal/mime_responds.rb | 31 ++++++++++++++----- .../test/controller/mime_responds_test.rb | 25 +++++++++++++++ actionpack/test/lib/controller/fake_models.rb | 10 ++++++ 4 files changed, 60 insertions(+), 8 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 1e446ad0befb9..398cdabca2846 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,5 +1,7 @@ ## Rails 3.2.2 (unreleased) ## +* Default responder will now always use your overridden block in `respond_with` to render your response. *Prem Sichanugrist* + * 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. This is a behavior change, previously the hidden tag had a value of the disabled checkbox. *Tadas Tamosauskas* diff --git a/actionpack/lib/action_controller/metal/mime_responds.rb b/actionpack/lib/action_controller/metal/mime_responds.rb index ca383be76b1ae..819bbb5463bf6 100644 --- a/actionpack/lib/action_controller/metal/mime_responds.rb +++ b/actionpack/lib/action_controller/metal/mime_responds.rb @@ -191,7 +191,8 @@ def clear_respond_to 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 @@ def respond_with(*resources, &block) 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 @@ def collect_mimes_from_class_level #: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 @@ def custom(mime_type, &block) 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 76a8c89e60845..7c4fb59c15a03 100644 --- a/actionpack/test/controller/mime_responds_test.rb +++ b/actionpack/test/controller/mime_responds_test.rb @@ -597,6 +597,19 @@ def index 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 @@ -968,6 +981,18 @@ def test_render_json_object_responds_to_str_still_produce_json 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 f2362714d70bc..bbb4cc5ef3149 100644 --- a/actionpack/test/lib/controller/fake_models.rb +++ b/actionpack/test/lib/controller/fake_models.rb @@ -34,6 +34,16 @@ class BadCustomer < Customer 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