From 5d246720ba39b1c5df8361b6159450ebe5fac9b6 Mon Sep 17 00:00:00 2001 From: Yauheni Dakuka Date: Mon, 3 Feb 2025 00:59:49 +0400 Subject: [PATCH] [Fix rubocop#1177] Fix for `Rails/FilePath` autocorrect removing an important trailing slash in a dynamic string containing `Rails.root` --- ...ath_to_properly_handle_trailing_slashes.md | 1 + lib/rubocop/cop/rails/file_path.rb | 20 +- spec/rubocop/cop/rails/file_path_spec.rb | 234 ++++++++++++++++++ 3 files changed, 252 insertions(+), 3 deletions(-) create mode 100644 changelog/fix_rails_file_path_to_properly_handle_trailing_slashes.md diff --git a/changelog/fix_rails_file_path_to_properly_handle_trailing_slashes.md b/changelog/fix_rails_file_path_to_properly_handle_trailing_slashes.md new file mode 100644 index 0000000000..ee75e0a143 --- /dev/null +++ b/changelog/fix_rails_file_path_to_properly_handle_trailing_slashes.md @@ -0,0 +1 @@ +* [#1177](https://github.com/rubocop/rubocop-rails/issues/1177): Enhance `Rails/FilePath` to properly handle trailing slashes. ([@ydakuka][]) diff --git a/lib/rubocop/cop/rails/file_path.rb b/lib/rubocop/cop/rails/file_path.rb index b9438dbc40..f41c800c25 100644 --- a/lib/rubocop/cop/rails/file_path.rb +++ b/lib/rubocop/cop/rails/file_path.rb @@ -80,6 +80,7 @@ def on_send(node) def check_for_slash_after_rails_root_in_dstr(node) rails_root_index = find_rails_root_index(node) slash_node = node.children[rails_root_index + 1] + return if slash_node&.source == File::SEPARATOR return unless slash_node&.str_type? && slash_node.source.start_with?(File::SEPARATOR) return unless node.children[rails_root_index].children.first.send_type? @@ -132,20 +133,29 @@ def check_for_rails_root_join_with_slash_separated_path(node) 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) } + arguments.none? do |arg| + arg.variable? || arg.const_type? || + string_contains_multiple_slashes?(arg) || string_with_trailing_slash?(arg) + end 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) } + arguments.none? do |arg| + string_with_leading_slash?(arg) || + string_contains_multiple_slashes?(arg) || string_with_trailing_slash?(arg) + end 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) } + arguments.none? do |arg| + string_with_leading_slash?(arg) || + string_contains_multiple_slashes?(arg) || string_with_trailing_slash?(arg) + end end def string_contains_slash?(node) @@ -160,6 +170,10 @@ def string_with_leading_slash?(node) node.str_type? && node.value.start_with?(File::SEPARATOR) end + def string_with_trailing_slash?(node) + node.str_type? && node.value.end_with?(File::SEPARATOR) + end + def register_offense(node, require_to_s:, &block) line_range = node.loc.column...node.loc.last_column source_range = source_range(processed_source.buffer, node.first_line, line_range) diff --git a/spec/rubocop/cop/rails/file_path_spec.rb b/spec/rubocop/cop/rails/file_path_spec.rb index 8f6d6ca825..fa15e19cec 100644 --- a/spec/rubocop/cop/rails/file_path_spec.rb +++ b/spec/rubocop/cop/rails/file_path_spec.rb @@ -140,6 +140,123 @@ end end + context 'when using File.join with Rails.root and a single forward slash as a path' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + File.join(Rails.root, "/") + RUBY + end + end + + context 'when using File.join with Rails.root and a single forward slash as the last argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + File.join(Rails.root, 'app', '/') + RUBY + end + end + + context 'when using File.join with Rails.root and multiple string paths with different trailing slash placements' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + File.join(Rails.root, 'app/models/', 'goober') + RUBY + + expect_no_offenses(<<~RUBY) + File.join(Rails.root, 'app/models/', '/goober') + RUBY + + expect_no_offenses(<<~RUBY) + File.join(Rails.root, 'app/models', 'goober/') + RUBY + + expect_no_offenses(<<~RUBY) + File.join(Rails.root, 'app/models/', 'goober/') + RUBY + end + end + + context 'when using Rails.root.join and a single forward slash as a path' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Rails.root.join("/") + RUBY + end + end + + context 'when using Rails.root.join and a single forward slash as the last argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Rails.root.join('app', '/') + RUBY + end + end + + context 'when using Rails.root.join and multiple string paths with different trailing slash placements' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Rails.root.join('app/models/', 'goober') + RUBY + + expect_no_offenses(<<~RUBY) + Rails.root.join('app/models/', '/goober') + RUBY + + expect_no_offenses(<<~RUBY) + Rails.root.join('app/models', 'goober/') + RUBY + + expect_no_offenses(<<~RUBY) + Rails.root.join('app/models/', 'goober/') + RUBY + end + end + + context 'when using Rails.root.join with Rails.root and a single forward slash as a path' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Rails.root.join(Rails.root, "/") + RUBY + end + end + + context 'when using Rails.root.join with Rails.root and a single forward slash as the last argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Rails.root.join(Rails.root, 'app', '/') + RUBY + end + end + + context 'when using Rails.root.join with Rails.root and multiple string paths ' \ + 'with different trailing slash placements' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Rails.root.join(Rails.root, 'app/models/', 'goober') + RUBY + + expect_no_offenses(<<~RUBY) + Rails.root.join(Rails.root, 'app/models/', '/goober') + RUBY + + expect_no_offenses(<<~RUBY) + Rails.root.join(Rails.root, 'app/models', 'goober/') + RUBY + + expect_no_offenses(<<~RUBY) + Rails.root.join(Rails.root, 'app/models/', 'goober/') + RUBY + end + end + + context 'when using Rails.root in string interpolation followed by a forward slash' do + it 'does not register an offense' do + expect_no_offenses(<<~'RUBY') + "#{Rails.root}/" + RUBY + end + end + context 'when using Rails.root called by double quoted string' do it 'registers an offense' do expect_offense(<<~'RUBY') @@ -526,6 +643,123 @@ end end + context 'when using File.join with Rails.root and a single forward slash as a path' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + File.join(Rails.root, "/") + RUBY + end + end + + context 'when using File.join with Rails.root and a single forward slash as the last argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + File.join(Rails.root, 'app', '/') + RUBY + end + end + + context 'when using File.join with Rails.root and multiple string paths with different trailing slash placements' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + File.join(Rails.root, 'app/models/', 'goober') + RUBY + + expect_no_offenses(<<~RUBY) + File.join(Rails.root, 'app/models/', '/goober') + RUBY + + expect_no_offenses(<<~RUBY) + File.join(Rails.root, 'app/models', 'goober/') + RUBY + + expect_no_offenses(<<~RUBY) + File.join(Rails.root, 'app/models/', 'goober/') + RUBY + end + end + + context 'when using Rails.root.join and a single forward slash as a path' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Rails.root.join("/") + RUBY + end + end + + context 'when using Rails.root.join and a single forward slash as the last argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Rails.root.join('app', '/') + RUBY + end + end + + context 'when using Rails.root.join and multiple string paths with different trailing slash placements' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Rails.root.join('app/models/', 'goober') + RUBY + + expect_no_offenses(<<~RUBY) + Rails.root.join('app/models/', '/goober') + RUBY + + expect_no_offenses(<<~RUBY) + Rails.root.join('app/models', 'goober/') + RUBY + + expect_no_offenses(<<~RUBY) + Rails.root.join('app/models/', 'goober/') + RUBY + end + end + + context 'when using Rails.root.join with Rails.root and a single forward slash as a path' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Rails.root.join(Rails.root, "/") + RUBY + end + end + + context 'when using Rails.root.join with Rails.root and a single forward slash as the last argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Rails.root.join(Rails.root, 'app', '/') + RUBY + end + end + + context 'when using Rails.root.join with Rails.root and multiple string paths ' \ + 'with different trailing slash placements' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Rails.root.join(Rails.root, 'app/models/', 'goober') + RUBY + + expect_no_offenses(<<~RUBY) + Rails.root.join(Rails.root, 'app/models/', '/goober') + RUBY + + expect_no_offenses(<<~RUBY) + Rails.root.join(Rails.root, 'app/models', 'goober/') + RUBY + + expect_no_offenses(<<~RUBY) + Rails.root.join(Rails.root, 'app/models/', 'goober/') + RUBY + end + end + + context 'when using Rails.root in string interpolation followed by a forward slash' do + it 'does not register an offense' do + expect_no_offenses(<<~'RUBY') + "#{Rails.root}/" + RUBY + end + end + context 'when using Rails.root called by double quoted string' do it 'registers an offense' do expect_offense(<<~'RUBY')