Feature #34766

Better error message when no API format is recognised

Added by Felix Schäfer over 1 year ago. Updated 6 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Marius BALTEANU% 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)

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


Related issues

Duplicated by Redmine - Feature #26709: Use correct http status codes Closed

Associated revisions

Revision 21392
Added by Marius BALTEANU 8 months ago

Return 406 status code instead of 500 when API request has an invalid format (#34766).

Patch by Felix Schäfer.

Revision 21393
Added by Marius BALTEANU 8 months ago

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 over 1 year ago

  • Target version set to Candidate for next major release

#5 Updated by Go MAEDA 9 months ago

#6 Updated by Mizuki ISHIKAWA 9 months ago

I attach a patch to add the test.

#7 Updated by Go MAEDA 9 months ago

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

Setting the target version to 5.0.0.

#8 Updated by Marius BALTEANU 8 months ago

Instead of adding this error message, is not better to support API requests also with accept header?

#9 Updated by Marius BALTEANU 8 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 8 months ago

#11 Updated by Marius BALTEANU 8 months ago

#12 Updated by Marius BALTEANU 8 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.

#13 Updated by Go MAEDA 6 months ago

  • Tracker changed from Patch to Feature

Also available in: Atom PDF