From 4cdcf29b294dcc6853deffc0eba5bfe9c9d191f7 Mon Sep 17 00:00:00 2001 From: ydah <13041216+ydah@users.noreply.github.com> Date: Sat, 23 Jul 2022 06:33:42 +0900 Subject: [PATCH] Add new `Rails/WhereMissing` cop Refs: https://rails.rubystyle.guide/#finding-missing-relationship-records This cop is check for the use `left_joins` and `where` method for finding missing relationship records. ```ruby # bad Foo.left_joins(:foo).where(foos: { id: nil }) # good Foo.where.missing(:foo) ``` --- .../new_add_new_rails_where_missing_cop.md | 1 + config/default.yml | 6 + lib/rubocop/cop/rails/where_missing.rb | 106 ++++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + spec/rubocop/cop/rails/where_missing_spec.rb | 157 ++++++++++++++++++ 5 files changed, 271 insertions(+) create mode 100644 changelog/new_add_new_rails_where_missing_cop.md create mode 100644 lib/rubocop/cop/rails/where_missing.rb create mode 100644 spec/rubocop/cop/rails/where_missing_spec.rb diff --git a/changelog/new_add_new_rails_where_missing_cop.md b/changelog/new_add_new_rails_where_missing_cop.md new file mode 100644 index 0000000000..0f32e082c8 --- /dev/null +++ b/changelog/new_add_new_rails_where_missing_cop.md @@ -0,0 +1 @@ +* [#744](https://github.com/rubocop/rubocop-rails/pull/744): Add new `Rails/WhereMissing` cop. ([@ydah][]) diff --git a/config/default.yml b/config/default.yml index 9b7cd2d1cc..f7fd84b778 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1018,6 +1018,12 @@ Rails/WhereExists: VersionAdded: '2.7' VersionChanged: '2.10' +Rails/WhereMissing: + Description: 'Check for the use `left_joins` and `where` method for finding missing relationship records.' + StyleGuide: 'https://rails.rubystyle.guide/#finding-missing-relationship-records' + Enabled: pending + VersionAdded: '<>' + Rails/WhereNot: Description: 'Use `where.not(...)` instead of manually constructing negated SQL in `where`.' StyleGuide: 'https://rails.rubystyle.guide/#hash-conditions' diff --git a/lib/rubocop/cop/rails/where_missing.rb b/lib/rubocop/cop/rails/where_missing.rb new file mode 100644 index 0000000000..b1c1ae73d8 --- /dev/null +++ b/lib/rubocop/cop/rails/where_missing.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Check for the use `left_joins` and `where` method for finding missing relationship records. + # + # This cop is enabled in Rails 6.1 or higher. + # + # @example + # # bad + # Foo.left_joins(:foo).where(foos: { id: nil }) + # + # # good + # Foo.where.missing(:foo) + # + class WhereMissing < Base + include RangeHelp + extend AutoCorrector + extend TargetRailsVersion + + MSG = 'Use `where.missing(:%s)` instead of ' \ + '`%s(:%s).where(%ss: { id: nil })`.' + RESTRICT_ON_SEND = %i[left_joins left_outer_joins].freeze + + minimum_target_rails_version 6.1 + + # @!method where_node_and_argument(node) + def_node_search :where_node_and_argument, <<~PATTERN + $(send ... :where (hash <(pair $(sym _) (hash (pair (sym :id) (nil))))...> )) + PATTERN + + # @!method missing_relationship?(node) + def_node_search :missing_relationship?, <<~PATTERN + (pair (sym _) (hash (pair (sym :id) (nil)))) + PATTERN + + def on_send(node) + where_node_and_argument(node.ancestors.last) do |where_node, where_argument| + next unless same_relationship?(where_argument, node.first_argument) + + range = range_between(node.loc.selector.begin_pos, node.loc.expression.end_pos) + register_offense(node, where_node, range) + break + end + end + + private + + def same_relationship?(where, left_joins) + "#{left_joins.value}s" == where.value.to_s + end + + def register_offense(node, where_node, range) + add_offense(range, message: message(node)) do |corrector| + corrector.replace(node.loc.selector, 'where.missing') + if multi_condition?(where_node.first_argument) + replace_where_method(corrector, where_node) + else + remove_where_method(corrector, node, where_node) + end + end + end + + def replace_where_method(corrector, where_node) + where_node.first_argument.children.each do |child| + next unless missing_relationship?(child) + + corrector.remove(replace_range(child)) + end + end + + def replace_range(child) + if (right_sibling = child.right_sibling) + range_between(child.loc.expression.begin_pos, right_sibling.loc.expression.begin_pos) + else + range_between(child.left_sibling.loc.expression.end_pos, child.loc.expression.end_pos) + end + end + + def remove_where_method(corrector, node, where_node) + range = range_between(where_node.loc.selector.begin_pos, where_node.loc.end.end_pos) + if node.multiline? && !same_line?(node, where_node) + range = range_by_whole_lines(range, include_final_newline: true) + else + corrector.remove(where_node.loc.dot) + end + + corrector.remove(range) + end + + def same_line?(left_joins_node, where_node) + left_joins_node.loc.selector.line == where_node.loc.selector.line + end + + def multi_condition?(where_arg) + where_arg.children.count > 1 + end + + def message(node) + format(MSG, association: node.first_argument.value, left_joins_method: node.method_name) + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 7ff7ac5e18..498066d08e 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -121,4 +121,5 @@ require_relative 'rails/validation' require_relative 'rails/where_equals' require_relative 'rails/where_exists' +require_relative 'rails/where_missing' require_relative 'rails/where_not' diff --git a/spec/rubocop/cop/rails/where_missing_spec.rb b/spec/rubocop/cop/rails/where_missing_spec.rb new file mode 100644 index 0000000000..84fb09f10a --- /dev/null +++ b/spec/rubocop/cop/rails/where_missing_spec.rb @@ -0,0 +1,157 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::WhereMissing, :config do + context 'Rails 6.1', :rails61 do + it 'registers an offense when using `left_joins(foo).where(foos: {id: nil})`' do + expect_offense(<<~RUBY) + Foo.left_joins(:foo).where(foos: { id: nil }).where(bar: "bar") + ^^^^^^^^^^^^^^^^ Use `where.missing(:foo)` instead of `left_joins(:foo).where(foos: { id: nil })`. + RUBY + + expect_correction(<<~RUBY) + Foo.where.missing(:foo).where(bar: "bar") + RUBY + end + + it 'registers an offense when using `left_outer_joins(foo).where(foos: {id: nil})`' do + expect_offense(<<~RUBY) + Foo.left_outer_joins(:foo).where(foos: { id: nil }).where(bar: "bar") + ^^^^^^^^^^^^^^^^^^^^^^ Use `where.missing(:foo)` instead of `left_outer_joins(:foo).where(foos: { id: nil })`. + RUBY + + expect_correction(<<~RUBY) + Foo.where.missing(:foo).where(bar: "bar") + RUBY + end + + it 'registers an offense when using `where(foos: {id: nil}).left_joins(foo)`' do + expect_offense(<<~RUBY) + Foo.where(foos: { id: nil }).left_joins(:foo).where(bar: "bar") + ^^^^^^^^^^^^^^^^ Use `where.missing(:foo)` instead of `left_joins(:foo).where(foos: { id: nil })`. + RUBY + + expect_correction(<<~RUBY) + Foo.where.missing(:foo).where(bar: "bar") + RUBY + end + + it "registers an offense when using `left_joins(foo).where(foos: {id: nil}, bar: 'bar')`" do + expect_offense(<<~RUBY) + Foo.left_joins(:foo).where(foos: { id: nil }, bar: "bar") + ^^^^^^^^^^^^^^^^ Use `where.missing(:foo)` instead of `left_joins(:foo).where(foos: { id: nil })`. + RUBY + + expect_correction(<<~RUBY) + Foo.where.missing(:foo).where(bar: "bar") + RUBY + end + + it "registers an offense when using `left_joins(foo).where(bar: 'bar', foos: {id: nil})`" do + expect_offense(<<~RUBY) + Foo.left_joins(:foo).where(bar: "bar", foos: { id: nil }) + ^^^^^^^^^^^^^^^^ Use `where.missing(:foo)` instead of `left_joins(:foo).where(foos: { id: nil })`. + RUBY + + expect_correction(<<~RUBY) + Foo.where.missing(:foo).where(bar: "bar") + RUBY + end + + it 'registers an offense when using `where(foos: {id: nil}).joins(:bar).left_joins(foo)`' do + expect_offense(<<~RUBY) + Foo.left_joins(:foo).joins(:bar).where(foos: { id: nil }) + ^^^^^^^^^^^^^^^^ Use `where.missing(:foo)` instead of `left_joins(:foo).where(foos: { id: nil })`. + RUBY + + expect_correction(<<~RUBY) + Foo.where.missing(:foo).joins(:bar) + RUBY + end + + it 'registers an offense when using `left_joins(foo).where(foos: {id: nil})` with multi-line leading ' \ + 'dot method calls' do + expect_offense(<<~RUBY) + Foo + .left_joins(:foo) + ^^^^^^^^^^^^^^^^ Use `where.missing(:foo)` instead of `left_joins(:foo).where(foos: { id: nil })`. + .where(foos: { id: nil }) + .where(bar: "bar") + RUBY + + expect_correction(<<~RUBY) + Foo + .where.missing(:foo) + .where(bar: "bar") + RUBY + end + + it 'registers an offense when using `left_joins(foo).where(foos: {id: nil})` with multi-line trailing ' \ + 'dot method calls' do + expect_offense(<<~RUBY) + Foo. + left_joins(:foo). + ^^^^^^^^^^^^^^^^ Use `where.missing(:foo)` instead of `left_joins(:foo).where(foos: { id: nil })`. + where(foos: { id: nil }). + where(bar: "bar") + RUBY + + expect_correction(<<~RUBY) + Foo. + where.missing(:foo). + where(bar: "bar") + RUBY + end + + it 'registers an offense when using `left_joins(foo).where(foos: {id: nil})` and there is a line break after' \ + '`left_joins.where`' do + expect_offense(<<~RUBY) + Foo.left_joins(:foo).where(foos: { id: nil }) + ^^^^^^^^^^^^^^^^ Use `where.missing(:foo)` instead of `left_joins(:foo).where(foos: { id: nil })`. + .where(bar: "bar") + RUBY + + expect_correction(<<~RUBY) + Foo.where.missing(:foo) + .where(bar: "bar") + RUBY + end + + it 'registers an offense when using `left_joins(foo).where(foos: {id: nil})` and there is a line break after' \ + '`left_joins.where` and receiver' do + expect_offense(<<~RUBY) + Foo + .left_joins(:foo).where(foos: { id: nil }) + ^^^^^^^^^^^^^^^^ Use `where.missing(:foo)` instead of `left_joins(:foo).where(foos: { id: nil })`. + .where(bar: "bar") + RUBY + + expect_correction(<<~RUBY) + Foo + .where.missing(:foo) + .where(bar: "bar") + RUBY + end + + it 'does not register an offense when not finding missing relationship records`' do + expect_no_offenses(<<~RUBY) + Foo.left_joins(:foo).where(bazs: { id: nil }) + Foo.left_joins(:foo).where(foos: { name: nil }) + Foo.left_joins(:foo).where(foos: { id: 1 }) + RUBY + end + end + + context 'Rails 6.0', :rails60 do + it 'does not register an offense when using `left_joins(foo).where(foos: {id: nil})`' do + expect_no_offenses(<<~RUBY) + Foo.left_joins(:foo).where(foos: { id: nil }).where(bar: "bar") + RUBY + end + + it 'does not register an offense when using `left_outer_joins(foo).where(foos: {id: nil})`' do + expect_no_offenses(<<~RUBY) + Foo.left_outer_joins(:foo).where(foos: { id: nil }).where(bar: "bar") + RUBY + end + end +end