[Fix #3455] Add new Rails/DynamicFindBy cop

CHANGELOG.md

@@ -9,6 +9,7 @@
 * Add a new `AllCops/StyleGuideBaseURL` setting that allows the use of relative paths and/or fragments within each cop's `StyleGuide` setting, to make forking of custom style guides easier. ([@scottmatthewman][])
 * [#3566](https://github.com/bbatsov/rubocop/issues/3566): Add new `Metric/BlockLength` cop to ensure blocks don't get too long. ([@savef][])
 * [#3428](https://github.com/bbatsov/rubocop/issues/3428): Add support for configuring `Style/PreferredHashMethods` with either `short` or `verbose` style method names. ([@abrom][])
+* [#3455](https://github.com/bbatsov/rubocop/issues/3455): Add new `Rails/DynamicFindBy` cop. ([@pocke][])
 
 ### Bug fixes
 

config/default.yml

@@ -1233,6 +1233,10 @@ Rails/Date:
     - strict
     - flexible
 
+Rails/DynamicFindBy:
+  Whitelist:
+    - find_by_sql
+
 Rails/Exit:
   Include:
     - app/**/*.rb

config/enabled.yml

@@ -1439,6 +1439,11 @@ Rails/Delegate:
   Description: 'Prefer delegate method for delegations.'
   Enabled: true
 
+Rails/DynamicFindBy:
+  Description: 'Use `find_by` instead of dynamic `find_by_*`.'
+  StyleGuide: 'https://github.com/bbatsov/rails-style-guide#find_by'
+  Enabled: true
+
 Rails/Exit:
   Description: >-
                   Favor `fail`, `break`, `return`, etc. over `exit` in

lib/rubocop.rb

@@ -384,6 +384,7 @@
 
 require 'rubocop/cop/rails/action_filter'
 require 'rubocop/cop/rails/date'
+require 'rubocop/cop/rails/dynamic_find_by'
 require 'rubocop/cop/rails/delegate'
 require 'rubocop/cop/rails/exit'
 require 'rubocop/cop/rails/find_by'

lib/rubocop/cop/rails/dynamic_find_by.rb

@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # This cop checks dynamic `find_by_*` methods.
+      # Use `find_by` instead of dynamic method.
+      # See. https://github.com/bbatsov/rails-style-guide#find_by
+      #
+      # @example
+      #   # bad
+      #   User.find_by_name(name)
+      #
+      #   # bad
+      #   User.find_by_name_and_email(name)
+      #
+      #   # bad
+      #   User.find_by_email!(name)
+      #
+      #   # good
+      #   User.find_by(name: name)
+      #
+      #   # good
+      #   User.find_by(name: name, email: email)
+      #
+      #   # good
+      #   User.find_by!(email: email)
+      class DynamicFindBy < Cop
+        MSG = 'Use `%s` instead of dynamic `%s`.'.freeze
+        METHOD_PATTERN = /^find_by_(.+?)(!)?$/
+
+        def on_send(node)
+          _receiver, method, _args = *node
+          method_name = method.to_s
+          return if whitelist.include?(method_name)
+          static_name = static_method_name(method_name)
+          return unless static_name
+
+          add_offense(node, :expression, format(MSG, static_name, method))
+        end
+
+        def autocorrect(node)
+          _receiver, method, *args = *node
+          static_name = static_method_name(method.to_s)
+          keywords = column_keywords(method)
+          return if keywords.size != args.size
+
+          lambda do |corrector|
+            corrector.replace(node.loc.selector, static_name)
+            keywords.each.with_index do |keyword, idx|
+              corrector.insert_before(args[idx].loc.expression, keyword)
+            end
+          end
+        end
+
+        private
+
+        def whitelist
+          cop_config['Whitelist']
+        end
+
+        def column_keywords(method)
+          keyword_string = method.to_s[METHOD_PATTERN, 1]
+          keyword_string.split('_and_').map { |keyword| "#{keyword}: " }
+        end
+
+        # Returns static method name.
+        # If code isn't wrong, returns nil
+        def static_method_name(method_name)
+          match = METHOD_PATTERN.match(method_name)
+          return nil unless match
+          match[2] ? 'find_by!' : 'find_by'
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/rails/dynamic_find_by_spec.rb

@@ -0,0 +1,141 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe RuboCop::Cop::Rails::DynamicFindBy, :config do
+  subject(:cop) { described_class.new(config) }
+  let(:cop_config) do
+    { 'Whitelist' => %w(find_by_sql) }
+  end
+
+  shared_examples 'register an offense and auto correct' do |message, corrected|
+    it 'registers an offense' do
+      inspect_source(cop, source)
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.messages)
+        .to eq([message])
+    end
+
+    it 'auto-corrects' do
+      new_source = autocorrect_source(cop, source)
+      expect(new_source).to eq(corrected)
+    end
+  end
+
+  context 'with dynamic find_by_*' do
+    let(:source) { 'User.find_by_name(name)' }
+
+    include_examples(
+      'register an offense and auto correct',
+      'Use `find_by` instead of dynamic `find_by_name`.',
+      'User.find_by(name: name)'
+    )
+  end
+
+  context 'with dynamic find_by_*_and_*' do
+    let(:source) { 'User.find_by_name_and_email(name, email)' }
+
+    include_examples(
+      'register an offense and auto correct',
+      'Use `find_by` instead of dynamic `find_by_name_and_email`.',
+      'User.find_by(name: name, email: email)'
+    )
+  end
+
+  context 'with dynamic find_by_*!' do
+    let(:source) { 'User.find_by_name!(name)' }
+
+    include_examples(
+      'register an offense and auto correct',
+      'Use `find_by!` instead of dynamic `find_by_name!`.',
+      'User.find_by!(name: name)'
+    )
+  end
+
+  context 'with dynamic find_by_*_and_*_and_*' do
+    let(:source) { 'User.find_by_name_and_email_and_token(name, email, token)' }
+
+    include_examples(
+      'register an offense and auto correct',
+      'Use `find_by` instead of dynamic `find_by_name_and_email_and_token`.',
+      'User.find_by(name: name, email: email, token: token)'
+    )
+  end
+
+  context 'with dynamic find_by_*_and_*_and_*!' do
+    let(:source) do
+      'User.find_by_name_and_email_and_token!(name, email, token)'
+    end
+
+    include_examples(
+      'register an offense and auto correct',
+      'Use `find_by!` instead of dynamic `find_by_name_and_email_and_token!`.',
+      'User.find_by!(name: name, email: email, token: token)'
+    )
+  end
+
+  context 'with dynamic find_by_*_and_*_and_* with newline' do
+    let(:source) do
+      [
+        'User.find_by_name_and_email_and_token(',
+        '  name,',
+        '  email,',
+        '  token',
+        ')'
+      ]
+    end
+
+    include_examples(
+      'register an offense and auto correct',
+      'Use `find_by` instead of dynamic `find_by_name_and_email_and_token`.',
+      "User.find_by(\n" \
+      "  name: name,\n" \
+      "  email: email,\n" \
+      "  token: token\n" \
+      ')' \
+    )
+  end
+
+  context 'with column includes undersoce' do
+    let(:source) { 'User.find_by_first_name(name)' }
+
+    include_examples(
+      'register an offense and auto correct',
+      'Use `find_by` instead of dynamic `find_by_first_name`.',
+      'User.find_by(first_name: name)'
+    )
+  end
+
+  context 'with too much arguments' do
+    let(:source) { 'User.find_by_name_and_email(name, email, token)' }
+
+    include_examples(
+      'register an offense and auto correct',
+      'Use `find_by` instead of dynamic `find_by_name_and_email`.',
+      # Do not correct
+      'User.find_by_name_and_email(name, email, token)'
+    )
+  end
+
+  context 'with too few arguments' do
+    let(:source) { 'User.find_by_name_and_email(name)' }
+
+    include_examples(
+      'register an offense and auto correct',
+      'Use `find_by` instead of dynamic `find_by_name_and_email`.',
+      # Do not correct
+      'User.find_by_name_and_email(name)'
+    )
+  end
+
+  it 'accepts' do
+    inspect_source(cop, 'User.find_by(name: name)')
+    expect(cop.offenses).to be_empty
+  end
+
+  it 'accepts method in whitelist' do
+    source = 'User.find_by_sql(["select * from users where name = ?", name])'
+    inspect_source(cop, source)
+    expect(cop.offenses).to be_empty
+  end
+end