Skip to content

Deduplicate joins values#36834

Merged
kamipo merged 1 commit intorails:masterfrom
kamipo:deduplicate_joins
Aug 1, 2019
Merged

Deduplicate joins values#36834
kamipo merged 1 commit intorails:masterfrom
kamipo:deduplicate_joins

Conversation

@kamipo
Copy link
Copy Markdown
Member

@kamipo kamipo commented Aug 1, 2019

#36805 have one possible regression that failing deduplication if
joins_values have complex order (e.g. joins_values = [join_node_a, :comments, :tags, join_node_a]).

This fixes the deduplication to take it in the first phase before
grouping.

I believe the deduplication issue #36805 (comment) might be fixed by this.

cc @eileencodes

rails#36805 have one possible regression that failing deduplication if
`joins_values` have complex order (e.g. `joins_values = [join_node_a,
:comments, :tags, join_node_a]`).

This fixes the deduplication to take it in the first phase before
grouping.
@eileencodes
Copy link
Copy Markdown
Member

Going to test this out, I literally just got a reproduction script working 😆

@kamipo kamipo merged commit 4f1bd29 into rails:master Aug 1, 2019
@kamipo kamipo deleted the deduplicate_joins branch August 1, 2019 19:02
@eileencodes
Copy link
Copy Markdown
Member

eileencodes commented Aug 1, 2019

Yes! This fixes it thank you!

For posterity here's the script I got working:

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection({ "adapter" => "mysql2", "username" => "rails", "database" => "activerecord_unittest" })
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :events, force: true do |t| 
    t.integer :project_id
  end 

  create_table :event_details, force: true do |t| 
    t.integer :event_id
  end 

  create_table :projects, force: true do |t| 
  end 
end

class Event < ActiveRecord::Base
  has_one :event_detail
  belongs_to :project

  def self.visible_in_timeline_for
    events_for
  end 

  scope :events_for, -> {
    join_project.
    join_event_detail
  }

  scope :join_project, -> {
    join_event_detail.joins(:project)
  }

  scope :join_event_detail, -> {
    joins(<<~SQL)
    INNER JOIN `event_details`
    ON `event_details`.`event_id` = `events`.`id`
    SQL
  }
end

class EventDetail < ActiveRecord::Base
  belongs_to :event
end

class Project < ActiveRecord::Base
  has_many :events
end

class BugTest < Minitest::Test
  def test_scoped_joins
    # throws a ActiveRecord::StatementInvalid: Mysql2::Error: Not unique table/alias: 'event_details'
    p Event.visible_in_timeline_for
  end
end

kamipo added a commit that referenced this pull request Aug 1, 2019
@kamipo
Copy link
Copy Markdown
Member Author

kamipo commented Aug 1, 2019

❤️ Backported 97f1fef.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants