Fix bug causing polymorphic_name not to be respected with belongs_to associations not using polymorphic_integer_type - #3
Conversation
Ruby 3 and rails 7 support
handle ruby 3.x keyword arguments delegation
Updates CI to support Ruby 3+ and Rails v7
Fix gem publish action
Updates push gem action with a valid ruby version
Write attribute with polymorphic integer type
Should still able to set other reflection successfully
Bump the version to 3.2.2
Upgrade rails to 7.2.x version; clean up unsupported rails and ruby version
There was a problem hiding this comment.
I found that these commands don't run locally until I make these changes. Seems like some of these APIs have been removed in more recent versions of ActiveRecord
| end | ||
| end | ||
|
|
||
| ActiveRecord::Associations::BelongsToPolymorphicAssociation.prepend(PolymorphicIntegerType::BelongsToPolymorphicAssociationExtension) |
There was a problem hiding this comment.
In order to have super above appropriately use the Rails-defined method in ActiveRecord::Associations::BelongsToPolymorphicAssociation, we need to prepend this code instead of override the method altogether.
| def replace_keys(record) | ||
| super | ||
|
|
||
| if reflection.options[:integer_type] || reflection.options[:polymorphic].is_a?(Hash) |
There was a problem hiding this comment.
The clauses here and on L17 are similar to the conditionals added to the belongs_to here to prevent polymorphic_integer_type behavior from leaking into all belongs_to associations.
| @@ -0,0 +1,11 @@ | |||
| module PolymorphicIntegerType | |||
| module BelongsToValidOptionsExtension | |||
| def valid_options(options) | |||
There was a problem hiding this comment.
Without adding :integer_type to valid_options, Rails throws an unsupported argument error. Previously, this library was simply deleting this option before building the association, but I can't find any way to distinguish belongs_to associations that utilize this gem vs those that don't without the option. Curious if the reviewer can think of other ways to achieve this though.
There was a problem hiding this comment.
I think this makes sense - we need to be able to determine whether the association uses some polymorphic_integer_type-specific config at the time replace_keys is called, so persisting that information on reflection options feels appropriate.
| end | ||
| end | ||
|
|
||
| context "When a link is given polymorphic record via a non-integer type association" do |
There was a problem hiding this comment.
This spec mimic the behavior we're seeing in the main rma repo and fails on the main branch, but passes on this branch
| t.integer :source_id | ||
| t.integer :source_type | ||
|
|
||
| t.references :legacy_source, polymorphic: true |
There was a problem hiding this comment.
Maybe we should add a new migration instead of adding this to an existing migration? Adding this here requires library maintainers to run a bin/rake db:reset to get their DBs in the appropriate state.
757fd35 to
a501d33
Compare
ihollander
left a comment
There was a problem hiding this comment.
Nice work on this! I think this is a good fix to get polymorphic associations behaving as expected for the enrollments work.
Longer-term, it does give me some pause seeing how this gem works under the hood and the amount of monkey-patching that's happening, plus I know this gem has been a source of issues for Rails upgrades in the past. I wonder if it's worth re-evaluating whether it's worth keeping this gem around given that we only use it for one association (AFAIK)? Maybe there's a lighter-weight approach we can hand-roll just using polymorphic_name and polymorphic_class_for with no gem...
If we do decide to keep the gem around, I'm definitely in favor of opening a PR with your changes on the gem repo so we don't have to keep this fork up to date!
| @@ -0,0 +1,11 @@ | |||
| module PolymorphicIntegerType | |||
| module BelongsToValidOptionsExtension | |||
| def valid_options(options) | |||
There was a problem hiding this comment.
I think this makes sense - we need to be able to determine whether the association uses some polymorphic_integer_type-specific config at the time replace_keys is called, so persisting that information on reflection options feels appropriate.
Yeah, the benefit we get out of using this gem for one association probably hasn't outweighed the pain caused by it in terms of Rails upgrades and the unexpected behavior fixed in this PR. I'll take a stab at seeing what we can do without a gem before deciding to open this PR on the main gem. |
…morphic_name guard + regression
Branched off of #2 to get our fork into sync w/upstream
Overview / Approach
Spurred on by some weirdness observed in our enrollments/segments STI remodel, we discovered a bug where
polymorphic_nameis not being respected bybelongs_toassociations that do NOT utilizepolymorphic_integer_type. Instead of storing thepolymorphic_namein the polymorphic type column, the base model's type is stored.After debugging the code, I identified this piece of Rails code that's being monkey-patched by
polymorphic_integer_typein allbelongs_topolymorphic associations, including those that are not indicated to usepolymorphic_integer_type. In order to appropriately apply the monkey patched code to onlybelongs_toassociations utilizinginteger_type, I addedinteger_typeas an available option to thebelongs_toassociation and inspect the association's reflection's options.An alternative approach I considered was updating the code in
PolymorphicIntegerType::BelongsToPolymorphicAssociationExtensionto properly work forbelongs_toassociations that do utilizepolymorphic_integer_typeand those that don't in the same code path, but decided against that. The approach that the library already takes is to avoid running this library's code for association extensions. The approach taken in this PR follows that approach by avoiding running this library's code when extendingActiveRecord::Associations::BelongsToPolymorphicAssociation. A fix for the case of abelongs_toassociation that does utilizepolymorphic_integer_typecan happen in a separate PR.Testing
I added a spec that reproduces the behavior we're observing on main and confirmed that the spec passes on this branch. I also confirmed the fixed behavior in a Rails console.