respond_with and Location header for failed 'create' actions

[szimek]

In my app I've got nested models and when the parent is successfully created, I'm redirecting to its child form. However there's a small problem with specifying :location param for respond_with in create actions like this one:

def create
  @program = Program.new(params[:program])
  @program.save
  respond_with(@program, {:location => new_program_product_path(@program))
end

The problem is that when @program.save fails, @program doesn't have an ID and thus generating the route raises an exception.

I assumed that in this particular case :location param is only used in successful response, as I don't think it really makes sense to return it with failed responses. If that was the case one could modify ActionController::Responder#resource_location to accept procs and they'd be evaluated only when the response is successful and object has an ID.

However, it turns out that Rails sets Location header even for failed responses. It looks like it just sets it always whenever there's a :location param - ActionController::Rendering#_process_options.

I'm not sure if it makes sense, but maybe it could be changed that in case of failed 'create' action Rails would not set Location header (e.g. remove it in ActionController::Responder) and one simply could do:

respond_with(@program, {:location => proc { new_program_product_path(@program) })

[synewaves]

In the meantime, I've started doing this in my controllers as a "fix" (to use your example):

def create
  @program = Program.new(params[:program])
  @saved = @program.save
  respond_with @program, :location => new_program_product_path(@program) if @saved
end

[jeremyf]

@szimek I believe that this behavior would be doing too much; Shouldn't you be checking if the object saved and directing accordingly?

@josevalim I believe this should be closed

(@jeremyf)

[jeremyf]

@jonleighton @tenderlove I believe this is pushing too much responsibility onto respond_with; The controller should tell respond_with what to do. Can this be closed?

[maletor]

If save fails it should return 422 with errors hash.

[erichmenge]

Why not:

def create
  @program = Program.new(params[:program])
  if @program.save
    respond_with @program, :location => new_program_product_path(@program)
  else
    respond_with @program, :location => nil
  end
end

You have to do stuff like this with your controllers that serve HTML anyway.

[szimek]

It's rather a question than bug report. If you think that it would be moving too much responsibility onto respond_with then it's ok to close this issue.

[carlosantoniodasilva]

The failed action actually does not use any :location, the error happens before this, when trying to generate the url to be passed to respond_with:

>> app.new_program_product_path Program.new
ActionController::RoutingError: No route matches {:action=>"new", :controller=>"products", :program_id=>#<Program id: nil, name: nil, created_at: nil, updated_at: nil>}

As you're redirecting to a nested route under the program resource, in this case you do have to check if it was saved to properly generate the url for :location, otherwise it'll fail during url generation as expected.

Thanks!

[szimek]

That's why I suggested accepting procs as location:

respond_with(@program, {:location => proc { new_program_product_path(@program) })

but I agree that it may be a bit too much :)

[carlosantoniodasilva]

Yeah, I'm not sure that adding proc support would be a good change to the respond_with :location option for this particular case.

@josevalim do you have any thoughts on this?

[akshayrawat]

Just stumbled upon this. Need this too. Yes procs are a good idea to support basic use cases of responding to nested resources.

responds_with should either let go of all routing decision making, or implement it for nested resources.

[dmathieu]

We currently do the following :

respond_with @program, location: @program.valid? ? @program : nil

I'd actually prefer to have a proc.

[akshayrawat]

@carlosantoniodasilva
@josevalim

Any thoughts? I could make a quick pull request if you guys agree about it conceptually.

[carlosantoniodasilva]

I think most of the time a proc is not going to be required, I still don't feel the need for it.

@dmathieu in your case you shouldn't need to give @program to :location, it should use the given object instead. If it's not, then we might have a problem :)

The only case that seems to be required is the one in the issue itself, where it's required to redirect to some nested url after the object has been saved, which, in turn, could be solved simply with:

respond_with @program, location: @program.save && new_program_product_path(@program)
# or
location = new_program_product_path(@program) if @program.save
respond_with @program, location: location

We don't need to add anything new to Rails to handle that with our current tools, in a way that looks quite ok to me.
Thanks!