issue with primary key other than `id` #868

notEthan · 2016-09-27T17:06:21Z

attempting to reify records with a primary key other than id can run into an error NoMethodError: undefined method 'find_by_id' when the Version#item association does not exist, due to either not being in the default scope, or the record no longer existing.

this fixes use of find_by_id to instead use whatever the primary key is.

I tried to heed rubocop warnings and follow existing testing conventions, but that may need some changes. I had some trouble testing the full matrix, will see what travis does with this PR.

notEthan · 2016-09-27T17:08:34Z

test/dummy/db/migrate/20110208155312_set_up_test_tables.rb

+      t.text     :object
+      t.datetime :created_at
+    end
+    add_index :orange_versions, [:item_type, :item_id]


orange is largely modeled after banana, without the Kitchen namespace, and changed to use a string item_id

notEthan · 2016-09-27T17:37:37Z

ugh. I don't know why it is complaining about this

lib/paper_trail/reifier.rb:34:23: C: Space inside { missing.
          find_cond = {klass.primary_key => version.item_id}
                      ^
lib/paper_trail/reifier.rb:34:60: C: Space inside } missing.
          find_cond = {klass.primary_key => version.item_id}
                                                           ^

that is a hash, it should not have spaces

jaredbeck · 2016-09-27T18:52:08Z

lib/paper_trail/reifier.rb

@@ -31,7 +31,8 @@ def reify(version, options)
          klass = version_reification_class(version, attrs)
          # The `dup` option always returns a new object, otherwise we should
          # attempt to look for the item outside of default scope(s).
-          if options[:dup] || (item_found = klass.unscoped.find_by_id(version.item_id)).nil?
+          find_cond = { klass.primary_key => version.item_id }
+          if options[:dup] || (item_found = klass.unscoped.where(find_cond).first).nil?


This part looks good to me.

jaredbeck · 2016-09-27T18:55:08Z

test/dummy/app/models/orange.rb

+  before_create do
+    self.uuid ||= SecureRandom.uuid
+  end
+end


Instead of calling this model Orange, what about CustomPrimaryKeyRecord?

We have so many models in our test suite it's getting hard to remember how they differ.

Please require "securerandom" at the top of this file.

jaredbeck · 2016-09-27T18:56:35Z

test/dummy/app/versions/orange_version.rb

@@ -0,0 +1,3 @@
+class OrangeVersion < PaperTrail::Version
+  self.table_name = "orange_versions"
+end


The orange_versions table is necessary because you need a table whose item_id column is a string, right?

jaredbeck · 2016-09-27T19:00:05Z

test/dummy/db/migrate/20110208155312_set_up_test_tables.rb

+      t.datetime :created_at
+    end
+    add_index :orange_versions, [:item_type, :item_id]
+


When we commit changes to 20110208155312_set_up_test_tables.rb, we also must commit the corresponding changes to test/dummy/db/schema.rb.

jaredbeck · 2016-09-27T19:02:22Z

that is a hash, it should not have spaces

This project follows the Ruby Style Guide, see https://github.com/bbatsov/ruby-style-guide#collections

batter · 2016-09-27T19:13:42Z

test/dummy/app/models/orange.rb

+  has_paper_trail class_name: "OrangeVersion"
+  default_scope -> { where(name: "orange") }
+
+  self.primary_key = :uuid


Can we make this the first declaration within the model?

notEthan · 2016-09-27T19:33:37Z

that is a hash, it should not have spaces

This project follows the Ruby Style Guide, see https://github.com/bbatsov/ruby-style-guide#collections

well, quoting

# good - space after { and before }
{ one: 1, two: 2 }

# good - no space after { and before }
{one: 1, two: 2}

but I have changed it.

notEthan · 2016-09-27T19:34:52Z

I'm not sure how to get the schema.rb updated, I have had trouble getting the tests to run locally in many cases. I think I have addressed the rest of the feedback, can you help me with the schema.rb bit?

notEthan · 2016-09-27T19:50:34Z

hm. item_id as string doesn't seem to work so well with postgres. investigating further ...

jaredbeck · 2016-09-27T21:22:44Z

can you help me with the schema.rb bit?

I think I ususally cd into test/dummy and run db:migrate.

notEthan · 2016-09-27T22:14:42Z

okay, so this error:

PG::UndefinedFunction: ERROR:  operator does not exist: character varying = integer
LINE 1: ...RE "custom_primary_key_record_versions"."item_id" = 651 AND ...
                                                             ^
HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.
: SELECT  "custom_primary_key_record_versions".* FROM "custom_primary_key_record_versions"  WHERE "custom_primary_key_record_versions"."item_id" = 651 AND "custom_primary_key_record_versions"."item_type" = 'CustomPrimaryKeyRecord' ORDER BY "custom_primary_key_record_versions"."created_at" DESC, "custom_primary_key_record_versions"."id" DESC LIMIT 1

was just an issue with how I was specifying the uuid primary key, in a way that wasn't supported by older activerecord, and uuid was being incorrectly created as an int instead of string.

that does surface an issue that if you have a versions table with a string versions.item_id, it can only be used with string primary key fields in postgres. if you have an application you perhaps started with traditional numeric id fields but are transitioning to a uuid (or any string) primary key, you won't be able to use the same versions table to track both sorts of models.

I don't have a solution for that (perhaps smart type casting in a query somewhere - not sure what capability activerecord even offers in that regard), just noting it as it occurs to me.

notEthan · 2016-09-27T22:27:23Z

seems to be largely passing, I think feedback is addressed. Gem::InstallError: pg requires Ruby version >= 2.0.0. on 1.9.3 appears unrelated to this pull request

jaredbeck · 2016-09-28T19:48:39Z

.. Gem::InstallError: pg requires Ruby version >= 2.0.0. on 1.9.3 appears unrelated to this pull request

Correct, and fixed by #870. Please rebase. It would be great if you could get the tests green before we review again. Let us know if you have trouble, thanks.

notEthan · 2016-09-28T20:03:44Z

💥 rebased, green

jaredbeck · 2016-09-28T20:18:14Z

test/dummy/app/models/custom_primary_key_record.rb

+  self.primary_key = :uuid
+
+  has_paper_trail class_name: "CustomPrimaryKeyRecordVersion"
+  default_scope -> { where(name: "custom_primary_key_record") }


What's the purpose of this default_scope?

it is to prevent the record from being returned on version.item. this bug surfaces in the else path of https://github.com/airblade/paper_trail/blob/v5.2.2/lib/paper_trail/reifier.rb#L25-L40

version.item returns nil when the item is destroyed, and when a default scope on the model prevents the association from returning it. I added this default scope to test both of those cases.

ok that makes sense. Please add your explanation as a comment above the default_scope.

Also, please add a line in CHANGELOG.md and I think we'll be done here.

jaredbeck · 2016-09-28T20:19:30Z

This all looks good, my only question is what is the purpose of the default_scope in custom_primary_key_record.rb?

…eird default scope

… long for mysql

…R 3 supports - previous way worked with 4 and 5 but not 3

notEthan · 2016-09-28T20:54:44Z

added explanatory comment and CHANGELOG entry

jaredbeck · 2016-09-28T22:01:13Z

Merged, thanks!

notEthan · 2016-09-28T23:36:42Z

cool, thanks

* support reifying using primary_key field other than 'id' * custom_primary_key_record - model to use a uuid primary key, with a weird default scope * test custom_primary_key_record also with destroyed record * name index on custom_primary_key_record_versions item; default is too long for mysql * specify custom_pk_record uuid as a string primary key in a way that AR 3 supports - previous way worked with 4 and 5 but not 3 * update schema for custom_primary_key_records * note fix in changelog

notEthan commented Sep 27, 2016

View reviewed changes

jaredbeck requested changes Sep 27, 2016

View reviewed changes

batter reviewed Sep 27, 2016

View reviewed changes

jaredbeck reviewed Sep 28, 2016

View reviewed changes

jaredbeck approved these changes Sep 28, 2016

View reviewed changes

notEthan added 7 commits September 28, 2016 13:52

custom_primary_key_record - model to use a uuid primary key, with a w…

afa8781

…eird default scope

support reifying using primary_key field other than 'id'

4d9ebd5

test custom_primary_key_record also with destroyed record

59d564c

name index on custom_primary_key_record_versions item; default is too…

ce2ce84

… long for mysql

specify custom_pk_record uuid as a string primary key in a way that A…

65e2f72

…R 3 supports - previous way worked with 4 and 5 but not 3

update schema for custom_primary_key_records

a2dc186

note fix in changelog

e8bad18

jaredbeck merged commit c900f2d into paper-trail-gem:master Sep 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue with primary key other than `id` #868

issue with primary key other than `id` #868

notEthan commented Sep 27, 2016

notEthan Sep 27, 2016 •

edited

Loading

notEthan commented Sep 27, 2016

jaredbeck Sep 27, 2016

jaredbeck Sep 27, 2016

jaredbeck Sep 27, 2016 •

edited

Loading

jaredbeck Sep 27, 2016 •

edited

Loading

notEthan Sep 27, 2016

jaredbeck Sep 27, 2016

jaredbeck commented Sep 27, 2016

batter Sep 27, 2016

notEthan commented Sep 27, 2016

notEthan commented Sep 27, 2016

notEthan commented Sep 27, 2016

jaredbeck commented Sep 27, 2016

notEthan commented Sep 27, 2016

notEthan commented Sep 27, 2016

jaredbeck commented Sep 28, 2016

notEthan commented Sep 28, 2016

jaredbeck Sep 28, 2016

notEthan Sep 28, 2016

jaredbeck Sep 28, 2016

jaredbeck commented Sep 28, 2016

notEthan commented Sep 28, 2016

jaredbeck commented Sep 28, 2016

notEthan commented Sep 28, 2016

issue with primary key other than id #868

issue with primary key other than id #868

Conversation

notEthan commented Sep 27, 2016

notEthan Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

notEthan commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredbeck Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

jaredbeck Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredbeck commented Sep 27, 2016

Choose a reason for hiding this comment

notEthan commented Sep 27, 2016

notEthan commented Sep 27, 2016

notEthan commented Sep 27, 2016

jaredbeck commented Sep 27, 2016

notEthan commented Sep 27, 2016

notEthan commented Sep 27, 2016

jaredbeck commented Sep 28, 2016

notEthan commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredbeck commented Sep 28, 2016

notEthan commented Sep 28, 2016

jaredbeck commented Sep 28, 2016

notEthan commented Sep 28, 2016

issue with primary key other than `id` #868

issue with primary key other than `id` #868

notEthan Sep 27, 2016 •

edited

Loading

jaredbeck Sep 27, 2016 •

edited

Loading

jaredbeck Sep 27, 2016 •

edited

Loading