Fix get version data with full name of class #996

chimame · 2017-09-20T13:21:22Z

Overview

Unable to acquire association history of model name including module name.

Impact point

It can be reproduced in the following cases.

class Family::Person < ApplicationRecord
  has_paper_trail

  has_many :children
end

class Family::Child < ApplicationRecord
  has_paper_trail

  belongs_to :person
end

person = Family::Person.new(name: 'person1')
person.children.build(name: 'child1')
person.save #=> create versions. 1 is person1, 2 is child1

person.name = 'person2'
person.children.build(name: 'child2')
person.save #=> create versions. 3 is person2, 4 is child2

version = PaperTrail::Version.find(3) #=> version of person
person_history = version.reify(has_many: true)
person_history.children #=> Get current child1 and child2. Correctly, it is to get history of child1.

This happens because item_type does not include the module name when acquiring an association.

SELECT
  "versions".*
FROM
  "versions"
WHERE (id IN (
  SELECT
    MIN(version_id)
  FROM
    "version_associations"
  INNER JOIN
    "versions"
  ON
    "versions"."id" = "version_associations"."version_id"
  WHERE
    (foreign_key_name = 'person_id') AND
    (foreign_key_id = 1) AND
    (versions.item_type = 'Child') AND -- <= It does not include module name
    (created_at >= '2017-09-20 12:20:26.080113' OR transaction_id = 4)
  GROUP BY item_id))

jaredbeck · 2017-09-20T17:21:42Z

Hi @chimame, thanks for the contribution!

Please include:

a test (maybe in spec/paper_trail/associations_spec.rb or in spec/models)
a changelog entry under Unreleased -> Fixed

Thanks!

jaredbeck

changes requested

Needs second review

jaredbeck · 2017-09-22T19:41:13Z

It would be great if we could do a test without introducing any new AR models; we already have so many.

chimame · 2017-09-23T02:15:32Z

Hi @jaredbeck, Thank you for reviewing.

What's going on?

Do you mean that we want you to create a test without creating a new AR?
If so, does it make sense to create a test using Kitchen::Banana?

jaredbeck · 2017-09-24T05:15:55Z

It would be great if we could do a test without introducing any new AR models; we already have so many.

Do you mean that we want you to create a test without creating a new AR?

Yes. Currently this PR adds five new tables to the test suite. Please add fewer tables, if possible. Thanks!

jaredbeck · 2017-09-25T22:53:14Z

Let me know if you need help with getting the tests to pass.

chimame · 2017-09-26T01:39:25Z

Thank you for following me.
I am planning to deal with less generation of models.

chimame · 2017-09-28T14:32:50Z

Please review once again.

jaredbeck

This looks good, thanks! Only one minor change: "mentor" instead of "menter".

jaredbeck · 2017-09-28T16:05:55Z

spec/dummy_app/app/models/family.rb

+module Family
+  def self.table_name_prefix
+    "family_"
+  end


Neat! I didn't know about this method. I will be able to use this at work.

jaredbeck · 2017-09-28T16:19:26Z

spec/paper_trail/associations_spec.rb

+          expect(previous_children.parent.name).to eq "parent1"
+        end
+      end
+    end


Thank you for testing all four association types.

jaredbeck · 2017-09-28T16:21:14Z

spec/dummy_app/app/models/family/person.rb

+      belongs_to :parent, class_name: "Family::Person", foreign_key: :parent_id
+    end
+    if ActiveRecord.gem_version >= Gem::Version.new("5.0")
+      belongs_to :menter, class_name: "Family::Person", foreign_key: :partner_id, optional: true


"Mentor" is the correct spelling. English is silly.

jaredbeck · 2017-09-28T16:26:23Z

Actually, I think we can delete the menter association, because it is not used anywhere?

jaredbeck · 2017-09-28T16:32:36Z

Hmmm ... bad news. I can apply the following patch and all tests pass, including your new tests.

diff --git a/lib/paper_trail/reifiers/belongs_to.rb b/lib/paper_trail/reifiers/belongs_to.rb
index 483f791..3ff54b8 100644
--- a/lib/paper_trail/reifiers/belongs_to.rb
+++ b/lib/paper_trail/reifiers/belongs_to.rb
@@ -37,7 +37,7 @@ module PaperTrail
         # @api private
         def load_version(assoc, id, transaction_id, version_at)
           assoc.klass.paper_trail.version_class.
-            where("item_type = ?", assoc.klass.name).
+            where("item_type = ?", assoc.class_name).
             where("item_id = ?", id).
             where("created_at >= ? OR transaction_id = ?", version_at, transaction_id).
             order("id").limit(1).first
diff --git a/lib/paper_trail/reifiers/has_many.rb b/lib/paper_trail/reifiers/has_many.rb
index 2c1c2c7..486acb2 100644
--- a/lib/paper_trail/reifiers/has_many.rb
+++ b/lib/paper_trail/reifiers/has_many.rb
@@ -98,7 +98,7 @@ module PaperTrail
             select("MIN(version_id)").
             where("foreign_key_name = ?", assoc.foreign_key).
             where("foreign_key_id = ?", model.id).
-            where("#{version_table}.item_type = ?", assoc.klass.name).
+            where("#{version_table}.item_type = ?", assoc.class_name).
             where("created_at >= ? OR transaction_id = ?", version_at, tx_id).
             group("item_id").
             to_sql
diff --git a/lib/paper_trail/reifiers/has_many_through.rb b/lib/paper_trail/reifiers/has_many_through.rb
index af24206..fac4ef9 100644
--- a/lib/paper_trail/reifiers/has_many_through.rb
+++ b/lib/paper_trail/reifiers/has_many_through.rb
@@ -73,7 +73,7 @@ module PaperTrail
         def load_versions_for_hmt_association(assoc, ids, tx_id, version_at)
           version_id_subquery = assoc.klass.paper_trail.version_class.
             select("MIN(id)").
-            where("item_type = ?", assoc.klass.name).
+            where("item_type = ?", assoc.class_name).
             where("item_id IN (?)", ids).
             where(
               "created_at >= ? OR transaction_id = ?",
diff --git a/lib/paper_trail/reifiers/has_one.rb b/lib/paper_trail/reifiers/has_one.rb
index 85b96b7..ad66112 100644
--- a/lib/paper_trail/reifiers/has_one.rb
+++ b/lib/paper_trail/reifiers/has_one.rb
@@ -36,7 +36,7 @@ module PaperTrail
           model.class.paper_trail.version_class.joins(:version_associations).
             where("version_associations.foreign_key_name = ?", assoc.foreign_key).
             where("version_associations.foreign_key_id = ?", model.id).
-            where("#{version_table_name}.item_type = ?", assoc.klass.name).
+            where("#{version_table_name}.item_type = ?", assoc.class_name).
             where("created_at >= ? OR transaction_id = ?", version_at, transaction_id).
             order("#{version_table_name}.id ASC").
             first

Assuming I'm running the test suite correctly, this means that your new tests do not actually demonstrate a problem.

What am I missing?

chimame · 2017-10-01T12:51:44Z

I'm sorry.
The bug reported by me does not seem to occur when I give table name prefix.
So, I changed it to not give it.

Actually, I think we can delete the menter association, because it is not used anywhere?

It can not be deleted.
Because we need to define "belongs_to" when creating version associations.

jaredbeck · 2017-10-03T15:59:02Z

The directory name models/Family does not match the file naming conventions for ruby. Please avoid using upper case letters in file names.

jaredbeck · 2017-10-03T16:03:11Z

I'm sorry. The bug reported by me does not seem to occur when I give table name prefix. So, I changed it to not give it.

I want the tests to fail when I apply this patch. Are you ready for me to perform that experiment? Thanks!

chimame · 2017-10-04T00:13:36Z

The test will fail if the specified patch is applied. please make sure.
Thank you!

jaredbeck · 2017-10-05T01:58:55Z

The test will fail if the specified patch is applied. please make sure.

When I apply the patch, the tests fail. Thanks!

jaredbeck · 2017-10-05T02:16:03Z

Released as 8.0.0.

I avoid using `let` and `before` unless necessary.

When a record is inserted into the `versions` table, it is given an `item_type` like `Foo::Bar`, but the association reification queries were searching for an `item_type` like `::Foo::Bar`.

I avoid using `let` and `before` unless necessary.

Fix get version data with full name of class

5c1c543

jaredbeck previously requested changes Sep 20, 2017

View reviewed changes

chimame force-pushed the fix_association_can_not_be_loaded branch 4 times, most recently from ad06cc2 to ebe3766 Compare September 22, 2017 15:36

Add spec for model with module name give

3f714e7

chimame force-pushed the fix_association_can_not_be_loaded branch from ebe3766 to 3f714e7 Compare September 22, 2017 15:46

Modify version was not created correctly for spec

aea6b54

Modify model definition for module name spec

775dae5

Modify schema.rb

25f23c7

jaredbeck requested changes Sep 28, 2017

View reviewed changes

Delete table name prefix

275c4e3

Change directory name to lower case

7c34f5c

jaredbeck approved these changes Oct 5, 2017

View reviewed changes

jaredbeck merged commit 56991ae into paper-trail-gem:master Oct 5, 2017

jaredbeck added a commit that referenced this pull request Oct 5, 2017

Simplify tests added by #996

383b4d2

I avoid using `let` and `before` unless necessary.

chimame deleted the fix_association_can_not_be_loaded branch October 5, 2017 13:02

aried3r pushed a commit to aried3r/paper_trail that referenced this pull request Dec 14, 2020

Simplify tests added by paper-trail-gem#996

5fbd236

I avoid using `let` and `before` unless necessary.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix get version data with full name of class #996

Fix get version data with full name of class #996

chimame commented Sep 20, 2017

jaredbeck commented Sep 20, 2017

jaredbeck left a comment

jaredbeck commented Sep 22, 2017

chimame commented Sep 23, 2017

jaredbeck commented Sep 24, 2017

jaredbeck commented Sep 25, 2017

chimame commented Sep 26, 2017

chimame commented Sep 28, 2017

jaredbeck left a comment •

edited

Loading

jaredbeck Sep 28, 2017

jaredbeck Sep 28, 2017

jaredbeck Sep 28, 2017

jaredbeck commented Sep 28, 2017

jaredbeck commented Sep 28, 2017

chimame commented Oct 1, 2017

jaredbeck commented Oct 3, 2017

jaredbeck commented Oct 3, 2017

chimame commented Oct 4, 2017

jaredbeck commented Oct 5, 2017

jaredbeck commented Oct 5, 2017

Fix get version data with full name of class #996

Fix get version data with full name of class #996

Conversation

chimame commented Sep 20, 2017

Overview

Impact point

jaredbeck commented Sep 20, 2017

jaredbeck left a comment

Choose a reason for hiding this comment

jaredbeck commented Sep 22, 2017

chimame commented Sep 23, 2017

jaredbeck commented Sep 24, 2017

jaredbeck commented Sep 25, 2017

chimame commented Sep 26, 2017

chimame commented Sep 28, 2017

jaredbeck left a comment • edited Loading

Choose a reason for hiding this comment

jaredbeck Sep 28, 2017

Choose a reason for hiding this comment

jaredbeck Sep 28, 2017

Choose a reason for hiding this comment

jaredbeck Sep 28, 2017

Choose a reason for hiding this comment

jaredbeck commented Sep 28, 2017

jaredbeck commented Sep 28, 2017

chimame commented Oct 1, 2017

jaredbeck commented Oct 3, 2017

jaredbeck commented Oct 3, 2017

chimame commented Oct 4, 2017

jaredbeck commented Oct 5, 2017

jaredbeck commented Oct 5, 2017

jaredbeck left a comment •

edited

Loading