From d587d732ae23d41e6c2c9a913a24bb0767a7094b 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 | 1 + 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, 314 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 49042496f3..2b9bb4bd66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### New features * [#52](https://github.com/rubocop-hq/rubocop-rails/issues/52): Add new `Rails/AfterCommitOverride` cop. ([@fatkodima][]) +* [#274](https://github.com/rubocop-hq/rubocop-rails/pull/274): Add new `Rails/WhereNot` cop. ([@fatkodima][]) ## 2.7.1 (2020-07-26) diff --git a/config/default.yml b/config/default.yml index eea9acef7e..155053e46e 100644 --- a/config/default.yml +++ b/config/default.yml @@ -687,6 +687,12 @@ Rails/WhereExists: Enabled: 'pending' VersionAdded: '2.7' +Rails/WhereNot: + Description: 'Use `where.not(...)` instead of manually constructing negated SQL in `where`.' + StyleGuide: 'https://rails.rubystyle.guide/#where-not' + Enabled: 'pending' + VersionAdded: '2.8' + # 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 0d141ea14d..ffc7c924fa 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -80,5 +80,6 @@ * xref:cops_rails.adoc#railsunknownenv[Rails/UnknownEnv] * xref:cops_rails.adoc#railsvalidation[Rails/Validation] * xref:cops_rails.adoc#railswhereexists[Rails/WhereExists] +* 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 e7ad8457ed..f30bd34714 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -4134,3 +4134,39 @@ User.exists?(name: 'john') User.where('length(name) > 10').exists? user.posts.exists?(published: true) ---- + +== Rails/WhereNot + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Pending +| Yes +| Yes +| 2.8 +| - +|=== + +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 c10962a60d..50957f1c77 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -82,3 +82,4 @@ require_relative 'rails/unknown_env' require_relative 'rails/validation' require_relative 'rails/where_exists' +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