Skip to content

Commit

Permalink
[Fix rubocop#51] Add ability to whitelist receiver class names
Browse files Browse the repository at this point in the history
In `Rails/DynamicFindBy`.

Also use `AllowedReceivers` & `AllowedMethods` instead of `Whitelist`
which will be deprecated soon.

Fixes rubocop#51.
  • Loading branch information
tejasbubane committed Mar 6, 2020
1 parent b43a7fe commit 0cfe340
Show file tree
Hide file tree
Showing 7 changed files with 91 additions and 26 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -146,3 +147,4 @@
[@hanachin]: https://github.com/hanachin
[@joshpencheon]: https://github.com/joshpencheon
[@djudd]: https://github.com/djudd
[@tejasbubane]: https://github.com/tejasbubane
5 changes: 5 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.'
Expand Down
36 changes: 36 additions & 0 deletions lib/rubocop/cop/mixin/allowed.rb
Original file line number Diff line number Diff line change
@@ -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
22 changes: 5 additions & 17 deletions lib/rubocop/cop/rails/dynamic_find_by.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 `%<static_name>s` instead of dynamic `%<method>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

Expand Down Expand Up @@ -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}: " }
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
@@ -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'

Expand Down
10 changes: 2 additions & 8 deletions manual/cops_rails.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
```

Expand All @@ -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

Expand Down
41 changes: 40 additions & 1 deletion spec/rubocop/cop/rails/dynamic_find_by_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down Expand Up @@ -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)' }
Expand All @@ -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

0 comments on commit 0cfe340

Please sign in to comment.