paper-trail-gem / paper_trail

Track changes to your rails models
MIT License
6.79k stars 896 forks source link

Incompatible with ActiveRecord 7's composite primary keys #1455

Closed contentfree closed 5 months ago

contentfree commented 10 months ago

Thank you for your contribution!

Due to limited volunteers, issues that do not follow these instructions will be closed without comment.

Check the following boxes:

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 "3.2.2"
  source "https://rubygems.org"
  gem "activerecord", "~> 7.1.2"
  gem "minitest", "5.11.3"
  gem "paper_trail", "15.1.0", require: false
  gem "sqlite3", "~> 1.6.0"
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 :campaigns, force: true do |t|
    t.timestamps null: false
  end

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

  create_table :campaign_options, force: true, id: false, primary_key: [:campaign_id, :option_id] do |t|
    t.bigint :campaign_id, null: false
    t.bigint :option_id, null: false
    t.text :value, null: false
    t.timestamps null: false
  end

  create_table :versions do |t|
    t.string :item_type, null: false
    t.text :item_id, null: false      # <<<<< note: using text here instead of bigint (as one issue suggested as a workaround)
    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 Campaign < ActiveRecord::Base 
  has_many :campaign_options
end
class Option < ActiveRecord::Base
  has_many :campaign_options
end

class CampaignOption < ActiveRecord::Base
  self.primary_key = [:campaign_id, :option_id]

  belongs_to :campaign 
  belongs_to :option

  has_paper_trail versions: {query_constraints: [:campaign_id, :option_id]}
end

# STEP FIVE: Please write a test that demonstrates your issue.
class BugTest < ActiveSupport::TestCase
  def test_1
    assert_difference(-> { PaperTrail::Version.count }, +1) {
      campaign = Campaign.create!
      option = Option.create!
      CampaignOption.create!(campaign: campaign, option: option, value: "Test")
    }
  end
end

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

Results in:

ActiveModel::MissingAttributeError: can't write unknown attribute ``
contentfree commented 10 months ago

This is possibly due to needing more configuration options in the belongs_to definition in VersionConcern at paper_trail-15.1.0/lib/paper_trail/version_concern.rb:25

belongs_to :item, polymorphic: true, optional: true, inverse_of: false
contentfree commented 9 months ago

Ran into this again today and Google lead me back to my own filed issue :D :'(

amozoss commented 8 months ago

Prior to 7.1, Composite primary keys had a way to represent ids as a string (e.g. 1,1) and convert it back so it kind of mostly worked by accident with paper trail

However, I think the main issue is rails 7.1 composite keys don't have a way to map the composite primary keys (:campaign_id, :option_id) into a single polymorphic field (:item_id). So if you add an extra field and set the query_constraints, it works.... (see below for what I mean) but that's not really a solution here

diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb
index c49bfe0..74d2014 100644
--- a/lib/paper_trail/version_concern.rb
+++ b/lib/paper_trail/version_concern.rb
@@ -22,7 +22,7 @@ module PaperTrail
     EOS

     included do
-      belongs_to :item, polymorphic: true, optional: true, inverse_of: false
+      belongs_to :item, polymorphic: true, optional: true, inverse_of: false, query_constraints: [:item_id, :item_id_2]
       validates_presence_of :event
       after_create :enforce_version_limit!
     end
diff --git a/my_bug_report.rb b/my_bug_report.rb
index 687c029..4e11112 100755
--- a/my_bug_report.rb
+++ b/my_bug_report.rb
@@ -11,7 +11,7 @@ gemfile(true) do
   source "https://rubygems.org"
   gem "activerecord", "~> 7.1.2"
   gem "minitest", "5.11.3"
-  gem "paper_trail", "15.1.0", require: false
+  gem "paper_trail", "15.1.0", require: false, path: "."
   gem "sqlite3", "~> 1.6.0"
 end

@@ -42,6 +42,7 @@ ActiveRecord::Schema.define do
   create_table :versions do |t|
     t.string :item_type, null: false
     t.text :item_id, null: false      # <<<<< note: using text here instead of bigint (as one issue suggested as a workaround)
+    t.text :item_id_2, null: true
     t.string :event, null: false
     t.string :whodunnit
     t.text :object, limit: 1_073_741_823

Something more dynamic is to not rely on polymorphism and just set the item_id and item_type directly.

diff --git a/lib/paper_trail/events/create.rb b/lib/paper_trail/events/create.rb
index 8042f37..cb63e1b 100644
--- a/lib/paper_trail/events/create.rb
+++ b/lib/paper_trail/events/create.rb
@@ -13,7 +13,8 @@ module PaperTrail
       # @api private
       def data
         data = {
-          item: @record,
+          item_id: @record.id,
+          item_type: @record.class.name,
           event: @record.paper_trail_event || "create",
           whodunnit: PaperTrail.request.whodunnit
         }

Then if you change the item_id to a json field and override item, it'll work with any kind of record.

diff --git a/my_bug_report.rb b/my_bug_report.rb
index 687c029..d43d00b 100755
--- a/my_bug_report.rb
+++ b/my_bug_report.rb
@@ -11,7 +11,7 @@ gemfile(true) do
   source "https://rubygems.org"
   gem "activerecord", "~> 7.1.2"
   gem "minitest", "5.11.3"
-  gem "paper_trail", "15.1.0", require: false
+  gem "paper_trail", "15.1.0", require: false, path: "."
   gem "sqlite3", "~> 1.6.0"
 end

@@ -41,7 +41,7 @@ ActiveRecord::Schema.define do

   create_table :versions do |t|
     t.string :item_type, null: false
-    t.text :item_id, null: false      # <<<<< note: using text here instead of bigint (as one issue suggested as a workaround)
+   t.json :item_id, null: false 
     t.string :event, null: false
     t.string :whodunnit
     t.text :object, limit: 1_073_741_823
@@ -53,10 +53,25 @@ end
 ActiveRecord::Base.logger = Logger.new(STDOUT)
 require "paper_trail"

+
 # STEP FOUR: Define your AR models here.
+
+class AuditLog < ActiveRecord::Base
+  include PaperTrail::VersionConcern
+  self.table_name = 'versions'
+  def item
+    item_type.constantize.find(item_id)
+  end
+end
+
 class Campaign < ActiveRecord::Base
   has_many :campaign_options
+  has_paper_trail versions: {class_name: 'AuditLog'}
 end
+
 class Option < ActiveRecord::Base
   has_many :campaign_options
 end
@@ -67,7 +82,7 @@ class CampaignOption < ActiveRecord::Base
   belongs_to :campaign
   belongs_to :option

-  has_paper_trail versions: {query_constraints: [:campaign_id, :option_id]}
+  has_paper_trail versions: { class_name: 'AuditLog', query_constraints: [:campaign_id, :option_id] }
 end

 # STEP FIVE: Please write a test that demonstrates your issue.

Not sure what else breaks doing it this way, but it works for my use case. Would be awesome for this to work out of the box though without my silly workarounds :)

github-actions[bot] commented 5 months ago

This issue has been automatically marked as stale due to inactivity. The resources of our volunteers are limited. Bug reports must provide a script that reproduces the bug, using our template. Feature suggestions must include a promise to build the feature yourself. Thank you for all your contributions.