From 211fed05425dc7029f1e82cd20040e25215faa61 Mon Sep 17 00:00:00 2001 From: Rob Wise Date: Fri, 9 Sep 2016 22:47:41 -0400 Subject: [PATCH] Fix symlink regression (#541) Fix AssetsPrecompile#symlink_file logic When we changed this method in #513 we introduced a regression due to the difference between calling the shell's `ln -s` command and using Ruby's `File.symlink` command. Specifically, the former would not error if the symlink already existed, while the latter did throw an error. Because it is sometimes the case that the symlink will already exist, throwing in this case is not desirable. This should have not been a problem, however, as this scenario was supposed to have been properly handled, but that code was not correct. This commit fixes that code. * Allow filtering react_on_rails specs with :focus * Fix possible open resource leak in AssetsPrecompile spec --- lib/react_on_rails/assets_precompile.rb | 70 +++++++++----- spec/react_on_rails/assets_precompile_spec.rb | 96 +++++++++++++------ spec/react_on_rails/spec_helper.rb | 4 +- 3 files changed, 116 insertions(+), 54 deletions(-) diff --git a/lib/react_on_rails/assets_precompile.rb b/lib/react_on_rails/assets_precompile.rb index 7004ecea5..8514ac83d 100644 --- a/lib/react_on_rails/assets_precompile.rb +++ b/lib/react_on_rails/assets_precompile.rb @@ -1,5 +1,7 @@ module ReactOnRails class AssetsPrecompile + class SymlinkTargetDoesNotExistException < StandardError; end + # Used by the rake task def default_asset_path dir = File.join(Rails.configuration.paths["public"].first, @@ -20,30 +22,31 @@ def initialize(assets_path: nil, def symlink_file(target, symlink) target_path = @assets_path.join(target) symlink_path = @assets_path.join(symlink) + target_exists = File.exist?(target_path) + raise SymlinkTargetDoesNotExistException, "Target Path was: #{target_path}" unless target_exists - # File.exist?(symlink_path) will check the file the sym is pointing to is existing - # File.lstat(symlink_path).symlink? confirms that this is a symlink - symlink_already_there_and_valid = File.exist?(symlink_path) && - File.lstat(symlink_path).symlink? - if symlink_already_there_and_valid - puts "React On Rails: Digested #{symlink} already exists indicating #{target} did not change." - elsif target_exists - if File.exist?(symlink_path) && File.lstat(symlink_path).symlink? - puts "React On Rails: Removing invalid symlink #{symlink_path}" - `cd #{@assets_path} && rm #{symlink}` - end - # Might be like: - # "images/5cf5db49df178f9357603f945752a1ef.png": - # "images/5cf5db49df178f9357603f945752a1ef-033650e1d6193b70d59bb60e773f47b6d9aefdd56abc7cc.png" - # need to cd to directory and then symlink - target_sub_path, _divider, target_filename = target.rpartition("/") - _symlink_sub_path, _divider, symlink_filename = symlink.rpartition("/") - puts "React On Rails: Symlinking \"#{target}\" to \"#{symlink}\"" - dest_path = File.join(@assets_path, target_sub_path) - FileUtils.chdir(dest_path) do - File.symlink(target_filename, symlink_filename) - end + if symlink_and_points_to_existing_file?(symlink_path) + puts "React On Rails: Digested version of #{symlink} already exists indicating #{target} did not change." + return + end + + if file_or_symlink_exists_at_path?(symlink_path) + puts "React On Rails: Removing existing invalid symlink or file #{symlink_path}" + FileUtils.remove_file(symlink_path, true) + end + + # Might be like: + # "images/5cf5db49df178f9357603f945752a1ef.png": + # "images/5cf5db49df178f9357603f945752a1ef-033650e1d6193b70d59bb60e773f47b6d9aefdd56abc7cc.png" + # need to cd to directory and then symlink + target_sub_path, _divider, target_filename = target.rpartition("/") + _symlink_sub_path, _divider, symlink_filename = symlink.rpartition("/") + dest_path = File.join(@assets_path, target_sub_path) + + puts "React On Rails: Symlinking \"#{target}\" to \"#{symlink}\"" + FileUtils.chdir(dest_path) do + File.symlink(target_filename, symlink_filename) end end @@ -74,8 +77,10 @@ def symlink_non_digested_assets # already been symlinked by Webpack symlink_file(rails_digested_filename, original_filename) - # We want the gz ones as well - symlink_file("#{rails_digested_filename}.gz", "#{original_filename}.gz") + # We want the gz ones as well if they exist + if File.exist?(@assets_path.join("#{rails_digested_filename}.gz")) + symlink_file("#{rails_digested_filename}.gz", "#{original_filename}.gz") + end end end end @@ -108,5 +113,22 @@ def clobber puts "Could not find generated_assets_dir #{dir} defined in react_on_rails initializer: " end end + + private + + def symlink_and_points_to_existing_file?(symlink_path) + # File.exist?(symlink_path) will check the file the sym is pointing to is existing + # File.lstat(symlink_path).symlink? confirms that this is a symlink + File.exist?(symlink_path) && File.lstat(symlink_path).symlink? + end + + def file_or_symlink_exists_at_path?(path) + # We use lstat and not stat, we we don't want to visit the file that the symlink maybe + # pointing to. We can't use File.exist?, as that would check the file pointed at by the symlink. + File.lstat(path) + true + rescue + false + end end end diff --git a/spec/react_on_rails/assets_precompile_spec.rb b/spec/react_on_rails/assets_precompile_spec.rb index 364c157cc..a87d993f4 100644 --- a/spec/react_on_rails/assets_precompile_spec.rb +++ b/spec/react_on_rails/assets_precompile_spec.rb @@ -54,6 +54,58 @@ module ReactOnRails expect(File.identical?(assets_path.join(filename), assets_path.join(digest_filename))).to be true end + + context "when no file exists at the target path" do + it "raises a ReactOnRails::AssetsPrecompile::SymlinkTargetDoesNotExistException" do + expect do + AssetsPrecompile.new(assets_path: assets_path).symlink_file("non_existent", "non_existent-digest") + end.to raise_exception(AssetsPrecompile::SymlinkTargetDoesNotExistException) + end + end + + it "creates a proper symlink when a file exists at destination" do + filename = File.basename(Tempfile.new("tempfile", assets_path)) + existing_filename = File.basename(Tempfile.new("tempfile", assets_path)) + digest_filename = existing_filename + AssetsPrecompile.new(assets_path: assets_path).symlink_file(filename, digest_filename) + + expect(assets_path.join(digest_filename).lstat.symlink?).to be true + expect(File.identical?(assets_path.join(filename), + assets_path.join(digest_filename))).to be true + end + + it "creates a proper symlink when a symlink file exists at destination" do + filename = File.basename(Tempfile.new("tempfile", assets_path)) + existing_filename = File.basename(Tempfile.new("tempfile", assets_path)) + digest_file = Tempfile.new("tempfile", assets_path) + digest_filename = File.basename(digest_file) + File.delete(digest_file) + File.symlink(existing_filename, digest_filename) + AssetsPrecompile.new(assets_path: assets_path).symlink_file(filename, digest_filename) + + expect(assets_path.join(digest_filename).lstat.symlink?).to be true + expect(File.identical?(assets_path.join(filename), + assets_path.join(digest_filename))).to be true + + File.delete(digest_filename) + end + + it "creates a proper symlink when an invalid symlink exists at destination" do + filename = File.basename(Tempfile.new("tempfile", assets_path)) + existing_file = Tempfile.new("tempfile", assets_path) + existing_filename = File.basename(existing_file) + digest_file = Tempfile.new("tempfile", assets_path) + digest_filename = File.basename(digest_file) + File.symlink(existing_filename, digest_filename) + File.delete(existing_file) # now digest_filename is an invalid link + AssetsPrecompile.new(assets_path: assets_path).symlink_file(filename, digest_filename) + + expect(assets_path.join(digest_filename).lstat.symlink?).to be true + expect(File.identical?(assets_path.join(filename), + assets_path.join(digest_filename))).to be true + + File.delete(digest_filename) + end end describe "symlink_non_digested_assets" do @@ -61,43 +113,31 @@ module ReactOnRails let(:nondigest_filename) { "alfa.js" } let(:checker) do - f = File.new(assets_path.join("manifest-alfa.json"), "w") - f.write("{\"assets\":{\"#{nondigest_filename}\": \"#{digest_filename}\"}}") - f.close + File.open(assets_path.join("manifest-alfa.json"), "w") do |f| + f.write("{\"assets\":{\"#{nondigest_filename}\": \"#{digest_filename}\"}}") + end AssetsPrecompile.new(assets_path: assets_path, symlink_non_digested_assets_regex: Regexp.new('.*\.js$')) end - context "correct nondigest filename" do - it "creates valid symlink" do - FileUtils.touch assets_path.join(digest_filename) - checker.symlink_non_digested_assets - - expect(assets_path.join(nondigest_filename).lstat.symlink?).to be true - expect(File.identical?(assets_path.join(nondigest_filename), - assets_path.join(digest_filename))).to be true - end - end - - context "zipped nondigest filename" do - it "creates valid symlink" do - FileUtils.touch assets_path.join("#{digest_filename}.gz") - checker.symlink_non_digested_assets + it "creates a symlink with the original filename that points to the digested filename" do + FileUtils.touch assets_path.join(digest_filename) + checker.symlink_non_digested_assets - expect(assets_path.join("#{nondigest_filename}.gz").lstat.symlink?).to be true - expect(File.identical?(assets_path.join("#{nondigest_filename}.gz"), - assets_path.join("#{digest_filename}.gz"))).to be true - end + expect(assets_path.join(nondigest_filename).lstat.symlink?).to be true + expect(File.identical?(assets_path.join(nondigest_filename), + assets_path.join(digest_filename))).to be true end - context "wrong nondigest filename" do - it "should not create symlink" do - FileUtils.touch assets_path.join("alfa.12345.jsx") - checker.symlink_non_digested_assets + it "creates a symlink with the original filename plus .gz that points to the gzipped digested filename" do + FileUtils.touch assets_path.join(digest_filename) + FileUtils.touch assets_path.join("#{digest_filename}.gz") + checker.symlink_non_digested_assets - expect(assets_path.join("alfa.jsx")).not_to exist - end + expect(assets_path.join("#{nondigest_filename}.gz").lstat.symlink?).to be true + expect(File.identical?(assets_path.join("#{nondigest_filename}.gz"), + assets_path.join("#{digest_filename}.gz"))).to be true end end diff --git a/spec/react_on_rails/spec_helper.rb b/spec/react_on_rails/spec_helper.rb index fcbcaec23..e876639be 100644 --- a/spec/react_on_rails/spec_helper.rb +++ b/spec/react_on_rails/spec_helper.rb @@ -67,8 +67,8 @@ # # to individual examples or groups you care about by tagging them with # # `:focus` metadata. When nothing is tagged with `:focus`, all examples # # get run. - # config.filter_run :focus - # config.run_all_when_everything_filtered = true + config.filter_run :focus + config.run_all_when_everything_filtered = true # # # Allows RSpec to persist some state between runs in order to support # # the `--only-failures` and `--next-failure` CLI options. We recommend