Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Align error! method signatures across different places. #2468

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

numbata
Copy link
Contributor

@numbata numbata commented Jul 3, 2024

This pull request fix a compatibility issue with the error! method when used in rescue_from block.

Previously, the rescue_from block ran in context of Grape::Middleware::Error, where error! method had one signature. Now, block run in context of Grape::DSL::InsideRoute, where error! method has different signature. This change cause runtime errors when error! method was called with extra arguments like backtrace and original_exception.

This update make error! method signatures same across different contexts. By aligning these signatures, error! method can handle additional arguments consistently and preventing errors.

@numbata
Copy link
Contributor Author

numbata commented Jul 3, 2024

The 2eabc9b is on top the commit before the 2377 PR was merged. And it passes now.

@numbata numbata force-pushed the align_error_methods_signature branch from 2eabc9b to 1a8899d Compare July 3, 2024 12:42
@numbata
Copy link
Contributor Author

numbata commented Jul 3, 2024

The branch was rebased from the master and the spec are failing now with:

$ bundle exec rspec spec/grape/api_spec.rb:2541
Run options: include {:locations=>{"./spec/grape/api_spec.rb"=>[2541]}}

Randomized with seed 55390

Grape::API
  .error_format
    class
      returns a modified error  with a custom error format (FAILED - 1)

Failures:

  1) Grape::API.error_format class returns a modified error  with a custom error format
     Failure/Error:
       def error!(message, status = nil, additional_headers = nil)
         status = self.status(status || namespace_inheritable(:default_error_status))
         headers = additional_headers.present? ? header.merge(additional_headers) : header
         throw :error, message: message, status: status, headers: headers

     ArgumentError:
       wrong number of arguments (given 5, expected 1..3)
     # ./lib/grape/dsl/inside_route.rb:168:in `error!'
     # ./spec/grape/api_spec.rb:2538:in `block (5 levels) in <top (required)>'
     # ./lib/grape/middleware/error.rb:120:in `instance_exec'
     # ./lib/grape/middleware/error.rb:120:in `block in run_rescue_handler'
     # ./lib/grape/middleware/error.rb:119:in `catch'
     # ./lib/grape/middleware/error.rb:119:in `run_rescue_handler'
     # ./lib/grape/middleware/error.rb:36:in `rescue in call!'
     # ./lib/grape/middleware/error.rb:32:in `call!'
     # ./lib/grape/middleware/base.rb:27:in `call'
     # ./lib/grape/endpoint.rb:225:in `call!'
     # ./lib/grape/endpoint.rb:219:in `call'
     # ./lib/grape/router/route.rb:19:in `exec'
     # ./lib/grape/router.rb:121:in `process_route'
     # ./lib/grape/router.rb:68:in `block in identity'
     # ./lib/grape/router.rb:101:in `transaction'
     # ./lib/grape/router.rb:66:in `identity'
     # ./lib/grape/router.rb:50:in `block in call'
     # ./lib/grape/router.rb:137:in `with_optimization'
     # ./lib/grape/router.rb:49:in `call'
     # ./lib/grape/api/instance.rb:164:in `call'
     # ./lib/grape/api/instance.rb:68:in `call!'
     # ./lib/grape/api/instance.rb:63:in `call'
     # ./lib/grape/api.rb:78:in `call'
     # ./spec/grape/api_spec.rb:2544:in `block (4 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # RuntimeError:
     #   rain!
     #   ./spec/grape/api_spec.rb:2542:in `block (5 levels) in <top (required)>'

Finished in 0.01702 seconds (files took 0.56084 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/grape/api_spec.rb:2536 # Grape::API.error_format class returns a modified error  with a custom error format

Randomized with seed 55390

Coverage report generated for RSpec to ./grape/coverage. 2039 / 3670 LOC (55.56%) covered.
Stopped processing SimpleCov as a previous error not related to SimpleCov has been detected

@numbata numbata force-pushed the align_error_methods_signature branch from 1a8899d to 9a551b1 Compare July 3, 2024 12:56
@numbata numbata changed the title Align the error! helper methods Align error! method signatures across different places to fix compatibility issues. Jul 3, 2024
@numbata numbata force-pushed the align_error_methods_signature branch from 61cd18a to 26db6d6 Compare July 3, 2024 14:16
@numbata numbata marked this pull request as ready for review July 3, 2024 14:46
@numbata numbata force-pushed the align_error_methods_signature branch from 26db6d6 to 8cde122 Compare July 3, 2024 14:47
@numbata numbata changed the title Align error! method signatures across different places to fix compatibility issues. Align error! method signatures across different places. Jul 3, 2024
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@dblock dblock merged commit 5affa8f into ruby-grape:master Jul 3, 2024
52 checks passed
@numbata
Copy link
Contributor Author

numbata commented Jul 8, 2024

@dblock any plans for release this?

@dblock
Copy link
Member

dblock commented Jul 8, 2024

I can do it this week unless @ericproulx or another maintainer wants to beat me to it?

@numbata
Copy link
Contributor Author

numbata commented Jul 12, 2024

Just a quick reminder!
The Renovate bot is eager to update our dependencies (including grape), but we're holding off until these changes will be published.

@dblock
Copy link
Member

dblock commented Jul 12, 2024

Andrei, it will get done when it will get done. No reminders needed. Your company is welcome to hire any of the maintainers of grape to do it on your schedule, or at least sponsor the project.

@numbata
Copy link
Contributor Author

numbata commented Jul 12, 2024

Andrei, it will get done when it will get done.

No pressure! And I would be happy to hire or sponsor any or all of the maintainers, but I hope I can contribute in other ways as well. By my PR's for example.

@numbata
Copy link
Contributor Author

numbata commented Jul 12, 2024

Off topic about sponsoring the project: I was looking for an easy way to enter my credit card information and set up a monthly "buy a couple of coffees for grape maintainers" donation. But it's not very clear to me where to do that on the Tidelift page. So I just gave up.

@dblock
Copy link
Member

dblock commented Jul 12, 2024

Appreciate it. I do have a sponsor profile :)

@ericproulx
Copy link
Contributor

@numbata Grape 2.1.3 is out :) Thank you for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants