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

Incorrect use of table alias when joining with same table twice (Rails 6.0.2.2 issue) #1119

Closed
nikajukic opened this issue Apr 29, 2020 · 14 comments

Comments

@nikajukic
Copy link

nikajukic commented Apr 29, 2020

Ruby 2.6.1
Ransack 2.3.2
I tried using master branch as well and got the same issue: 6434e3b

I have an issue with generating queries on Rails 6.0.2.2.
With Rails 6.0.0. everything worked fine.

class User < ApplicationRecord
end

class Song < ApplicationRecord
  belongs_to :artist, class_name: 'User'
end

class Track < ApplicationRecord
  belongs_to :song
  belongs_to :producer, class_name: 'User'
end

Issue happens when you try to join with same table twice.

Query:
Track.ransack(song_title_cont_any: 'iva', song_artist_name_cont_any: 'cold', producer_name_cont_any: 'artin').result

In Rails 6.0.0. it generates correct query. Notice how producer_tracks alias is used in both JOIN and WHERE.

"SELECT \"tracks\".* 
FROM \"tracks\"
LEFT OUTER JOIN \"songs\" ON \"songs\".\"id\" = \"tracks\".\"song_id\"
LEFT OUTER JOIN \"users\" ON \"users\".\"id\" = \"songs\".\"artist_id\"
LEFT OUTER JOIN \"users\" \"producer_tracks\" ON \"producer_tracks\".\"id\" = \"tracks\".\"producer_id\"
WHERE ((\"songs\".\"title\" ILIKE '%iva%') AND
(\"users\".\"name\" ILIKE '%cold%') AND
(\"producer_tracks\".\"name\" ILIKE '%artin%'))"

In Rails 6.0.2.2 it generates incorrect query and fails with PG::UndefinedTable: ERROR error:

Notice how:

  • it uses artists_songs alias in JOIN part, but doesn't use it in WHERE part
  • it doesn't use producers_tracks alias in JOIN part, but uses it in WHERE part
"SELECT \"tracks\".*
FROM \"tracks\"
LEFT OUTER JOIN \"users\" ON \"users\".\"id\" = \"tracks\".\"producer_id\"
LEFT OUTER JOIN \"songs\" ON \"songs\".\"id\" = \"tracks\".\"song_id\"
LEFT OUTER JOIN \"users\" \"artists_songs\" ON \"artists_songs\".\"id\" = \"songs\".\"artist_id\" WHERE ((\"songs\".\"title\" ILIKE '%iva%') AND
(\"users\".\"name\" ILIKE '%cold%') AND
(\"producers_tracks\".\"name\" ILIKE '%artin%'))"

This query fails with following error:

       PG::UndefinedTable: ERROR:  missing FROM-clause entry for table "producer_tracks"
       LINE 1: ...'%iva%') AND ("users"."name" ILIKE '%cold%') AND ("producers...
@nikajukic
Copy link
Author

Update: Figured out this issue was introduced even before Rails 6.0.2.2, specifically in Rails 6.0.1. https://github.com/rails/rails/releases/tag/v6.0.1

Issue is still present in Rails 6.0.3.1

@PhilCoggins
Copy link
Contributor

Error can be observed by adding s.first after this line

@nikajukic
Copy link
Author

Thanks @PhilCoggins this was helpful!

I've investigated this a bit further and noticed something. Here are two test examples you should add to https://github.com/activerecord-hackery/ransack/blob/master/spec/ransack/search_spec.rb

I've managed to get the same setup with your test models.
I'm joining comment with person directly and then again through article association.

Passing spec:

it 'use appropriate table alias when joining with same table twice' do
        s = Search.new(Comment, {
          person_name_eq: "Person Name",
          article_title_eq: "Article Title",
          article_person_name_eq: "Article Person Name"
        }).result
        real_query = remove_quotes_and_backticks(s.to_sql)

        if ::Gem::Version.new(::ActiveRecord::VERSION::STRING) > ::Gem::Version.new(Constants::RAILS_6_0)
          expect(real_query)
                  .to match(%r{LEFT OUTER JOIN people ON people.id = comments.person_id})
          expect(real_query)
                  .to match(%r{LEFT OUTER JOIN articles ON articles.id = comments.article_id AND \('default_scope' = 'default_scope'\)})
          expect(real_query)
                  .to match(%r{LEFT OUTER JOIN people people_articles ON people_articles.id = articles.person_id})
        end

        expect(real_query)
          .to include "people.name = 'Person Name'"
        expect(real_query)
          .to include "articles.title = 'Article Title'"
        expect(real_query)
          .to include "people_articles.name = 'Article Person Name'"
      end

However, if I change the order of conditions this test starts to fail and you get the same invalid SQL generated as in my initial example:

person_name_eq: "Person Name",
article_title_eq: "Article Title",
article_person_name_eq: "Article Person Name"

to

article_title_eq: "Article Title",
article_person_name_eq: "Article Person Name",
person_name_eq: "Person Name"

Failing spec:

it 'use appropriate table alias when joining with same table twice' do
        s = Search.new(Comment, {
          article_title_eq: "Article Title",
          article_person_name_eq: "Article Person Name",
          person_name_eq: "Person Name",
        }).result
        real_query = remove_quotes_and_backticks(s.to_sql)

        if ::Gem::Version.new(::ActiveRecord::VERSION::STRING) > ::Gem::Version.new(Constants::RAILS_6_0)
          expect(real_query)
                  .to match(%r{LEFT OUTER JOIN people ON people.id = comments.person_id})
          expect(real_query)
                  .to match(%r{LEFT OUTER JOIN articles ON articles.id = comments.article_id AND \('default_scope' = 'default_scope'\)})
          expect(real_query)
                  .to match(%r{LEFT OUTER JOIN people people_articles ON people_articles.id = articles.person_id})
        end

        expect(real_query)
          .to include "people.name = 'Person Name'"
        expect(real_query)
          .to include "articles.title = 'Article Title'"
        expect(real_query)
          .to include "people_articles.name = 'Article Person Name'"
      end
  1) Ransack::Search#result use appropriate table alias when joining with same table twice
     Failure/Error:
       expect(real_query)
         .to include "people.name = 'Person Name'"

       expected "SELECT comments.* FROM comments LEFT OUTER JOIN people ON people.id = comments.person_id LEFT OUTER ...= 'Article Title' AND people.name = 'Article Person Name' AND people_comments.name = 'Person Name')" to include "people.name = 'Person Name'"
     # ./spec/ransack/search_spec.rb:314:in `block (3 levels) in <module:Ransack>'

Invalid SQL

SELECT comments.*
FROM comments
LEFT OUTER JOIN people ON people.id = comments.person_id
LEFT OUTER JOIN articles ON articles.id = comments.article_id AND ('default_scope' = 'default_scope')
LEFT OUTER JOIN people people_articles ON people_articles.id = articles.person_id
WHERE comments.disabled = 0 AND
(articles.title = 'Article Title' AND
people.name = 'Article Person Name' AND
people_comments.name = 'Person Name')"

Same culprit as in my initial example:

  • people_articles alias is used in JOIN, but is not used in WHERE
  • people_comments alias is NOT used in JOIN, but is used in WHERE

@PhilCoggins
Copy link
Contributor

Nice find, I think the culprit can be found in rails/rails#37235, which fixed inconsistent join ordering in ActiveRecord. If there's a way to pass joins in the proper order from Ransack to ActiveRecord, I think that would fix this.

@PhilCoggins
Copy link
Contributor

Looks like these are a lot of the same issues observed in #1081

This was referenced May 20, 2020
@varyonic
Copy link
Contributor

Does rails/rails#39305 help?

@deivid-rodriguez
Copy link
Contributor

Sounds super promising!

@nikajukic
Copy link
Author

nikajukic commented May 20, 2020

I've tried rails/rails#39305 that @varyonic suggested and I've run the failing test again and it seems like the query isn't invalid anymore:
Before:

SELECT comments.*
FROM comments
LEFT OUTER JOIN people ON people.id = comments.person_id
LEFT OUTER JOIN articles ON articles.id = comments.article_id AND ('default_scope' = 'default_scope')
LEFT OUTER JOIN people people_articles ON people_articles.id = articles.person_id
WHERE comments.disabled = 0 AND
(articles.title = 'Article Title' AND
people.name = 'Article Person Name' AND
people_comments.name = 'Person Name')"

After:

SELECT comments.*
FROM comments
LEFT OUTER JOIN articles ON articles.id = comments.article_id AND ('default_scope' = 'default_scope')
LEFT OUTER JOIN people ON people.id = articles.person_id
LEFT OUTER JOIN people people_comments ON people_comments.id = comments.person_id WHERE comments.disabled = 0 AND
(articles.title = 'Article Title' AND
people.name = 'Article Person Name' AND
people_comments.name = 'Person Name')

Query itself is valid and uses correct aliases in JOIN and WHERE part. 🎉

My failing test is still failing, but with different error and it is actually supposed to fail in this case because the JOINs are in different order, depending on the order of given conditions, which is how it's supposed to work.

Point of my failing test was to catch invalid query you get when you change the order of given conditions.

@scarroll32
Copy link
Member

@nikajukic could you please rebase and see if the problem still exists?

@prsanjay
Copy link

I have faced a similar issue with Rails 6.0.2 and Ransack 2.4.0

After upgrading Rails version to 6.1.0 it all works well

@scarroll32
Copy link
Member

After upgrading Rails version to 6.1.0 it all works well

Wow that is unexpected good news!!

@nikajukic
Copy link
Author

@seanfcarroll I'll upgrade to Rails 6.1 in the next few days and put an update here.

@nikajukic
Copy link
Author

@seanfcarroll I've successfully upgraded Rails to 6.1.1 and ransack to 2.4.1 and this issue is now fixed 🎉

@scarroll32
Copy link
Member

Hey that is great news @nikajukic thanks for posting back here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants