Fix multi-database polymorphic preloading with equivalent table names

[summera]

Motivation / Background

I work with an application that utilizes a polymorphic association that spans multiple databases. A basic example would be something like this, where Notification#notifiable could reference Comment or OtherNamespace::Comment, which exist on two separate databases but happen to have the same table name:

# app/models/notification.rb
class Notification < ApplicationRecord
  belongs_to :notifiable, polymorphic: true
end

# app/models/comment.rb
class Comment < ApplicationRecord
  self.table_name = "comments"
end

# app/models/application_record.rb
class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true

  connects_to database: { writing: :primary }
end

# app/models/other_namespace/comment.rb
module OtherNamespace
  class Comment < OtherNamespace::Base
    self.table_name = "comments"
  end
end

# app/models/other_namespace/base.rb
module OtherNamespace
  class Base < ApplicationRecord
    self.abstract_class = true

    connects_to database: { writing: :other_namespace }
  end
end

Prior to upgrading to Rails 7.0, the following would preload both Comments and OtherNamespace::Comments:

comment = Comment.create
other_namespace_comment = OtherNamespace::Comment.create

Notification.create(notifiable: comment)
Notification.create(notifiable: other_namespace_comment)

# Prior to Rails 7.0, this would preload the related `Comment`s and `OtherNamespace::Comment`s
Notification.all.includes(:notifiable)

After upgrading to Rails 7.0, we noticed that only Comments were being preloading and OtherNamespace::Comments were not.

After some digging, I tracked the difference in behavior down to PR #41385. In this PR there were some optimizations made to combine association preloading into less queries when possible. This was later optimized a bit further in PR #41597.

From what I can tell ActiveRecord::Associations::Preloader::Batch#group_and_load_similar, queries are being grouped and loaded. That comparison is being done based on ActiveRecord::Associations::Preloader::Association::LoaderQuery#eql?, which is the following:

  def eql?(other)
    association_key_name == other.association_key_name &&
      scope.table_name == other.scope.table_name &&
      scope.values_for_queries == other.scope.values_for_queries
  end

As you can see, in the polymorphic use case shown above the loaders for OtherNamespace::Comment and Comment would be considered equivalent since they have the same association_key_name (notifiable in this case), table_name (comments in this case), and values_for_queries (id in this case). By comparing them this way it results in OtherNamespace::Comment not being preloaded after the Rails 7.0 upgrade.

Detail

Using the existing models and relationships in the test suite, I was able to generate a failing test case. I've also included a proposed fix which adds a loader comparison on scope.connection_specification_name. The idea is to prevent two tables with the same name on two separate databases from being considered equivalent when preloading associations.

Another idea would be to replace the table_name comparison with name (the class name). I tried this and all tests pass, but I'm unsure if this could be problematic in other cases, such as when using single table inheritance. If there's another way of achieving the desired result with a different comparison, I'm happy to change the logic!

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

Happy to add a Changelog entry if it's needed.

[summera]

@jhawthorn @eileencodes sorry to bother y'all, but would love to get some feedback on this one. We've been using this patch in production without issues. Thanks in advance!

[byroot]

Backported to 7-1-stable as 1c90604183bd1d3e6c3da4e11627d393481ff92f.