Understanding a conflict between two ActiveSupport.on_load calls

duncanjbrown · May 11, 2020, 4:06pm

Hello.

Our application depends on two gems: GitHub - dxw/mail-notify: ActionMailer support for the GOV.UK Notify API and GitHub - collectiveidea/audited: Audited (formerly acts_as_audited) is an ORM extension that logs all changes to your Rails models..

The mailer preview index at rails/mailers is broken. It throws ActionView::Template::Error (arguments passed to url_for can't be handled. Please require routes or provide your own implementation). This seems to be because the definition of url_for comes from ActionView::Helpers::UrlHelper. Removing the audited gem makes the preview index work correctly, because url_for then resolves to ActionDispatch::Routing::UrlFor#url_for.

Here is a minimal rails app displaying the behaviour: GitHub - duncanjbrown/audited-bug.

I can make the exception go away by editing these lines of the audited gem in one of two ways.

I can wrap them in a Railtie under an initializer like this: Wrap the ActiveSupport.on_load hook in a railtie · duncanjbrown/audited@fa11351 · GitHub
I can comment out the reference to ActionController::API.

I would like to understand a few things.

Why does this happen?
What has ActionController::API got to do with it?
Why does it manifest in the way it does (the url_for definition)
Is it ever right to set an ActiveSupport.on_load hook outside an initializer (ie is this a mistake in audited?)

Thanks for your help!

petrik · May 11, 2020, 5:08pm

From the guides:

Rails code can often be referenced on load of an application. Rails is responsible for the load order of these frameworks, so when you load frameworks, such as ActiveRecord::Base , prematurely you are violating an implicit contract your application has with Rails. Moreover, by loading code such as ActiveRecord::Base on boot of your application you are loading entire frameworks which may slow down your boot time and could cause conflicts with load order and boot of your application.

Load and configuration hooks are the API that allow you to hook into this initialization process without violating the load contract with Rails. This will also mitigate boot performance degradation and avoid conflicts.

When you call ActionController::Base in lib/audited.rb outside of an on_load hook it causes Ruby to look for the definition of that constant and will require it. This causes the entire ActionPack framework to be loaded before boot.

Acts as audited should probably wrap the following line in an on_load hook as well:

github.com

collectiveidea/audited/blob/main/lib/audited.rb#L30


      
              @audit_class ||= Audited::Audit
            end
          
            # remove audit_model in next major version it was only shortly present in 5.1.0
            alias_method :audit_model, :audit_class
            deprecate audit_model: "use Audited.audit_class instead of Audited.audit_model. This method will be removed.",
                      deprecator: ActiveSupport::Deprecation.new('6.0.0', 'Audited')
          
            def store
              RequestStore.store[:audited_store] ||= {}
            end
          
            def config
              yield(self)
            end
          end
          
          @ignored_attributes = %w[lock_version created_at updated_at created_on updated_on]
          
          @current_user_method = :current_user
          @auditing_enabled = true

petrik · May 11, 2020, 5:11pm

If would be nice if prematurely calling a framework generates a warning instead of continuing and failing later on. There is the PR:

https://github.com/rails/rails/pull/38024

petrik · May 11, 2020, 5:22pm

The url_for method is included here:

github.com

rails/rails/blob/03351cd5410b3ad3d3e51f44ce60aa553ddeef14/actionpack/lib/action_controller.rb#L53


      
              autoload :ParamsWrapper
              autoload :Redirecting
              autoload :Renderers
              autoload :Rendering
              autoload :RequestForgeryProtection
              autoload :Rescue
              autoload :Streaming
              autoload :StrongParameters
              autoload :ParameterEncoding
              autoload :Testing
              autoload :UrlFor
            end
          
          
  autoload_under "api" do
              autoload :ApiRendering
            end
          
          
  autoload :TestCase,           "action_controller/test_case"
            autoload :TemplateAssertions, "action_controller/test_case"
          end

If you prematurely initialize ActionController::API the UrlFor module hasn’t yet been included resulting in an error.

duncanjbrown · May 11, 2020, 9:33pm

Thank you @petrik!

I’ll see if I can submit something like this as a PR to audited: Comparing collectiveidea:master...duncanjbrown:use-railtie-for-on-load · collectiveidea/audited · GitHub. Does that look idiomatic to you? (I haven’t worked with this part of Rails before).

petrik · May 12, 2020, 10:16am

@duncanjbrown Looks fine to me!