Feature #34766
closedBetter error message when no API format is recognised
0%
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
Related issues
Updated by Pavel Rosický over 3 years ago
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.
Updated by Felix Schäfer over 3 years ago
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.
Updated by Go MAEDA over 3 years ago
- Target version set to Candidate for next major release
Updated by Go MAEDA almost 3 years ago
- Related to Feature #26709: Use correct http status codes added
Updated by Mizuki ISHIKAWA almost 3 years ago
- File add_test.patch add_test.patch added
I attach a patch to add the test.
Updated by Go MAEDA almost 3 years ago
- Target version changed from Candidate for next major release to 5.0.0
Setting the target version to 5.0.0.
Updated by Marius BĂLTEANU over 2 years ago
Instead of adding this error message, is not better to support API requests also with accept header?
Updated by Marius BĂLTEANU over 2 years ago
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.
Updated by Marius BĂLTEANU over 2 years ago
- Related to deleted (Feature #26709: Use correct http status codes)
Updated by Marius BĂLTEANU over 2 years ago
- Has duplicate Feature #26709: Use correct http status codes added
Updated by Marius BĂLTEANU over 2 years ago
- 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.