Patch #34766

Better error message when no API format is recognised

Added by Felix Schäfer 11 months ago. Updated 19 days ago.

Status:NewStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:REST API
Target version:5.0.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)

add_test.patch Magnifier (987 Bytes) Mizuki ISHIKAWA, 2022-01-04 03:42


Related issues

Related to Redmine - Patch #26709: Use correct http status codes New

History

#1 Updated by Pavel Rosický 11 months 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 11 months 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 11 months ago

  • Category set to REST API

#4 Updated by Go MAEDA 11 months ago

  • Target version set to Candidate for next major release

#5 Updated by Go MAEDA 20 days ago

  • Related to Patch #26709: Use correct http status codes added

#6 Updated by Mizuki ISHIKAWA 20 days ago

I attach a patch to add the test.

#7 Updated by Go MAEDA 19 days ago

  • Target version changed from Candidate for next major release to 5.0.0

Setting the target version to 5.0.0.

Also available in: Atom PDF