WIP: Association route refactor

[matthewd]

No description provided.

[adrianna-chang-shopify]

Reviewed the last four commits incrementally (earlier ones appear to overlap with the stack you extracted?) with my agent, tried to spot check a couple things and look for refactors / simplifications. If there are any changes you'd like a closer look at in particular please advise 😅 Overall I think the architecture makes sense, and we're solving quite a few association-related bugs throughout this PR, so despite the large changeset I think it's going in the right direction. From a correctness standpoint seems good to go.

[adrianna-chang-shopify]

normalize_single_column_keys might be a bit more descriptive here, and then we could drop the comment?

[adrianna-chang-shopify]

Nit, but I think it might simplify thing slightly to always return a list here, because in #route_groups we can avoid branching again depending on if ids is composite / scalar. Relatedly, since the hash key is already storing fixed_values and origin_key, we could have the current value contain only the list of ids. e.g.:

--- a/activerecord/lib/active_record/relation/predicate_builder/polymorphic_array_value.rb
+++ b/activerecord/lib/active_record/relation/predicate_builder/polymorphic_array_value.rb
@@ -12,7 +12,7 @@
       def queries
         return [ reference_key_name => values ] if values.empty?

-        route_groups.filter_map do |_key, (fixed_values, origin_key, ids)|
+        route_groups.filter_map do |(fixed_values, origin_key), ids|
           fixed_values.merge(origin_key => ids) unless ids.empty?
         end
       end
@@ -32,14 +32,8 @@
               origin_key = reference_key_name
             end

-            key = [fixed_values, origin_key]
-            group = groups[key] ||= [fixed_values, origin_key, []]
-            ids = convert_to_id(value, route)
-            if value.is_a?(Relation) && route.destination_key.composite?
-              group.last.concat(ids)
-            else
-              group.last << ids
-            end
+            ids = groups[[fixed_values, origin_key]] ||= []
+            ids.concat(ids_for(value, route))
           end
         end

@@ -54,17 +48,20 @@
           end
         end

-        def convert_to_id(value, route)
-          if value.is_a?(Base)
-            route.destination_key.value_of(value)
-          elsif value.is_a?(Relation)
-            if route.destination_key.composite?
-              value.pluck(route.destination_key.name)
+        def ids_for(value, route)
+          case value
+          when Base
+            [route.destination_key.value_of(value)]
+          when Relation
+            key = route.destination_key
+
+            if key.composite?
+              value.pluck(key.name)
             else
-              value.select(route.destination_key.name)
+              [value.select(key.name)]
             end
           else
-            value
+            [value]
           end
         end

[adrianna-chang-shopify]

We have similar logic in AssociationScope#next_chain_scope (iterating through route.each_match, resolving both attributes through predicate_attribute, and constructing an equality), is it worth extracting a helper on PredicateBuilder?

def association_match_predicates(route, origin_table:, destination_table:)
  route.each_match.map do |origin_column, destination_column|
    destination_attribute = predicate_attribute(destination_table[destination_column])
    origin_attribute = predicate_attribute(origin_table[origin_column])

    destination_attribute.eq(origin_attribute)
  end
end

(join_scope would add the where! and next_chain_scope would combine the predicates for the join still)

[adrianna-chang-shopify]

Loader groups could technically contain different key types, is that a use case we should consider right now? We currently use the first owner's key conversions for all owners:

  # activerecord/lib/active_record/associations/preloader/association.rb
  # L115
  @model = owners.first && owners.first.class

  # L274
  def match_normalizers
    @match_normalizers ||= association_route.match_normalizers(origin_class: model, destination_class: klass)
  end

  def owners_by_key
    @owners_by_key ||= begin
      route = association_route
      owners.each_with_object({}) do |owner, result|
        next unless route.origin_reference_complete?(owner)

        (result[origin_key_value(owner)] ||= []) << owner # for every owner, call #origin_key_value, which uses match_nromalizers.first
       end
    end
  end

We have similar coverage in AssociationRouteTest#test_matching_origins_compare_each_class_by_its_own_types_and_preserve_order

[adrianna-chang-shopify]

Could we build one frozen normalizer list and return it for both sides? Both sides always use the same conversion, so the two lists are identical.

Suggested change

	def build_match_normalizers(origin_class, destination_class)
	origin_normalizers = destination_normalizers = nil
	each_match.with_index do |(origin_column, destination_column), index|
	origin_type = origin_class.type_for_attribute(origin_column)
	destination_type = destination_class.type_for_attribute(destination_column)
	origin_normalizer, destination_normalizer = normalizers_for_types(origin_type, destination_type)
	if origin_normalizer
	(origin_normalizers ||= Array.new(@key_pairs.length))[index] = origin_normalizer
	end
	if destination_normalizer
	(destination_normalizers ||= Array.new(@key_pairs.length))[index] = destination_normalizer
	end
	end
	[origin_normalizers&.freeze, destination_normalizers&.freeze].freeze
	end
	def normalizers_for_types(origin_type, destination_type)
	unless origin_type.type == destination_type.type
	[STRING_NORMALIZER, STRING_NORMALIZER]
	end
	end
	def build_match_normalizers(origin_class, destination_class)
	normalizers = nil
	each_match.with_index do |(origin_column, destination_column), index|
	origin_type = origin_class.type_for_attribute(origin_column)
	destination_type = destination_class.type_for_attribute(destination_column)
	next if origin_type.type == destination_type.type
	(normalizers ||= Array.new(@key_pairs.length))[index] = STRING_NORMALIZER
	end
	normalizers&.freeze
	[normalizers, normalizers].freeze
	end

[adrianna-chang-shopify]

Would it be better to rename this to matches_association_key? since we now compare the QC and reference columns?

[adrianna-chang-shopify]

Nit, we could combine these branches since only the grouping key is different:

Suggested change

	elsif group_by_route
	reflection_records.group_by do |record|
	klass = record.association(association).klass
	[klass, loader.association_route_for(reflection, record.class, klass)]
	end.map do |(rhs_klass, _), records|
	loader.new(rhs_klass, records, reflection, scope, nil, associate_by_default)
	end
	else
	reflection_records.group_by do |record|
	record.association(association).klass
	end.map do |rhs_klass, records|
	preloader_for(reflection).new(rhs_klass, records, reflection, scope, nil, associate_by_default)
	loader.new(rhs_klass, records, reflection, scope, nil, associate_by_default)
	end
	else
	groups = reflection_records.group_by do |record|
	klass = record.association(association).klass
	if group_by_route
	[klass, loader.association_route_for(reflection, record.class, klass)]
	else
	klass
	end
	end
	groups.map do |key, records|
	rhs_klass = group_by_route ? key.first : key
	loader.new(rhs_klass, records, reflection, scope, nil, associate_by_default)
	end

[adrianna-chang-shopify]

We're only checking the FK here, so if an owner's QC value changes, we might miss updates (e.g. for counter caches). There are a couple of other places where we encounter a similar issue; BelongsToAssociation#stale_state also only checks the FK, same with .touch_record in activerecord/lib/active_record/associations/builder/belongs_to.rb:51. Does it make sense to have these check QCs?

[adrianna-chang-shopify]

Suggested change

	assert_equal original_time, first_decoy.reload.updated_at
	assert_operator new_ship.reload.updated_at, :>, original_time
	assert_equal original_time, first_decoy.reload.updated_at

[adrianna-chang-shopify]

Suggested additional test based on

# activerecord/lib/active_record/associations/belongs_to_association.rb L128-130

# An unselected primary key can be present as nil. Use the owner's
# reference for that case, but preserve nil query-constraint values.
if !destination_value.nil? || !route.reference_destination_key.include?(destination_column)

Suggested change

	test "belongs to preserves a nil query constraint from the persisted target" do
	book = BookDestroyAsync.create!(name: nil)
	other = BookDestroyAsync.create!(name: "Owner constraint")
	essay = EssayDestroyAsync.create!(name: other.name, book: book)
	reflection = EssayDestroyAsync.reflect_on_association(:book)
	constraints = ActiveRecord::Key::Mapping.new(
	reference_key: ActiveRecord::Key.for(:name),
	target_key: ActiveRecord::Key.for(:name)
	)
	reflection.stub(:association_route_constraints, constraints) do
	reflection.clear_association_scope_cache
	essay.destroy!
	end
	perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
	assert_not BookDestroyAsync.exists?(book.id)
	assert BookDestroyAsync.exists?(other.id)
	ensure
	reflection&.clear_association_scope_cache
	EssayDestroyAsync.delete_all
	BookDestroyAsync.delete_all
	end