playframework / play1

Play framework
https://www.playframework.com/documentation/1.4.x/home
Other
1.58k stars 684 forks source link

Wrong HTTP status (500) when content negotiation fails #1364

Open cies opened 3 years ago

cies commented 3 years ago

Play1 has some content negotiation built in. For incoming requests it will try to establish the required return content type based on request header (Accept) and possibly also the extension of the last path segment.

In order to render the content in the requested content type, Play1 will look for a template with a certain extension. If it cannot find that template, it generates a 500 response and puts the following in the logs:

play.mvc.Controller.renderTemplate(Controller.java:813)
play.mvc.Controller.renderTemplate(Controller.java:782)
play.mvc.Controller.render(Controller.java:843)
controllers.SomeController.show(SomeController.java:42)
@7l7kg8b6p Internal Server Error (500) for request GET /some_controller/1234
  Template not found (In {unknown source file.  appclass=controllers.SomeController (compiled:true)} around line 42)
  The template SomeController/show.txt does not exist.

The client will see this:

curl -H "Accept: text/plain" https://example.com/some_controller/1234
Template not found 
The template SomeController/show.txt does not exist.

This behavior from Play1 makes my app very 500 happy. And I want 500s to be reserved for situation where either (a) we made a mistake programming the application, or (b) a service our app depends on is not available.

I'd argue that these 500s should be reclassified as (this is what I expect as behavior):

500s in this case are a bit too heavy handed.

Maybe there is a way to mitigate this that I'm not aware of. As it looks I'd need to put guards in many of my controllers to prevent these "bad requests" to become 500s. Which leads me to the conclusion that 500s are the wrong default in this case.

I'd gladly submit a PR fixing this, but I'd like to know first if that'd be something the core contributors are interested in. And on top of that we need to figure out what the new status will be (400, 404, or something else) and if this then still requires any logging.

cies commented 2 months ago

More infor on how we have not yet fixed this in RePlay can be found here: https://github.com/replay-framework/replay/issues/224