ActionController::UnknownFormat should return HTTP 406

[chancancode]

From what I can tell, we are supposed to rescue this by default and return a 406 (Not Acceptable). At least on Rails 4.1 and 4.2 this doesn't seem to be the case – I generated a fresh Rails app with a scaffold resource and ran Rails server in production. curl -H "Accept: text/xml" http://localhost:3000/posts and curl http://localhost:3000/posts.xml both gives me a 500 instead.

Would be great if someone can look into this 😉

[philipgiuliani]

In Rails 5 it is changed to return :no_content what i know. I dont think that this will be updated in Rails 4.1 / 4.2.

[chancancode]

You might be right! (#19036 / #19377)

But I think there is a difference. I believe the intention of #19036 is that if an action has no (0) templates at all, we should assume that it doesn't need one. In that case, 204 as a default seems fine.

But here, we are looking at an action with an HTML template. If the HTML version requires a template, to me it definitely doesn't make sense to render a blank page for XML and other formats, and I doubt that's @dhh's intention in #19036.

I haven't checked the code on master to see what it does, but if the change in #19377 changed this behaviour to 204 from 500, that seems bad and we should fix it.

[chancancode]

By the way, this is probably related to some existing tickets. The reason it is 500-ing is we ended up raising a ActionView::MissingTemplate instead of a ActionController::UnknownFormat at some point in this case.

[chancancode]

Also, there are actually two cases here:

1. curl -H "Accept: lol/i-dont-even" http://localhost:3000/posts / curl http://localhost:3000/posts.lol
2. curl -H "Accept: text/xml" http://localhost:3000/posts / curl http://localhost:3000/posts.xml

Both of these are currently 500-ing. Seems pretty clear that the first case should be a 406. I think the second case is supposed to/at some point returned a 406, but I could be completely wrong.

There are some tests and documentation that seems to suggest that, but I have not dug enough to be 100% sure.

e.g. #2313

I remember seeing @matthewd commenting on something similar before ;)

[dhh]

@chancancode Yes, I'd be good with 406 if either a) some of the formats have templates and implicitly render those or b) respond_to has been described, which should be an exhaustive list of all formats supported by the action.

[chancancode]

Yep, that sounds correct! Now we just have to fix it :)

[todd]

@chancancode The behavior for this issue on master seems to fall in line with what was added in #19377 - since this is the case, master should be updated to return a 406 for undefined action formats?

$ curl -H "Accept: text/xml" -I http://localhost:3000/posts
HTTP/1.1 204 No Content
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block
X-Content-Type-Options: nosniff
Cache-Control: no-cache
X-Request-Id: 4b87a212-f015-4135-90b7-9b0ed45cb68c
X-Runtime: 0.001523
Server: WEBrick/1.3.1 (Ruby/2.2.2/2015-04-13)
Date: Mon, 29 Jun 2015 23:00:59 GMT
Connection: Keep-Alive
Set-Cookie: request_method=HEAD; path=/

[chancancode]

Yeah, so I believe what we want is..

1. If an action has no templates defined at all, AND it has no respond_to { ... } block, then it should do a 204
2. If an action has certain templates defined, AND it has no respond_to { ... } block, then it should do a 406 for formats with no templates
3. If an action has a respond_to { ... } block, then it should do a 406 for formats not in the list

Does that make sense?

[chancancode]

...and by "406", I really mean "raise an ActionController::UnknownFormat error, catch it upstream and default to a 406 response but allow users to override that" (which, as far as I can tell, is how it used to work)

[todd]

Yeah, I think that all makes sense. I'll try to take a stab at this sometime within the next couple of days.

[chancancode]

@davetron5000 do you know anything about this? 😁

[davetron5000]

I do not. I added a feature to allow customizing what happens when there is no template so that libraries relying on the old behavior could opt back into it. See #20276

[jackturnbull]

@chancancode: What you propose in #20666 (comment) seems like expected behaviour to me.

In Rails 4.2.3, if I have:

# app/controllers/posts_controller.rb
class PostsController < ApplicationController
  respond_to :json

  def index
    @post = Post.all
  end
end

Then I get the 500-level ActionView::MissingTemplate error.

If you move that respond_to :json line from the application controller to the bottom of #index then it works as expected:

# app/controllers/posts_controller.rb
class PostsController < ApplicationController
  def index
    @post = Post.all
    respond_to :json
  end
end

This also works when you use respond_to blocks, a-la scaffold_controller#createstyle.

Our hot-fix for the moment has been to not define any class-level (or inherited) respond_to types, and move these down into individual methods.

It first came to light when we started getting a load of 500's reported on NewRelic when clients were requesting the wrong page so moving these to 400-level error would be very helpful.

[huoxito]

stumbled through it last week too, I think this could be a workaround in case you don't want to add per action respond_to (not sure the overhead it might cause though perhaps it's better to fix the issue before it hits rails server):

before_action :check_mime_types

respond_to :html

def check_mime_types
  mimes = collect_mimes_from_class_level()
  return if mimes.empty?

  collector = ActionController::MimeResponds::Collector.new(mimes, request.variant)
  unless collector.negotiate_format(request)
    raise ActionController::UnknownFormat 
  end
end

From what I could see ActionController::UnknownFormat is never raised unless you explicitly call respond_to at instance level. The difference I see in master now is if request a .xml you'd get a 204 instead of a 500 missing template response.

It feels like "a fix" should go to the responders gem not here given rails doesn't have respond_to at class level anymore. The format check here could could stay a bit behind on the stack but I don't know exactly how to go about it yet