Add logging for unhandled exceptions #27

rreinhardt9 · 2020-05-29T21:10:30Z

Why?

resolves #23
fixes #25
fixes #26

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.

This also fixes some pains encountered during development with running tasks and commands

Testing Notes

This one is a bit complicated to test because the exception catching behavior is only configured to happen in production environments.

You can make sure that the existing spec pass, and to see the exception handling in action you can pull down the code and make the following modifications:

In app/controllers/concerns/scim_rails/exception_handler.rb change Rails.env.production? to true
Then, add a custom error handler to the dummy app's initializer: config.on_error = ->(e) { puts "Found error #{e}" }
You can then open the users controller and add raise "a fuss" at the top of one of the actions.

Run the test suite now... you should see failures related to the response not matching and you should see the output from the puts.

Merge Instructions

Please rebase these commits when merging

fixes lessonly#25 fixes lessonly#26 Why Since we are using the spec/ director instead of the default test/ we need to specify the locations of the dummy application and it's rake file. With out this, you cannot run things like db:migrate from the root of the engine as you should. This goes for any rake task you would usually call from the root. Additionally, commands like rails console would not work because it would not be able to find the dummy application to load it.

rreinhardt9 · 2020-05-29T21:16:00Z

@wernull I think you've had experience with working on this gem in the past so I tagged you as reviewer; would you mind looking at this with me?

rreinhardt9 · 2020-05-29T21:21:45Z

bin/rails

@@ -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__)


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!

rreinhardt9 · 2020-05-29T21:22:25Z

Rakefile

@@ -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__)


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).

rreinhardt9 · 2020-05-29T21:24:35Z

CHANGELOG.md

@@ -0,0 +1,3 @@
+# Upcoming Release


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.

I like this plan

rreinhardt9 · 2020-05-29T21:26:59Z

lib/scim_rails/config.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true


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?

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 😆

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 · 2020-05-29T21:41:41Z

.gitignore

@@ -5,3 +5,4 @@ spec/dummy/db/*.sqlite3
 spec/dummy/db/*.sqlite3-journal
 spec/dummy/log/*.log
 spec/dummy/tmp/
+.rubocop-https*


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

rreinhardt9 · 2020-05-29T21:42:45Z

.rubocop.yml

@@ -0,0 +1,2 @@
+inherit_from:
+  - https://raw.githubusercontent.com/lessonly/rubocop-default-configuration/master/.rubocop.yml


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 😆

rreinhardt9 · 2020-05-29T21:43:32Z

lib/scim_rails/config.rb

 module ScimRails
  class << self
    def configure
      yield config
    end

    def config
-      @_config ||= Config.new
+      @config ||= Config.new


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

rreinhardt9 · 2020-06-01T14:39:38Z

lib/scim_rails/config.rb


-    attr_accessor \
+    attr_writer \


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.

wernull

This looks pretty good! Thanks for the effort here. It looks like we should be able to update the version in production applications using it without any issues then implement the custom error reporting later. It might be time to get another release out there soon. I'm thinking a minor update like 0.4.0. We should save 1.0.0 for full scim compliance

wernull · 2020-06-02T17:54:20Z

CHANGELOG.md

@@ -0,0 +1,3 @@
+# Upcoming Release


I like this plan

rreinhardt9 · 2020-06-02T19:33:51Z

Thanks for the review and merge @wernull !!!! 🙏 🎉

rreinhardt9 marked this pull request as ready for review May 29, 2020 21:13

rreinhardt9 self-assigned this May 29, 2020

rreinhardt9 requested a review from wernull May 29, 2020 21:14

rreinhardt9 commented May 29, 2020

View reviewed changes

rreinhardt9 force-pushed the add-error-logging branch from bb439f5 to 7ba7da5 Compare May 29, 2020 21:39

rreinhardt9 commented May 29, 2020

View reviewed changes

rreinhardt9 commented Jun 1, 2020

View reviewed changes

wernull approved these changes Jun 2, 2020

View reviewed changes

CHANGELOG.md

@@ -0,0 +1,3 @@

# Upcoming Release

Copy link

Member

wernull Jun 2, 2020

Choose a reason for hiding this comment

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

I like this plan

rreinhardt9 reacted with thumbs up emoji

wernull merged commit e05971b into lessonly:master Jun 2, 2020

rreinhardt9 deleted the add-error-logging branch June 2, 2020 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logging for unhandled exceptions #27

Add logging for unhandled exceptions #27

rreinhardt9 commented May 29, 2020 •

edited

Loading

rreinhardt9 commented May 29, 2020

rreinhardt9 May 29, 2020

rreinhardt9 May 29, 2020 •

edited

Loading

rreinhardt9 May 29, 2020

wernull Jun 2, 2020

rreinhardt9 May 29, 2020

rreinhardt9 May 29, 2020

rreinhardt9 May 29, 2020

rreinhardt9 May 29, 2020

rreinhardt9 May 29, 2020

rreinhardt9 Jun 1, 2020

wernull left a comment

wernull Jun 2, 2020

rreinhardt9 commented Jun 2, 2020

		@@ -0,0 +1,2 @@
		inherit_from:
		- https://raw.githubusercontent.com/lessonly/rubocop-default-configuration/master/.rubocop.yml

Add logging for unhandled exceptions #27

Add logging for unhandled exceptions #27

Conversation

rreinhardt9 commented May 29, 2020 • edited Loading

Why?

Testing Notes

Merge Instructions

rreinhardt9 commented May 29, 2020

Choose a reason for hiding this comment

rreinhardt9 May 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wernull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreinhardt9 commented Jun 2, 2020

rreinhardt9 commented May 29, 2020 •

edited

Loading

rreinhardt9 May 29, 2020 •

edited

Loading