Rails/WhereEquals autocorrect changes query generation behavior

[yitznewton]

On updating 2.8.1 to 2.9.0, running autocorrect on a particular piece of code results in Rails incorrectly inferring the table of a column (vendors.staged), and generating a different Postgres query for the wrong table. In this case the code itself can be fixed after autocorrect by specifying the table name.

Active Record code before:

  def self.include_staged(staged)
    s = all.joins(:vendor)
    s = s.where("staged = ?", false) unless staged
    s
  end

SQL before:

SELECT  DISTINCT "vendor_updates"."id", vendors.name AS alias_0
FROM "vendor_updates"
INNER JOIN "public"."vendors" ON "public"."vendors"."id" = "vendor_updates"."vendor_id"
LEFT OUTER JOIN "pricing_zones_vendors" ON "pricing_zones_vendors"."vendor_id" = "public"."vendors"."id"
LEFT OUTER JOIN "zones" ON "zones"."id" = "pricing_zones_vendors"."pricing_zone_id"
LEFT OUTER JOIN "data_files" ON "data_files"."id" = "vendor_updates"."parsed_file_id"
WHERE "vendor_updates"."month" = $1 AND (staged = 'f') 
ORDER BY vendors.name asc LIMIT 100 OFFSET 0

Active Record after:

  def self.include_staged(staged)
    s = all.joins(:vendor)
    s = s.where(staged: false) unless staged
    s
  end

SQL after:

SELECT  DISTINCT "vendor_updates"."id", vendors.name AS alias_0
FROM "vendor_updates"
INNER JOIN "public"."vendors" ON "public"."vendors"."id" = "vendor_updates"."vendor_id"
LEFT OUTER JOIN "pricing_zones_vendors" ON "pricing_zones_vendors"."vendor_id" = "public"."vendors"."id"
LEFT OUTER JOIN "zones" ON "zones"."id" = "pricing_zones_vendors"."pricing_zone_id"
LEFT OUTER JOIN "data_files" ON "data_files"."id" = "vendor_updates"."parsed_file_id"
WHERE "vendor_updates"."month" = $1 AND "vendor_updates"."staged" = 'f' 
ORDER BY vendors.name asc LIMIT 100 OFFSET 0

RuboCop version

Before

$ rubocop -V
1.5.2 (using Parser 2.7.2.0, rubocop-ast 1.3.0, running on ruby 2.6.6 x86_64-linux)
  - rubocop-performance 1.9.1
  - rubocop-rails 2.8.1

After

$ rubocop -V
1.5.2 (using Parser 2.7.2.0, rubocop-ast 1.3.0, running on ruby 2.6.6 x86_64-linux)
  - rubocop-performance 1.9.1
  - rubocop-rails 2.9.0

[andyw8]

Possibly due to #362 @eugeneius ?

[koic]

The query string has changed, but I think the result will not changed.
It looks like "SQL after" is preferable because table name is definitely uniquely qualified.
I think that the table that was originally guessed uniquely with "SQL before" is qualified to "SQL after" because if table cannot be uniquely made with "SQL before", I think it is an error.
So, I think this is an expected behavior. How about that?

[yitznewton]

In this case there is no vendor_updates.staged column. We found out about this difference because one of our tests failed after autocorrection, with a Postgres error because of the non-existent column. It seems to me this correction should be changed, or moved under the unsafe -A corrections.

[koic]

That's interesting. Is it possible to provide (minimum) executable reproducible code and Rails version? It's possible that Active Record behaves unintentionally.

Anyway, if it's difficult to resolve false positives for incompatible behavior, we can consider the unsafe -A correction.

[yitznewton]

Thanks @koic - here's a minimal Rails 4.2 project which shows off the difference: https://github.com/yitznewton/rubocop-where-equal-issue

1. Run rake db:migrate
2. Run rake test - the test passes
3. Run rubocop -a - autocorrects the Active Record code
4. Run rake test - the test errors out

[koic]

Thank you for the additional information. RuboCop Rails is conservative about support for Rails 4.2. The RuboCop Rails 3.0 will drop support for Rails 4.2 in near future. So, I'd like to see if there is a similar feedback in Rails 5 (Preferably maintained 5.2 or higher).

[yitznewton]

@koic I replaced the repo with a Rails 5.2 version at the same URL; the behavior is still observed.

[ilkkao]

I found a related issue (I can file this separately if you prefer). The following Active Record model

# frozen_string_literal: true

class User < ApplicationRecord
  scope :for_account, (lambda do |account_ids|
    where(
      'users.account_id IN (?)', account_ids.map { |id| "\\x#{id}" }
    )
  end)
end

Produces the output:

app/models/user.rb:5:5: C: [Correctable] Rails/WhereEquals: Use where('users.account_id' => account_ids.map { |id| "\\x#{id}" }) instead of manually constructing SQL.

But in this case account_id is a Postgres bytea column and scope gets it in a hex format (documented in: https://www.postgresql.org/docs/9.0/datatype-binary.html) so manual \x escaping is needed. That's not the case with "automatic" mode. There Rails does the conversion on your behalf behind the scenes (in this Rails project at least). The correct fix in this case is simply:

where('users.account_id' => account_ids.map { |id| "#{id}" })

which can be shortened to

where('users.account_id' => account_ids)

What rubocop did caused double encoding in our Rails app. Because of this encoding corner case difference, automatic fix is not always safe. But it was very helpful to spot unnecessary manual SQL!

[ilkkao]

btw @yitznewton in your example, this would have worked right?

  def self.include_staged(staged)
    s = all.joins(:vendor)
    s = s.where(vendors: { staged: false}) unless staged
    s
  end

joins affect how where needs to be constructed in the non-manual case. That is most likely too hard for autocorrect to solve. Maybe not classify this cop as SafeAutoCorrect.