Skip to content
Browse files

Return a valid empty JSON on successful PUT and DELETE requests. [#5199

… state:resolved]

Signed-off-by: José Valim <jose.valim@gmail.com>
  • Loading branch information...
1 parent 582a088 commit 0d3333257156544feba729ba28f6874d5a30d561 @szimek szimek committed with josevalim
View
20 actionpack/lib/action_controller/metal/responder.rb
@@ -161,6 +161,8 @@ def api_behavior(error)
display resource.errors, :status => :unprocessable_entity
elsif post?
display resource, :status => :created, :location => api_location
+ elsif has_empty_resource_definition?

How about adding another condition check if resource is nil or not
if resource and status is not nil, that means the caller explicitly set the response data? e.g.

    def api_behavior(error)
      raise error unless resourceful?

      if get?
        display resource
      elsif has_errors?
        display resource.errors, :status => :unprocessable_entity
      elsif !resource.nil?
        display resource, :status => :ok, :location => api_location
      elsif post?
        display resource, :status => :created, :location => api_location
      elsif has_empty_resource_definition?
        display empty_resource, :status => :ok
      else
        head :ok
      end
    end

By default, put/delete don't return any resources.
But sometime we still need to override the default and receive and return a modified version of resources (incase there are any special logic in the model)

@josevalim Ruby on Rails member

The resources are available in those cases, we simply don't want to render them back. You should fix this by passing a block to respond_with or by using a custom responder.

I couldn't find any clean code to handle this. So even the controller has a respond_to :json in controller, we still have to use the block syntax?
Do you mean with the following two options?

first option:

respond_with @user do |format|
   format.json { render :json => @user.to_json(:includes=>:entry), :status => :accepted }
end

second option:

respond_to do |format|
  format.json { render :json => @user.to_json(:includes=>:entry), :status => :accepted }
end

Or do you have better way to handle this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ display empty_resource, :status => :ok
else
head :ok
end
@@ -221,5 +223,23 @@ def has_errors?
def default_action
@action ||= ACTIONS_FOR_VERBS[request.request_method_symbol]
end
+
+ # Check whether resource needs a specific definition of empty resource to be valid
+ #
+ def has_empty_resource_definition?
+ respond_to?("empty_#{format}_resource")
+ end
+
+ # Delegate to proper empty resource method
+ #
+ def empty_resource
+ send("empty_#{format}_resource")
+ end
+
+ # Return a valid empty JSON resource
+ #
+ def empty_json_resource
+ "{}"
+ end
end
end
View
19 actionpack/test/controller/mime_responds_test.rb
@@ -709,6 +709,15 @@ def test_using_resource_for_put_with_xml_yields_ok_on_success
assert_equal " ", @response.body
end
+ def test_using_resource_for_put_with_json_yields_ok_on_success
+ Customer.any_instance.stubs(:to_json).returns('{"name": "David"}')
+ @request.accept = "application/json"
+ put :using_resource
+ assert_equal "application/json", @response.content_type
+ assert_equal 200, @response.status
+ assert_equal "{}", @response.body
+ end
+
def test_using_resource_for_put_with_xml_yields_unprocessable_entity_on_failure
@request.accept = "application/xml"
errors = { :name => :invalid }
@@ -739,6 +748,16 @@ def test_using_resource_for_delete_with_xml_yields_ok_on_success
assert_equal " ", @response.body
end
+ def test_using_resource_for_delete_with_json_yields_ok_on_success
+ Customer.any_instance.stubs(:to_json).returns('{"name": "David"}')
+ Customer.any_instance.stubs(:destroyed?).returns(true)
+ @request.accept = "application/json"
+ delete :using_resource
+ assert_equal "application/json", @response.content_type
+ assert_equal 200, @response.status
+ assert_equal "{}", @response.body
+ end
+
def test_using_resource_for_delete_with_html_redirects_on_failure
with_test_route_set do
errors = { :name => :invalid }

13 comments on commit 0d33332

@sikachu
Ruby on Rails member

+1 for this commit. It was a bit pain that jQuery doesn't take head :ok as a success response.

@dmathieu

What's the rationale of returning an empty hash ? Couldn't it return the resource, like we do in POST ?

@josevalim
Ruby on Rails member
@dmathieu

We use the data returned by the PUT request to update our UI (some parameters are calculated, so we can't just get them from the form).
With the request returning an empty hash, we need to, either patch the responder, or make a second GET request.

Would there be a possibility to change that default to return an empty hash for PUT requests ?
I think we probably aren't the only ones doing something similar.

@josevalim
Ruby on Rails member

In general we keep the responder the same implementation as the scaffold. This allows us to say: replace respond_to by respond_with and everything will work exactly the same. If we want to change this on the responder, we should start a discussion on the mailing list or Campfire about changing scaffold to return the resource on update. Otherwise, you should be able to easily inherit from the the Rails responder and modify it to suit your application better.

@dmathieu

Yup, inheriting from the rails responder and override the behavior is what we currently do. Thank you, will start a discussion.

@jeremy
Ruby on Rails member

Seems the PUT response should be head :no_content, not an empty JSON object. What's that mean?

@sikachu
Ruby on Rails member
@josevalim
Ruby on Rails member

I am happy with whatever result that doesn't make jquery freak out. That said, will jquery work fine by default with 204? Or will it try to parse an empty body?

@sakuro

JSON must be an object or an array. Bare (empty) strings are not valid for JSON's sense.

@jeremy
Ruby on Rails member

Yes, "" is not valid JSON. We have application/json content type but an invalid body.

But rather than fix that by returning an empty JSON object, which is valid but makes no sense, why no return 204 No Content (with no Content-Type header)?

@josevalim
Ruby on Rails member
@sakuro

This gist serves /test/test action and sends head no_content.

rails new testapp -m https://raw.github.com/gist/1312547/5ab6a104d7a125bba5cb1b1d7f59a4a03f857e1e/gistfile1.rb
cd testapp
script/rails s

open browser's javascript console and click links one kicks $.getJSON and another does $.ajax.

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