Skip to content

Commit

Permalink
[Fix #137] Make Rails/HasManyOrHasOneDependent aware of readonly?
Browse files Browse the repository at this point in the history
… is `true`

Fixes #137.

This PR makes `Rails/HasManyOrHasOneDependent` aware of `readonly?` is `true`.

There may be false positives or false negatives with just `def readonly? = true`,
but it makes it possible to detect perhaps general definition of `readonly?` method.
  • Loading branch information
koic committed May 2, 2021
1 parent bc2319f commit 6a415a8
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* [#103](https://github.com/rubocop/rubocop-rails/issues/103): Fix a false positive for `Rails/FindEach` when not inheriting `ActiveRecord::Base` and using `all.each`. ([@koic][])
* [#466](https://github.com/rubocop/rubocop-rails/pull/466): Fix a false positive for `Rails/DynamicFindBy` when not inheriting `ApplicationRecord` and without no receiver. ([@koic][])
* [#147](https://github.com/rubocop/rubocop-rails/issues/147): Fix a false positive for `Rails/HasManyOrHasOneDependent` when specifying default `dependent: nil` strategy. ([@koic][])
* [#137](https://github.com/rubocop/rubocop-rails/issues/137): Make `Rails/HasManyOrHasOneDependent` aware of `readonly?` is `true`. ([@koic][])

### Changes

Expand Down
12 changes: 11 additions & 1 deletion docs/modules/ROOT/pages/cops_rails.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -1570,8 +1570,9 @@ This cop checks for the use of the has_and_belongs_to_many macro.

This cop looks for `has_many` or `has_one` associations that don't
specify a `:dependent` option.

It doesn't register an offense if `:through` or `dependent: nil`
was specified.
is specified, or if the model is read-only.

=== Examples

Expand All @@ -1590,6 +1591,15 @@ class User < ActiveRecord::Base
has_many :articles, dependent: nil
has_many :patients, through: :appointments
end
class User < ActiveRecord::Base
has_many :comments
has_one :avatar
def readonly?
true
end
end
----

=== Configurable attributes
Expand Down
26 changes: 24 additions & 2 deletions lib/rubocop/cop/rails/has_many_or_has_one_dependent.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ module Cop
module Rails
# This cop looks for `has_many` or `has_one` associations that don't
# specify a `:dependent` option.
#
# It doesn't register an offense if `:through` or `dependent: nil`
# was specified.
# is specified, or if the model is read-only.
#
# @example
# # bad
Expand All @@ -22,6 +23,15 @@ module Rails
# has_many :articles, dependent: nil
# has_many :patients, through: :appointments
# end
#
# class User < ActiveRecord::Base
# has_many :comments
# has_one :avatar
#
# def readonly?
# true
# end
# end
class HasManyOrHasOneDependent < Base
MSG = 'Specify a `:dependent` option.'
RESTRICT_ON_SEND = %i[has_many has_one].freeze
Expand Down Expand Up @@ -59,8 +69,14 @@ class HasManyOrHasOneDependent < Base
(args) ...)
PATTERN

def_node_matcher :readonly?, <<~PATTERN
(def :readonly?
(args)
(true))
PATTERN

def on_send(node)
return if active_resource?(node.parent)
return if active_resource?(node.parent) || readonly_model?(node)
return if !association_without_options?(node) && valid_options?(association_with_options?(node))
return if valid_options_in_with_options_block?(node)

Expand All @@ -69,6 +85,12 @@ def on_send(node)

private

def readonly_model?(node)
return false unless (parent = node.parent)

parent.each_descendant(:def).any? { |def_node| readonly?(def_node) }
end

def valid_options_in_with_options_block?(node)
return true unless node.parent

Expand Down
27 changes: 27 additions & 0 deletions spec/rubocop/cop/rails/has_many_or_has_one_dependent_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,33 @@ class User < ActiveResource::Base
end
end

context 'when defining `readonly?` method' do
it 'does not register an offense for `readonly?` is `true`' do
expect_no_offenses(<<~RUBY)
class Person < ActiveRecord::Base
has_one :foo
def readonly?
true
end
end
RUBY
end

it 'registers an offense for `readonly?` is not `true`' do
expect_offense(<<~RUBY)
class Person < ActiveRecord::Base
has_one :foo
^^^^^^^ Specify a `:dependent` option.
def readonly?
false
end
end
RUBY
end
end

context 'when an Active Record model does not have any associations' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY)
Expand Down

0 comments on commit 6a415a8

Please sign in to comment.