Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for unpersisted CPK has_one/has_many through associations #48552

Merged
merged 1 commit into from
Jun 23, 2023
+26 −2
Merged

Add support for unpersisted CPK has_one/has_many through associations #48552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/associations/has_many_through_association.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ def remove_records(existing_records, records, method)
end

def target_reflection_has_associated_record?
!(through_reflection.belongs_to? && owner[through_reflection.foreign_key].blank?)
!(through_reflection.belongs_to? && Array(through_reflection.foreign_key).all? { |foreign_key_column| owner[foreign_key_column].blank? })
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be using a _read_attribute here for cases where a composite foreign key contains id column as owner["id"] returns a whole identifier and not just value of the id column

Suggested change
!(through_reflection.belongs_to? && Array(through_reflection.foreign_key).all? { |foreign_key_column| owner[foreign_key_column].blank? })
!(through_reflection.belongs_to? && Array(through_reflection.foreign_key).all? { |foreign_key_column| owner._read_attribute(foreign_key_column).blank? })

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I'll see if I can add a regression test for this.

Copy link
Member Author

@gmcgibbon gmcgibbon Jun 23, 2023 •
edited

Choose a reason for hiding this comment

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

Thinking about it a little more, I can't see a reason for the foreign key to ever be id. That is what primary keys are named. I suppose we can support this use-case for completeness if you feel strongly about it. Testing might be easier once I change Cpk::Book to have an id instead of number in #48564.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually you are right, having :id as part of the foreign key would be a very unconventional scenario, can't say I can come up with a meaningful use-case. Let's not rush with switching to _read_attribute then until we have a meaningful reproduction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now, the only reason I can see to use _read_attribute here is to bypass alias checking. See

rails/activerecord/lib/active_record/attribute_methods/read.rb

Lines 29 to 34 in ba9c2e5

def read_attribute(attr_name, &block)
name = attr_name.to_s
name = self.class.attribute_aliases[name] || name
return @attributes.fetch_value(name, &block) unless name == "id" && @primary_key
vs

rails/activerecord/lib/active_record/attribute_methods/read.rb

Lines 44 to 49 in ba9c2e5

def _read_attribute(attr_name, &block) # :nodoc:
@attributes.fetch_value(attr_name, &block)
end
alias :attribute :_read_attribute
private :attribute

Do we think this is necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

@eileencodes Nikita and I chatted about this and we agree it isn't necessary anymore to use _read_attribute, so I'm going to merge as-is. I'm happy to revert/fix forward in another patch if you feel strongly this use-case should be covered, but the foreign key columns should never use id, and I don't want to further change behaviour of associations if we aren't proving value.

end

def update_through_counter?(method)
Expand Down
4 changes: 3 additions & 1 deletion activerecord/lib/active_record/associations/through_association.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ def stale_state
end

def foreign_key_present?
through_reflection.belongs_to? && !owner[through_reflection.foreign_key].nil?
through_reflection.belongs_to? && Array(through_reflection.foreign_key).all? do |foreign_key_column|
!owner[foreign_key_column].nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Suggested change
!owner[foreign_key_column].nil?
!owner._read_attribute(foreign_key_column).nil?

end
end

def ensure_mutable
Expand Down
8 changes: 8 additions & 0 deletions activerecord/test/cases/associations/has_many_through_associations_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1631,6 +1631,14 @@ def test_tags_has_manu_posts_through_association_with_composite_query_constraint
assert_equal(expected_blog_post_ids.sort, blog_post_ids.sort)
end

def test_loading_cpk_association_with_unpersisted_owner
order = Cpk::Order.create!(shop_id: 1)
book = Cpk::BookWithOrderAgreements.new(number: 2, author_id: 1, order: order)
order_agreement = Cpk::OrderAgreement.create!(order: order)

assert_equal([order_agreement], book.order_agreements.to_a)
end

private
def make_model(name)
Class.new(ActiveRecord::Base) { define_singleton_method(:name) { name } }
Expand Down
9 changes: 9 additions & 0 deletions activerecord/test/cases/associations/has_one_through_associations_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
require "models/carrier"
require "models/shop_account"
require "models/customer_carrier"
require "models/cpk"

class HasOneThroughAssociationsTest < ActiveRecord::TestCase
fixtures :member_types, :members, :clubs, :memberships, :sponsors, :organizations, :minivans,
Expand Down Expand Up @@ -445,4 +446,12 @@ def test_has_one_through_do_not_cache_association_reader_if_the_though_method_ha
ensure
CustomerCarrier.current_customer = nil
end

def test_loading_cpk_association_with_unpersisted_owner
order = Cpk::Order.create!(shop_id: 1)
book = Cpk::BookWithOrderAgreements.new(number: 2, author_id: 1, order: order)
order_agreement = Cpk::OrderAgreement.create!(order: order)

assert_equal(order_agreement, book.order_agreement)
end
end
5 changes: 5 additions & 0 deletions activerecord/test/models/cpk/book.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,9 @@ class BrokenBook < Book
class NullifiedBook < Book
has_one :chapter, query_constraints: [:author_id, :book_number], dependent: :nullify
end

class BookWithOrderAgreements < Book
has_many :order_agreements, through: :order
has_one :order_agreement, through: :order, source: :order_agreements
end
end