From 1c4affb93d827510949dcacb97fd81cf1588d578 Mon Sep 17 00:00:00 2001 From: Rob Wise Date: Sun, 4 Sep 2016 19:15:09 -0400 Subject: [PATCH 1/5] Allow filtering react_on_rails specs with :focus --- spec/react_on_rails/spec_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 From 8e72bf87f8cae22a82af46078781d5be0ff1a587 Mon Sep 17 00:00:00 2001 From: Rob Wise Date: Sun, 4 Sep 2016 22:49:24 -0400 Subject: [PATCH 2/5] 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. --- lib/react_on_rails/assets_precompile.rb | 62 ++++++++++++------- spec/react_on_rails/assets_precompile_spec.rb | 46 +++++++------- 2 files changed, 61 insertions(+), 47 deletions(-) diff --git a/lib/react_on_rails/assets_precompile.rb b/lib/react_on_rails/assets_precompile.rb index 7004ecea5..7d868d9e0 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,35 @@ 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 + valid_symlink_already_exists = File.exist?(symlink_path) && File.lstat(symlink_path).symlink? + + if valid_symlink_already_exists + 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 +81,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 +117,14 @@ def clobber puts "Could not find generated_assets_dir #{dir} defined in react_on_rails initializer: " end end + + private + + def file_or_symlink_exists_at_path?(path) + 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..88b14d5be 100644 --- a/spec/react_on_rails/assets_precompile_spec.rb +++ b/spec/react_on_rails/assets_precompile_spec.rb @@ -54,6 +54,14 @@ 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 end describe "symlink_non_digested_assets" do @@ -69,35 +77,23 @@ module ReactOnRails 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 From 7ecdc36cb68d58d5ab9817455949fad962b0ebcb Mon Sep 17 00:00:00 2001 From: Rob Wise Date: Sun, 4 Sep 2016 22:57:31 -0400 Subject: [PATCH 3/5] Fix possible open resource leak in AssetsPrecompile spec --- spec/react_on_rails/assets_precompile_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/react_on_rails/assets_precompile_spec.rb b/spec/react_on_rails/assets_precompile_spec.rb index 88b14d5be..703706a3c 100644 --- a/spec/react_on_rails/assets_precompile_spec.rb +++ b/spec/react_on_rails/assets_precompile_spec.rb @@ -69,9 +69,9 @@ 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$')) From 168f3c8ba61c9a9b8d1a64e4e6f650bdef075b1d Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Tue, 6 Sep 2016 20:25:52 -1000 Subject: [PATCH 4/5] Add tests that confirm the new changes --- lib/react_on_rails/assets_precompile.rb | 14 ++++--- spec/react_on_rails/assets_precompile_spec.rb | 42 +++++++++++++++++++ 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/lib/react_on_rails/assets_precompile.rb b/lib/react_on_rails/assets_precompile.rb index 7d868d9e0..8514ac83d 100644 --- a/lib/react_on_rails/assets_precompile.rb +++ b/lib/react_on_rails/assets_precompile.rb @@ -26,11 +26,7 @@ def symlink_file(target, 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 - valid_symlink_already_exists = File.exist?(symlink_path) && File.lstat(symlink_path).symlink? - - if valid_symlink_already_exists + 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 @@ -120,7 +116,15 @@ def clobber 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 diff --git a/spec/react_on_rails/assets_precompile_spec.rb b/spec/react_on_rails/assets_precompile_spec.rb index 703706a3c..a3b2acaf1 100644 --- a/spec/react_on_rails/assets_precompile_spec.rb +++ b/spec/react_on_rails/assets_precompile_spec.rb @@ -62,6 +62,48 @@ module ReactOnRails 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 From d32a8cb7c4204ff73f203fd78c6953eb115afd5a Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Fri, 9 Sep 2016 16:46:02 -1000 Subject: [PATCH 5/5] whitespace --- spec/react_on_rails/assets_precompile_spec.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/spec/react_on_rails/assets_precompile_spec.rb b/spec/react_on_rails/assets_precompile_spec.rb index a3b2acaf1..a87d993f4 100644 --- a/spec/react_on_rails/assets_precompile_spec.rb +++ b/spec/react_on_rails/assets_precompile_spec.rb @@ -67,8 +67,8 @@ module ReactOnRails 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 @@ -81,11 +81,12 @@ module ReactOnRails 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 @@ -97,11 +98,12 @@ module ReactOnRails 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