Passing status to the ErrorFormatter #2528

drewnichols · 2025-01-28T06:34:51Z

The status paramter is not passed into the format_message method here in error!:

Lines 132 to 137 in 5ce44de

    
           def error!(message, status = options[:default_status], headers = {}, backtrace = [], original_exception = nil) 
        
             rack_response( 
        
               status, headers.reverse_merge(Rack::CONTENT_TYPE => content_type), 
        
               format_message(message, backtrace, original_exception) 
        
             ) 
        
           end

Passing it allow it to be included in the response body JSON. This is the way it's included in the JSON API error response bodies:

# HTTP/1.1 422 Unprocessable Entity
# Content-Type: application/vnd.api+json

{
  "errors": [
    {
      "status": "422",
      "source": { "pointer": "/data/attributes/firstName" },
      "title":  "Invalid Attribute",
      "detail": "First name must contain at least two characters."
    }
  ]
}

…ible to ErrorFormatter children

dblock

Looks good, fix tests, likely needs documentation in https://github.com/ruby-grape/grape?tab=readme-ov-file#exception-handling and see if we can add lower level tests that a custom formatter for this.

Check failing tests for whether there's a non-backwards-compatible scenario, we don't want that.

dblock · 2025-01-28T09:30:56Z

lib/grape/error_formatter/base.rb

@@ -4,7 +4,7 @@ module Grape
  module ErrorFormatter
    class Base
      class << self
-        def call(message, backtrace, options = {}, env = nil, original_exception = nil)
+        def call(message, backtrace, options = {}, env = nil, original_exception = nil, _status = nil) # rubocop:disable Metrics/ParameterLists


Use rubocop -a ; rubocop --auto-gen-config instead of an inline #rubocop:disable, we have a lot of violations.

done, though I would probably just update the Metrics/ParameterLists settings in .rubocop.yml as 5 is a really small number limit parameter count.

CHANGELOG.md

Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>

dblock

Question below.

If the answer is yes, we need a backwards compatible implementation, maybe passing status within options?

dblock · 2025-01-28T18:42:50Z

README.md

@@ -2772,8 +2772,8 @@ Custom error formatters for existing and additional types can be defined with a

 ```ruby
 class Twitter::API < Grape::API
-  error_formatter :txt, ->(message, backtrace, options, env, original_exception) {
-    "error: #{message} from #{backtrace}"
+  error_formatter :txt, ->(message, backtrace, options, env, original_exception, status) {


Is this a breaking change? Meaning if I had the old code and upgrade Grape, will this fail with an invalid number of parameters?

Best I could tell this calls Grape::Middleware::Error.format_message where I defaulted status to nil. If someone created a custom Middleware there could be a problem. Fairpoint though.. I can check this with my own app and also look into making some more specs to verify.

Since we document custom middleware this would be a breaking change :(

dblock

Let's discuss @ericproulx's comment on this interface in #2527 (comment). If we are going to introduce a breaking change, I think we want something future-proof.

The other option is to add status to the local scope, and not change this interface, which seems desirable.

dblock · 2025-01-30T08:41:42Z

README.md

@@ -2772,8 +2772,8 @@ Custom error formatters for existing and additional types can be defined with a

 ```ruby
 class Twitter::API < Grape::API
-  error_formatter :txt, ->(message, backtrace, options, env, original_exception) {
-    "error: #{message} from #{backtrace}"
+  error_formatter :txt, ->(message, backtrace, options, env, original_exception, status) {


Since we document custom middleware this would be a breaking change :(

drewnichols · 2025-02-10T22:04:20Z

Closing this PR. With the fix in #2533 we can now access the HTTP status from the env argument to CustomFormatter.call(..) per this #2527 (comment).

dblock · 2025-02-11T18:30:17Z

Thanks for hanging in here with us @drewnichols! I still wish we had a local scope for request and/or response here.

drewnichols · 2025-02-11T18:33:26Z

No worries, as this would be a breaking change it makes sense to do it in a major version update and put more thought into it to allow for future extensibility.

drewnichols force-pushed the make-http-status-accessible-to-error-formatters branch from e8f0495 to cb87e68 Compare January 28, 2025 06:37

added http status to the format_error parameter list making it access…

d05f0ec

…ible to ErrorFormatter children

drewnichols force-pushed the make-http-status-accessible-to-error-formatters branch from cb87e68 to d05f0ec Compare January 28, 2025 06:48

drewnichols changed the title ~~added http status to the format_error parameter list making it access…~~ Passing status to the ErrorFormatter Jan 28, 2025

drewnichols marked this pull request as ready for review January 28, 2025 06:51

dblock requested changes Jan 28, 2025

View reviewed changes

drewnichols and others added 2 commits January 28, 2025 09:34

fixed CHANGELOG.md tense

5c868d4

Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>

moving rubocop violations to the todo list

b5d5cae

dblock reviewed Jan 28, 2025

View reviewed changes

fixing spec

8694586

dblock requested changes Jan 30, 2025

View reviewed changes

drewnichols closed this Feb 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Passing status to the ErrorFormatter #2528

Passing status to the ErrorFormatter #2528

drewnichols commented Jan 28, 2025 •

edited

Loading

dblock left a comment •

edited

Loading

dblock Jan 28, 2025

drewnichols Jan 28, 2025 •

edited

Loading

dblock left a comment

dblock Jan 28, 2025

drewnichols Jan 28, 2025

dblock Jan 30, 2025

dblock left a comment

dblock Jan 30, 2025

drewnichols commented Feb 10, 2025

dblock commented Feb 11, 2025 •

edited

Loading

drewnichols commented Feb 11, 2025 •

edited

Loading

	def error!(message, status = options[:default_status], headers = {}, backtrace = [], original_exception = nil)
	rack_response(
	status, headers.reverse_merge(Rack::CONTENT_TYPE => content_type),
	format_message(message, backtrace, original_exception)
	)
	end

Passing status to the ErrorFormatter #2528

Passing status to the ErrorFormatter #2528

Conversation

drewnichols commented Jan 28, 2025 • edited Loading

dblock left a comment • edited Loading

Choose a reason for hiding this comment

dblock Jan 28, 2025

Choose a reason for hiding this comment

drewnichols Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

dblock Jan 28, 2025

Choose a reason for hiding this comment

drewnichols Jan 28, 2025

Choose a reason for hiding this comment

dblock Jan 30, 2025

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

dblock Jan 30, 2025

Choose a reason for hiding this comment

drewnichols commented Feb 10, 2025

dblock commented Feb 11, 2025 • edited Loading

drewnichols commented Feb 11, 2025 • edited Loading

drewnichols commented Jan 28, 2025 •

edited

Loading

dblock left a comment •

edited

Loading

drewnichols Jan 28, 2025 •

edited

Loading

dblock commented Feb 11, 2025 •

edited

Loading

drewnichols commented Feb 11, 2025 •

edited

Loading