Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rails/FilePath autocorrect shouldn't remove important trailing slash in dynamic string containing Rails.root #1177

Open
henrahmagix opened this issue Nov 2, 2023 · 1 comment · May be fixed by #1437
Open

Rails/FilePath autocorrect shouldn't remove important trailing slash in dynamic string containing Rails.root #1177

henrahmagix opened this issue Nov 2, 2023 · 1 comment · May be fixed by #1437

Comments

@henrahmagix
Copy link

I'm trying to use Rails.root to remove it from an absolute filepath to result in a filepath relative to Rails.root.

my_abs_path              = "/Users/me/rails_app/config/locales/text_en.yml"
expected_relative_result = "config/locales/text_en.yml"

my_abs_path.delete_prefix("#{Rails.root}/") == expected_relative_result
# => true

Expected behavior

# this should be allowed by Rails/FilePath cop because the last / in /Users/me/rails_app/ must be deleted, not just /Users/me/rails_app
my_abs_path.delete_prefix("#{Rails.root}/")

Actual behavior

my_abs_path.delete_prefix("#{Rails.root}/")
# raises Rails/FilePath, and is autocorrected to
my_abs_path.delete_prefix("#{Rails.root.join('')}")
# which raises Style/RedundantInterpolation, so we manually correct to
my_abs_path.delete_prefix(Rails.root.join('').to_s)
# which is wrong:
actual = my_abs_path.delete_prefix(Rails.root.join('').to_s)
actual == expected_relative_result
# => false
puts actual
# => "/config/locales/text_en.yml"

# so we try giving an argument to join, but still wrong because Rails.root.join never adds a trailing slash:
actual = my_abs_path.delete_prefix(Rails.root.join('./').to_s)
actual == expected_relative_result
# => false
puts actual
# => "/config/locales/text_en.yml"

Steps to reproduce the problem

Copy the below into a file, run RuboCop with autocorrect enabled and manually fix any other cops that are raised, then assert that the result is true.
rubocop-rails-filepath-test.rb

# where Rails.root is /Users/me/rails_app
my_abs_path = "/Users/me/rails_app/config/locales/text_en.yml"
expected_relative_result = "config/locales/text_en.yml"

actual = my_abs_path.delete_prefix("#{Rails.root}/")
if actual != expected_relative_result
  raise "Invalid autocorrection: #{actual.inspect} != #{expected_relative_result.inspect}"
end
rubocop -a rubocop-rails-filepath-test.rb
rubocop -A rubocop-rails-filepath-test.rb # autofix Style/RedundantInterpolation
bin/rails c < rubocop-rails-filepath-test.rb

RuboCop version

1.57.2 (using Parser 3.2.2.4, rubocop-ast 1.30.0, running on ruby 3.0.6) [arm64-darwin21]
  - rubocop-rails 2.22.1
  - rubocop-rspec 2.18.1
@henrahmagix
Copy link
Author

I can see autocorrection was added in #991 so is this considered a bug of that? If so, then I can submit a fix that allows "#{Rails.root}/" if that's acceptable?

I also saw #1071 which might get fixed at the same time; I can check.

Also theres #434 which includes the exact same case as this.

@henrahmagix henrahmagix mentioned this issue Nov 2, 2023
Closed
@Earlopain Earlopain mentioned this issue Nov 10, 2024
Closed
corsonknowles added a commit to corsonknowles/rubocop-rails that referenced this issue Dec 23, 2024
@corsonknowles
[Fix rubocop#1177] Partially fix Rails/FilePath autocorrect for root …
c559f7a 
…filepaths by marking cop as unsafe for autocorrection

We correctly add a @safety annotation for Rails/FilePath since it can produce invalid code like this:

# pass any string argument beginning with '/' to Rails.root.join and it will omit the Rails path

(dev)> Rails.root.join '/'
=> #<Pathname:/>
(dev)> Rails.root.join '//'
=> #<Pathname://>
(dev)> Rails.root.join '/abc'
=> #<Pathname:/abc>
(dev)> Rails.root.join '/abc/'
=> #<Pathname:/abc/>
@corsonknowles corsonknowles mentioned this issue Dec 23, 2024
Closed
9 tasks
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Feb 8, 2025
@ydakuka
[Fix rubocop#1177] Fix for Rails/FilePath autocorrect removing an i…
fa8c533 
…mportant trailing slash in a dynamic string containing `Rails.root`
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Feb 9, 2025
@ydakuka
[Fix rubocop#1177] Fix for Rails/FilePath autocorrect removing an i…
9a254a6 
…mportant trailing slash in a dynamic string containing `Rails.root`
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Feb 9, 2025
@ydakuka
[Fix rubocop#1177] Fix for Rails/FilePath autocorrect removing an i…
6880526 
…mportant trailing slash in a dynamic string containing `Rails.root`
ydakuka added a commit to ydakuka/rubocop-rails that referenced this issue Feb 9, 2025
@ydakuka
[Fix rubocop#1177] Fix for Rails/FilePath autocorrect removing an i…
c3f06ab 
…mportant trailing slash in a dynamic string containing `Rails.root`
@ydakuka ydakuka linked a pull request Feb 9, 2025 that will close this issue
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Fix #1177] Enhance Rails/FilePath to properly handle trailing slashes ydakuka/rubocop-rails
1 participant
@henrahmagix