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 Apr 20, 2020
1 parent 2c860fc commit 43b6d78
Show file tree
Hide file tree
Showing 5 changed files with 128 additions and 23 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## master (unreleased)

### New features

* [#51](https://github.com/rubocop-hq/rubocop-rails/issues/51): Add allowed receiver class names option for `Rails/DynamicFindBy`. ([@tejasbubane][])

### Bug fixes

* [#12](https://github.com/rubocop-hq/rubocop-rails/issues/12): Fix a false positive for `Rails/SkipsModelValidations` when passing a boolean literal to `touch`. ([@eugeneius][])
Expand Down Expand Up @@ -178,3 +182,4 @@
[@djudd]: https://github.com/djudd
[@sunny]: https://github.com/sunny
[@hoshinotsuyoshi]: https://github.com/hoshinotsuyoshi
[@tejasbubane]: https://github.com/tejasbubane
6 changes: 6 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,14 @@ Rails/DynamicFindBy:
StyleGuide: 'https://rails.rubystyle.guide#find_by'
Enabled: true
VersionAdded: '0.44'
VersionChanged: '2.6'
# 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
55 changes: 40 additions & 15 deletions lib/rubocop/cop/rails/dynamic_find_by.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,37 +10,41 @@ 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)
# User.find_by(name: name, email: email)
# User.find_by!(email: email)
#
# @example AllowedMethods: find_by_sql
# # bad
# User.find_by_query(users_query)
#
# # good
# User.find_by(name: name, email: email)
# User.find_by_sql(users_sql)
#
# @example AllowedReceivers: Gem::Specification
# # bad
# Specification.find_by_name('backend').gem_dir
#
# # good
# User.find_by!(email: email)
# Gem::Specification.find_by_name('backend').gem_dir
class DynamicFindBy < Cop
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_invocation?(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 All @@ -57,6 +61,31 @@ def autocorrect(node)

private

def allowed_invocation?(node)
allowed_method?(node) || allowed_receiver?(node) ||
whitelisted?(node)
end

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)
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)
end

def autocorrect_method_name(corrector, node)
corrector.replace(node.loc.selector,
static_method_name(node.method_name.to_s))
Expand All @@ -68,10 +97,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
26 changes: 19 additions & 7 deletions manual/cops_rails.md
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,7 @@ delegate :foo, to: :bar, allow_nil: true

Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
--- | --- | --- | --- | ---
Enabled | Yes | Yes | 0.44 | -
Enabled | Yes | Yes | 0.44 | 2.6

This cop checks dynamic `find_by_*` methods.
Use `find_by` instead of dynamic method.
Expand All @@ -659,28 +659,40 @@ 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)
User.find_by(name: name, email: email)
User.find_by!(email: email)
```
#### AllowedMethods: find_by_sql

```ruby
# bad
User.find_by_query(users_query)

# good
User.find_by(name: name, email: email)
User.find_by_sql(users_sql)
```
#### AllowedReceivers: Gem::Specification

```ruby
# bad
Specification.find_by_name('backend').gem_dir

# good
User.find_by!(email: email)
Gem::Specification.find_by_name('backend').gem_dir
```

### Configurable attributes

Name | Default value | Configurable values
--- | --- | ---
Whitelist | `find_by_sql` | Array
AllowedMethods | `find_by_sql` | Array
AllowedReceivers | `Gem::Specification` | Array

### References

Expand Down
59 changes: 58 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,32 @@
RUBY
end

context 'with allowed receiver name' do
let(:cop_config) do
{ 'AllowedReceivers' => %w[Gem::Specification] }
end

it 'accepts dynamic find_by for receiver names in whitelist' do
expect_no_offenses(<<~RUBY)
Gem::Specification.find_by_name("backend").gem_dir
RUBY
end

it 'registers offense for receiver name with no namespace' do
expect_offense(<<~RUBY)
Specification.find_by_name("backend").gem_dir
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `find_by` instead of dynamic `find_by_name`.
RUBY
end

it 'registers offense for receiver name with different namespace' do
expect_offense(<<~RUBY)
RubyGems::Specification.find_by_name("backend").gem_dir
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `find_by` instead of dynamic `find_by_name`.
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 +176,35 @@
)
end
end

# Whitelisted config will be deprecated.
context 'with WhiteListed config' do
context 'allowed class-names' do
let(:cop_config) do
{ 'Whitelist' => %w[Specification] }
end

# `Whitelist` should not allow receivers for API compatibility.
# New API `AllowedReceivers` should be used to add a receiver
# to the allowed receivers.
it 'registers offense for class methods in whitelist' do
expect_offense(<<~RUBY)
Specification.find_by_name("backend").gem_dir
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `find_by` instead of dynamic `find_by_name`.
RUBY
end
end

context 'allowed method-names' do
let(:cop_config) do
{ 'Whitelist' => %w[find_by_name] }
end

it 'allows for 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 43b6d78

Please sign in to comment.