Move the inclusion of mixin to the bottom of the controller for customizable error handling

[hrtshu]

Hi, thank you very much for awesome gem.
I'm developing SCIM feature with your gem in my company.

Suggestion

I would like you to move the inclusion of application_controller_mixin to the bottom of the controller to allow to override the error handler methods or some other methods.

Or I would like you to create a new option to customize error handling.

This PR is for 1st suggestion.

Background

If the inclusion of application_controller_mixin is before the method definitions, the methods defined in mixin are overridden by the methods originally defined in Scimitar::ApplicationController.

scimitar/app/controllers/scimitar/application_controller.rb

Lines 12 to 14 in 9b4899c

	if Scimitar.engine_configuration.application_controller_mixin
	include Scimitar.engine_configuration.application_controller_mixin
	end

This prevents overriding the controller's method with the mixin. This may be intentional.

In fact, we cannot override instance method itself, but we can override rescue_from like the following.
(Assume that we want to change the behavior of handle_unexpected_error (error handling for StandardError).)

    application_controller_mixin: Module.new do
      def self.included(base)
        base.class_eval do
          rescue_from StandardError, with: :custom_unexpected_error_handler
          ...

However, it changes the priority of rescue_from like the following.

rescue_from StandardError, with: :handle_unexpected_error
rescue_from ActionDispatch::Http::Parameters::ParseError, with: :handle_bad_json_error # Via "ActionDispatch::Request.parameter_parsers" block in lib/scimitar/engine.rb
rescue_from Scimitar::ErrorResponse, with: :handle_scim_error
rescue_from StandardError, with: :custom_unexpected_error_handler

As a result, all errors, including ActionDispatch::Http::Parameters::ParseError and Scimitar::ErrorResponse, will be handled by the custom_unexpected_error_handler.

If we want to avoid this, we will need to enumerate all originally defined rescue_from like below.

    application_controller_mixin: Module.new do
      def self.included(base)
        base.class_eval do
          rescue_from StandardError, with: :custom_unexpected_error_handler
          rescue_from ActionDispatch::Http::Parameters::ParseError, with: :handle_bad_json_error # Via "ActionDispatch::Request.parameter_parsers" block in lib/scimitar/engine.rb
          rescue_from Scimitar::ErrorResponse, with: :handle_scim_error
          ...

However, this is unhealthy.
If the original definitions of Scimitar::ApplicationController change, we must follow them.

I believe allowing to override the controller methods is better for any cases.

My Final Goal

Ultimately, I want to add the error reporting and customize the error message for 500 errors like the following.

def handle_unexpected_error(exception)
  Rollbar.error(exception)
  handle_scim_error(Scimitar::ErrorResponse.new(status: 500, detail: 'internal error'), exception)
end

I want to hide the concrete error message to prevent the internal matter is exposed to out customer.

[pond]

Yeah, I can't really see an issue with this. Anyone who was deliberately overriding would have code that should still work; anyone not overriding sees no change; only code that accidentally overrode would encounter trouble.

[pond]

Published as v2.10.0 - see https://rubygems.org/gems/scimitar