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

Incompatible with ActiveRecord 7's composite primary keys #1455

Closed
3 tasks done
contentfree opened this issue Dec 26, 2023 · 4 comments
Closed
3 tasks done

Incompatible with ActiveRecord 7's composite primary keys #1455

contentfree opened this issue Dec 26, 2023 · 4 comments
Labels

Comments

@contentfree
Copy link

contentfree commented Dec 26, 2023

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:

  • This is not a usage question, this is a bug report
  • This bug can be reproduced with the script I provide below
  • 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 "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
Copy link
Author

contentfree commented Dec 26, 2023

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
Copy link
Author

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

@amozoss
Copy link

amozoss commented Feb 15, 2024

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 :)

Copy link

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.

@github-actions github-actions bot added the Stale label May 16, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants