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

Resolve lingering issue from collectiveidea/audited#532 #592

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

mvastola
Copy link
Contributor

While PR #532 worked to prevent audited from initializing certain rails components too early, an issue remains wherein the gem entrypoint, lib/audited.rb, immediately requires lib/audited/audit.rb.

Therein, Audited::Audit subclasses, ActiveRecord::Base and thereby causes it to load too early in the Rails boot process.

This simple change merely moves the require of audited/audit into the AR on_load block.

While PR collectiveidea#532 worked to prevent `audited` from
initializing certain rails components too early, an issue remains
wherein the gem entrypoint, `lib/audited.rb`, immediately requires
`lib/audited/audit.rb`.

Therein `Audited::Audit` subclasses, `ActiveRecord::Base` and thereby
causes it to load too early in the Rails boot process.

This change merely moves the `require` of `audited/audit` into the `AR`
`on_load` block.
@danielmorrison danielmorrison merged commit fa96432 into collectiveidea:master Sep 16, 2021
@akostadinov
Copy link
Contributor

hmm, I'm gonna check if this fixes #599

@mvastola
Copy link
Contributor Author

hmm, I'm gonna check if this fixes #599

Yup. It will. I didn't want to write an essay for the PR description, but this load order issue has been a huuuge pain for us, and it sucks there's no system in place to warn when configs start getting discarded.

@dmke
Copy link

dmke commented Sep 29, 2021

I've stumbled upon this, because after upgrading from 5.0.1 to 5.0.2 the specs of a Rails engine threw uninitialized constant: Audited::Audit errors.

The engine basically does this:

# lib/foo/engine.rb
module Foo
  class Engine < ::Rails::Engine
    require "audited"

    initializer "set audited audit class" do
      Audited.config do |config|
        config.audit_class = Foo::Audit
      end
    end
  end
end

# app/models/foo/audit.rb
class Foo::Audit < Audited::Audit
  # do some multi-tenancy shenanigans
end

My current workaround is to just add an require "audited/audit" into the engine:

   class Engine < ::Rails::Engine
     require "audited"
+    require "audited/audit"

The specs now run again, and the apps using the engine seem to be stable. But I do wonder, is this the right approach?

@akostadinov
Copy link
Contributor

akostadinov commented Sep 29, 2021

@dmke , I just keep audited automatically loaded by bundler. i.e. don't add require: false in Gemfile.

Do you have any initializers that refer to Audited::Audit? You may need to enclose them in a ActiveSupport.on_load :active_record do block.

@dmke
Copy link

dmke commented Sep 29, 2021

I just keep audited automatically loaded by bundler. i.e. don't add require: false in Gemfile.

There's no require: false, Audited is part of the engine's .gemspec.

Do you have any initializers that refer to Audited::Audit? You may need to enclose them in a ActiveSupport.on_load :active_record do block.

Yes, the engine initializer code runs in that phase, IIRC. Let me try your suggestion:

initializer "set audited audit class" do
  ActiveSupport.on_load :active_record do
    Audited.config do |config|
      config.audit_class = Foo::Audit
    end
  end
end

Edit: This seems to work, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants