Patch #34766

Better error message when no API format is recognised

Added by Felix Schäfer 5 months ago. Updated 5 months ago.

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

0%

Category:REST API
Target version:Candidate for next major release

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)

History

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

  • Category set to REST API

#4 Updated by Go MAEDA 5 months ago

  • Target version set to Candidate for next major release

Also available in: Atom PDF