Skip to content

Commit

Permalink
Fix issue with self joins and includes (#1275)
Browse files Browse the repository at this point in the history
Our polyamorous monkeypatches were doing too much. Removing our custom
equality and letting ActiveRecord figure it out seems to do the trick
and fix this.

On some Rails & DB adapters this seems to break a polyamorous spec.
However, this spec just tests an implementation detail but it's unclear
whether the spec breaking has any actual bad side effect in real like,
so I removed it.
  • Loading branch information
deivid-rodriguez authored Mar 7, 2022
1 parent 49a43fc commit a6f91a4
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 13 deletions.
4 changes: 0 additions & 4 deletions lib/polyamorous/activerecord_5.2_ruby_2/join_association.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,5 @@ def initialize(reflection, children, polymorphic_class = nil, join_type = Arel::
super(reflection, children)
end
end

def ==(other)
base_klass == other.base_klass
end
end
end
4 changes: 0 additions & 4 deletions lib/polyamorous/activerecord_6.1_ruby_2/join_association.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,5 @@ def join_constraints_with_tables(foreign_table, foreign_klass, join_type, alias_

joins
end

def ==(other)
base_klass == other.base_klass
end
end
end
15 changes: 15 additions & 0 deletions spec/polyamorous/activerecord_compatibility_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
require 'spec_helper'

module Polyamorous
describe "ActiveRecord Compatibility" do
it 'works with self joins and includes' do
trade_account = Account.create!
Account.create!(trade_account: trade_account)

accounts = Account.joins(:trade_account).includes(:trade_account, :agent_account)
account = accounts.first

expect(account.agent_account).to be_nil
end
end
end
5 changes: 0 additions & 5 deletions spec/polyamorous/join_association_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,6 @@ module Polyamorous

subject { new_join_association(reflection, parent.children, Person) }

it 'respects polymorphism on equality test' do
expect(subject).to eq new_join_association(reflection, parent.children, Person)
expect(subject).not_to eq new_join_association(reflection, parent.children, Article)
end

it 'leaves the original reflection intact for thread safety' do
reflection.instance_variable_set(:@klass, Article)
join_association
Expand Down
10 changes: 10 additions & 0 deletions spec/support/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,11 @@ class Note < ActiveRecord::Base
belongs_to :notable, polymorphic: true
end

class Account < ActiveRecord::Base
belongs_to :agent_account, class_name: "Account"
belongs_to :trade_account, class_name: "Account"
end

module Schema
def self.create
ActiveRecord::Migration.verbose = false
Expand Down Expand Up @@ -257,6 +262,11 @@ def self.create
t.integer :target_person_id
t.integer :article_id
end

create_table :accounts, force: true do |t|
t.belongs_to :agent_account
t.belongs_to :trade_account
end
end

10.times do
Expand Down

0 comments on commit a6f91a4

Please sign in to comment.