Redefinition of PaperTrail::Version stopped working in v12

[toncid]

Hello,

It seems that the v12 somehow interferes with Rails lazy autoloading in the development environment. The overriding class is no longer concerned. See below the script for more info.

---

Check the following boxes:

• This is not a usage question, this is a bug report
• This bug can be reproduced with the script I provide below ==> the script cannot be used because it does not rely on autoloading
• This bug can be reproduced in the latest release of the paper_trail gem

Due to limited volunteers, we cannot answer usage questions. Please ask such
questions on StackOverflow.

Bug reports must use the following template:

# frozen_string_literal: true

# Use this template to report PaperTrail bugs.
# Please include only the minimum code necessary to reproduce your issue.
require "bundler/inline"

# STEP ONE: What versions are you using?
gemfile(true) do
  ruby "2.6.6"
  source "https://rubygems.org"
  gem "activerecord", "5.2.5"
  gem "minitest", "5.14.4"
  gem "paper_trail", "12.0.0", require: false
  gem "sqlite3", "1.4.2"
end

require "active_record"
require "minitest/autorun"
require "logger"

# Please use sqlite for your bug reports, if possible.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = nil
ActiveRecord::Schema.define do
  # STEP TWO: Define your tables here.
  create_table :users, force: true do |t|
    t.text :first_name, null: false
    t.timestamps null: false
  end

  create_table :versions do |t|
    t.string :item_type, null: false
    t.integer :item_id, null: false
    t.bigint :user_id, null: false
    t.string :event, null: false
    t.string :whodunnit
    t.text :object, limit: 1_073_741_823
    t.text :object_changes, limit: 1_073_741_823
    t.datetime :created_at
  end
  add_index :versions, %i[item_type item_id]
end
ActiveRecord::Base.logger = Logger.new(STDOUT)
require "paper_trail"

# STEP FOUR: Define your AR models here.
class User < ActiveRecord::Base
  has_paper_trail meta: {user: ->(item) { item }}

  validates_presence_of :first_name
end

module PaperTrail
  class Version < ActiveRecord::Base
    include PaperTrail::VersionConcern

    belongs_to :user

    validates_presence_of :user
  end
end

# STEP FIVE: Please write a test that demonstrates your issue.
class BugTest < ActiveSupport::TestCase
  def test_1
    assert_difference(-> { PaperTrail::Version.count }, +1) {
      User.create!(first_name: "Jane")
    }
  end
end

# STEP SIX: Run this script using `ruby my_bug_report.rb`

# Running:

D, [2021-04-06T14:36:22.417428 #91222] DEBUG -- :    (0.1ms)  SELECT COUNT(*) FROM "versions"
D, [2021-04-06T14:36:22.420818 #91222] DEBUG -- :    (0.0ms)  begin transaction
D, [2021-04-06T14:36:22.421598 #91222] DEBUG -- :   User Create (0.1ms)  INSERT INTO "users" ("first_name", "created_at", "updated_at") VALUES (?, ?, ?)  [["first_name", "Jane"], ["created_at", "2021-04-06 12:36:22.421102"], ["updated_at", "2021-04-06 12:36:22.421102"]]
D, [2021-04-06T14:36:22.435035 #91222] DEBUG -- :   PaperTrail::Version Create (0.1ms)  INSERT INTO "versions" ("item_type", "item_id", "user_id", "event", "object_changes", "created_at") VALUES (?, ?, ?, ?, ?, ?)  [["item_type", "User"], ["item_id", 1], ["user_id", 1], ["event", "create"], ["object_changes", "---\nid:\n-\n- 1\nfirst_name:\n-\n- Jane\ncreated_at:\n-\n- 2021-04-06 12:36:22.421102000 Z\nupdated_at:\n-\n- 2021-04-06 12:36:22.421102000 Z\n"], ["created_at", "2021-04-06 12:36:22.421102"]]
D, [2021-04-06T14:36:22.439861 #91222] DEBUG -- :    (0.1ms)  commit transaction
D, [2021-04-06T14:36:22.440312 #91222] DEBUG -- :    (0.1ms)  SELECT COUNT(*) FROM "versions"
.

Finished in 0.025732s, 38.8621 runs/s, 38.8621 assertions/s.

1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

---

The script runs well, but this is due to avoided autoloading (PaperTrail::Version is defined in the same script).

The problem is with Rails when PaperTrail::Version is redefined in another file (see: https://github.com/paper-trail-gem/paper_trail/blob/v12.0.0/README.md#5b2-item-association). This no longer works in development where autoloading is lazy.

Any advice?

[toncid]

For some reason, the default class from the gem is never overridden by our code in v12:

v11.1.0

Module.const_source_location("PaperTrail::Version")
"/home/tonci/dev/myapp/app/models/paper_trail/version.rb"

v12.0.0

Module.const_source_location("PaperTrail::Version")
"/home/tonci/.rvm/gems/ruby-2.6.6/gems/paper_trail-12.0.0/lib/paper_trail/frameworks/active_record/models/paper_trail/version.rb"

[StanBright]

Hey @toncid, thanks for sharing your experience. I'm just chiming in that I'm fighting the same issue... without much success. I tried a few things but couldn't make it work.

I reverted back to to v11.1, until there's some clarity what's the issue here ¯\(ツ)/¯

Edit:
p.s. overriding in v12 doesn't work in production in my case, too.

[toncid]

@StanBright Thanks for the follow-up and testing in the production mode. We have tried a few different approaches to circumvent the problem, none of them successful, so reverting to v11.1 was also the only option left.

[criess]

I can confirm the issue, also i have no resolution. On our projects we go with a lower version of papertrail for now.

[tlynam]

Thank you for opening this and for everyone providing all of the extra details. Can confirm this issue. We recently changed loading in #1281
I'm trying to understand the issue. It's interesting that this appears to work with Rails 6 and zeitwerk enabled.

[tlynam]

It seems like it could potentially work if we move the overridden Version class into an initializer. I'm curious what you think @jaredbeck?

[jaredbeck]

Hi y'all. Thanks for the report. I tried to reproduce this in a new rails app (6.1) but no luck.

bin/rails console
Loading development environment (Rails 6.1.3.1)
PaperTrail.gem_version
=> #<Gem::Version "12.0.0">
Module.const_source_location("PaperTrail::Version")
=> ["redacted/pt_issue_1305/app/models/paper_trail/version.rb", 2]
PaperTrail::Version.new.is_a?(PaperTrail::VersionConcern)
=> true

What am I missing?

[tlynam]

We can reproduce in Rails 6.1 by enabling classic mode config.autoloader = :classic

[jaredbeck]

We can reproduce in Rails 6.1 by enabling classic mode config.autoloader = :classic

Thanks Todd. With that setting, I can reproduce the issue:

Module.const_source_location("PaperTrail::Version")
=> ["redacted/paper_trail/frameworks/active_record/models/paper_trail/version.rb", 13]

Classic mode is deprecated, but I'd like PT to support it as long as is practical. Contributions are welcome. If there are practical workarounds, let's document them.

In the changelog, I will reclassify #1281 as a breaking change.

[tlynam]

Hi @toncid, @StanBright, @criess, does it work if you move the overridden Version class into an initializer?

[jaredbeck]

You might also want to play around with adding a require_dependency in your app/models/paper_trail/version.rb.

[fsateler]

This is happening because the new railtie is requiring the active_record framework: https://github.com/paper-trail-gem/paper_trail/pull/1281/files#diff-f226b8d714b180354bcadd32e8b0f55afb2e4d9e2c639c28bcf248aba53b38e0R26 , and the previous engine didn't. The AR framework in turn requires the model class. Apparently using AR-only PT never supported overriding the Version class. (possibly because it doesn't make much sense. Who is doing the autoloading if not rails?)

I have worked around this by adding this in my PT initializer:

module PaperTrail
  remove_const :Version
end

I believe the proper fix is to not require the Version model, but to keep adding the autoload path. Here is a StackOverflow answer suggesting how to do it: https://stackoverflow.com/a/6394946 . So maybe something like moving the require 'paper_trail/frameworks/active_record/models/paper_trail/version.rb' out of the active_record framework, and add an initializer that preserves the autoload mangling from 11.1 .

[jaredbeck]

Thanks everyone for a productive discussion.

Classic mode is deprecated, but I'd like PT to support it as long as is practical. Contributions are welcome. If there are practical workarounds, let's document them.

I'll close this for now with the understanding that our preferred solution is zeitwerk.

- config.autoloader = :classic # deprecated
+ config.autoloader = :zeitwerk

PRs welcome.

[fsateler]

Sorry for not mentioning this. I'm seeing the error with the zeitwerk loader.

[jnimety]

Hmmmm, we're using zeitwerk and ran into this issue. However, we've solved it by creating a new class with our additions (PaperTrail::ApplicationVersion) that inherits from PaperTrail::Version and configuring our models to use that class for versioning.

[jaredbeck]

Hmmmm, we're using zeitwerk and ran into this issue. However, we've solved it by creating a new class with our additions (PaperTrail::ApplicationVersion) that inherits from PaperTrail::Version and configuring our models to use that class for versioning.

I like the sound of that, Joel. You might also try include VersionConcern if you prefer that over inheritance. You might want to inherit from ApplicationRecord, for example.

class ApplicationVersion < ApplicationRecord
  include PaperTrail::VersionConcern
end

I haven't tested the above, and I don't use Custom Version Classes personally, so I'd warmly welcome documentation PRs from people who do.

[jnimety]

Well, I spoke too soon. That change worked just fine in rails console but completely blew up our spec suite (specs won't event run because of an error during setup). I'll report back after I know more.

[jnimety]

@jaredbeck your include PaperTrail::VersionConcern solution seems to work but I need to do more testing to confirm. Are we losing anything from PaperTrail::Version when we do it that way?

[istvan-ujjmeszaros]

Hey guys, why is this issue closed? I am still experimenting the same and I couldn't make any of the workarounds work. Maybe I am doing something wrong, that is our custom module which is getting ignored when upgrading from v11.1.0 to v12.3.0 (tried with :zeitwerk and :classic as well, no difference):

module PaperTrail
  class Version < ActiveRecord::Base
    include PaperTrail::VersionConcern
    belongs_to :user, foreign_key: :whodunnit, optional: true
  end
end

[weezing]

@istvan-ujjmeszaros I was able to work around this bug by defining Version model in models folder and inherit from PaperTrail::Version to have access to such methods as changeset

Works like a charm!

# frozen_string_literal: true

class Version < PaperTrail::Version
  belongs_to :item, polymorphic: true
  belongs_to :user
end

[thebravoman]

Switched to 6.1.7 with zeitwerk. Facing the same issue with paper_trail 13.0.0 and with 14.0.0
Is it possible that there are still cases that haven't been addressed with the fix and this should not be closed.

We define

module PaperTrail
  class Version < ActiveRecord::Base
    include PaperTrail::VersionConcern

in
app/models/paper_trail/version.rb

and this never gets loaded because of

[email protected]: file /Users/kireto/..../app/models/paper_trail/version.rb is ignored because PaperTrail::Version is already defined

What I had to do for the spec to pass is to do

# config/initializers/paper_trail.rb
require "paper_trail/version"

And I don't believe this should be used, especially with zeitwerk. Right?

[AdrienGiboire]

March 2023: I can report this issue to be current still running Rails 6.1 with Zeitwerk and PT 14.

Loading development environment (Rails 6.1.3.2)
irb(main):001:0> Rails.application.config.autoloader
=> :zeitwerk
irb(main):002:0> Module.const_source_location("PaperTrail::Version")
=> ["/home/adrien/.gem/ruby/2.7.4/gems/paper_trail-14.0.0/lib/paper_trail/frameworks/active_record/models/paper_trail/version.rb", 13]
irb(main):003:0>

[AdrienGiboire]

@weezing Inheritance only works if you work from the item and up, but is useless if your starting point is PaperTrail as in:

Contract.new.versions # works
# versus
PaperTrail::Version.includes(:item).where(item_type: 'Contract').order(created_at: :desc) # does not work

[AdrienGiboire]

I have tried @thebravoman solution and it works.

Create an initializer that requires our custom model.