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

@@ -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

spec/react_on_rails/assets_precompile_spec.rb

@@ -54,50 +54,90 @@ 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
       let(:digest_filename) { "alfa.12345.js" }
       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
 

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