Adding CPK to a repo breaks polymorphic joins to legacy single-PK classes whose `to_param` is not based upon the `id`

brightbytes-dude commented 3 years ago

I have a legacy class with the normal id as its PK, but whose to_param method returns the (unique) slug of the object.

Several places in my codebase have a polymorphic join to instances of that class, and they've been working fine for years.

However, when I drop in CPK and use the self.primary_keys = declaration on another, entirely unrelated class (so unrelated it's in a different DB entirely), all specs that involve creating a polymorphic join to an instance of that legacy class fail.

It's not an option for me to modify either the PK or the to_param of the legacy class: it's used in too many places for me to be comfortable with the side-effects.

I'm able to work around the issue by explicitly setting the _type and _id attribute of the polymorphic joins when creating/updating them - and am forced to do so because I absolutely need CPK for its use case - but it's unsatisfying to do so.

And, with test coverage not where it should be (the legacy app is going on 9 years old), there are probably some cases I'll miss until QA or our customers hit them, which is unfortunate.

So, a general fix would be awesome.

cfis commented 3 years ago

Sorry for the late reply.

Ok so I would need some more information to really understand what is happening. Can you provide:

A traceback of what error you are seeing?
The code you are using to workaround the issue

And what would be really great, if possible, is a failing test that uses CPK's existing test database.

brightbytes-dude commented 3 years ago

No worries: I had a workaround, so it wasn't a big deal.

Below is the issue; sorry I'm not doing it in your test context, but I'm slammed right now, and it's pretty straightforward. :-)

Notes:

Before adding CPK to this repo, the erring example below worked fine.
Both of the classes below have the normal id field as their PK.
I added CPK to this repo because I needed to nuke the id PK of a completely unrelated model (in another DB entirely) that fortunately had a uniqueness constraint already present.
As an irrelevant aside, I needed to nuke the id PK of that table because it was created as an :integer by an earlier version of Rails, and the background process that creates rows in that table had just hit the max allocatable integer value in staging (thankfully!!!). Changing to :bigint wasn't an option, since it would take many hours to complete. (I mention this because CPK and the preexisting unique index saved the app from downtime doom.)

The players:

# Legacy model
class UserComment < ApplicationRecord
  # ...
  belongs_to :commentable, polymorphic: true
  # ...
end

# Has :id as its PK, but :to_param returns the slug field
class AddOn < ApplicationRecord
  # ...
  alias_attribute :to_param, :slug
  # ...
end

The error with CPK installed; this worked fine previous to CPK:

> UserComment.where(user: User.first, commentable: AddOn.first, comment_data: 'blah blah blah').create!
  User Load (0.8ms)  SELECT  "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT $1  [["LIMIT", 1]]
  AddOn Load (0.4ms)  SELECT  "add_ons".* FROM "add_ons" ORDER BY "add_ons"."id" ASC LIMIT $1  [["LIMIT", 1]]
   (0.2ms)  BEGIN
   (0.2ms)  ROLLBACK
Validation failed: Commentable can't be blank, Commentable can't be blank, Commentable type is not included in the list, Commentable type can't be blank
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/validations.rb:80:in `raise_validation_error'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/validations.rb:52:in `save!'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/transactions.rb:315:in `block in save!'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/transactions.rb:387:in `block in with_transaction_returning_status'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/connection_adapters/abstract/database_statements.rb:267:in `block in transaction'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/connection_adapters/abstract/transaction.rb:239:in `block in within_new_transaction'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/connection_adapters/abstract/transaction.rb:236:in `synchronize'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/connection_adapters/abstract/transaction.rb:236:in `within_new_transaction'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/connection_adapters/abstract/database_statements.rb:267:in `transaction'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/transactions.rb:212:in `transaction'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/transactions.rb:385:in `with_transaction_returning_status'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/transactions.rb:315:in `save!'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/suppressor.rb:48:in `save!'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/persistence.rb:53:in `create!'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/relation.rb:99:in `block in create!'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/relation.rb:281:in `scoping'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activerecord-5.2.4.4/lib/active_record/relation.rb:99:in `create!'
(pry):3:in `<main>'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/pry_instance.rb:290:in `eval'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/pry_instance.rb:290:in `evaluate_ruby'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/pry_instance.rb:659:in `handle_line'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/pry_instance.rb:261:in `block (2 levels) in eval'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/pry_instance.rb:260:in `catch'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/pry_instance.rb:260:in `block in eval'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/pry_instance.rb:259:in `catch'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/pry_instance.rb:259:in `eval'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/repl.rb:77:in `block in repl'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/repl.rb:67:in `loop'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/repl.rb:67:in `repl'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/repl.rb:38:in `block in start'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/input_lock.rb:61:in `__with_ownership'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/input_lock.rb:78:in `with_ownership'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/repl.rb:38:in `start'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/repl.rb:15:in `start'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-0.13.1/lib/pry/pry_class.rb:191:in `start'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/pry-byebug-3.9.0/lib/pry-byebug/pry_ext.rb:13:in `start_with_pry_byebug'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/railties-5.2.4.4/lib/rails/commands/console/console_command.rb:64:in `start'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/railties-5.2.4.4/lib/rails/commands/console/console_command.rb:19:in `start'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/railties-5.2.4.4/lib/rails/commands/console/console_command.rb:96:in `perform'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/thor-0.20.3/lib/thor/command.rb:27:in `run'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/thor-0.20.3/lib/thor/invocation.rb:126:in `invoke_command'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/thor-0.20.3/lib/thor.rb:387:in `dispatch'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/railties-5.2.4.4/lib/rails/command/base.rb:69:in `perform'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/railties-5.2.4.4/lib/rails/command.rb:46:in `invoke'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/railties-5.2.4.4/lib/rails/commands.rb:18:in `<top (required)>'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:23:in `require'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:23:in `block in require_with_bootsnap_lfi'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/loaded_features_index.rb:92:in `register'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:22:in `require_with_bootsnap_lfi'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:31:in `require'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activesupport-5.2.4.4/lib/active_support/dependencies.rb:291:in `block in require'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activesupport-5.2.4.4/lib/active_support/dependencies.rb:257:in `load_dependency'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activesupport-5.2.4.4/lib/active_support/dependencies.rb:291:in `require'
/Users/aaron/git/clarity/bin/rails:9:in `<top (required)>'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:59:in `load'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:59:in `load'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activesupport-5.2.4.4/lib/active_support/dependencies.rb:285:in `block in load'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activesupport-5.2.4.4/lib/active_support/dependencies.rb:257:in `load_dependency'
/Users/aaron/.rvm/gems/ruby-2.7.2@brightbytes/gems/activesupport-5.2.4.4/lib/active_support/dependencies.rb:285:in `load'
/Users/aaron/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:72:in `require'
/Users/aaron/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:72:in `require'

The fix:

> UserComment.where(user: User.first, commentable_id: AddOn.first.id, commentable_type: 'AddOn', comment_data: 'blah blah blah').create!
  User Load (0.7ms)  SELECT  "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT $1  [["LIMIT", 1]]
  AddOn Load (0.4ms)  SELECT  "add_ons".* FROM "add_ons" ORDER BY "add_ons"."id" ASC LIMIT $1  [["LIMIT", 1]]
   (0.2ms)  BEGIN
  AddOn Load (0.4ms)  SELECT  "add_ons".* FROM "add_ons" WHERE "add_ons"."id" = $1 LIMIT $2  [["id", 1], ["LIMIT", 1]]
  UserComment Create (56.9ms)  INSERT INTO "user_comments" ("user_id", "comment_data", "commentable_id", "commentable_type", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5, $6) RETURNING "id"  [["user_id", 1], ["comment_data", "\"blah blah blah\""], ["commentable_id", 1], ["commentable_type", "AddOn"], ["created_at", "2021-02-16 20:33:53.573036"], ["updated_at", "2021-02-16 20:33:53.573036"]]
  PaperTrail::Version Create (38.7ms)  INSERT INTO "versions" ("item_type", "item_id", "event", "created_at", "object_changes") VALUES ($1, $2, $3, $4, $5) RETURNING "id"  [["item_type", "UserComment"], ["item_id", 1], ["event", "create"], ["created_at", "2021-02-16 20:33:53.573036"], ["object_changes", "---\nid:\n-\n- 1\nuser_id:\n-\n- 1\ncomment_data:\n- \"{}\"\n- '\"blah blah blah\"'\ncommentable_id:\n-\n- 1\ncommentable_type:\n-\n- AddOn\ncreated_at:\n-\n- &1 2021-02-16 20:33:53.573036000 Z\nupdated_at:\n-\n- *1\n"]]
   (3.4ms)  COMMIT
=> #<UserComment:0x00007f861a59ad38
 id: 1,
 user_id: 1,
 comment_data: "blah blah blah",
 commentable_id: 1,
 commentable_type: "AddOn",
 created_at: Tue, 16 Feb 2021 12:33:53 PST -08:00,
 updated_at: Tue, 16 Feb 2021 12:33:53 PST -08:00>

Sorry for not providing this the first time around.

cfis commented 3 years ago

Thanks - that's helpful. So your fix is:

Change:

commentable: AddOn.first -> commentable_id: AddOn.first.id

And add commentable_type.

And what code did you change to do that? Via has_one, has_many, etc?

Would it work not using the id, but keeping the commentable_type? Just trying to figure out where CPK is going wrong. Suppose you didn't dive into CPK to find the issue? Not quite sure how to reproduce this yet.

brightbytes-dude commented 3 years ago

To be clear, it's a workaround, not a fix. :-)

But yeah, the workaround was to not use the polymorphic association, but rather just use the individual attributes of the polymorphic association. So, all I had to do was ag commentable and do the replacement, where appropriate. Totally easy, if inelegant.

Ideally of course, I wouldn't have had to do that though. :-)

cfis commented 3 years ago

Sorry again for not following up.

So it sounds like you have custom to_param method that CPK breaks. Can you share your overridden to_param method? CPK overrides it here:

https://github.com/composite-primary-keys/composite_primary_keys/blob/edfd7fdded0ac10e887d6d907b4f4ef77aa07180/lib/composite_primary_keys/base.rb#L137

But only for classes that have a composite key.

brightbytes-dude commented 3 years ago

I shared it above: see the class AddOn ... definition.

brightbytes-dude commented 3 years ago

Oh, and the problem was that CPK overrode it for a totally unrelated class.

Could it be because the to_param is declared as via alias_attribute??

cfis commented 3 years ago

Could be. Looks like to_param is now in integration.rb (ActiveRecord::Integration) so the cpk definition should be updated anyway.

What happens if you just comment out the CPK definition?

What version of Rails are you using?

cfis commented 2 years ago

I removed CPK's override on the to_param method, so hopefully that will fix this issue (you can remove it yourself if an older version of Rails (6.0 or older) and see what if the issue is fixed.

Thanks!

composite-primary-keys / composite_primary_keys

Adding CPK to a repo breaks polymorphic joins to legacy single-PK classes whose `to_param` is not based upon the `id` #541