Skip to content
Browse files

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

Fix override API response bug in respond_with
  • Loading branch information...
2 parents b1faa35 + 3def1c8 commit 776a3736e4a536ec32a82c26f39ca68cee9aeebe @josevalim josevalim committed Feb 4, 2012
View
2 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
View
31 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
View
25 actionpack/test/controller/mime_responds_test.rb
@@ -593,6 +593,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
@@ -964,6 +977,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
View
10 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

0 comments on commit 776a373

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