Rails/WhereEquals Hash conditions over SQL fragment issue

[vpereira]

Hi rubocop team!

Expected Behavior

Using the cop Rails/WhereEquals, constant strings shouldn't get auto corrected to hash

The snippet where('links_to_id is null') should not be auto corrected to where(links_to_id: nil)

Actual Behavior

The cop auto-correct queries without placeholders or interpolation, like where('links_to_id is null')

[koic]

This looks like an expected behavior. What kind of problem is there?

# bad
User.where('name IS NULL')

# good
User.where(name: nil)

https://docs.rubocop.org/rubocop-rails/2.9/cops_rails.html#railswhereequals

[dvandersluis]

One potential issue (and maybe the cop should be not marked as Safe) is that with a string query the DB will match it to any joined column if it's non-ambiguous (ie. User.joins(:books).where("title == 'Lord of the Rings'") will use books.title), whereas with rails it'll raise an error (ie. User.joins(:books).where(title: 'Lord of the Rings') users.title is not a column).

[vpereira]

Hi @koic

Thank you for your time and answer!

# bad
User.where('name IS NULL')

Well, except that it isn't bad. I agree with User.where("name IS ?", nil) being substituted, however, to substitute a constant by a dynamic variable isn't really a best practice, right?

Doing a small benchmark: https://gist.github.com/vpereira/797d594d2bd9abfe35ae8bfb5a78cf85

You get the following results:

Ruby benchmark:

       user     system      total        real
constant  0.052057   0.000033   0.052090 (  0.052152)
dynamic  0.209342   0.000185   0.209527 (  0.209661)


Benchmark.ip

Warming up --------------------------------------
            constant    10.617k i/100ms
             dynamic     2.394k i/100ms
Calculating -------------------------------------
            constant    113.281k (± 1.6%) i/s -    573.318k in   5.062291s
             dynamic     24.383k (± 2.7%) i/s -    122.094k in   5.011607s

Comparison:
            constant:   113280.7 i/s
             dynamic:    24383.5 i/s - 4.65x  slower

Probably the performance deterioration comes from the fact that we have to evaluate the hash, construct the query, etc, etc. So switch blindly constant by a variable, IMO, isn't safe. Disable the cop isn't an option for me. Disable it inline is just ok if you have just a few of such entries. Unfortunately not my case.

My suggestion would be to split this Cop in two:

One cop for dynamic queries using question marks or named bind variables, enabled by default, and
another cop for constants, disabled by default (or moved under the unsafe -A corrections).

[chvp]

The autocorrect of this cop should definitely be marked as unsafe. We just ran into an issue where the following was autocorrected

User.joins(:comment).where("text = ?", example)

to

User.joins(:comment).where(text: example)

which doesn't work, since the text column doesn't exist on the users table, but does on the comments table, resulting in errors.

[koic]

This issue has been resolved by #403. Thank you.