Merge pull request #1433 from ydakuka/1071-fix-an-error-occurring-in-…

…the-rails-file-path-cop-when-file-join-is-used-with-a-variable

[Fix #1071] Fix `Rails/FilePath` cop to correctly handle `File.join` with variables and ignore leading and multiple slashes in string literal arguments for `Rails.root.join` and `File.join`

.rubocop_todo.yml

@@ -29,7 +29,9 @@ InternalAffairs/OnSendWithoutOnCSend:
 # Offense count: 10
 # Configuration parameters: CountComments, CountAsOne.
 Metrics/ClassLength:
-  Max: 179
+  Max: 163
+  Exclude:
+    - 'lib/rubocop/cop/rails/file_path.rb'
 
 # Offense count: 41
 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns.

changelog/fix_an_error_occurring_in_the_rails_file_path_cop_when_file_join_is_used_with_a_variable.md

@@ -0,0 +1 @@
+* [#1071](https://github.com/rubocop/rubocop-rails/issues/1071): Fix `Rails/FilePath` cop to correctly handle `File.join` with variables and ignore leading and multiple slashes in string literal arguments for `Rails.root.join` and `File.join`. ([@ydakuka][])

lib/rubocop/cop/rails/file_path.rb

@@ -6,6 +6,9 @@ module Rails
       # Identifies usages of file path joining process to use `Rails.root.join` clause.
       # It is used to add uniformity when joining paths.
       #
+      # NOTE: This cop ignores leading slashes in string literal arguments for `Rails.root.join`
+      #       and multiple slashes in string literal arguments for `Rails.root.join` and `File.join`.
+      #
       # @example EnforcedStyle: slashes (default)
       #   # bad
       #   Rails.root.join('app', 'models', 'goober')
@@ -97,7 +100,7 @@ def check_for_extension_after_rails_root_join_in_dstr(node)
 
         def check_for_file_join_with_rails_root(node)
           return unless file_join_nodes?(node)
-          return unless node.arguments.any? { |e| rails_root_nodes?(e) }
+          return unless valid_arguments_for_file_join_with_rails_root?(node.arguments)
 
           register_offense(node, require_to_s: true) do |corrector|
             autocorrect_file_join(corrector, node) unless node.first_argument.array_type?
@@ -108,8 +111,7 @@ def check_for_rails_root_join_with_string_arguments(node)
           return unless style == :slashes
           return unless rails_root_nodes?(node)
           return unless rails_root_join_nodes?(node)
-          return unless node.arguments.size > 1
-          return unless node.arguments.all?(&:str_type?)
+          return unless valid_string_arguments_for_rails_root_join?(node.arguments)
 
           register_offense(node, require_to_s: false) do |corrector|
             autocorrect_rails_root_join_with_string_arguments(corrector, node)
@@ -120,15 +122,42 @@ def check_for_rails_root_join_with_slash_separated_path(node)
           return unless style == :arguments
           return unless rails_root_nodes?(node)
           return unless rails_root_join_nodes?(node)
-          return unless node.arguments.any? { |arg| string_with_slash?(arg) }
+          return unless valid_slash_separated_path_for_rails_root_join?(node.arguments)
 
           register_offense(node, require_to_s: false) do |corrector|
             autocorrect_rails_root_join_with_slash_separated_path(corrector, node)
           end
         end
 
-        def string_with_slash?(node)
-          node.str_type? && node.source.include?(File::SEPARATOR)
+        def valid_arguments_for_file_join_with_rails_root?(arguments)
+          return false unless arguments.any? { |arg| rails_root_nodes?(arg) }
+
+          arguments.none? { |arg| arg.variable? || arg.const_type? || string_contains_multiple_slashes?(arg) }
+        end
+
+        def valid_string_arguments_for_rails_root_join?(arguments)
+          return false unless arguments.size > 1
+          return false unless arguments.all?(&:str_type?)
+
+          arguments.none? { |arg| string_with_leading_slash?(arg) || string_contains_multiple_slashes?(arg) }
+        end
+
+        def valid_slash_separated_path_for_rails_root_join?(arguments)
+          return false unless arguments.any? { |arg| string_contains_slash?(arg) }
+
+          arguments.none? { |arg| string_with_leading_slash?(arg) || string_contains_multiple_slashes?(arg) }
+        end
+
+        def string_contains_slash?(node)
+          node.str_type? && node.value.include?(File::SEPARATOR)
+        end
+
+        def string_contains_multiple_slashes?(node)
+          node.str_type? && node.value.include?('//')
+        end
+
+        def string_with_leading_slash?(node)
+          node.str_type? && node.value.start_with?(File::SEPARATOR)
         end
 
         def register_offense(node, require_to_s:, &block)
@@ -226,7 +255,7 @@ def autocorrect_rails_root_join_with_string_arguments(corrector, node)
 
         def autocorrect_rails_root_join_with_slash_separated_path(corrector, node)
           node.arguments.each do |argument|
-            next unless string_with_slash?(argument)
+            next unless string_contains_slash?(argument)
 
             index = argument.source.index(File::SEPARATOR)
             rest = inner_range_of(argument).adjust(begin_pos: index - 1)

spec/rubocop/cop/rails/file_path_spec.rb

@@ -322,6 +322,128 @@
         RUBY
       end
     end
+
+    context 'when using File.join with a local variable' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          default_path = '/models'
+          File.join(Rails.root, 'app', default_path)
+        RUBY
+      end
+    end
+
+    context 'when using File.join with an instance variable' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          File.join(Rails.root, 'app', @default_path)
+        RUBY
+      end
+    end
+
+    context 'when using File.join with a class variable' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          File.join(Rails.root, 'app', @@default_path)
+        RUBY
+      end
+    end
+
+    context 'when using File.join with a global variable' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          File.join(Rails.root, 'app', $default_path)
+        RUBY
+      end
+    end
+
+    context 'when using File.join with a constant' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          File.join(Rails.root, 'app', DEFAULT_PATH)
+        RUBY
+      end
+    end
+
+    context 'when using Rails.root.join with a local variable' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          default_path = '/models'
+          Rails.root.join(Rails.root, 'app', default_path)
+        RUBY
+      end
+    end
+
+    context 'when using Rails.root.join with an instance variable' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          Rails.root.join(Rails.root, 'app', @default_path)
+        RUBY
+      end
+    end
+
+    context 'when using Rails.root.join with a class variable' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          Rails.root.join(Rails.root, 'app', @@default_path)
+        RUBY
+      end
+    end
+
+    context 'when using Rails.root.join with a global variable' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          Rails.root.join(Rails.root, 'app', $default_path)
+        RUBY
+      end
+    end
+
+    context 'when using Rails.root.join with a constant' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          Rails.root.join(Rails.root, 'app', DEFAULT_PATH)
+        RUBY
+      end
+    end
+
+    context 'when using Rails.root.join with a leading slash' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          Rails.root.join('/app/models')
+        RUBY
+      end
+    end
+
+    context 'when using Rails.root.join with mixed leading and normal path strings' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          Rails.root.join('/app', 'models')
+        RUBY
+      end
+    end
+
+    context 'when using Rails.root.join with mixed normal and leading path strings' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          Rails.root.join('app', '/models')
+        RUBY
+      end
+    end
+
+    context 'when using Rails.root.join with multiple slashes in a path' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          Rails.root.join('public//', 'assets')
+        RUBY
+      end
+    end
+
+    context 'when using File.join with multiple slashes in a path' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          File.join(Rails.root, 'public//', 'assets')
+        RUBY
+      end
+    end
   end
 
   context 'when EnforcedStyle is `arguments`' do
@@ -592,5 +714,45 @@
         RUBY
       end
     end
+
+    context 'when using Rails.root.join with a leading slash' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          Rails.root.join('/app/models')
+        RUBY
+      end
+    end
+
+    context 'when using Rails.root.join with mixed leading and normal path strings' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          Rails.root.join('/app', 'models')
+        RUBY
+      end
+    end
+
+    context 'when using Rails.root.join with mixed normal and leading path strings' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          Rails.root.join('app', '/models')
+        RUBY
+      end
+    end
+
+    context 'when using Rails.root.join with multiple slashes in a path' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          Rails.root.join('public//', 'assets')
+        RUBY
+      end
+    end
+
+    context 'when using File.join with multiple slashes in a path' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          File.join(Rails.root, 'public//', 'assets')
+        RUBY
+      end
+    end
   end
 end