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

Add ability to surface general exceptions that are captured #23

Closed
rreinhardt9 opened this issue May 26, 2020 · 1 comment · Fixed by #27
Closed

Add ability to surface general exceptions that are captured #23

rreinhardt9 opened this issue May 26, 2020 · 1 comment · Fixed by #27
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@rreinhardt9
Copy link
Contributor

Currently in the code, we rescue all exceptions and provide a custom JSON response for the 500 error:

rescue_from StandardError do
json_response(
{
schemas: ["urn:ietf:params:scim:api:messages:2.0:Error"],
status: "500"
},
:internal_server_error
)
end

The catch is, that leaves the host system unaware of the exception. Since the exception was caught it doesn't bubble up to the parent system to be caught by it's error handling system.

From what I understand, it's also not possible to both render a response and raise an exception.

What if, we expose the ability to configure a "callable" in an initializer that will be called with the given exception in the event that one is rescued?

For example, the proc could be a call to Honeybadger:

# In host app initializer

ScimRails.configure do |config|
  config.error_handler = -> (e){ Honeybadger.notify(e) }
end

By default, we could potentially make it a call to the rails logger so that "silence" is not the default:

-> (e) { Rails.logger.error(e.inspect) }
@rreinhardt9 rreinhardt9 added enhancement New feature or request good first issue Good for newcomers labels May 29, 2020
@rreinhardt9 rreinhardt9 self-assigned this May 29, 2020
@rreinhardt9
Copy link
Contributor Author

I'm gonna work on this one; I want to get familiar with the project anyway and this is a great starting point!

rreinhardt9 added a commit to rreinhardt9/scim_rails that referenced this issue May 29, 2020
resolves lessonly#23

Why?
The scim engine returns a custom response to the caller in the event
that there is a 500 error in the engine. It rescues any standard error
in order to do this.

Unfortunately this means that when something is broken it does not
bubble up to the parent app's error handling system or even be printed
in the logs.

We cannot just re-raise the exception because you cannot return a
response AND raise an exception as a part of the same request.

To help, this PR will make is possible for you to supply a callable
object that take the exception as it's argument.

If no callable object is provided, we will output the exception to the
logs so that silence is not the default.

To get the old behavior of completely ignoring an exception, you could
supply an empty proc.
rreinhardt9 added a commit to rreinhardt9/scim_rails that referenced this issue May 29, 2020
resolves lessonly#23

Why?
The scim engine returns a custom response to the caller in the event
that there is a 500 error in the engine. It rescues any standard error
in order to do this.

Unfortunately this means that when something is broken it does not
bubble up to the parent app's error handling system or even be printed
in the logs.

We cannot just re-raise the exception because you cannot return a
response AND raise an exception as a part of the same request.

To help, this PR will make is possible for you to supply a callable
object that take the exception as it's argument.

If no callable object is provided, we will output the exception to the
logs so that silence is not the default.

To get the old behavior of completely ignoring an exception, you could
supply an empty proc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant