Fix symlink regression

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