Merge pull request #1145 from koic/make_rails_duplicate_association_a…

…ware_of_duplicate_class_name

[Fix #1128] Make `Rails/DuplicateAssociation` aware of duplicate `class_name`

changelog/new_make_rails_duplicate_association_aware_of_duplicate_class_name.md

@@ -0,0 +1 @@
+* [#1128](https://github.com/rubocop/rubocop-rails/issues/1128): Make `Rails/DuplicateAssociation` aware of duplicate `class_name`. ([@koic][])

lib/rubocop/cop/rails/duplicate_association.rb

@@ -20,38 +20,91 @@ module Rails
       #   belongs_to :bar
       #   has_one :foo
       #
+      #   # bad
+      #   belongs_to :foo, class_name: 'Foo'
+      #   belongs_to :bar, class_name: 'Foo'
+      #   has_one :baz
+      #
+      #   # good
+      #   belongs_to :bar, class_name: 'Foo'
+      #   has_one :foo
+      #
       class DuplicateAssociation < Base
         include RangeHelp
         extend AutoCorrector
         include ClassSendNodeHelper
         include ActiveRecordHelper
 
         MSG = "Association `%<name>s` is defined multiple times. Don't repeat associations."
+        MSG_CLASS_NAME = "Association `class_name: %<name>s` is defined multiple times. Don't repeat associations."
 
         def_node_matcher :association, <<~PATTERN
-          (send nil? {:belongs_to :has_one :has_many :has_and_belongs_to_many} ({sym str} $_) ...)
+          (send nil? {:belongs_to :has_one :has_many :has_and_belongs_to_many} ({sym str} $_) $...)
+        PATTERN
+
+        def_node_matcher :class_name, <<~PATTERN
+          (hash (pair (sym :class_name) $_))
         PATTERN
 
         def on_class(class_node)
           return unless active_record?(class_node.parent_class)
 
-          offenses(class_node).each do |name, nodes|
-            nodes.each do |node|
-              add_offense(node, message: format(MSG, name: name)) do |corrector|
-                next if same_line?(nodes.last, node)
+          association_nodes = association_nodes(class_node)
 
-                corrector.remove(range_by_whole_lines(node.source_range, include_final_newline: true))
-              end
-            end
+          duplicated_association_name_nodes(association_nodes).each do |name, nodes|
+            register_offense(name, nodes, MSG)
+          end
+
+          duplicated_class_name_nodes(association_nodes).each do |class_name, nodes|
+            register_offense(class_name, nodes, MSG_CLASS_NAME)
           end
         end
 
         private
 
-        def offenses(class_node)
-          class_send_nodes(class_node).select { |node| association(node) }
-                                      .group_by { |node| association(node).to_sym }
-                                      .select { |_, nodes| nodes.length > 1 }
+        def register_offense(name, nodes, message_template)
+          nodes.each do |node|
+            add_offense(node, message: format(message_template, name: name)) do |corrector|
+              next if same_line?(nodes.last, node)
+
+              corrector.remove(range_by_whole_lines(node.source_range, include_final_newline: true))
+            end
+          end
+        end
+
+        def association_nodes(class_node)
+          class_send_nodes(class_node).select do |node|
+            association(node)&.first
+          end
+        end
+
+        def duplicated_association_name_nodes(association_nodes)
+          grouped_associations = association_nodes.group_by do |node|
+            association(node).first.to_sym
+          end
+
+          leave_duplicated_association(grouped_associations)
+        end
+
+        def duplicated_class_name_nodes(association_nodes)
+          grouped_associations = association_nodes.group_by do |node|
+            arguments = association(node).last
+            next unless arguments.count == 1
+
+            if (class_name = class_name(arguments.first))
+              class_name.source
+            end
+          end
+
+          grouped_associations.delete(nil)
+
+          leave_duplicated_association(grouped_associations)
+        end
+
+        def leave_duplicated_association(grouped_associations)
+          grouped_associations.select do |_, nodes|
+            nodes.length > 1
+          end
         end
       end
     end

spec/rubocop/cop/rails/duplicate_association_spec.rb

@@ -210,6 +210,52 @@ class Post < ApplicationRecord
       RUBY
     end
 
+    it 'registers offenses when using duplicate associations of same class without other arguments' do
+      expect_offense(<<~RUBY)
+        class Post < ApplicationRecord
+          has_many :foos, class_name: 'Foo'
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Association `class_name: 'Foo'` is defined multiple times. Don't repeat associations.
+          has_many :bars, class_name: 'Foo'
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Association `class_name: 'Foo'` is defined multiple times. Don't repeat associations.
+
+          has_one :baz, class_name: 'Bar'
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Association `class_name: 'Bar'` is defined multiple times. Don't repeat associations.
+          has_one :qux, class_name: 'Bar'
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Association `class_name: 'Bar'` is defined multiple times. Don't repeat associations.
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class Post < ApplicationRecord
+          has_many :bars, class_name: 'Foo'
+
+          has_one :qux, class_name: 'Bar'
+        end
+      RUBY
+    end
+
+    it 'does not register an offense when using duplicate associations of same class with other arguments' do
+      expect_no_offenses(<<~RUBY)
+        class Post < ApplicationRecord
+          has_many :foos, if: condition, class_name: 'Foo'
+          has_many :bars, if: some_condition, class_name: 'Foo'
+
+          has_one :baz, -> { condition }, class_name: 'Bar'
+          has_one :qux, -> { some_condition }, class_name: 'Bar'
+
+          belongs_to :group, class_name: 'IndustryGroup', foreign_key: :industry_group_id
+        end
+      RUBY
+    end
+
+    it 'does not register an offense when not using association method' do
+      expect_no_offenses(<<~RUBY)
+        class Post < ApplicationRecord
+          validates_presence_of :name
+        end
+      RUBY
+    end
+
     it 'does not flag non-duplicate associations' do
       expect_no_offenses(<<-RUBY)
         class Post < ApplicationRecord