Searching across multiple associations that reference the same model/table joining on the wrong association #374

jhdavids8 · 2014-05-14T04:18:32Z

Hi all!

Let's say I have a model such as:

class Recommendation < ActiveRecord::Base
  belongs_to :user
  belongs_to :target_user, class_name: 'User'
end

Now, say my Ransack search filters looks like this (as JSON, as I'm using Ransack with a REST API. Hopefully, it's more readable):

q: {
  user: {
    job_title: { 
      eq: "Designer"
    }
  },
  target_user: {
    group: {
      name: {
        eq: "Design"
      }
    }
  }
}

When I was on Rails 4.0.2 and Ransack 1.2.1, this was the SQL output I received:

"SELECT \"recommendations\".* FROM \"recommendations\" 
LEFT OUTER JOIN \"users\" ON \"users\".\"id\" = \"recommendations\".\"user_id\" 
LEFT OUTER JOIN \"users\" \"target_users_recommendations\" ON \"target_users_recommendations\".\"id\" = \"recommendations\".\"target_user_id\" 
LEFT OUTER JOIN \"groups\" ON \"groups\".\"id\" = \"target_users_recommendations\".\"department_id\" 
WHERE ((\"users\".\"job_title\" = 'Designer' AND \"groups\".\"name\" = 'Design'))"

Notice how the group is joined here (4th line, last LEFT OUTER JOIN).

However, after upgrading to Rails 4.1.1 and the Ransack 4.1 branch, this is the SQL output, which is wrong, as notice now how the group is joined (it's joined on the first user association, not the target user association).

"SELECT \"recommendations\".* FROM \"recommendations\"
LEFT OUTER JOIN \"users\" ON \"users\".\"id\" = \"recommendations\".\"user_id\" 
LEFT OUTER JOIN \"users\" \"target_users_recommendations\" ON \"target_users_recommendations\".\"id\" = \"recommendations\".\"target_user_id\" 
LEFT OUTER JOIN \"groups\" ON \"groups\".\"id\" = \"users\".\"department_id\" 
WHERE ((\"users\".\"job_title\" = 'Designer' AND \"groups\".\"name\" = 'Design'))"

This is consistent across several specs I have failing at the moment. Any query on an association for the second user association will fail, as it joins on the former user association.

Hopefully this is enough to get you guys started, but if not, please let me know. I'm going to try and find some time to dig deeper myself. Thanks!

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

jonatack · 2014-05-15T00:05:21Z

Hi @jhdavids8, thanks for the report. Just merged in a fix by @dush for a merged join issue on the rails-4.1 branch. I think it's a different issue, but could you give it a try and see if it helps?

jhdavids8 · 2014-05-15T01:35:05Z

Unfortunately, it doesn't seem to have fixed the issue. I'm still seeing the same specs fail for the same reasons.

arjes · 2014-05-16T16:26:02Z

I'm getting hit with this issue too when upgrading to Rails 4.1

class User < ActiveRecord::Base
  has_many :comments
  has_many :foo, through: :comments
end
 class Comment < ActiveRecord::Base
  belongs_to :user
  has_many :foo
end
class Foo < ActiveRecord::Base
  belongs_to :comment
end

User.first.foo.search(:comment_user_name_eq => "omg").result.to_sql

This produces different code in 4.0 vs 4.1

Rails 4.1

SELECT "foos".* FROM "foos" 
LEFT OUTER JOIN "comments" "comments_foos" ON "comments_foos"."id" = "foos"."comment_id" 
LEFT OUTER JOIN "users" ON "users"."id" = "comments"."user_id" 
INNER JOIN "comments" ON "foos"."comment_id" = "comments"."id" 
WHERE "comments"."user_id" = ? AND "users"."name" = 'omg'

Rails 4.0

SELECT "foos".* FROM "foos" 
LEFT OUTER JOIN "comments" "comments_foos" ON "comments_foos"."id" = "foos"."comment_id" 
LEFT OUTER JOIN "users" ON "users"."id" = "comments_foos"."user_id" 
INNER JOIN "comments" ON "foos"."comment_id" = "comments"."id" 
WHERE "comments"."user_id" = ? AND "users"."name" = 'omg'

Note that the differences is in this line

LEFT OUTER JOIN "users" ON "users"."id" = "comments"."user_id"

This happens on Master and Rails 4.1. Here is a repo with the code I used to create the above:
https://github.com/arjes/ransack-rails4.1-join-on-bug

If you can point me in the right direction I can try to help, when I'm not working.

jonatack · 2014-05-16T16:36:38Z

It's likely to be in Ransack::Adapters::ActiveRecord:Context. Or possibly Ransack::Context.

@huoxito wdyt?

huoxito · 2014-05-18T10:48:26Z

My initial guess is Ransack::Context#build_for_find_association. It's hard to tell for sure without further investigation though. It might as well as be something wrong in rails. Looking into it. I think we could also try to improve that same method to use more of activerecord public api. Chances are we'd have a more reliable implementation.

@arjes @jhdavids8 any chance you guys could give us a spec that would fail on 4.1 but pass on 4.0? That should help a lot to fix the issue and ensure it doesn't happen again.

jonatack · 2014-05-18T13:10:02Z

@huoxito yes, my initial guess would be in that method.

jhdavids8 · 2014-05-19T22:48:40Z

Hi @huoxito I'm looking into this now. Stay tuned!

jhdavids8 · 2014-05-20T02:57:55Z

Well, this is a pretty poor effort at a spec that fails on 4.1 but not on 4.0, but here it is.

I did as much digging as time allowed, but my lack of knowledge on the inner workings of ActiveRecord and Polyamorous prevented me from getting too far. The error definitely appears to be in Ransack::Context#build_for_find_association though. Everything appeared normal up to line 177, where the JoinDependency is built. That dependency appeared to have nothing that linked it to the aliased association (in my code, this was :target_person) over the non-aliased one (just :person).

Sorry, but that's as far as I could get for now. If you guys don't beat me to a solution, I'll try to find some more time to dig deeper later in the week. Thanks!

huoxito · 2014-05-20T14:22:13Z

Thank you @jhdavids8! I'll take a look again and try out the spec as soon as I get some spare time, let you know if I find any solution

avit · 2014-08-15T09:07:56Z

It looks like the 4.1 conditional branch builds a new JoinDependency, but the "else" branch uses the assigned @join_dependency instance variable. That's a likely clue here.

JanStevens · 2014-12-12T11:35:54Z

Any update on this issue? I have the exact same issue where ransack fails to find the renamed column and thus generates a postgres error.

jonatack · 2014-12-12T15:15:16Z

Thanks for the heads up. If there was an update I would try to make sure it was referenced here and visible. Anyone who is affected by this issue is most welcome to work on a fix.

clammers · 2015-01-02T11:56:33Z

Issue also occured in rails 4.2 with ransack 1.5.1. Fix would be great!

yairlev · 2015-01-16T23:30:51Z

Same issue with rails 4.1.0 and ransack 1.6.2

sitch · 2015-03-24T15:48:46Z

Same issue with rails 4.1.8 and ransack 1.6.4, 4.1 branch, and current master branch

jonatack · 2015-03-25T09:27:44Z

@jhdavid8 thanks for the regression test, which is the first step toward a fix 👍

Everyone else: Thanks for the reports, but you need to provide a test or use someone's test like @jhdavids8's and show the test and its output, otherwise your report is not useful because not precisely reproducible and testable. You need to show a failing test.

I merged @jhdavids8's spec into Ransack and can confirm that it passes in Rails 4-0-stable, but not in 4-1-stable, 4-2-stable and 5.0.0.alpha / master. Here is the output when the spec fails (the result is the same with 4-1-stable, 4-2-stable, 5.0.0.alpha, and using SQLite3):

∴ DB=PostgreSQL bundle exec rake spec
===================================================================================
Running specs against PostgreSQL, Active Record 5.0.0.alpha and Arel 7.0.0.alpha...
===================================================================================
...................................................................................
...................................................................................
....................................................................F..............
.....

Failures:

1) Ransack::Search#result evaluates conditions for multiple belongs_to associations
to the same table contextually

expected:
"SELECT \"recommendations\".* FROM \"recommendations\"
LEFT OUTER JOIN \"people\" ON \"people\".\"id\" = \"recommendations\".\"person_id\"
LEFT OUTER JOIN \"people\" \"target_people_recommendations\"
  ON \"target_people_recommendations\".\"id\" = \"recommendations\".\"target_person_id\"
LEFT OUTER JOIN \"people\" \"parents_people\"
  ON \"parents_people\".\"id\" = \"target_people_recommendations\".\"parent_id\"
WHERE ((\"people\".\"name\" = 'Ernie' AND \"parents_people\".\"name\" = 'Test'))"

got: 
"SELECT \"recommendations\".* FROM \"recommendations\"
LEFT OUTER JOIN \"people\" ON \"people\".\"id\" = \"recommendations\".\"person_id\"
LEFT OUTER JOIN \"people\" \"target_people_recommendations\"
  ON \"target_people_recommendations\".\"id\" = \"recommendations\".\"target_person_id\"
LEFT OUTER JOIN \"people\" \"parents_people\"
  ON \"parents_people\".\"id\" = \"people\".\"parent_id\"
WHERE (\"people\".\"name\" = 'Ernie' AND \"parents_people\".\"name\" = 'Test')"

# ./spec/ransack/search_spec.rb:220:in `block (3 levels) in <module:Ransack>'

jonatack · 2015-03-25T10:21:28Z

Pushed the spec to Ransack master to help anyone who wants / needs to fix this issue:

Fork Ransack to your repo
Open the file spec/ransack/search_spec.rb and comment out the ActiveRecord::VERSION conditional (lines 220 and 238)
Set the Rails version you'd like to test in ransack's Gemfile ('4-1-stable', '4.2.1', etc.)
In the console, run bundle install, then bundle exec rake spec to run the tests
Write a fix that makes the tests pass!
Send a PR 😃

jonatack · 2015-03-25T10:42:30Z

The fix might be related to Rails issue #19276 and Rails PR #19452.

- Add a Active Record conditional so there are only 2 lines to comment out if someone would like to work on #374. - Improve the SQL query readability.

jonatack · 2015-03-29T05:29:55Z

There is also this: rails/rails#19427 (comment)

linkyndy · 2015-04-14T12:35:46Z

Here's yet another gist related to this issue: https://gist.github.com/linkyndy/e84534e1cdc19dd8e4cc

I've posted it just to raise awareness of this bug :)

sitch · 2015-04-14T13:19:09Z

Looks like it's not properly referencing a table alias. You can see a diff from the two AST's here:
https://www.diffchecker.com/1d3kkrjq

bertBruynooghe · 2015-04-30T16:34:32Z

FYI: I was able to work around the issue of @linkyndy by defining an extra association has_one :master_party, through: :master_client, source: :party and use Client.ransack(master_party_name_cont: 'arty') instead.
I guess the issue of @jhdavids8 could also be worked around by creating a target_group associaton, and using that in the ransack expression.

sitch · 2015-04-30T16:53:25Z

The main reason for these issues is the Rails 4.1 code transitioning from matching joins on JoinAssociations to JoinDependencys (which have their own AliasTracker that is not coalesced with other JoinDependency's AliasTrackers). I am not sure if this is a active design decision on the part of the Rails team, but for a patch on the Ransack side, we would need to build a singular (correctly aliased) JoinDependency (or monkeypatch the Rails code to support attaching JoinAssociations

bertBruynooghe · 2015-06-30T14:12:07Z

I worked around the issue by dynamically creating the necessary associations in a Rails initialiser (code here). It might be an idea to fix the issue by relying on a more stable Rails API?

Any ideas on possible side effects of this patch are highly appreciated.

…ery#374

scarroll32 · 2020-01-10T23:32:27Z

Closed due to age

jonatack closed this as completed Mar 25, 2015

jonatack reopened this Mar 25, 2015

jonatack closed this as completed in 8fe8368 Mar 25, 2015

jonatack reopened this Mar 25, 2015

jonatack added a commit that referenced this issue Mar 26, 2015

Improve test for #374

29a78f5

- Add a Active Record conditional so there are only 2 lines to comment out if someone would like to work on #374. - Improve the SQL query readability.

wuboy0307 mentioned this issue Aug 18, 2017

Ransack is not correctly overriding/using previous joins #824

Open

y-zoe added a commit to y-zoe/ransack that referenced this issue Feb 21, 2019

Fix wrong table alias when using nested join. issue activerecord-hack…

97d1782

…ery#374

y-zoe mentioned this issue Feb 21, 2019

Fix wrong table alias when using nested join. #1005

Closed

scarroll32 closed this as completed Jan 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Searching across multiple associations that reference the same model/table joining on the wrong association #374

Searching across multiple associations that reference the same model/table joining on the wrong association #374

jhdavids8 commented May 14, 2014

jonatack commented May 15, 2014

jhdavids8 commented May 15, 2014

arjes commented May 16, 2014

jonatack commented May 16, 2014

huoxito commented May 18, 2014

jonatack commented May 18, 2014

jhdavids8 commented May 19, 2014

jhdavids8 commented May 20, 2014

huoxito commented May 20, 2014

avit commented Aug 15, 2014

JanStevens commented Dec 12, 2014

jonatack commented Dec 12, 2014

clammers commented Jan 2, 2015

yairlev commented Jan 16, 2015

sitch commented Mar 24, 2015

jonatack commented Mar 25, 2015

jonatack commented Mar 25, 2015

jonatack commented Mar 25, 2015

jonatack commented Mar 29, 2015

linkyndy commented Apr 14, 2015

sitch commented Apr 14, 2015

bertBruynooghe commented Apr 30, 2015

sitch commented Apr 30, 2015

bertBruynooghe commented Jun 30, 2015

scarroll32 commented Jan 10, 2020

Searching across multiple associations that reference the same model/table joining on the wrong association #374

Searching across multiple associations that reference the same model/table joining on the wrong association #374

Comments

jhdavids8 commented May 14, 2014

jonatack commented May 15, 2014

jhdavids8 commented May 15, 2014

arjes commented May 16, 2014

jonatack commented May 16, 2014

huoxito commented May 18, 2014

jonatack commented May 18, 2014

jhdavids8 commented May 19, 2014

jhdavids8 commented May 20, 2014

huoxito commented May 20, 2014

avit commented Aug 15, 2014

JanStevens commented Dec 12, 2014

jonatack commented Dec 12, 2014

clammers commented Jan 2, 2015

yairlev commented Jan 16, 2015

sitch commented Mar 24, 2015

jonatack commented Mar 25, 2015

jonatack commented Mar 25, 2015

jonatack commented Mar 25, 2015

jonatack commented Mar 29, 2015

linkyndy commented Apr 14, 2015

sitch commented Apr 14, 2015

bertBruynooghe commented Apr 30, 2015

sitch commented Apr 30, 2015

bertBruynooghe commented Jun 30, 2015

scarroll32 commented Jan 10, 2020