From 637f69dc7c100988e62a6c908f7a75d6d973362a Mon Sep 17 00:00:00 2001 From: fatkodima Date: Sat, 27 Jun 2020 23:09:46 +0300 Subject: [PATCH] Add new `Rails/WhereNot` cop --- CHANGELOG.md | 4 + config/default.yml | 6 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_rails.adoc | 36 +++++ lib/rubocop/cop/rails/where_not.rb | 106 +++++++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + spec/rubocop/cop/rails/where_not_spec.rb | 163 +++++++++++++++++++++++ 7 files changed, 317 insertions(+) create mode 100644 lib/rubocop/cop/rails/where_not.rb create mode 100644 spec/rubocop/cop/rails/where_not_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b04b67f66..6f3c844952 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## master (unreleased) +### New features + +* [#274](https://github.com/rubocop-hq/rubocop-rails/pull/274): Add new `Rails/WhereNot` cop. ([@fatkodima][]) + ### Bug fixes * [#261](https://github.com/rubocop-hq/rubocop-rails/issues/261): Fix auto correction for `Rails/ContentTag` when `content_tag` is called with options hash and block. ([@fatkodima][]) diff --git a/config/default.yml b/config/default.yml index 26a281953a..1a304cd1ef 100644 --- a/config/default.yml +++ b/config/default.yml @@ -570,6 +570,12 @@ Rails/Validation: Include: - app/models/**/*.rb +Rails/WhereNot: + Description: 'Use `where.not(...)` instead of manually constructing negated SQL in `where`.' + StyleGuide: 'https://rails.rubystyle.guide/#where-not' + Enabled: true + VersionAdded: '2.7' + # Accept `redirect_to(...) and return` and similar cases. Style/AndOr: EnforcedStyle: conditionals diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 01c3a80912..52c9a088d2 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -65,5 +65,6 @@ * 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#railswherenot[Rails/WhereNot] // END_COP_LIST diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index efad08b461..b800852be4 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -3487,3 +3487,39 @@ validates :foo, uniqueness: true | `app/models/**/*.rb` | Array |=== + +== Rails/WhereNot + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Enabled +| Yes +| Yes +| 2.7 +| - +|=== + +This cop identifies places where manually constructed SQL +in `where` can be replaced with `where.not(...)`. + +=== Examples + +[source,ruby] +---- +# bad +User.where('name != ?', 'Gabe') +User.where('name != :name', name: 'Gabe') +User.where('name IS NOT NULL') +User.where('name NOT IN (?)', ['john', 'jane']) +User.where('name NOT IN (:names)', names: ['john', 'jane']) + +# good +User.where.not(name: 'Gabe') +User.where.not(name: nil) +User.where.not(name: ['john', 'jane']) +---- + +=== References + +* https://rails.rubystyle.guide/#where-not diff --git a/lib/rubocop/cop/rails/where_not.rb b/lib/rubocop/cop/rails/where_not.rb new file mode 100644 index 0000000000..6c784d978d --- /dev/null +++ b/lib/rubocop/cop/rails/where_not.rb @@ -0,0 +1,106 @@ +# 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.not(...)`. + # + # @example + # # bad + # User.where('name != ?', 'Gabe') + # User.where('name != :name', name: 'Gabe') + # User.where('name IS NOT NULL') + # User.where('name NOT IN (?)', ['john', 'jane']) + # User.where('name NOT IN (:names)', names: ['john', 'jane']) + # + # # good + # User.where.not(name: 'Gabe') + # User.where.not(name: nil) + # User.where.not(name: ['john', 'jane']) + # + class WhereNot < Cop + include RangeHelp + + MSG = 'Use `%s` instead of manually constructing negated SQL in `where`.' + + 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 + + NOT_EQ_ANONYMOUS_RE = /\A([\w.]+)\s+!=\s+\?\z/.freeze # column != ? + NOT_IN_ANONYMOUS_RE = /\A([\w.]+)\s+NOT\s+IN\s+\(\?\)\z/i.freeze # column NOT IN (?) + NOT_EQ_NAMED_RE = /\A([\w.]+)\s+!=\s+:(\w+)\z/.freeze # column != :column + NOT_IN_NAMED_RE = /\A([\w.]+)\s+NOT\s+IN\s+\(:(\w+)\)\z/i.freeze # column NOT IN (:column) + IS_NOT_NULL_RE = /\A([\w.]+)\s+IS\s+NOT\s+NULL\z/i.freeze # column IS NOT 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 NOT_EQ_ANONYMOUS_RE, NOT_IN_ANONYMOUS_RE + value_node.source + when NOT_EQ_NAMED_RE, NOT_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_NOT_NULL_RE + 'nil' + else + return + end + + [Regexp.last_match(1), value] + end + + def build_good_method(column, value) + if column.include?('.') + "where.not('#{column}' => #{value})" + else + "where.not(#{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 ec291ddc5b..3ce519d132 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -67,3 +67,4 @@ require_relative 'rails/unique_validation_without_index' require_relative 'rails/unknown_env' require_relative 'rails/validation' +require_relative 'rails/where_not' diff --git a/spec/rubocop/cop/rails/where_not_spec.rb b/spec/rubocop/cop/rails/where_not_spec.rb new file mode 100644 index 0000000000..965aacd8c0 --- /dev/null +++ b/spec/rubocop/cop/rails/where_not_spec.rb @@ -0,0 +1,163 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::WhereNot 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.not(name: 'Gabe')` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + User.where.not(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.not(name: 'Gabe')` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + User.where.not(name: 'Gabe') + RUBY + end + + it 'registers an offense and corrects when using `IS NOT NULL`' do + expect_offense(<<~RUBY) + User.where('name IS NOT NULL') + ^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where.not(name: nil)` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + User.where.not(name: nil) + RUBY + end + + it 'registers an offense and corrects when using `NOT IN` and anonymous placeholder' do + expect_offense(<<~RUBY) + User.where("name NOT IN (?)", ['john', 'jane']) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where.not(name: ['john', 'jane'])` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + User.where.not(name: ['john', 'jane']) + RUBY + end + + it 'registers an offense and corrects when using `NOT IN` and named placeholder' do + expect_offense(<<~RUBY) + User.where("name NOT IN (:names)", names: ['john', 'jane']) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where.not(name: ['john', 'jane'])` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + User.where.not(name: ['john', 'jane']) + RUBY + end + + it 'registers an offense and corrects when using namespaced columns' do + expect_offense(<<~RUBY) + Course.where('enrollments.student_id != ?', student.id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where.not('enrollments.student_id' => student.id)` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + Course.where.not('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.not(name: 'Gabe')` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + User.where.not(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.not(name: 'Gabe')` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + User.where.not(name: 'Gabe') + RUBY + end + + it 'registers an offense and corrects when using `IS NOT NULL`' do + expect_offense(<<~RUBY) + User.where(['name IS NOT NULL']) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where.not(name: nil)` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + User.where.not(name: nil) + RUBY + end + + it 'registers an offense and corrects when using `NOT IN` and anonymous placeholder' do + expect_offense(<<~RUBY) + User.where(["name NOT IN (?)", ['john', 'jane']]) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where.not(name: ['john', 'jane'])` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + User.where.not(name: ['john', 'jane']) + RUBY + end + + it 'registers an offense and corrects when using `NOT IN` and named placeholder' do + expect_offense(<<~RUBY) + User.where(["name NOT IN (:names)", names: ['john', 'jane']]) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where.not(name: ['john', 'jane'])` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + User.where.not(name: ['john', 'jane']) + RUBY + end + + it 'registers an offense and corrects when using namespaced columns' do + expect_offense(<<~RUBY) + Course.where(['enrollments.student_id != ?', student.id]) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where.not('enrollments.student_id' => student.id)` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + Course.where.not('enrollments.student_id' => student.id) + RUBY + end + end + + it 'does not register an offense when using `where.not`' do + expect_no_offenses(<<~RUBY) + User.where.not(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 not contain negation' do + expect_no_offenses(<<~RUBY) + User.where('name = ?', 'john') + RUBY + end + + it 'does not register an offense when template string contains negation and additional boolean logic' do + expect_no_offenses(<<~RUBY) + User.where('name != ? AND age != ?', 'john', 19) + RUBY + end +end