Merge pull request #530 from rails/schneems/double-render

Raise error when two assets produce same path

lib/sprockets/base.rb

@@ -16,6 +16,18 @@
 require 'sprockets/uri_tar'
 
 module Sprockets
+
+  class DoubleLinkError < Sprockets::Error
+    def initialize(parent_filename:, logical_path:, last_filename:, filename:)
+      message = String.new
+      message << "Multiple files with the same output path cannot be linked (#{logical_path.inspect})\n"
+      message << "In #{parent_filename.inspect} these files were linked:\n"
+      message << "  - #{last_filename}\n"
+      message << "  - #{filename}\n"
+      super(message)
+    end
+  end
+
   # `Base` class for `Environment` and `CachedEnvironment`.
   class Base
     include PathUtils, PathDependencyUtils, PathDigestUtils, DigestUtils, SourceMapUtils
@@ -73,14 +85,22 @@ def find_asset(*args)
     def find_all_linked_assets(*args)
       return to_enum(__method__, *args) unless block_given?
 
-      asset = find_asset(*args)
+      parent_asset = asset = find_asset(*args)
       return unless asset
 
       yield asset
       stack = asset.links.to_a
+      linked_paths = {}
 
       while uri = stack.shift
         yield asset = load(uri)
+        if linked_paths[asset.logical_path]
+          raise DoubleLinkError.new(parent_filename: parent_asset.filename,
+                                    last_filename:   linked_paths[asset.logical_path],
+                                    logical_path:    asset.logical_path,
+                                    filename:        asset.filename )
+        end
+        linked_paths[asset.logical_path] = asset.filename
         stack = asset.links.to_a + stack
       end
 

lib/sprockets/manifest.rb

@@ -165,7 +165,12 @@ def compile(*args)
       filenames            = []
       concurrent_exporters = []
 
+      assets_to_export = Concurrent::Array.new
       find(*args) do |asset|
+        assets_to_export << asset
+      end
+
+      assets_to_export.each do |asset|
         mtime = Time.now.iso8601
         files[asset.digest_path] = {
           'logical_path' => asset.logical_path,

test/fixtures/double/link_directory_manifest.js

@@ -0,0 +1 @@
+//= link_directory ./stylesheets .css

test/fixtures/double/stylesheets/a.css

@@ -0,0 +1 @@
+body {color: red;}

test/fixtures/double/stylesheets/a.css.erb

@@ -0,0 +1 @@
+body {color: blue;}

test/test_manifest.rb

@@ -16,7 +16,18 @@ def setup
 
   def teardown
     FileUtils.remove_entry_secure(@dir) if File.exist?(@dir)
-    assert Dir["#{@dir}/*"].empty?
+    assert Dir["#{@dir}/*"].empty?, "Expected #{Dir["#{@dir}/*"]} to be empty but it was not"
+  end
+
+  test 'double rendering with link_directory raises an error' do
+    config_manifest = 'link_directory_manifest.js'
+
+    @env.append_path(fixture_path('double'))
+    output_manifest_json = File.join(@dir, "manifest.json")
+    manifest = Sprockets::Manifest.new(@env, @dir, output_manifest_json)
+    assert_raises(Sprockets::DoubleLinkError) do
+      manifest.compile(config_manifest)
+    end
   end
 
   test "specify full manifest filename" do

test/test_path_utils.rb

@@ -31,6 +31,7 @@ def test_entries
       "context",
       "default",
       "directives",
+      "double",
       "encoding",
       "errors",
       "index-assets",
@@ -44,7 +45,7 @@ def test_entries
       "source-maps",
       "symlink"
     ], entries(File.expand_path("../fixtures", __FILE__))
-    
+
     [ ['a', 'b'], ['a', 'b', '.', '..'] ].each do |dir_contents|
       Dir.stub :entries, dir_contents do
         assert_equal ['a', 'b'], entries(Dir.tmpdir)