Skip to content

Fix crash when deleting an association with composite primary keys#450

Merged
brendon merged 3 commits intobrendon:masterfrom
tangibleMaterials:composite-pk
Oct 20, 2025
Merged

Fix crash when deleting an association with composite primary keys#450
brendon merged 3 commits intobrendon:masterfrom
tangibleMaterials:composite-pk

Conversation

@smathieu
Copy link
Copy Markdown
Contributor

Problem

acts_as_list fails with NoMethodError: undefined method 'to_sym' for an instance of Array when used with Rails 8.0's composite foreign key feature. This occurs when destroying records through associations with dependent: :destroy.

Why

The destroyed_via_scope? method in scope_method_definer.rb calls .to_sym on destroyed_by_association.foreign_key, which returns an Array for composite keys in Rails 8.0+.

Proposed Solution

Adds support for array in relationships.

Copy link
Copy Markdown
Owner

@brendon brendon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! :) I'm now wondering if this is one I've missed in my other ordering gem:

https://github.com/brendon/positioning/blob/2a8766e70c1c6442a6a3ef217ff7388c50e07a59/lib/positioning/mechanisms.rb#L223-L227

What do you think?

foreign_key = caller_class.reflections[name.to_s].foreign_key
# Handle composite foreign keys (Arrays) by returning as-is
# Single foreign keys should be converted to symbols
foreign_key.is_a?(Array) ? foreign_key : foreign_key.to_sym
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering here. We convert the columns to symbols for single foreign keys but not for the composite ones? I'm not sure why we do this anyway but maybe those should be converted also?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I think we get a symbol back already, but it might be rails version dependent? Good to add for consistency.

@smathieu
Copy link
Copy Markdown
Contributor Author

Yes - it looks like it would also fail in the positioning gem. Also seeing I didn't exclude the test from older version of Rails, so I'll do that.

@smathieu
Copy link
Copy Markdown
Contributor Author

@brendon do you know if the failing MySQL tests are a known issue? I don't think it's related to my change and I don't have mysql installed locally.

@smathieu smathieu closed this Oct 16, 2025
@smathieu smathieu reopened this Oct 16, 2025
@smathieu
Copy link
Copy Markdown
Contributor Author

@brendon I'm seeing these same MySQL test failures on the master branch on my laptop. Seems unrelated to my change. Can you confirm if you're seeing this as well?

@brendon
Copy link
Copy Markdown
Owner

brendon commented Oct 17, 2025

That's interesting. I'm not seeing any errors with this run: RAILS_VERSION=8.0 DB=mysql rake test locally. I'm on Ruby 3.3.9.

@brendon
Copy link
Copy Markdown
Owner

brendon commented Oct 19, 2025

Seems related to brianmario/mysql2#1414 so I've fixed the version of the gem to 0.5.6 for now.

Try merging master in and it should all pass :)

@brendon brendon merged commit c1d9640 into brendon:master Oct 20, 2025
54 checks passed
@brendon
Copy link
Copy Markdown
Owner

brendon commented Oct 20, 2025

Thanks @smathieu, I've released it as 1.2.5 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants