Merge pull request #1281 from paper-trail-gem/jb_lazy_load_rails

Fix lazy-loading of ActiveRecord

CHANGELOG.md

@@ -7,9 +7,11 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/).
 
 ### Breaking Changes
 
-- In the PT rails engine, the `paper_trail` configuration has been
-  removed. This configuration object was deprecated in 10.2.0. Please review
-  docs section [2.d. Turning PaperTrail
+- Rails: Instead of an `Engine`, PT now provides a `Railtie`, which is simpler.
+- Rails: The deprecated `config.paper_trail` configuration technique
+  has been removed. This configuration object was deprecated in 10.2.0. It only
+  had one key, `config.paper_trail.enabled`. Please review docs section [2.d.
+  Turning PaperTrail
   Off](https://github.com/paper-trail-gem/paper_trail/#2d-turning-papertrail-off)
   for alternatives.
 

lib/paper_trail.rb

@@ -8,34 +8,17 @@
 # can revisit this decision.
 require "active_support/all"
 
-# AR is required for, eg. has_paper_trail.rb, so we could put this `require` in
-# all of those files, but it seems easier to troubleshoot if we just make sure
-# AR is loaded here before loading *any* of PT. See discussion of
-# performance/simplicity tradeoff for activesupport above.
-require "active_record"
-
-require "request_store"
 require "paper_trail/cleaner"
 require "paper_trail/compatibility"
 require "paper_trail/config"
-require "paper_trail/has_paper_trail"
 require "paper_trail/record_history"
-require "paper_trail/reifier"
 require "paper_trail/request"
-require "paper_trail/version_concern"
 require "paper_trail/version_number"
 require "paper_trail/serializers/json"
-require "paper_trail/serializers/yaml"
 
 # An ActiveRecord extension that tracks changes to your models, for auditing or
 # versioning.
 module PaperTrail
-  E_RAILS_NOT_LOADED = <<-EOS.squish.freeze
-    PaperTrail has been loaded too early, before rails is loaded. This can
-    happen when another gem defines the ::Rails namespace, then PT is loaded,
-    all before rails is loaded. You may want to reorder your Gemfile, or defer
-    the loading of PT by using `require: false` and a manual require elsewhere.
-  EOS
   E_TIMESTAMP_FIELD_CONFIG = <<-EOS.squish.freeze
     PaperTrail.timestamp_field= has been removed, without replacement. It is no
     longer configurable. The timestamp column in the versions table must now be
@@ -126,27 +109,18 @@ module PaperTrail
   end
 end
 
-# We use the `on_load` "hook" instead of `ActiveRecord::Base.include` because we
-# don't want to cause all of AR to be autloaded yet. See
-# https://guides.rubyonrails.org/engines.html#what-are-on-load-hooks-questionmark
-# to learn more about `on_load`.
-ActiveSupport.on_load(:active_record) do
-  include PaperTrail::Model
-end
-
-# Require frameworks
-if defined?(::Rails)
-  # Rails module is sometimes defined by gems like rails-html-sanitizer
-  # so we check for presence of Rails.application.
-  if defined?(::Rails.application)
-    require "paper_trail/frameworks/rails"
-  else
-    ::Kernel.warn(::PaperTrail::E_RAILS_NOT_LOADED)
-  end
+# PT is built on ActiveRecord, but does not require Rails. If Rails is defined,
+# our Railtie makes sure not to load the AR-dependent parts of PT until AR is
+# ready. A typical Rails `application.rb` has:
+#
+# ```
+# require 'rails/all' # Defines `Rails`
+# Bundler.require(*Rails.groups) # require 'paper_trail' (this file)
+# ```
+#
+# Non-rails applications should take similar care to load AR before PT.
+if defined?(Rails)
+  require "paper_trail/frameworks/rails"
 else
   require "paper_trail/frameworks/active_record"
 end
-
-if defined?(::ActiveRecord)
-  ::PaperTrail::Compatibility.check_activerecord(::ActiveRecord.gem_version)
-end

lib/paper_trail/frameworks/active_record.rb

@@ -1,5 +1,12 @@
 # frozen_string_literal: true
 
-# This file only needs to be loaded if the gem is being used outside of Rails,
-# since otherwise the model(s) will get loaded in via the `Rails::Engine`.
+# Either ActiveRecord has already been loaded by the Lazy Load Hook in our
+# Railtie, or else we load it now.
+require "active_record"
+::PaperTrail::Compatibility.check_activerecord(::ActiveRecord.gem_version)
+
+# Now we can load the parts of PT that depend on AR.
+require "paper_trail/has_paper_trail"
+require "paper_trail/reifier"
 require "paper_trail/frameworks/active_record/models/paper_trail/version"
+ActiveRecord::Base.include PaperTrail::Model

lib/paper_trail/frameworks/rails.rb

@@ -1,4 +1,3 @@
 # frozen_string_literal: true
 
-require "paper_trail/frameworks/rails/controller"
-require "paper_trail/frameworks/rails/engine"
+require "paper_trail/frameworks/rails/railtie"

lib/paper_trail/frameworks/rails/controller.rb

@@ -101,9 +101,3 @@ module PaperTrail
     end
   end
 end
-
-if defined?(::ActionController)
-  ::ActiveSupport.on_load(:action_controller) do
-    include ::PaperTrail::Rails::Controller
-  end
-end

lib/paper_trail/frameworks/rails/engine.rb

@@ -1,10 +0,0 @@
-# frozen_string_literal: true
-
-module PaperTrail
-  module Rails
-    # See http://guides.rubyonrails.org/engines.html
-    class Engine < ::Rails::Engine
-      paths["app/models"] << "lib/paper_trail/frameworks/active_record/models"
-    end
-  end
-end

lib/paper_trail/frameworks/rails/railtie.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+module PaperTrail
+  # Represents code to load within Rails framework. See documentation in
+  # `railties/lib/rails/railtie.rb`.
+  # @api private
+  class Railtie < ::Rails::Railtie
+    # PaperTrail only has one initializer.
+    #
+    # We specify `before: "load_config_initializers"` to ensure that the PT
+    # initializer happens before "app initializers" (those defined in
+    # the app's `config/initalizers`).
+    initializer "paper_trail", before: "load_config_initializers" do
+      # `on_load` is a "lazy load hook". It "declares a block that will be
+      # executed when a Rails component is fully loaded". (See
+      # `active_support/lazy_load_hooks.rb`)
+      ActiveSupport.on_load(:action_controller) do
+        require "paper_trail/frameworks/rails/controller"
+
+        # Mix our extensions into `ActionController::Base`, which is `self`
+        # because of the `class_eval` in `lazy_load_hooks.rb`.
+        include PaperTrail::Rails::Controller
+      end
+
+      ActiveSupport.on_load(:active_record) do
+        require "paper_trail/frameworks/active_record"
+      end
+    end
+  end
+end

spec/dummy_app/config/application.rb

@@ -2,12 +2,14 @@
 
 require File.expand_path("boot", __dir__)
 
-# Pick the frameworks you want:
-require "active_record/railtie"
-require "action_controller/railtie"
+# Here a conventional app would load the Rails components it needs, but we have
+# already loaded these in our spec_helper.
+# require "active_record/railtie"
+# require "action_controller/railtie"
 
-Bundler.require(:default, Rails.env)
-require "paper_trail"
+# Here a conventional app would require gems, but again, we have already loaded
+# these in our spec_helper.
+# Bundler.require(:default, Rails.env)
 
 module Dummy
   class Application < Rails::Application

spec/dummy_app/config/boot.rb

@@ -1,18 +1,3 @@
 # frozen_string_literal: true
 
-require "rubygems"
-
-# When you run rake locally (not on travis) in this dummy app, set the
-# BUNDLE_GEMFILE env. variable to ensure that the correct version of AR is used
-# for e.g. migrations. See examples in CONTRIBUTING.md.
-unless ENV.key?("BUNDLE_GEMFILE")
-  gemfile = File.expand_path("../../../Gemfile", __dir__)
-  if File.exist?(gemfile)
-    puts "Booting PT test dummy app: Using gemfile: #{gemfile}"
-    ENV["BUNDLE_GEMFILE"] = gemfile
-  end
-end
-require "bundler"
-Bundler.setup
-
-$LOAD_PATH.unshift(File.expand_path("../../../lib", __dir__))
+# Unlike a conventional Rails app, our "dummy" app is booted by our spec_helper.

spec/spec_helper.rb

@@ -6,8 +6,16 @@ ENV["DB"] ||= "sqlite"
 require "byebug"
 require_relative "support/pt_arel_helpers"
 
+unless ENV["BUNDLE_GEMFILE"].match?(/rails_\d\.\d\.gemfile/)
+  warn(
+    "It looks like you're trying to run the PT test suite, but you're not " \
+    'using appraisal. Please see "Development" in CONTRIBUTING.md.'
+  )
+  exit 1
+end
 unless File.exist?(File.expand_path("dummy_app/config/database.yml", __dir__))
-  warn "No database.yml detected for the dummy app, please run `rake prepare` first"
+  warn "No database.yml detected for the dummy app, please run `rake install_database_yml` first"
+  exit 1
 end
 
 RSpec.configure do |config|
@@ -30,16 +38,45 @@ RSpec.configure do |config|
   Kernel.srand config.seed
 end
 
-require File.expand_path("dummy_app/config/environment", __dir__)
-require "rspec/rails"
-require "paper_trail/frameworks/rspec"
-require "ffaker"
+# At this point, totally isolated unit tests could be run. But the PT test suite
+# also has "integration" tests, via a "dummy" Rails app. Here, we boot that
+# "dummy" app. The following process follows the same order, roughly, as a
+# conventional Rails app.
+#
+# In the past, this boot process was partially implemented here, and partially
+# in `dummy_app/config/*`. By consolidating it here,
+#
+# - It can better be understood, and documented in one place
+# - It can more closely resememble a conventional app boot. For example, loading
+# gems (like rspec-rails) _before_ loading the app.
 
-# Migrate
+# First, `config/boot.rb` would add gems to $LOAD_PATH.
+Bundler.setup
+
+# Then, the chosen components of Rails would be loaded. In our case, we only
+# test with AR and AC.
+require "active_record/railtie"
+require "action_controller/railtie"
+
+# Then, gems are loaded. In a conventional Rails app, this would be done with
+# by the `Bundler.require` in `config/application.rb`.
+require "paper_trail"
+require "ffaker"
+require "rspec/rails"
+require "rails-controller-testing"
+
+# Now we can load our dummy app. Its boot process does not perfectly match a
+# conventional Rails app, but it's what we were able to fit in our test suite.
+require File.expand_path("dummy_app/config/environment", __dir__)
+
+# Now that AR has a connection pool, we can migrate the database.
 require_relative "support/paper_trail_spec_migrator"
 ::PaperTrailSpecMigrator.new.migrate
 
+# This final section reselmbles what might be dummy_app's spec_helper, if it
+# had one.
+require "paper_trail/frameworks/rspec"
 RSpec.configure do |config|
-  config.fixture_path = "#{::Rails.root}/spec/fixtures"
+  config.fixture_path = nil # we use factories, not fixtures
   config.use_transactional_fixtures = true
 end