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.

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

Deduplicate joins values #36834

Merged
merged 1 commit into from Aug 1, 2019
Merged

Deduplicate joins values #36834

merged 1 commit into from Aug 1, 2019

Conversation

kamipo
Copy link
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
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
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
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.

None yet

2 participants