(#6353) Restore the ability to store paths in the filebucket

Commit 2274d5104f6e413a2b8899a3c3111a17bbb2f4d7 optimized network
usage for the case where a file is already in the filebucket.
However, it took away the ability to store paths.

This change restores the ability to store paths while maintaining
optimal network usage for the case where the file is already in the
filebucket with the given path.  This is expected to be the most
common case.

Paired-with: Jesse Wolfe <jesse@puppetlabs.com>

5 files changed, 145 insertions, 61 deletions

diff --git a/lib/puppet/file_bucket/dipper.rb b/lib/puppet/file_bucket/dipper.rb
index f4bef28a8..de4c01b78 100644
--- a/lib/puppet/file_bucket/dipper.rb
+++ b/lib/puppet/file_bucket/dipper.rb
@@ -34,11 +34,12 @@ class Puppet::FileBucket::Dipper
     contents = ::File.read(file)
     begin
       file_bucket_file = Puppet::FileBucket::File.new(contents, :bucket_path => @local_path)
-      dest_path = "#{@rest_path}#{file_bucket_file.name}"
+      files_original_path = absolutize_path(file)
+      dest_path = "#{@rest_path}#{file_bucket_file.name}#{files_original_path}"
 
       # Make a HEAD request for the file so that we don't waste time
       # uploading it if it already exists in the bucket.
-      unless Puppet::FileBucket::File.head("#{@rest_path}#{file_bucket_file.checksum_type}/#{file_bucket_file.checksum_data}")
+      unless Puppet::FileBucket::File.head("#{@rest_path}#{file_bucket_file.checksum_type}/#{file_bucket_file.checksum_data}#{files_original_path}")
         file_bucket_file.save(dest_path)
       end
 
diff --git a/lib/puppet/indirector/file_bucket_file/file.rb b/lib/puppet/indirector/file_bucket_file/file.rb
index 8bea2d767..0fd8a914f 100644
--- a/lib/puppet/indirector/file_bucket_file/file.rb
+++ b/lib/puppet/indirector/file_bucket_file/file.rb
@@ -14,10 +14,12 @@ module Puppet::FileBucketFile
     end
 
     def find( request )
-      checksum = request_to_checksum( request )
-      file_path = path_for(request.options[:bucket_path], checksum, 'contents')
+      checksum, files_original_path = request_to_checksum_and_path( request )
+      dir_path = path_for(request.options[:bucket_path], checksum)
+      file_path = ::File.join(dir_path, 'contents')
 
       return nil unless ::File.exists?(file_path)
+      return nil unless path_match(dir_path, files_original_path)
 
       if request.options[:diff_with]
         hash_protocol = sumtype(checksum)
@@ -32,32 +34,47 @@ module Puppet::FileBucketFile
     end
 
     def head(request)
-      checksum = request_to_checksum(request)
-      file_path = path_for(request.options[:bucket_path], checksum, 'contents')
-      ::File.exists?(file_path)
+      checksum, files_original_path = request_to_checksum_and_path(request)
+      dir_path = path_for(request.options[:bucket_path], checksum)
+
+      ::File.exists?(::File.join(dir_path, 'contents')) and path_match(dir_path, files_original_path)
     end
 
     def save( request )
       instance = request.instance
+      checksum, files_original_path = request_to_checksum_and_path(request)
 
-      save_to_disk(instance)
+      save_to_disk(instance, files_original_path)
       instance.to_s
     end
 
     private
 
-    def save_to_disk( bucket_file )
+    def path_match(dir_path, files_original_path)
+      return true unless files_original_path # if no path was provided, it's a match
+      paths_path = ::File.join(dir_path, 'paths')
+      return false unless ::File.exists?(paths_path)
+      ::File.open(paths_path) do |f|
+        f.each do |line|
+          return true if line.chomp == files_original_path
+        end
+      end
+      return false
+    end
+
+    def save_to_disk( bucket_file, files_original_path )
       filename = path_for(bucket_file.bucket_path, bucket_file.checksum_data, 'contents')
-      dirname = path_for(bucket_file.bucket_path, bucket_file.checksum_data)
+      dir_path = path_for(bucket_file.bucket_path, bucket_file.checksum_data)
+      paths_path = ::File.join(dir_path, 'paths')
 
       # If the file already exists, do nothing.
       if ::File.exist?(filename)
         verify_identical_file!(bucket_file)
       else
         # Make the directories if necessary.
-        unless ::File.directory?(dirname)
+        unless ::File.directory?(dir_path)
           Puppet::Util.withumask(0007) do
-            ::FileUtils.mkdir_p(dirname)
+            ::FileUtils.mkdir_p(dir_path)
           end
         end
 
@@ -68,15 +85,27 @@ module Puppet::FileBucketFile
           ::File.open(filename, ::File::WRONLY|::File::CREAT, 0440) do |of|
             of.print bucket_file.contents
           end
+          ::File.open(paths_path, ::File::WRONLY|::File::CREAT, 0640) do |of|
+            # path will be written below
+          end
+        end
+      end
+
+      unless path_match(dir_path, files_original_path)
+        ::File.open(paths_path, 'a') do |f|
+          f.puts(files_original_path)
         end
       end
     end
 
-    def request_to_checksum( request )
-      checksum_type, checksum, path = request.key.split(/\//, 3) # Note: we ignore path if present.
+    def request_to_checksum_and_path( request )
+      checksum_type, checksum, path = request.key.split(/\//, 3)
+      if path == '' # Treat "md5/<checksum>/" like "md5/<checksum>"
+        path = nil
+      end
       raise "Unsupported checksum type #{checksum_type.inspect}" if checksum_type != 'md5'
       raise "Invalid checksum #{checksum.inspect}" if checksum !~ /^[0-9a-f]{32}$/
-      checksum
+      [checksum, path]
     end
 
     def path_for(bucket_path, digest, subfile = nil)
diff --git a/spec/unit/file_bucket/dipper_spec.rb b/spec/unit/file_bucket/dipper_spec.rb
index db40c6296..c40d79589 100755
--- a/spec/unit/file_bucket/dipper_spec.rb
+++ b/spec/unit/file_bucket/dipper_spec.rb
@@ -92,7 +92,7 @@ describe Puppet::FileBucket::Dipper do
     [request1, request2].each do |r|
       r.server.should == 'puppetmaster'
       r.port.should == 31337
-      r.key.should == "md5/#{checksum}"
+      r.key.should == "md5/#{checksum}#{real_path}"
     end
   end
 
diff --git a/spec/unit/file_bucket/file_spec.rb b/spec/unit/file_bucket/file_spec.rb
index 82063c2e3..f80b16238 100644
--- a/spec/unit/file_bucket/file_spec.rb
+++ b/spec/unit/file_bucket/file_spec.rb
@@ -64,30 +64,6 @@ describe Puppet::FileBucket::File do
     end
   end
 
-  describe "when saving files" do
-    it "should save the contents to the calculated path" do
-      ::File.stubs(:directory?).with(@dir).returns(true)
-      ::File.expects(:exist?).with("#{@dir}/contents").returns false
-
-      mockfile = mock "file"
-      mockfile.expects(:print).with(@contents)
-      ::File.expects(:open).with("#{@dir}/contents", ::File::WRONLY|::File::CREAT, 0440).yields(mockfile)
-
-      Puppet::FileBucket::File.new(@contents).save
-    end
-
-    it "should make any directories necessary for storage" do
-      FileUtils.expects(:mkdir_p).with do |arg|
-        ::File.umask == 0007 and arg == @dir
-      end
-      ::File.expects(:directory?).with(@dir).returns(false)
-      ::File.expects(:open).with("#{@dir}/contents", ::File::WRONLY|::File::CREAT, 0440)
-      ::File.expects(:exist?).with("#{@dir}/contents").returns false
-
-      Puppet::FileBucket::File.new(@contents).save
-    end
-  end
-
   it "should return a url-ish name" do
     Puppet::FileBucket::File.new(@contents).name.should == "md5/4a8ec4fa5f01b4ab1a0ab8cbccb709f0"
   end
@@ -105,17 +81,6 @@ describe Puppet::FileBucket::File do
     Puppet::FileBucket::File.from_pson({"contents"=>"file contents"}).contents.should == "file contents"
   end
 
-  it "should save a file" do
-    ::File.expects(:exist?).with("#{@dir}/contents").returns false
-    ::File.expects(:directory?).with(@dir).returns false
-    ::FileUtils.expects(:mkdir_p).with(@dir)
-    ::File.expects(:open).with("#{@dir}/contents",  ::File::WRONLY|::File::CREAT, 0440)
-
-    bucketfile = Puppet::FileBucket::File.new(@contents)
-    bucketfile.save
-
-  end
-
   def make_bucketed_file
     FileUtils.mkdir_p(@dir)
     File.open("#{@dir}/contents", 'w') { |f| f.write @contents }
diff --git a/spec/unit/indirector/file_bucket_file/file_spec.rb b/spec/unit/indirector/file_bucket_file/file_spec.rb
index 9187f4da0..0c33593d7 100755
--- a/spec/unit/indirector/file_bucket_file/file_spec.rb
+++ b/spec/unit/indirector/file_bucket_file/file_spec.rb
@@ -22,13 +22,97 @@ describe Puppet::FileBucketFile::File do
       Puppet[:bucketdir] = tmpdir('bucketdir')
     end
 
-    describe "when diffing files" do
-      def save_bucket_file(contents)
-        bucket_file = Puppet::FileBucket::File.new(contents)
-        bucket_file.save
-        bucket_file.checksum_data
+    def save_bucket_file(contents, path = "/who_cares")
+      bucket_file = Puppet::FileBucket::File.new(contents)
+      bucket_file.save("md5/#{Digest::MD5.hexdigest(contents)}#{path}")
+      bucket_file.checksum_data
+    end
+
+    describe "when servicing a save request" do
+      describe "when supplying a path" do
+        it "should store the path if not already stored" do
+          checksum = save_bucket_file("stuff", "/foo/bar")
+          dir_path = "#{Puppet[:bucketdir]}/c/1/3/d/8/8/c/b/c13d88cb4cb02003daedb8a84e5d272a"
+          File.read("#{dir_path}/contents").should == "stuff"
+          File.read("#{dir_path}/paths").should == "foo/bar\n"
+        end
+
+        it "should leave the paths file alone if the path is already stored" do
+          checksum = save_bucket_file("stuff", "/foo/bar")
+          checksum = save_bucket_file("stuff", "/foo/bar")
+          dir_path = "#{Puppet[:bucketdir]}/c/1/3/d/8/8/c/b/c13d88cb4cb02003daedb8a84e5d272a"
+          File.read("#{dir_path}/contents").should == "stuff"
+          File.read("#{dir_path}/paths").should == "foo/bar\n"
+        end
+
+        it "should store an additional path if the new path differs from those already stored" do
+          checksum = save_bucket_file("stuff", "/foo/bar")
+          checksum = save_bucket_file("stuff", "/foo/baz")
+          dir_path = "#{Puppet[:bucketdir]}/c/1/3/d/8/8/c/b/c13d88cb4cb02003daedb8a84e5d272a"
+          File.read("#{dir_path}/contents").should == "stuff"
+          File.read("#{dir_path}/paths").should == "foo/bar\nfoo/baz\n"
+        end
+      end
+
+      describe "when not supplying a path" do
+        it "should save the file and create an empty paths file" do
+          checksum = save_bucket_file("stuff", "")
+          dir_path = "#{Puppet[:bucketdir]}/c/1/3/d/8/8/c/b/c13d88cb4cb02003daedb8a84e5d272a"
+          File.read("#{dir_path}/contents").should == "stuff"
+          File.read("#{dir_path}/paths").should == ""
+        end
+      end
+    end
+
+    describe "when servicing a head/find request" do
+      describe "when supplying a path" do
+        it "should return false/nil if the file isn't bucketed" do
+          Puppet::FileBucket::File.head("md5/0ae2ec1980410229885fe72f7b44fe55/foo/bar").should == false
+          Puppet::FileBucket::File.find("md5/0ae2ec1980410229885fe72f7b44fe55/foo/bar").should == nil
+        end
+
+        it "should return false/nil if the file is bucketed but with a different path" do
+          checksum = save_bucket_file("I'm the contents of a file", '/foo/bar')
+          Puppet::FileBucket::File.head("md5/#{checksum}/foo/baz").should == false
+          Puppet::FileBucket::File.find("md5/#{checksum}/foo/baz").should == nil
+        end
+
+        it "should return true/file if the file is already bucketed with the given path" do
+          contents = "I'm the contents of a file"
+          checksum = save_bucket_file(contents, '/foo/bar')
+          Puppet::FileBucket::File.head("md5/#{checksum}/foo/bar").should == true
+          find_result = Puppet::FileBucket::File.find("md5/#{checksum}/foo/bar")
+          find_result.should be_a(Puppet::FileBucket::File)
+          find_result.checksum.should == "{md5}#{checksum}"
+          find_result.to_s.should == contents
+        end
+      end
+
+      describe "when not supplying a path" do
+        [false, true].each do |trailing_slash|
+          describe "#{trailing_slash ? 'with' : 'without'} a trailing slash" do
+            trailing_string = trailing_slash ? '/' : ''
+
+            it "should return false/nil if the file isn't bucketed" do
+              Puppet::FileBucket::File.head("md5/0ae2ec1980410229885fe72f7b44fe55#{trailing_string}").should == false
+              Puppet::FileBucket::File.find("md5/0ae2ec1980410229885fe72f7b44fe55#{trailing_string}").should == nil
+            end
+
+            it "should return true/file if the file is already bucketed" do
+              contents = "I'm the contents of a file"
+              checksum = save_bucket_file(contents, '/foo/bar')
+              Puppet::FileBucket::File.head("md5/#{checksum}#{trailing_string}").should == true
+              find_result = Puppet::FileBucket::File.find("md5/#{checksum}#{trailing_string}")
+              find_result.should be_a(Puppet::FileBucket::File)
+              find_result.checksum.should == "{md5}#{checksum}"
+              find_result.to_s.should == contents
+            end
+          end
+        end
       end
+    end
 
+    describe "when diffing files" do
       it "should generate an empty string if there is no diff" do
         checksum = save_bucket_file("I'm the contents of a file")
         Puppet::FileBucket::File.find("md5/#{checksum}", :diff_with => checksum).should == ''
@@ -102,7 +186,7 @@ HERE
 
               key = "md5/#{@digest}"
               if supply_path
-                key += "//path/to/file"
+                key += "/path/to/file"
               end
 
               @request = Puppet::Indirector::Request.new(:indirection_name, :find, key, request_options)
@@ -116,10 +200,15 @@ HERE
             it "should return an instance of Puppet::FileBucket::File created with the content if the file exists" do
               make_bucketed_file
 
-              bucketfile = @store.find(@request)
-              bucketfile.should be_a(Puppet::FileBucket::File)
-              bucketfile.contents.should == @contents
-              @store.head(@request).should == true
+              if supply_path
+                @store.find(@request).should == nil
+                @store.head(@request).should == false # because path didn't match
+              else
+                bucketfile = @store.find(@request)
+                bucketfile.should be_a(Puppet::FileBucket::File)
+                bucketfile.contents.should == @contents
+                @store.head(@request).should == true
+              end
             end
 
             it "should return nil if no file is found" do