Feature #34766
closed
Better error message when no API format is recognised
Added by Felix Schäfer almost 4 years ago.
Updated over 2 years ago.
Description
At Planio sometimes see users trying to make API requests using the HTTP Accept: application/json
header but forgetting to add .json
at the end of the path. This currently leads to a raise
with a 500 Internal Server Error
.
We propose the following patch to correctly return a 406 Not Acceptable
(this is the same HTTP status Rails uses when the passed Accept
HTTP header can not be served), log the error instead of raise
-ing, and returning a helpful message in the HTTP response.
diff --git a/lib/redmine/views/builders.rb b/lib/redmine/views/builders.rb
index 653c1bd6b..3c977df67 100644
--- a/lib/redmine/views/builders.rb
+++ b/lib/redmine/views/builders.rb
@@ -30,7 +30,9 @@ module Redmine
when 'xml', :xml then Builders::Xml.new(request, response)
when 'json', :json then Builders::Json.new(request, response)
else
- raise "No builder for format #{format}"
+ Rails.logger.error "No builder for format #{format.inspect}"
+ response.status = 406
+ return "We couldn't handle your request, sorry. If you were trying to access the API, make sure to append .json or .xml to your request URL.\n"
end
if block_given?
yield(builder)
Files
well, I had the same proposal 3 years ago https://redmine.org/issues/26709
I don't see a reason why not to provide a JSON response if "Accept: application/json" header is present. I agree, that the application shouldn't raise 500, 406 is definitely more appropriate.
Pavel Rosický wrote:
well, I had the same proposal 3 years ago https://redmine.org/issues/26709
Thank you for pointing this out, I was not aware this had already been worked on, I am sorry for overlooking it.
I don't see a reason why not to provide a JSON response if "Accept: application/json" header is present.
Providing a JSON or XML response when the Accept
header is set would also be coherent with the way Rails handles requests I think. This would be my preferred solution, but I am unsure whether not accepting the Accept
header in Redmine::Builders
was a conscious decision or not.
I agree, that the application shouldn't raise 500, 406 is definitely more appropriate.
This would indeed be the smallest possible change that would avoid raising an error in the logs/in the application and improving the HTTP response for users.
- Target version set to Candidate for next major release
I attach a patch to add the test.
- Target version changed from Candidate for next major release to 5.0.0
Setting the target version to 5.0.0.
Instead of adding this error message, is not better to support API requests also with accept header?
Marius BALTEANU wrote:
Instead of adding this error message, is not better to support API requests also with accept header?
Digging more in this and #26709, I think this change is useful regardless the accept header.
- Status changed from New to Closed
- Assignee set to Marius BĂLTEANU
Both patches committed, thanks!
I've created #36544 to allow the "Accept" header as well.
- Tracker changed from Patch to Feature
Also available in: Atom
PDF