Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
sikachu committed Feb 3, 2012
1 parent d709b12 commit 3def1c8
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 8 deletions.
2 changes: 2 additions & 0 deletions 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
Expand Down
31 changes: 23 additions & 8 deletions actionpack/lib/action_controller/metal/mime_responds.rb
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
25 changes: 25 additions & 0 deletions actionpack/test/controller/mime_responds_test.rb
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
10 changes: 10 additions & 0 deletions actionpack/test/lib/controller/fake_models.rb
Expand Up @@ -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
Expand Down

0 comments on commit 3def1c8

Please sign in to comment.