Feature #34766
Better error message when no API format is recognised
Status: | Closed | Start date: | ||
---|---|---|---|---|
Priority: | Normal | Due date: | ||
Assignee: | % Done: | 0% | ||
Category: | REST API | |||
Target version: | 5.0.0 | |||
Resolution: |
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)
Related issues
Associated revisions
Return 406 status code instead of 500 when API request has an invalid format (#34766).
Patch by Felix Schäfer.
Add test for #34766.
Patch by Mizuki ISHIKAWA.
History
#1
Updated by Pavel Rosický over 1 year 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.
#2
Updated by Felix Schäfer over 1 year 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.
#3
Updated by Go MAEDA over 1 year ago
- Category set to REST API
#4
Updated by Go MAEDA about 1 year ago
- Target version set to Candidate for next major release
#5
Updated by Go MAEDA 5 months ago
- Related to Feature #26709: Use correct http status codes added
#6
Updated by Mizuki ISHIKAWA 5 months ago
- File add_test.patch
added
I attach a patch to add the test.
#8
Updated by Marius BALTEANU 4 months ago
Instead of adding this error message, is not better to support API requests also with accept header?
#9
Updated by Marius BALTEANU 4 months 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.
#10
Updated by Marius BALTEANU 4 months ago
- Related to deleted (Feature #26709: Use correct http status codes)
#11
Updated by Marius BALTEANU 4 months ago
- Duplicated by Feature #26709: Use correct http status codes added
#12
Updated by Marius BALTEANU 4 months ago
- Status changed from New to Closed
- Assignee set to Marius BALTEANU
Both patches committed, thanks!
I've created #36544 to allow the "Accept" header as well.