From 0cfe3408537bf55c9115691247c97bae71adb258 Mon Sep 17 00:00:00 2001 From: Tejas Bubane Date: Thu, 6 Jun 2019 13:00:04 +0530 Subject: [PATCH] [Fix #51] Add ability to whitelist receiver class names In `Rails/DynamicFindBy`. Also use `AllowedReceivers` & `AllowedMethods` instead of `Whitelist` which will be deprecated soon. Fixes https://github.com/rubocop-hq/rubocop-rails/issues/51. --- CHANGELOG.md | 2 + config/default.yml | 5 +++ lib/rubocop/cop/mixin/allowed.rb | 36 ++++++++++++++++ lib/rubocop/cop/rails/dynamic_find_by.rb | 22 +++------- lib/rubocop/cop/rails_cops.rb | 1 + manual/cops_rails.md | 10 +---- .../rubocop/cop/rails/dynamic_find_by_spec.rb | 41 ++++++++++++++++++- 7 files changed, 91 insertions(+), 26 deletions(-) create mode 100644 lib/rubocop/cop/mixin/allowed.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index ba79425031..71cb76b9c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * [#197](https://github.com/rubocop-hq/rubocop-rails/pull/197): Add `Rails/UniqueValidationWithoutIndex` cop. ([@pocke][]) * [#208](https://github.com/rubocop-hq/rubocop-rails/pull/208): Add new `Rails/IndexBy` and `Rails/IndexWith` cops. ([@djudd][], [@eugeneius][]) +* [#51](https://github.com/rubocop-hq/rubocop-rails/issues/51): Add ability to whitelist receiver class names in `Rails/DynamicFindBy`. ([@tejasbubane][]) ### Bug fixes @@ -146,3 +147,4 @@ [@hanachin]: https://github.com/hanachin [@joshpencheon]: https://github.com/joshpencheon [@djudd]: https://github.com/djudd +[@tejasbubane]: https://github.com/tejasbubane diff --git a/config/default.yml b/config/default.yml index c25b2ad9e6..3c6efad771 100644 --- a/config/default.yml +++ b/config/default.yml @@ -157,8 +157,13 @@ Rails/DynamicFindBy: StyleGuide: 'https://rails.rubystyle.guide#find_by' Enabled: true VersionAdded: '0.44' + # The `Whitelist` has been deprecated, Please use `AllowedMethods` instead. Whitelist: - find_by_sql + AllowedMethods: + - find_by_sql + AllowedReceivers: + - Gem::Specification Rails/EnumHash: Description: 'Prefer hash syntax over array syntax when defining enums.' diff --git a/lib/rubocop/cop/mixin/allowed.rb b/lib/rubocop/cop/mixin/allowed.rb new file mode 100644 index 0000000000..b8873919de --- /dev/null +++ b/lib/rubocop/cop/mixin/allowed.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Common functionality for checking whitelisted methods & class names. + module Allowed + def allowed?(node) + allowed_method?(node) || allowed_receiver?(node) || + whitelisted?(node) + end + + private + + def allowed_method?(node) + return unless cop_config['AllowedMethods'] + + cop_config['AllowedMethods'].include?(node.method_name.to_s) + end + + def allowed_receiver?(node) + return unless cop_config['AllowedReceivers'] && node.receiver + + cop_config['AllowedReceivers'].include?(node.receiver.source.to_s) + end + + # config option `WhiteList` will be deprecated soon + def whitelisted?(node) + whitelist_config = cop_config['Whitelist'] + return unless whitelist_config + + whitelist_config.include?(node.method_name.to_s) || + node.receiver && whitelist_config.include?(node.receiver.source.to_s) + end + end + end +end diff --git a/lib/rubocop/cop/rails/dynamic_find_by.rb b/lib/rubocop/cop/rails/dynamic_find_by.rb index d7d9292525..54f205ff9b 100644 --- a/lib/rubocop/cop/rails/dynamic_find_by.rb +++ b/lib/rubocop/cop/rails/dynamic_find_by.rb @@ -10,37 +10,29 @@ module Rails # @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 + include Allowed + MSG = 'Use `%s` instead of dynamic `%s`.' METHOD_PATTERN = /^find_by_(.+?)(!)?$/.freeze def on_send(node) - method_name = node.method_name.to_s - - return if whitelist.include?(method_name) + return if allowed?(node) + method_name = node.method_name static_name = static_method_name(method_name) - return unless static_name add_offense(node, message: format(MSG, static_name: static_name, - method: node.method_name)) + method: method_name)) end alias on_csend on_send @@ -68,10 +60,6 @@ def autocorrect_argument_keywords(corrector, node, keywords) end end - 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}: " } diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 58404c8e83..5d73f92773 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require_relative 'mixin/active_record_helper' +require_relative 'mixin/allowed' require_relative 'mixin/index_method' require_relative 'mixin/target_rails_version' diff --git a/manual/cops_rails.md b/manual/cops_rails.md index 2ffd766f10..58334b04ff 100644 --- a/manual/cops_rails.md +++ b/manual/cops_rails.md @@ -659,20 +659,12 @@ See. https://rails.rubystyle.guide#find_by ```ruby # 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) ``` @@ -681,6 +673,8 @@ User.find_by!(email: email) Name | Default value | Configurable values --- | --- | --- Whitelist | `find_by_sql` | Array +AllowedMethods | `find_by_sql` | Array +AllowedReceivers | `Gem::Specification` | Array ### References diff --git a/spec/rubocop/cop/rails/dynamic_find_by_spec.rb b/spec/rubocop/cop/rails/dynamic_find_by_spec.rb index 9d578966bb..01999ff308 100644 --- a/spec/rubocop/cop/rails/dynamic_find_by_spec.rb +++ b/spec/rubocop/cop/rails/dynamic_find_by_spec.rb @@ -4,7 +4,7 @@ subject(:cop) { described_class.new(config) } let(:cop_config) do - { 'Whitelist' => %w[find_by_sql] } + { 'AllowedMethods' => %w[find_by_sql] } end shared_examples 'register an offense and auto correct' do |message, corrected| @@ -139,6 +139,18 @@ RUBY end + context 'with whitelisted classname' do + let(:cop_config) do + { 'AllowedReceivers' => %w[Gem::Specification] } + end + + it 'accepts class methods in whitelist' do + expect_no_offenses(<<~RUBY) + Gem::Specification.find_by_name("backend").gem_dir + RUBY + end + end + context 'when using safe navigation operator' do context 'with dynamic find_by_*' do let(:source) { 'user&.find_by_name(name)' } @@ -150,4 +162,31 @@ ) end end + + # Whitelisted config will be deprecated. + context 'with WhiteListed config' do + context 'allowed class-names' do + let(:cop_config) do + { 'Whitelist' => %w[Gem::Specification] } + end + + it 'accepts class methods in whitelist' do + expect_no_offenses(<<~RUBY) + Gem::Specification.find_by_name("backend").gem_dir + RUBY + end + end + + context 'allowed method-names' do + let(:cop_config) do + { 'Whitelist' => %w[find_by_name] } + end + + it 'accepts class methods in whitelist' do + expect_no_offenses(<<~RUBY) + User.find_by_name("backend").gem_dir + RUBY + end + end + end end