Incorrect reference to table alias. #1153

MrJoy · 2020-10-10T00:48:16Z

I know this issue seems related to a couple others, but I think it may involve a slightly novel twist.

I have an indirect association I want to ransack, while also ransacking the intermediate association. It matters greatly what order these query options are provided to .ransack. If they follow the natural ordering (intermediate table first), all is well. If they go in the opposite order, then I wind up with a bad query because the LEFT OUTER JOIN aliases the table as <intermediate_table_name>_<direct_table_name>_join, but the WHERE clause refers to <intermediate_table_name>_<direct_table_name>.

This behavior is novel starting with Rails 6.0.1, and I have a nice little repro case:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  # Issue does NOT occur with Rails 6.0.0
  # Issue DOES occur with Rails 6.0.1, and 6.0.3.4
  gem "activerecord", "6.0.3.4", require: "active_record"
  gem "ransack", "2.3.2"
  gem "sqlite3"

  gem "pry"
end

require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table "users", force: :cascade do |t|
    t.string "email", default: "", null: false
    t.index ["email"], name: "idx_users_on_email", unique: true
  end

  create_table "google_accounts", force: :cascade do |t|
    t.bigint "user_id", null: false
    t.string "email", null: false
    t.index ["email"], name: "idx_g_accounts_on_email"
    t.index ["user_id"], name: "idx_g_accounts_on_user_id"
  end

  create_table "google_calendars", force: :cascade do |t|
    t.bigint "google_account_id", null: false
    t.string "google_id", null: false
    t.index ["google_account_id", "google_id"], name: "idx_g_calendars_on_g_account_id_and_g_id", unique: true
    t.index ["google_account_id"], name: "idx_g_calendars_on_g_account_id"
  end
end

class User < ActiveRecord::Base
  has_many :accounts,
           class_name: "GoogleAccount"
  has_many :contacts,
           class_name: "GoogleContact",
           through:    :accounts,
           source:     :contacts
  has_many :calendars,
           class_name: "GoogleCalendar",
           through:    :accounts,
           source:     :calendars
end

class GoogleAccount < ActiveRecord::Base
  belongs_to :user

  has_many :calendars, class_name: "GoogleCalendar"
end

class GoogleCalendar < ActiveRecord::Base
  belongs_to :account,
             class_name:  "GoogleAccount",
             foreign_key: "google_account_id",
             inverse_of:  :calendars

  has_one :user, through: :account
end


class BugTest < Minitest::Test
  def setup
    user1 = User.create!(email: "[email protected]")
    user2 = User.create!(email: "[email protected]")
    user1_acct1 = GoogleAccount.create!(user: user1, email: "[email protected]")
    user1_acct2 = GoogleAccount.create!(user: user1, email: "[email protected]")
    user2_acct1 = GoogleAccount.create!(user: user2, email: "[email protected]")
    user2_acct2 = GoogleAccount.create!(user: user2, email: "[email protected]")
    user1_acct1_cal1 = GoogleCalendar.create!(account: user1_acct1, google_id: "[email protected]")
    user1_acct1_cal2 = GoogleCalendar.create!(account: user1_acct1, google_id: "[email protected]")
    user1_acct2_cal1 = GoogleCalendar.create!(account: user1_acct2, google_id: "[email protected]")
    user1_acct2_cal2 = GoogleCalendar.create!(account: user1_acct2, google_id: "[email protected]")
    user2_acct1_cal1 = GoogleCalendar.create!(account: user2_acct1, google_id: "[email protected]")
    user2_acct1_cal2 = GoogleCalendar.create!(account: user2_acct1, google_id: "[email protected]")
    user2_acct2_cal1 = GoogleCalendar.create!(account: user2_acct2, google_id: "[email protected]")
    user2_acct2_cal2 = GoogleCalendar.create!(account: user2_acct2, google_id: "[email protected]")
  end

  def teardown
    User.destroy_all
    GoogleAccount.destroy_all
    GoogleCalendar.destroy_all
  end

  def test_good_generated_sql_join
    good_result = GoogleCalendar.
                  ransack({ "account_email_cont" => "account1", "user_email_eq" => "[email protected]" }).
                  result
    # SELECT "google_calendars".*
    # FROM "google_calendars"
    # LEFT OUTER JOIN "google_accounts" ON "google_accounts"."id" = "google_calendars"."google_account_id"
    # LEFT OUTER JOIN "users" ON "users"."id" = "google_accounts"."user_id"
    # LEFT OUTER JOIN "google_accounts" "accounts_google_calendars" ON "accounts_google_calendars"."id" = "google_calendars"."google_account_id"
    # WHERE ("google_accounts"."email" LIKE '%account1%' AND "users"."email" = '[email protected]')

    assert_equal ["[email protected]", "[email protected]"], good_result.map(&:google_id)
  end

  def test_bad_generated_sql_join
    # Reversing the order of the hash fields:
    bad_result = GoogleCalendar.
                 ransack({ "user_email_eq" => "[email protected]", "account_email_cont" => "account1" }).
                 result
    # SELECT "google_calendars".*
    # FROM "google_calendars"
    # LEFT OUTER JOIN "google_accounts" ON "google_accounts"."id" = "google_calendars"."google_account_id"
    # LEFT OUTER JOIN "google_accounts" "accounts_google_calendars_join" ON "accounts_google_calendars_join"."id" = "google_calendars"."google_account_id"
    # LEFT OUTER JOIN "users" ON "users"."id" = "accounts_google_calendars_join"."user_id"
    # WHERE ("users"."email" = '[email protected]' AND "accounts_google_calendars"."email" LIKE '%account1%')

    # Note that the `WHERE` clause refers to `accounts_google_calendars`, but the alias in the join is `accounts_google_calendars_join`!

    assert_equal ["[email protected]", "[email protected]"], bad_result.map(&:google_id)
  end
end

Doing my best to dig through Ransack internals I found Arel objects being created that include TableAliasobjects that have the name accounts_google_calendars, so I started looking for where the _join was added. I believe I found where it's hiding in ActiveRecord, and have a stack trace:

/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/activerecord-6.0.3.4/lib/active_record/associations/join_dependency.rb:190:in `table_alias_for'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/activerecord-6.0.3.4/lib/active_record/associations/join_dependency.rb:179:in `block in table_aliases_for'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/activerecord-6.0.3.4/lib/active_record/associations/join_dependency.rb:176:in `map'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/activerecord-6.0.3.4/lib/active_record/associations/join_dependency.rb:176:in `table_aliases_for'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/activerecord-6.0.3.4/lib/active_record/associations/join_dependency.rb:158:in `block in construct_tables!'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/activerecord-6.0.3.4/lib/active_record/associations/join_dependency/join_part.rb:38:in `block in each_children'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/activerecord-6.0.3.4/lib/active_record/associations/join_dependency/join_part.rb:37:in `each'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/activerecord-6.0.3.4/lib/active_record/associations/join_dependency/join_part.rb:37:in `each_children'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/activerecord-6.0.3.4/lib/active_record/associations/join_dependency.rb:157:in `construct_tables!'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/adapters/active_record/context.rb:316:in `build_association'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/adapters/active_record/context.rb:280:in `build_or_find_association'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/adapters/active_record/context.rb:205:in `get_parent_and_attribute_name'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/adapters/active_record/ransack/context.rb:40:in `bind_pair_for'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/context.rb:62:in `bind'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/nodes/condition.rb:151:in `block in build_attribute'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/nodes/condition.rb:150:in `tap'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/nodes/condition.rb:150:in `build_attribute'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/nodes/condition.rb:83:in `block in attributes='
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/nodes/condition.rb:82:in `each'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/nodes/condition.rb:82:in `attributes='
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/nodes/condition.rb:176:in `block in build'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/nodes/condition.rb:174:in `each'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/nodes/condition.rb:174:in `build'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/nodes/condition.rb:17:in `extract'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/nodes/grouping.rb:175:in `write_attribute'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/nodes/grouping.rb:115:in `method_missing'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/search.rb:44:in `block in build'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/search.rb:40:in `each'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/search.rb:40:in `build'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/search.rb:32:in `initialize'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/adapters/active_record/base.rb:18:in `new'
/Users/jfrisby/.rbenv/versions/2.6.6/gemsets/thryve-core/gems/ransack-2.3.2/lib/ransack/adapters/active_record/base.rb:18:in `ransack'
...

I'm not sure where the aliasing is getting out of sync, but somewhere along the way one part of the Arel tree seems to think one thing, and another part thinks another thing.

I wish I could send you a patch, but Ransack and Arel are very complex and I lack enough context to even properly understand what's going on, let alone come up with a patch that won't break other things.

The text was updated successfully, but these errors were encountered:

scarroll32 · 2020-11-27T19:11:00Z

Thank you @MrJoy could this awesome write-up be converted into a PR with the failing tests?

yahonda · 2020-12-02T03:04:24Z

Based on the git bisect result, this behavior had been introduced by this commit between Rails v6.0.0 and v6.0.1.

rails/rails@dd46d18

Then it has been addressed by this commit, which will be available if Rails 6.0.4 is released.

rails/rails@f9ba524

deivid-rodriguez · 2020-12-02T08:50:44Z

That's great to hear. Then maybe we can merge #1176, skipping the failing spec on v6.0.3, similarly to 61913a7?

yahonda · 2020-12-02T12:36:05Z

Yes. I think we can merge #1176 and skip this test on v6.0.3.
One concern is it fails against Rails 6.1.0.rc1 then we will need to find how to address these failures on Rails 6.1.

deivid-rodriguez · 2020-12-02T14:45:33Z

I see, then we need to work on the fix for sure! I don't care whether we merge the skipped tests first or together with the future fix.

istickz · 2020-12-30T14:24:56Z

I changed 6.0.3.4 version to
gem 'rails', github: 'rails/rails', branch: '6-0-stable'
And it works
Currently 6.0.4 hasn't been released

nikajukic · 2021-01-22T07:10:33Z

I think this could be a similar issue like #1119. I had the same problems where a table with alias in join wasn't properly referenced in where part.

…-alias Add spec described in #1153.

scarroll32 · 2021-11-10T15:44:20Z

Reverted #1263

…ncorrect-reference-to-table-alias Revert "Add spec described in #1153."

MrJoy · 2021-11-10T21:51:19Z

@scarroll32 Curious about the revert here. Should I re-file my PR so that there's something open to work against if/when y'all have time to investigate the bug?

scarroll32 · 2021-11-11T17:15:25Z

@MrJoy hey there, thanks for the ping. It was reverted as the CI failed on merge. If you can get the CI to pass, we can definitely merge it.

MrJoy · 2021-11-12T01:08:50Z

@scarroll32 Well, that's the challenging part. The code for Ransack is a bit brain-bendy to me, and I'm not deeply-rooted in the lineage of Rails enough to feel comfortable addressing version-specific test failures. I could re-submit the PR with the tests marked as pending/skip, perhaps? Alternatively, if you have an intuition around the root cause you could use to offer me a roadmap I could take a stab at fixing it. I can't guarantee I'll be super useful on that front, however. A third alternative would just be to leave an open PR -- possibly as a draft, if that's something I can do as an external contributor, so that it's there if/when your crew have the time/inclination to look at it.

scarroll32 · 2021-11-12T12:46:18Z

@MrJoy sure thing, it looks like it can't find a column, but if you want to just create the merged PR with a note linking back here, perhaps someone will pick it up.

Thanks for your contribution!

scarroll32 · 2021-11-12T12:46:53Z

so that it's there if/when your crew have the time/inclination to look at it.

Not much of a crew here, but I'll try to look at it eventually.

This reverts commit dcacf03.

deivid-rodriguez · 2023-10-23T14:36:36Z

Finally got a fix contributed for this by @oneiros, closing!

scarroll32 added the tests wanted We would love to see some tests added on a PR to demonstrate this issue label Nov 27, 2020

MrJoy added a commit to MrJoy/ransack that referenced this issue Nov 28, 2020

Add spec described in activerecord-hackery#1153.

6f6ca2b

MrJoy mentioned this issue Nov 28, 2020

Add spec described in #1153. #1176

Merged

MrJoy added a commit to MrJoy/ransack that referenced this issue Dec 1, 2020

Add spec described in activerecord-hackery#1153.

1fbca34

ozzyaaron mentioned this issue Mar 29, 2021

Incorrect references to table aliases in Rails 6.1 #1217

Closed

scarroll32 added a commit that referenced this issue Nov 10, 2021

Merge pull request #1176 from MrJoy/1153-incorrect-reference-to-table…

e50ee62

…-alias Add spec described in #1153.

scarroll32 added a commit that referenced this issue Nov 10, 2021

Revert "Add spec described in #1153."

dcacf03

scarroll32 added a commit that referenced this issue Nov 10, 2021

Merge pull request #1263 from activerecord-hackery/revert-1176-1153-i…

25c5c9d

…ncorrect-reference-to-table-alias Revert "Add spec described in #1153."

deivid-rodriguez added a commit that referenced this issue Mar 8, 2022

Revert "Revert "Add spec described in #1153.""

24b49eb

This reverts commit dcacf03.

deivid-rodriguez mentioned this issue Mar 8, 2022

Revert "Revert "Add spec described in #1153."" #1280

Closed

oneiros mentioned this issue Oct 18, 2023

Fix wrong table aliases (#1217) #1447

Merged

deivid-rodriguez closed this as completed Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorrect reference to table alias. #1153

Incorrect reference to table alias. #1153

MrJoy commented Oct 10, 2020

scarroll32 commented Nov 27, 2020

yahonda commented Dec 2, 2020

deivid-rodriguez commented Dec 2, 2020

yahonda commented Dec 2, 2020

deivid-rodriguez commented Dec 2, 2020

istickz commented Dec 30, 2020

nikajukic commented Jan 22, 2021

scarroll32 commented Nov 10, 2021

MrJoy commented Nov 10, 2021

scarroll32 commented Nov 11, 2021

MrJoy commented Nov 12, 2021

scarroll32 commented Nov 12, 2021

scarroll32 commented Nov 12, 2021

deivid-rodriguez commented Oct 23, 2023

Incorrect reference to table alias. #1153

Incorrect reference to table alias. #1153

Comments

MrJoy commented Oct 10, 2020

scarroll32 commented Nov 27, 2020

yahonda commented Dec 2, 2020

deivid-rodriguez commented Dec 2, 2020

yahonda commented Dec 2, 2020

deivid-rodriguez commented Dec 2, 2020

istickz commented Dec 30, 2020

nikajukic commented Jan 22, 2021

scarroll32 commented Nov 10, 2021

MrJoy commented Nov 10, 2021

scarroll32 commented Nov 11, 2021

MrJoy commented Nov 12, 2021

scarroll32 commented Nov 12, 2021

scarroll32 commented Nov 12, 2021

deivid-rodriguez commented Oct 23, 2023