Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[rails 3.0.9] Crash after using respond_with {} in a POST request (create action) #1939

Closed
NoICE opened this Issue · 17 comments

2 participants

@NoICE
class MailsController < ApiController
  respond_to :json, :xml

  def create
    # ...
    @data = { :success => true, :item => @mail.to_api_hash } # to_api_hash creates simple hash, like { :id => 'aaa', :name => 'bbb' }
    respond_with @data
  end
end

results in:

Started POST "/api/mails.json" for 127.0.0.1 at Sat Jul 02 15:57:35 +0200 2011
  Processing by Api::MailsController#create as JSON

NoMethodError (undefined method `model_name' for NilClass:Class):
  app/controllers/api/mails_controller.rb:48:in `create'

Full trace:

activemodel (3.0.9) lib/active_model/naming.rb:95:in `model_name_from_record_or_class'
activemodel (3.0.9) lib/active_model/naming.rb:74:in `plural'
actionpack (3.0.9) lib/action_dispatch/routing/polymorphic_routes.rb:166:in `build_named_route_call'
actionpack (3.0.9) lib/action_dispatch/routing/polymorphic_routes.rb:107:in `polymorphic_url'
actionpack (3.0.9) lib/action_dispatch/routing/url_for.rb:133:in `url_for'
actionpack (3.0.9) lib/action_controller/metal/rendering.rb:50:in `_process_options'
actionpack (3.0.9) lib/action_controller/metal/renderers.rb:35:in `_handle_render_options'
actionpack (3.0.9) lib/action_controller/metal/renderers.rb:47:in `render_to_body'
actionpack (3.0.9) lib/action_controller/metal/compatibility.rb:55:in `render_to_body'
actionpack (3.0.9) lib/abstract_controller/rendering.rb:102:in `render_to_string'
actionpack (3.0.9) lib/abstract_controller/rendering.rb:93:in `render'
actionpack (3.0.9) lib/action_controller/metal/rendering.rb:17:in `render'
actionpack (3.0.9) lib/action_controller/metal/instrumentation.rb:40:in `render'
activesupport (3.0.9) lib/active_support/core_ext/benchmark.rb:5:in `ms'
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/benchmark.rb:308:in `realtime'
activesupport (3.0.9) lib/active_support/core_ext/benchmark.rb:5:in `ms'
actionpack (3.0.9) lib/action_controller/metal/instrumentation.rb:40:in `render'
actionpack (3.0.9) lib/action_controller/metal/instrumentation.rb:78:in `cleanup_view_runtime'
activerecord (3.0.9) lib/active_record/railties/controller_runtime.rb:15:in `cleanup_view_runtime'
actionpack (3.0.9) lib/action_controller/metal/instrumentation.rb:39:in `render'
actionpack (3.0.9) lib/action_controller/metal/responder.rb:211:in `display'
actionpack (3.0.9) lib/action_controller/metal/responder.rb:163:in `api_behavior'
actionpack (3.0.9) lib/action_controller/metal/responder.rb:138:in `to_format'
actionpack (3.0.9) lib/action_controller/metal/responder.rb:119:in `respond'
actionpack (3.0.9) lib/action_controller/metal/responder.rb:112:in `call'
actionpack (3.0.9) lib/action_controller/metal/mime_responds.rb:232:in `respond_with'
app/controllers/api/mails_controller.rb:48:in `create'

I know that respond_with should be used with ActiveModel or so, but according to documentation this should work.
I assume that respond_with is trying to get model_name from the hash and redirect to show action for it,
but it should not try to call model_name on the Hash.

It works on show, index, edit, etc., but for POST request it fails horribly like that.

I've got around for now by just calling old format block with explicit render :json and render :xml.

@dmathieu
Collaborator

This is because a POST request always redirects. Therefore, you need to provide a :location option to your respond_with.

respond_with @data, :location => root_url

This should work.
There might be a more explicit message though.

@NoICE

But... well, I don't need redirect, as I'm using it via cURL as API interface.
I just need JSON or XML to be spitted out, with no location header.

I still think that this method just shouldn't try to get URL from Hash. This is just wrong.

@dmathieu
Collaborator

I also think there is something wrong. Perhaps don't redirect but just render if there's no location.

What you can do in the mean time is create your own responder. That's very well explained in @josevalim's book.
And change the method api_behavior, specifically when it's a post method to remove the :location.
https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/responder.rb#L204

@NoICE

I got around this issue by just using plain old format block, till this is resolved, I'm happy with that for now.
But this surely need to get fixed, before someone's head explodes. :)

@dmathieu
Collaborator

All right, sorry for my previous comment. But I think this is not gonna be possible.
The HTTP protocol is quite clear about what the POST verb should render.

If a resource has been created on the origin server, the response SHOULD be 201 (Created) and contain an entity which describes the status of the request and refers to the new resource, and a Location header

This is exactly what responds_to does, rendering the resource with a location header.
On restful application, every created resource should have it's own URI. So the location header should be available.

There's not gonna be any change on that, unless the HTTP protocol changes ;)

You just have to provide a :location option to your respond_with, that is your fix.

@NoICE

Well... redirecting to another url is not a solution for me, I need resource status available and all it's parameters.
I could just rely on HTTP status code (which is surely right REST solution), but it involves doing another request (following that location).

Adding :location header solved this for me, as 201 header isn't auto-followed by cURL (which feels wrong at itself).

But one way or another, calling model_name on a Hash is still wrong. Should we just close the issue and let it be, or can I leave it open till someone prevents calling model name on Hash? :)

@dmathieu
Collaborator

There should be a more explicit error.

Actually, the fact that curl does not follow a Location header when the status is 201 is normal.
Redirect statuses are only 3xx. In any other case, there's no reason to redirect. Plain old HTTP protocol again.

@NoICE

I know I know, I ment it other way.
That the specification tells us that we "SHOULD" set Location header, but according to HTTP status rfc, It shouldn't be followed.

Seems like paradox.

Or it seems weird just to me?

I know that the agent then just "COULD" follow that Location or not, but it's unclear and weird.

@dmathieu
Collaborator

No, it's an other information, provided to the client, telling him "the URI of the new resource is xxx".
Having a 3xx status tells him "the URI is xxx. Now, go there".
That allows more flexibility.

@NoICE

Ok ok.

@dmathieu
Collaborator

See #1940

@NoICE

Thanks :)

@NoICE NoICE closed this
@NoICE

There is one issue unresolved though.

When the record couldn't be saved, I don't have its ID.
Which I can get around by sending status 400 and location to resource index, or maybe to some documentation.

Maybe passing the location shouldn't be forced, when programmer puts different status code, e.g. 400, or 200.
What do you think?

@dmathieu
Collaborator
@NoICE

That's OK and I like it, but I pass in a Hash.. :(

@dmathieu
Collaborator

Well, pass a hash with the errors ...

respond_with is meant to facilitate rendering in several formats for the most common case.
You seem to have something different. Perhaps you should either not use respond_with (respond_to is not gonna disappear), or try to stick to the convention.

@NoICE

Well, I searched around before I begun for some most common style of JSON API communications, but found none.

I'll just write my own responder then, which will not try to call method .errors on that hash, but instead just looks if there is a key named like that.

Thanks for your patience :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.