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 logging for unhandled exceptions #27

Merged
merged 2 commits into from
Jun 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ spec/dummy/db/*.sqlite3
spec/dummy/db/*.sqlite3-journal
spec/dummy/log/*.log
spec/dummy/tmp/
.rubocop-https*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rubocop caches the config locally, we don't want to include those files

2 changes: 2 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
inherit_from:
- https://raw.githubusercontent.com/lessonly/rubocop-default-configuration/master/.rubocop.yml
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a centrally located shared rubocop config we can use in our distributed projects. Does this sound like a good idea? I know I'm going to need it on some of my other projects too 😆

3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Upcoming Release
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this as a place to make note of version changes and upcoming changes. Does this sound like a good plan? We could then copy over the notes from here when tagging the release.

Copy link
Member

Choose a reason for hiding this comment

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

I like this plan


- Any unhandled error is now logged to the configured rails logger by default. You can also now supply a custom callable that will be used to handle those exceptions instead.
14 changes: 14 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,20 @@ Sample request:
$ curl -X PATCH 'http://username:password@localhost:3000/scim/v2/Users/1' -d '{"schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], "Operations": [{"op": "replace", "value": { "active": false }}]}' -H 'Content-Type: application/scim+json'
```

### Error Handling

By default, scim_rails will output any unhandled exceptions to your configured rails logs.

If you would like, you can supply a custom handler for exceptions in the initializer. The only requirement is that the value you supply responds to `#call`.

For example, you might want to notify Honeybadger:

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

## Contributing

### [Code of Conduct](https://github.com/lessonly/scim_rails/blob/master/CODE_OF_CONDUCT.md)
Expand Down
2 changes: 1 addition & 1 deletion Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ RDoc::Task.new(:rdoc) do |rdoc|
rdoc.rdoc_files.include('lib/**/*.rb')
end

APP_RAKEFILE = File.expand_path("../test/dummy/Rakefile", __FILE__)
APP_RAKEFILE = File.expand_path("../spec/dummy/Rakefile", __FILE__)
Copy link
Contributor Author

@rreinhardt9 rreinhardt9 May 29, 2020

Choose a reason for hiding this comment

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

This has to be updated because we used 'spec' instead of the 'test' directory. It tells the engine where it can find the dummy app rakefile so that you can run things like db:migrate from the root of the engine (instead of always having to cd to the dummy app).

load 'rails/tasks/engine.rake'


Expand Down
13 changes: 8 additions & 5 deletions app/controllers/concerns/scim_rails/exception_handler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,15 @@ class UnsupportedPatchRequest < StandardError
end

included do
# StandardError must be ordered _first_ or it will catch all exceptions
#
# TODO: Build a plugin/configuration for error handling so that the
# detailed production errors are logged somewhere if desired.
if Rails.env.production?
rescue_from StandardError do
rescue_from StandardError do |exception|
on_error = ScimRails.config.on_error
if on_error.respond_to?(:call)
on_error.call(exception)
else
Rails.logger.error(exception.inspect)
end

json_response(
{
schemas: ["urn:ietf:params:scim:api:messages:2.0:Error"],
Expand Down
1 change: 1 addition & 0 deletions bin/rails
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

ENGINE_ROOT = File.expand_path('../..', __FILE__)
ENGINE_PATH = File.expand_path('../../lib/scim_rails/engine', __FILE__)
APP_PATH = File.expand_path('../../spec/dummy/config/application', __FILE__)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tells rails where the dummy application is located. You wouldn't need this if we were using the default "test" directory but we do because we are using "spec" instead.

Interestingly, you can supply a flag when generating the engine to customize the location of the dummy application if you want it to be somewhere other than test. I learn that reading the rails source code today!


# Set up gems listed in the Gemfile.
ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../../Gemfile', __FILE__)
Expand Down
16 changes: 11 additions & 5 deletions lib/scim_rails/config.rb
Original file line number Diff line number Diff line change
@@ -1,26 +1,32 @@
# frozen_string_literal: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of this was updated when I ran rubocop autocorrect on the file. It made sense and made it more consistent with the rest of the app so I left it?

I do wonder though... we might want to use double quotes in here like we do in other Lessonly apps. Maybe I should make these double quotes again and then we can set up a rubocop file in here that is more inline with our common styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and just create a central base rubocop file we can use in our gems and update this with that; and then change the single quotes back to double 😬 Sorry for the switcheroo. As I typed out the above comment it dawned on me that letting it correct to single quotes was probably not the way we wanted to go 😆


module ScimRails
class << self
def configure
yield config
end

def config
@_config ||= Config.new
@config ||= Config.new
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rubocop indicates that the memoized instance variable should match the method name

end
end

# Class containing configuration of ScimRails
class Config
ALGO_NONE = "none".freeze
ALGO_NONE = "none"

attr_accessor \
attr_writer \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I broke these out as just "writers" because we override the readers below which create kind of confusing situation where we create a reader just to override it lower in the file. Rubocop flagged that situation for me and so I broke those out so we are just creating their writer up here.

:basic_auth_model,
:mutable_user_attributes_schema,
:scim_users_model

attr_accessor \
:basic_auth_model_authenticatable_attribute,
:basic_auth_model_searchable_attribute,
:mutable_user_attributes,
:mutable_user_attributes_schema,
:on_error,
:queryable_user_attributes,
:scim_users_list_order,
:scim_users_model,
:scim_users_scope,
:scim_user_prevent_update_on_create,
:signing_secret,
Expand Down