Does not track has_many :through deletion

[leoc]

I have switched from has_and_belongs_to_many to has_many :through, so that I can track the ProfileUser association in separate Version records. Unfortunately paper_trail does not track the deletion of related users when I change the users Array in my profile record.

# 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.3.0"
  source "https://rubygems.org"
  gem "activerecord", "5.0.0"
  gem "minitest", "5.9.0"
  gem "paper_trail", "5.2.0", require: false
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = nil
ActiveRecord::Schema.define do
  # STEP TWO: Define your tables here.
  create_table :profiles, force: true do |t|
    t.timestamps null: false
  end

  create_table :profile_users, force: true do |t|
    t.references :profile
    t.references :user
  end

  create_table :users, force: true do |t|
    t.timestamps null: false
  end

  create_table :versions do |t|
    t.string :item_type, null: false
    t.integer :item_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.integer :transaction_id
    t.datetime :created_at
  end
  add_index :versions, [:item_type, :item_id]
  add_index :versions, [:transaction_id]

  create_table :version_associations do |t|
    t.integer  :version_id
    t.string   :foreign_key_name, null: false
    t.integer  :foreign_key_id
  end
  add_index :version_associations, [:version_id]
  add_index :version_associations, [:foreign_key_name, :foreign_key_id],
    name: "index_version_associations_on_foreign_key"
end
ActiveRecord::Base.logger = Logger.new(STDOUT)
require "paper_trail/config"

# STEP THREE: Configure PaperTrail as you would in your initializer
PaperTrail::Config.instance.track_associations = false

require "paper_trail"

# STEP FOUR: Define your AR models here.
class User < ActiveRecord::Base
  has_paper_trail

  has_many :profile_users
  has_many :profiles, through: :profile_users
end

class Profile < ActiveRecord::Base
  has_paper_trail

  has_many :profile_users
  has_many :users, through: :profile_users
end

class ProfileUser < ActiveRecord::Base
  has_paper_trail

  belongs_to :profile
  belongs_to :user
end

# STEP FIVE: Please write a test that demonstrates your issue.
class BugTest < ActiveSupport::TestCase
  def test_1
    user = User.create
    profile = Profile.create

    assert_difference(-> { PaperTrail::Version.count }, +1) {
      profile.users = [user]
      profile.save
    }

    assert_difference(-> { PaperTrail::Version.count }, +1) {
      profile.users = []
      profile.save
    }
  end
end

=>

Finished in 0.067756s, 14.7587 runs/s, 29.5175 assertions/s.

  1) Failure:
BugTest#test_1 [test.rb:98]:
#<Proc:0x007fafc8336b00@test.rb:98 (lambda)> didn't change by 1.
Expected: 4
  Actual: 3

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

[leoc]

I found out about issue #113 .

But still the behaviour is not what I would expect. The object_changes is empty, resulting in a version that is not really useful.

# 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.3.0"
  source "https://rubygems.org"
  gem "activerecord", "5.0.0"
  gem "minitest", "5.9.0"
  gem "paper_trail", "5.2.0", require: false
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = nil
ActiveRecord::Schema.define do
  # STEP TWO: Define your tables here.
  create_table :profiles, force: true do |t|
    t.timestamps null: false
  end

  create_table :profile_users, force: true do |t|
    t.references :profile
    t.references :user
  end

  create_table :users, force: true do |t|
    t.timestamps null: false
  end

  create_table :versions do |t|
    t.string :item_type, null: false
    t.integer :item_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.integer :transaction_id
    t.datetime :created_at
  end
  add_index :versions, [:item_type, :item_id]
  add_index :versions, [:transaction_id]

  create_table :version_associations do |t|
    t.integer  :version_id
    t.string   :foreign_key_name, null: false
    t.integer  :foreign_key_id
  end
  add_index :version_associations, [:version_id]
  add_index :version_associations, [:foreign_key_name, :foreign_key_id],
    name: "index_version_associations_on_foreign_key"
end
ActiveRecord::Base.logger = Logger.new(STDOUT)
require "paper_trail/config"

# STEP THREE: Configure PaperTrail as you would in your initializer
PaperTrail::Config.instance.track_associations = false
module ActiveRecord
  module Associations
    class HasManyThroughAssociation < HasManyAssociation #:nodoc:
      alias_method :original_delete_records, :delete_records

      def delete_records(records, method)
        method ||= :destroy
        original_delete_records(records, method)
      end
    end
  end
end

require "paper_trail"

# STEP FOUR: Define your AR models here.
class User < ActiveRecord::Base
  has_paper_trail

  has_many :profile_users
  has_many :profiles, through: :profile_users
end

class Profile < ActiveRecord::Base
  has_paper_trail

  has_many :profile_users
  has_many :users, through: :profile_users
end

class ProfileUser < ActiveRecord::Base
  has_paper_trail

  belongs_to :profile
  belongs_to :user
end

# STEP FIVE: Please write a test that demonstrates your issue.
class BugTest < ActiveSupport::TestCase
  def test_1
    user = User.create
    profile = Profile.create

    assert_difference(-> { PaperTrail::Version.count }, +1) {
      profile.users = [user]
      profile.save
    }

    assert_equal <<YAML, PaperTrail::Version.last.object_changes
---
id:
- 
- 1
profile_id:
- 
- 1
user_id:
- 
- 1
YAML

    assert_difference(-> { PaperTrail::Version.count }, +1) {
      profile.users = []
      profile.save
    }

    assert_equal <<YAML, PaperTrail::Version.last.object_changes
---
id:
- 1
- 
profile_id:
- 1
- 
user_id:
- 1
- 
YAML
  end
end

=>

Finished in 0.083750s, 11.9404 runs/s, 47.7615 assertions/s.

  1) Failure:
BugTest#test_1 [test.rb:128]:
--- expected
+++ actual
@@ -1,11 +1 @@
-"---
-id:
-- 1
-- 
-profile_id:
-- 1
-- 
-user_id:
-- 1
-- 
-"
+nil


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

[jaredbeck]

Hi Arthur, thanks for testing the experimental association-tracking features.

The object_changes is empty, resulting in a version that is not really useful.

We only store attributes in the object_changes column. We do not store associations there.

[leoc]

Hi Jared, I don't really want to use the associations feature.
What I try to accomplish should work without it. I corrected the example and disabled the associations flag again.

I am using has_many :through through a many-many-relationship model, yes. But this model is destroyed via ActiveRecord::Relation#destroy_all which then invokes #destroy. Shouldn't that save the model with all its old attributes?

As in:

ProfileUser record with profile_id: 1, user_id: 1 gets destroyed by #destroy. This should create a version with the ProfileUser's old attributes, no? At least that's the behaviour I expect from all other models that have paper_trail.

Please correct me if I am wrong, maybe there is another solution for this.

And thanks for your prompt help 👍

[jaredbeck]

This is the first I've heard of overriding AR's delete_records method. I'm not familiar with either the method or the technique of overriding it. I'd encourage you to avoid such patches and use standard API like :dependent => :destroy if possible.

[leoc]

I know overriding delete_records might be a bad idea, although it is an advise from the README.

Wouldn't dependent: :destroy only affect how to proceed with associated objects if the owning record was destroyed? For this particular problem I don't think that's what I need?

user = User.create
profile = Profile.create(users: [user])
profile.users = []
profile.save

ActiveRecord takes care of calling #destroy on all removed ProfileUser records that where associating the profile with the respective users. Somehow the resulting Version does not hold the old attributes.

As far as I can see, that's a paper_trail issue. I haven't found a good hint yet, why that is, though. :)

[jaredbeck]

Wouldn't dependent: :destroy only affect how to proceed with associated objects if the owning record was destroyed?

When the associated objects are destroyed, a record representing each deletion will be inserted into versions. See https://github.com/airblade/paper_trail/#2a-choosing-lifecycle-events-to-monitor

[leoc]

Alright. Thanks for the hint! But the object_changes is still empty.

# 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.3.0"
  source "https://rubygems.org"
  gem "activerecord", "5.0.0"
  gem "minitest", "5.9.0"
  gem "paper_trail", "5.2.0", require: false
  gem "sqlite3"
  gem "pry"
  gem "pry-byebug"
end

require "active_record"
require "minitest/autorun"
require "logger"
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = nil
ActiveRecord::Schema.define do
  # STEP TWO: Define your tables here.
  create_table :profiles, force: true do |t|
    t.timestamps null: false
  end

  create_table :profile_users, force: true do |t|
    t.references :profile
    t.references :user
  end

  create_table :users, force: true do |t|
    t.timestamps null: false
  end

  create_table :versions do |t|
    t.string :item_type, null: false
    t.integer :item_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.integer :transaction_id
    t.datetime :created_at
  end
  add_index :versions, [:item_type, :item_id]
  add_index :versions, [:transaction_id]

  create_table :version_associations do |t|
    t.integer  :version_id
    t.string   :foreign_key_name, null: false
    t.integer  :foreign_key_id
  end
  add_index :version_associations, [:version_id]
  add_index :version_associations, [:foreign_key_name, :foreign_key_id],
    name: "index_version_associations_on_foreign_key"
end
ActiveRecord::Base.logger = Logger.new(STDOUT)
require "paper_trail/config"

# STEP THREE: Configure PaperTrail as you would in your initializer
PaperTrail::Config.instance.track_associations = false

require "paper_trail"

# STEP FOUR: Define your AR models here.
class User < ActiveRecord::Base
  has_paper_trail

  has_many :profile_users
  has_many :profiles, through: :profile_users, dependent: :destroy
end

class Profile < ActiveRecord::Base
  has_paper_trail

  has_many :profile_users
  has_many :users, through: :profile_users, dependent: :destroy
end

class ProfileUser < ActiveRecord::Base
  has_paper_trail

  belongs_to :profile
  belongs_to :user
end

# STEP FIVE: Please write a test that demonstrates your issue.
class BugTest < ActiveSupport::TestCase
  def test_1
    user = User.create
    profile = Profile.create

    assert_difference(-> { PaperTrail::Version.count }, +1) {
      profile.users = [user]
      profile.save
    }

    assert_equal <<YAML, PaperTrail::Version.last.object_changes
---
id:
- 
- 1
profile_id:
- 
- 1
user_id:
- 
- 1
YAML

    assert_difference(-> { PaperTrail::Version.count }, +1) {
      profile.users = []
      profile.save
    }

    assert_equal <<YAML, PaperTrail::Version.last.object_changes
---
id:
- 1
- 
profile_id:
- 1
- 
user_id:
- 1
- 
YAML
  end
end

=>

Finished in 0.104162s, 9.6005 runs/s, 38.4018 assertions/s.

  1) Failure:
BugTest#test_1 [test.rb:118]:
--- expected
+++ actual
@@ -1,11 +1 @@
-"---
-id:
-- 1
-- 
-profile_id:
-- 1
-- 
-user_id:
-- 1
-- 
-"
+nil


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

[jaredbeck]

Alright. Thanks for the hint! But the object_changes is still empty.

Because you are using PaperTrail::Version.last in your assertion it is not clear to me which version record the assertion applies to. Are we making an assertion about a Profile record or a User record?

[leoc]

The assertion is about the Version for the ProfileUser record.

ProfileUser is used for the many-to-many-relation between users and profiles. When I remove users from a profile, only the associating record (ProfileUser) should be deleted. But when deleting and tracking such a ProfileUser record, I want to assert that the version about the destroy event holds the deleted ProfileUser attributes.

[jaredbeck]

.. when deleting and tracking such a ProfileUser record, I want to assert that the version about the destroy event holds the deleted ProfileUser attributes.

Thanks for the clarification, I think I see what you're after now.

We currently set object_changes = null where versions.event = 'destroy'. See RecordTrail#record_destroy. I don't know why. Maybe @batter knows? I don't see any harm in adding this information to object_changes, other than increased disk usage.

Unless Ben says otherwise, feel free to implement this new feature in RecordTrail#record_destroy and submit a PR. Thanks.

[leoc]

Ah! I see, my bad, The information in object is enough, then!
I completely forgot about object which keeps the last attribute values.

Adding the object_changes would only add overhead.

I am sorry for the confusion! Thanks for your help, I owe you a beer or two. 👍