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

Fix absence of original_exception and/or backtrace even if passed in error! #2471

Conversation

numbata
Copy link
Contributor

@numbata numbata commented Jul 15, 2024

Fix the case where backtrace and/or original_exception arguments are passed, but then do not become part of the returned message, even if backtrace: true and/or original_exception: true are set at the rescue_from level.

Fixes #2470

Thanks to @ericproulx for pointing out the problem and provided specs.

@numbata numbata changed the title Fixes #2470 Expose original_exception and/or backtrace if present Fix absence of original_exception and/or backtrace even if passed in error! Jul 15, 2024
@ericproulx
Copy link
Contributor

@dblock anything to add ?

@ericproulx
Copy link
Contributor

ericproulx commented Jul 15, 2024

The only think that bugs me is the error? function. It looks for an hash with specific keys. Somehow, I think we could throw a more concrete object like a class or a struct instead of an hash. Something Like

Error = Struct.new(:message, :status, :headers, :backtrace, :original_exception)

....
throw Errow.new(message, status, headers, backtrace, original_exception)
...

case response
when Error
when Rack::Response
else
end

But this is another discussion 😛

@numbata
Copy link
Contributor Author

numbata commented Jul 15, 2024

The only think that bugs me is the error? function.

Totally agree. And we can do something around that (in separate PR ofc), but I have fear that will be breaking changes.

@ericproulx
Copy link
Contributor

The only think that bugs me is the error? function.

Totally agree. And we can do something around that (in separate PR ofc), but I have fear that will be breaking changes.

If its only for internal/private usage, we should be fine with the breaking changes :)

@ericproulx ericproulx merged commit 5671969 into ruby-grape:master Jul 15, 2024
52 checks passed
@ericproulx
Copy link
Contributor

Thanks @numbata

@numbata
Copy link
Contributor Author

numbata commented Jul 15, 2024

If its only for internal/private usage, we should be fine with the breaking changes :)

I just allow that some of the grape users have something like:

rescue_from do |e|
  throw :error, message: "foo", status: 200, headers: {}
end

or directly:

get "/foo" do
  throw :error, message: "oopsie", status: 200, headers: {}
end

@@ -6,7 +6,7 @@

#### Fixes

* Your contribution here.
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove this line next time ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's say that I believe that there will only be new features in the next release and no fixes 🤞.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about that @dblock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like a new "check" can be added to the "danger-changelog" gem.

Copy link
Member

Choose a reason for hiding this comment

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

For sure, we take PRs :)

@dblock
Copy link
Member

dblock commented Jul 15, 2024

@dblock anything to add ?

Ya'll are fast than me! Thanks.

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.

original_exception and backtrace are not present even if passed in error!
3 participants