From d4bd05f3a22d5ed1508bdc35793715cafd306091 Mon Sep 17 00:00:00 2001 From: Eugene Kenny Date: Mon, 21 Sep 2020 11:46:49 +0100 Subject: [PATCH] Add new Rails/WhereEquals cop This complements the existing `Rails/WhereNot` cop, so that simple equality and inequality conditions are corrected in the same way. For example, code like this: scope :incomplete, -> { where('completed_at IS NULL') } scope :complete, -> { where('completed_at IS NOT NULL') } Will now be autocorrected to: scope :incomplete, -> { where(completed_at: nil) } scope :complete, -> { where.not(completed_at: nil) } Instead of: scope :incomplete, -> { where('completed_at IS NULL') } scope :complete, -> { where.not(completed_at: nil) } --- CHANGELOG.md | 4 + config/default.yml | 8 +- docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_rails.adoc | 38 ++++- lib/rubocop/cop/rails/where_equals.rb | 105 +++++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + spec/rubocop/cop/rails/where_equals_spec.rb | 163 ++++++++++++++++++++ 7 files changed, 318 insertions(+), 2 deletions(-) create mode 100644 lib/rubocop/cop/rails/where_equals.rb create mode 100644 spec/rubocop/cop/rails/where_equals_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index e675b028da..bebe490d06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## master (unreleased) +### New features + +* [#362](https://github.com/rubocop-hq/rubocop-rails/pull/362): Add new `Rails/WhereEquals` cop. ([@eugeneius][]) + ## 2.8.1 (2020-09-16) ### Bug fixes diff --git a/config/default.yml b/config/default.yml index 02abf38546..b9d9d38571 100644 --- a/config/default.yml +++ b/config/default.yml @@ -705,6 +705,12 @@ Rails/Validation: Include: - app/models/**/*.rb +Rails/WhereEquals: + Description: 'Pass conditions to `where` as a hash instead of manually constructing SQL.' + StyleGuide: 'https://rails.rubystyle.guide/#hash-conditions' + Enabled: 'pending' + VersionAdded: '2.9' + Rails/WhereExists: Description: 'Prefer `exists?(...)` over `where(...).exists?`.' Enabled: 'pending' @@ -717,7 +723,7 @@ Rails/WhereExists: Rails/WhereNot: Description: 'Use `where.not(...)` instead of manually constructing negated SQL in `where`.' - StyleGuide: 'https://rails.rubystyle.guide/#where-not' + StyleGuide: 'https://rails.rubystyle.guide/#hash-conditions' Enabled: 'pending' VersionAdded: '2.8' diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 7b11177ea9..3f616650af 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -95,6 +95,7 @@ based on the https://rails.rubystyle.guide/[Rails Style Guide]. * xref:cops_rails.adoc#railsuniquevalidationwithoutindex[Rails/UniqueValidationWithoutIndex] * xref:cops_rails.adoc#railsunknownenv[Rails/UnknownEnv] * xref:cops_rails.adoc#railsvalidation[Rails/Validation] +* xref:cops_rails.adoc#railswhereequals[Rails/WhereEquals] * xref:cops_rails.adoc#railswhereexists[Rails/WhereExists] * xref:cops_rails.adoc#railswherenot[Rails/WhereNot] diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index 276b07f550..1556dccf99 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -4285,6 +4285,42 @@ validates :foo, uniqueness: true | Array |=== +== Rails/WhereEquals + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Pending +| Yes +| Yes +| 2.9 +| - +|=== + +This cop identifies places where manually constructed SQL +in `where` can be replaced with `where(attribute: value)`. + +=== Examples + +[source,ruby] +---- +# bad +User.where('name = ?', 'Gabe') +User.where('name = :name', name: 'Gabe') +User.where('name IS NULL') +User.where('name IN (?)', ['john', 'jane']) +User.where('name IN (:names)', names: ['john', 'jane']) + +# good +User.where(name: 'Gabe') +User.where(name: nil) +User.where(name: ['john', 'jane']) +---- + +=== References + +* https://rails.rubystyle.guide/#hash-conditions + == Rails/WhereExists |=== @@ -4387,4 +4423,4 @@ User.where.not(name: ['john', 'jane']) === References -* https://rails.rubystyle.guide/#where-not +* https://rails.rubystyle.guide/#hash-conditions diff --git a/lib/rubocop/cop/rails/where_equals.rb b/lib/rubocop/cop/rails/where_equals.rb new file mode 100644 index 0000000000..b43069acaa --- /dev/null +++ b/lib/rubocop/cop/rails/where_equals.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # This cop identifies places where manually constructed SQL + # in `where` can be replaced with `where(attribute: value)`. + # + # @example + # # bad + # User.where('name = ?', 'Gabe') + # User.where('name = :name', name: 'Gabe') + # User.where('name IS NULL') + # User.where('name IN (?)', ['john', 'jane']) + # User.where('name IN (:names)', names: ['john', 'jane']) + # + # # good + # User.where(name: 'Gabe') + # User.where(name: nil) + # User.where(name: ['john', 'jane']) + class WhereEquals < Cop + include RangeHelp + + MSG = 'Use `%s` instead of manually constructing SQL.' + + def_node_matcher :where_method_call?, <<~PATTERN + { + (send _ :where (array $str_type? $_ ?)) + (send _ :where $str_type? $_ ?) + } + PATTERN + + def on_send(node) + where_method_call?(node) do |template_node, value_node| + value_node = value_node.first + + range = offense_range(node) + + column_and_value = extract_column_and_value(template_node, value_node) + return unless column_and_value + + good_method = build_good_method(*column_and_value) + message = format(MSG, good_method: good_method) + + add_offense(node, location: range, message: message) + end + end + + def autocorrect(node) + where_method_call?(node) do |template_node, value_node| + value_node = value_node.first + + lambda do |corrector| + range = offense_range(node) + + column, value = *extract_column_and_value(template_node, value_node) + replacement = build_good_method(column, value) + + corrector.replace(range, replacement) + end + end + end + + EQ_ANONYMOUS_RE = /\A([\w.]+)\s+=\s+\?\z/.freeze # column = ? + IN_ANONYMOUS_RE = /\A([\w.]+)\s+IN\s+\(\?\)\z/i.freeze # column IN (?) + EQ_NAMED_RE = /\A([\w.]+)\s+=\s+:(\w+)\z/.freeze # column = :column + IN_NAMED_RE = /\A([\w.]+)\s+IN\s+\(:(\w+)\)\z/i.freeze # column IN (:column) + IS_NULL_RE = /\A([\w.]+)\s+IS\s+NULL\z/i.freeze # column IS NULL + + private + + def offense_range(node) + range_between(node.loc.selector.begin_pos, node.loc.expression.end_pos) + end + + def extract_column_and_value(template_node, value_node) + value = + case template_node.value + when EQ_ANONYMOUS_RE, IN_ANONYMOUS_RE + value_node.source + when EQ_NAMED_RE, IN_NAMED_RE + return unless value_node.hash_type? + + pair = value_node.pairs.find { |p| p.key.value.to_sym == Regexp.last_match(2).to_sym } + pair.value.source + when IS_NULL_RE + 'nil' + else + return + end + + [Regexp.last_match(1), value] + end + + def build_good_method(column, value) + if column.include?('.') + "where('#{column}' => #{value})" + else + "where(#{column}: #{value})" + end + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 8531298c27..ea05106e55 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -83,5 +83,6 @@ require_relative 'rails/unique_validation_without_index' require_relative 'rails/unknown_env' require_relative 'rails/validation' +require_relative 'rails/where_equals' require_relative 'rails/where_exists' require_relative 'rails/where_not' diff --git a/spec/rubocop/cop/rails/where_equals_spec.rb b/spec/rubocop/cop/rails/where_equals_spec.rb new file mode 100644 index 0000000000..938fdeb4dd --- /dev/null +++ b/spec/rubocop/cop/rails/where_equals_spec.rb @@ -0,0 +1,163 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::WhereEquals do + subject(:cop) { described_class.new } + + it 'registers an offense and corrects when using `=` and anonymous placeholder' do + expect_offense(<<~RUBY) + User.where('name = ?', 'Gabe') + ^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: 'Gabe')` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + User.where(name: 'Gabe') + RUBY + end + + it 'registers an offense and corrects when using `=` and named placeholder' do + expect_offense(<<~RUBY) + User.where('name = :name', name: 'Gabe') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: 'Gabe')` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + User.where(name: 'Gabe') + RUBY + end + + it 'registers an offense and corrects when using `IS NULL`' do + expect_offense(<<~RUBY) + User.where('name IS NULL') + ^^^^^^^^^^^^^^^^^^^^^ Use `where(name: nil)` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + User.where(name: nil) + RUBY + end + + it 'registers an offense and corrects when using `IN` and anonymous placeholder' do + expect_offense(<<~RUBY) + User.where("name IN (?)", ['john', 'jane']) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: ['john', 'jane'])` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + User.where(name: ['john', 'jane']) + RUBY + end + + it 'registers an offense and corrects when using `IN` and named placeholder' do + expect_offense(<<~RUBY) + User.where("name IN (:names)", names: ['john', 'jane']) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: ['john', 'jane'])` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + User.where(name: ['john', 'jane']) + RUBY + end + + it 'registers an offense and corrects when using `=` and namespaced columns' do + expect_offense(<<~RUBY) + Course.where('enrollments.student_id = ?', student.id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where('enrollments.student_id' => student.id)` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + Course.where('enrollments.student_id' => student.id) + RUBY + end + + context 'with array arguments' do + it 'registers an offense and corrects when using `=` and anonymous placeholder' do + expect_offense(<<~RUBY) + User.where(['name = ?', 'Gabe']) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: 'Gabe')` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + User.where(name: 'Gabe') + RUBY + end + + it 'registers an offense and corrects when using `=` and named placeholder' do + expect_offense(<<~RUBY) + User.where(['name = :name', { name: 'Gabe' }]) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: 'Gabe')` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + User.where(name: 'Gabe') + RUBY + end + + it 'registers an offense and corrects when using `IS NULL`' do + expect_offense(<<~RUBY) + User.where(['name IS NULL']) + ^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: nil)` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + User.where(name: nil) + RUBY + end + + it 'registers an offense and corrects when using `IN` and anonymous placeholder' do + expect_offense(<<~RUBY) + User.where(["name IN (?)", ['john', 'jane']]) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: ['john', 'jane'])` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + User.where(name: ['john', 'jane']) + RUBY + end + + it 'registers an offense and corrects when using `IN` and named placeholder' do + expect_offense(<<~RUBY) + User.where(["name IN (:names)", names: ['john', 'jane']]) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: ['john', 'jane'])` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + User.where(name: ['john', 'jane']) + RUBY + end + + it 'registers an offense and corrects when using `=` and namespaced columns' do + expect_offense(<<~RUBY) + Course.where(['enrollments.student_id = ?', student.id]) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where('enrollments.student_id' => student.id)` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + Course.where('enrollments.student_id' => student.id) + RUBY + end + end + + it 'does not register an offense when using `where`' do + expect_no_offenses(<<~RUBY) + User.where(name: nil) + RUBY + end + + it 'does not register an offense when not using template string' do + expect_no_offenses(<<~RUBY) + User.where(name: 'john') + RUBY + end + + it 'does not register an offense when template string does contains negation' do + expect_no_offenses(<<~RUBY) + User.where('name != ?', 'john') + RUBY + end + + it 'does not register an offense when template string contains `=` and additional boolean logic' do + expect_no_offenses(<<~RUBY) + User.where('name = ? AND age = ?', 'john', 19) + RUBY + end +end