Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix override API response bug in respond_with #4869

Merged
merged 1 commit into from

2 participants

Prem Sichanugrist José Valim
Prem Sichanugrist
Collaborator

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.

See #4796. This is for master.

José Valim
Owner

I don't think we should even call the responder. We should just yield the block, as in respond_to.

Prem Sichanugrist
Collaborator

Ah yeah, you're right. Fixing that now.

José Valim
Owner

retrieve_response_from_mimes could probably return the collector and then we ask it directly if there is a response (and invoke) or fallback to the default one (with the responder).

Prem Sichanugrist sikachu 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
3def1c8
Prem Sichanugrist
Collaborator

Code has been updated.

José Valim josevalim commented on the diff
actionpack/lib/action_controller/metal/mime_responds.rb
((6 lines not shown))
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)
José Valim Owner

Why do we need this exactly?

Prem Sichanugrist Collaborator
sikachu added a note

There's a functionality that you can pass in :action => 'some_action' to respond_with block, and the responder will ignore the response block given and render that action instead. I have to retain this functionality so that the test for this still pass.

  def using_resource_with_action
    respond_with(resource, :action => :foo) do |format|
      format.html { raise ActionView::MissingTemplate.new([], "bar", ["foo"], {}, false) }
    end
  end

#--

  def test_using_resource_with_action
    @controller.instance_eval do
      def render(params={})
        self.response_body = "#{params[:action]} - #{formats}"
      end
    end

    errors = { :name => :invalid }
    Customer.any_instance.stubs(:errors).returns(errors)

    post :using_resource_with_action
    assert_equal "foo - #{[:html].to_s}", @controller.response.body
  end
José Valim Owner

Awesome, thanks for the explanation. Meeeeerging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
José Valim josevalim merged commit 776a373 into from
Prem Sichanugrist
Collaborator

Yay, don't forget #4870 for 3-2-stable as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 3, 2012
  1. Prem Sichanugrist

    Fix override API response bug in respond_with

    sikachu authored
    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
This page is out of date. Refresh to see the latest.
2  actionpack/CHANGELOG.md
View
@@ -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
31 actionpack/lib/action_controller/metal/mime_responds.rb
View
@@ -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)
José Valim Owner

Why do we need this exactly?

Prem Sichanugrist Collaborator
sikachu added a note

There's a functionality that you can pass in :action => 'some_action' to respond_with block, and the responder will ignore the response block given and render that action instead. I have to retain this functionality so that the test for this still pass.

  def using_resource_with_action
    respond_with(resource, :action => :foo) do |format|
      format.html { raise ActionView::MissingTemplate.new([], "bar", ["foo"], {}, false) }
    end
  end

#--

  def test_using_resource_with_action
    @controller.instance_eval do
      def render(params={})
        self.response_body = "#{params[:action]} - #{formats}"
      end
    end

    errors = { :name => :invalid }
    Customer.any_instance.stubs(:errors).returns(errors)

    post :using_resource_with_action
    assert_equal "foo - #{[:html].to_s}", @controller.response.body
  end
José Valim Owner

Awesome, thanks for the explanation. Meeeeerging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ 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
25 actionpack/test/controller/mime_responds_test.rb
View
@@ -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
10 actionpack/test/lib/controller/fake_models.rb
View
@@ -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
Something went wrong with that request. Please try again.