Fix lazy loading for cookbook filestm/lazy

This introduces a new File class that allows us to clean up manifest and
version a bit.
Signed-off-by: Thom May <thom@chef.io>

11 files changed, 198 insertions, 163 deletions

diff --git a/lib/chef/cookbook/remote_file_vendor.rb b/lib/chef/cookbook/remote_file_vendor.rb
index cfd7789311..4d0b57008c 100644
--- a/lib/chef/cookbook/remote_file_vendor.rb
+++ b/lib/chef/cookbook/remote_file_vendor.rb
@@ -62,7 +62,7 @@ class Chef
         # If the checksums are different between on-disk (current) and on-server
         # (remote, per manifest), do the update. This will also execute if there
         # is no current checksum.
-        if current_checksum != found_manifest_record["checksum"]
+        if found_manifest_record.lazy? || current_checksum != found_manifest_record["checksum"]
           raw_file = @rest.streaming_request(found_manifest_record[:url])
 
           Chef::Log.debug("Storing updated #{cache_filename} in the cache.")
diff --git a/lib/chef/cookbook/synchronizer.rb b/lib/chef/cookbook/synchronizer.rb
index 625f3b4f20..c2dab1c9d9 100644
--- a/lib/chef/cookbook/synchronizer.rb
+++ b/lib/chef/cookbook/synchronizer.rb
@@ -70,6 +70,7 @@ class Chef
       @cookbooks_by_name, @events = cookbooks_by_name, events
 
       @cookbook_full_file_paths = {}
+      @lazy_files = {}
       @remove_obsoleted_files = true
     end
 
@@ -98,14 +99,19 @@ class Chef
     end
 
     def files
-      exclude = unless Chef::Config[:no_lazy_load]
-                  [ :files, :templates ]
-                else
-                  []
-                end
+      lazy = unless Chef::Config[:no_lazy_load]
+               %w{ files templates }
+             else
+               []
+             end
 
       @files ||= cookbooks.inject([]) do |memo, cookbook|
-        cookbook.each_file(excluded_parts: exclude) do |manifest_record|
+        cookbook.each_file do |manifest_record|
+          if lazy.include?(manifest_record.part)
+            manifest_record.lazy = true
+            @lazy_files[cookbook] ||= []
+            @lazy_files[cookbook] << manifest_record
+          end
           memo << CookbookFile.new(cookbook, manifest_record)
         end
         memo
@@ -146,6 +152,7 @@ class Chef
       queue = Chef::Util::ThreadedJobQueue.new
 
       files.each do |file|
+        next if file.manifest_record.lazy?
         queue << lambda do |lock|
           full_file_path = sync_file(file)
 
@@ -230,6 +237,10 @@ class Chef
       @cookbook_full_file_paths.each do |cookbook, full_paths|
         cookbook.all_files = full_paths
       end
+
+      @lazy_files.each do |cookbook, lazy_files|
+        cookbook.cb_files.concat(lazy_files)
+      end
     end
 
     def ensure_cookbook_paths
diff --git a/lib/chef/cookbook_manifest.rb b/lib/chef/cookbook_manifest.rb
index 125f353d21..081f0a38ea 100644
--- a/lib/chef/cookbook_manifest.rb
+++ b/lib/chef/cookbook_manifest.rb
@@ -1,5 +1,5 @@
 # Author:: Daniel DeLeo (<dan@chef.io>)
-# Copyright:: Copyright 2015-2016, Chef Software Inc.
+# Copyright:: Copyright 2015-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -17,8 +17,8 @@
 require "forwardable"
 require "chef/mixin/versioned_api"
 require "chef/util/path_helper"
-require "chef/cookbook/manifest_v0"
-require "chef/cookbook/manifest_v2"
+require "chef/cookbook_manifest/file"
+require "chef/cookbook_manifest/versions"
 require "chef/log"
 
 class Chef
@@ -38,6 +38,10 @@ class Chef
     def_delegator :@cookbook_version, :full_name
     def_delegator :@cookbook_version, :version
     def_delegator :@cookbook_version, :frozen_version?
+    def_delegator :@cookbook_version, :manifest_records_by_path
+    def_delegator :@cookbook_version, :files_for
+    def_delegator :@cookbook_version, :root_files
+    def_delegator :@cookbook_version, :each_file
 
     # Create a new CookbookManifest object for the given `cookbook_version`.
     # You can subsequently call #to_hash to get a Hash representation of the
@@ -65,7 +69,6 @@ class Chef
     def reset!
       @manifest = nil
       @checksums = nil
-      @manifest_records_by_path = nil
       true
     end
 
@@ -101,26 +104,25 @@ class Chef
     #     }
     #
     def manifest
-      @manifest || generate_manifest
+      generate_manifest
       @manifest
     end
 
     def checksums
-      @manifest || generate_manifest
+      generate_manifest
       @checksums
     end
 
-    def manifest_records_by_path
-      @manifest || generate_manifest
-      @manifest_records_by_path
-    end
-
     def policy_mode?
       @policy_mode
     end
 
+    def all_files
+      @cookbook_version.all_files
+    end
+
     def to_hash
-      CookbookManifestVersions.to_hash(self)
+      CookbookManifest::Versions.to_hash(self)
     end
 
     def to_json(*a)
@@ -155,51 +157,21 @@ class Chef
     # make the corresponding changes to the cookbook_version object. Required
     # to provide backward compatibility with CookbookVersion#manifest= method.
     def update_from(new_manifest)
-      @manifest = Chef::CookbookManifestVersions.from_hash(new_manifest)
-      @checksums = extract_checksums_from_manifest(@manifest)
-      @manifest_records_by_path = extract_manifest_records_by_path(@manifest)
-    end
-
-    def files_for(part)
-      return root_files if part.to_s == "root_files"
-      manifest[:all_files].select do |file|
-        seg = file[:name].split("/")[0]
-        part.to_s == seg
-      end
-    end
-
-    def each_file(excluded_parts: [], &block)
-      excluded_parts = Array(excluded_parts).map { |p| p.to_s }
-
-      manifest[:all_files].each do |file|
-        seg = file[:name].split("/")[0]
-        next if excluded_parts.include?(seg)
-        yield file if block_given?
+      manifest = CookbookManifest::Versions.from_hash(new_manifest)
+      cookbook_version.cb_files = manifest[:all_files].each_with_object([]) do |file, memo|
+        memo << CookbookManifest::File.from_hash(file)
       end
     end
 
     def by_parent_directory
       @by_parent_directory ||=
-        manifest[:all_files].inject({}) do |memo, file|
-          parts = file[:name].split("/")
-          parent = if parts.length == 1
-                     "root_files"
-                   else
-                     parts[0]
-                   end
-
+        all_files.each_with_object({}) do |file, memo|
+          parent = file.part
           memo[parent] ||= []
           memo[parent] << file
-          memo
         end
     end
 
-    def root_files
-      manifest[:all_files].select do |file|
-        file[:name].split("/").length == 1
-      end
-    end
-
     private
 
     def cookbook_url_path
@@ -214,29 +186,11 @@ class Chef
       })
       @checksums = {}
 
-      if !root_paths || root_paths.size == 0
-        Chef::Log.error("Cookbook #{name} does not have root_paths! Cannot generate manifest.")
-        raise "Cookbook #{name} does not have root_paths! Cannot generate manifest."
-      end
-
-      @cookbook_version.all_files.each do |file|
-        next if File.directory?(file)
-
-        name, path, specificity = parse_file_from_root_paths(file)
-
-        csum = checksum_cookbook_file(file)
+      all_files.each do |file|
+        csum = file.checksum
         @checksums[csum] = file
-        rs = Mash.new({
-          :name => name,
-          :path => path,
-          :checksum => csum,
-          :specificity => specificity,
-          # full_path is not a part of the normal manifest, but is very useful to keep around.
-          # uploaders should strip this out.
-          :full_path => file,
-        })
 
-        manifest[:all_files] << rs
+        manifest[:all_files] << file.to_hash
       end
 
       manifest[:metadata] = metadata
@@ -250,67 +204,8 @@ class Chef
         manifest[:cookbook_name] = name.to_s
       end
 
-      @manifest_records_by_path = extract_manifest_records_by_path(manifest)
       @manifest = manifest
     end
 
-    def parse_file_from_root_paths(file)
-      root_paths.each do |root_path|
-        pathname = Chef::Util::PathHelper.relative_path_from(root_path, file)
-
-        parts = pathname.each_filename.take(2)
-        # Check if path is actually under root_path
-        next if parts[0] == ".."
-
-        # if we have a root_file, such as metadata.rb, the first part will be "."
-        return [ pathname.to_s, pathname.to_s, "default" ] if parts.length == 1
-
-        segment = parts[0]
-
-        name = File.join(segment, pathname.basename.to_s)
-
-        if segment == "templates" || segment == "files"
-          # Check if pathname looks like files/foo or templates/foo (unscoped)
-          if pathname.each_filename.to_a.length == 2
-            # Use root_default in case the same path exists at root_default and default
-            return [ name, pathname.to_s, "root_default" ]
-          else
-            return [ name, pathname.to_s, parts[1] ]
-          end
-        else
-          return [ name, pathname.to_s, "default" ]
-        end
-      end
-      Chef::Log.error("Cookbook file #{file} not under cookbook root paths #{root_paths.inspect}.")
-      raise "Cookbook file #{file} not under cookbook root paths #{root_paths.inspect}."
-    end
-
-    def extract_checksums_from_manifest(manifest)
-      manifest[:all_files].inject({}) do |memo, manifest_record|
-        memo[manifest_record[:checksum]] = nil
-        memo
-      end
-    end
-
-    def checksum_cookbook_file(filepath)
-      CookbookVersion.checksum_cookbook_file(filepath)
-    end
-
-    def extract_manifest_records_by_path(manifest)
-      manifest[:all_files].inject({}) do |memo, manifest_record|
-        memo[manifest_record[:path]] = manifest_record
-        memo
-      end
-    end
-
-  end
-  class CookbookManifestVersions
-
-    extend Chef::Mixin::VersionedAPIFactory
-    add_versioned_api_class Chef::Cookbook::ManifestV0
-    add_versioned_api_class Chef::Cookbook::ManifestV2
-
-    def_versioned_delegator :from_hash
-    def_versioned_delegator :to_hash
   end
 end
diff --git a/lib/chef/cookbook_manifest/file.rb b/lib/chef/cookbook_manifest/file.rb
new file mode 100644
index 0000000000..eab4987f0c
--- /dev/null
+++ b/lib/chef/cookbook_manifest/file.rb
@@ -0,0 +1,101 @@
+# Copyright:: Copyright 2017, Chef Software Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+class Chef
+  class CookbookManifest
+    class File
+
+      attr_accessor :part, :path, :specificity, :full_path, :lazy, :url, :filename, :name
+
+      def initialize(name: nil, filename: nil, part: nil, path: nil, full_path: nil, specificity: "default", checksum: nil, lazy: false, url: nil)
+        @name = name
+        @part = part
+        @full_path = full_path
+        @path = path
+        @specificity = specificity
+        @checksum = checksum
+        @lazy = lazy
+        @url = url
+        @filename = filename
+      end
+
+      def [](key)
+        if respond_to?(key.to_sym)
+          send(key.to_sym)
+        else
+          nil
+        end
+      end
+
+      def lazy?
+        @lazy
+      end
+
+      def checksum
+        if lazy?
+          nil
+        else
+          @checksum ||= Chef::CookbookVersion.checksum_cookbook_file(full_path)
+        end
+      end
+
+      def to_hash
+        Mash.new({ name: name, path: path, specificity: specificity, checksum: checksum, url: url })
+      end
+
+      def self.from_hash(hash)
+        part, name = hash[:name].split("/")
+        if name.nil?
+          name = part
+          part = "root_files"
+        end
+        new(name: hash[:name], filename: name, part: part, path: hash[:path], specificity: hash[:specificity], checksum: hash[:checksum], url: hash[:url] || nil)
+      end
+
+      def self.from_full_path(full_path, root_paths)
+        Array(root_paths).each do |root|
+          pathname = Chef::Util::PathHelper.relative_path_from(root, full_path)
+
+          parts = pathname.each_filename.take(2)
+          # Check if path is actually under root_path
+          next if parts[0] == ".."
+
+          name = pathname.basename.to_s
+
+          # if we have a root_file, such as metadata.rb, the first part will be "."
+          return new(name: name, filename: name, part: "root_files", path: pathname.to_s, full_path: full_path, specificity: "default") if parts.length == 1
+
+          segment = parts[0]
+          cname = "#{parts[0]}/#{name}"
+
+          if segment == "templates" || segment == "files"
+            # Check if pathname looks like files/foo or templates/foo (unscoped)
+            if pathname.each_filename.to_a.length == 2
+              # Use root_default in case the same path exists at root_default and default
+              return new(name: cname, filename: name, part: segment, path: pathname.to_s, full_path: full_path, specificity: "root_default")
+            else
+              return new(name: cname, filename: name, part: segment, path: pathname.to_s, full_path: full_path, specificity: parts[1])
+            end
+          else
+            return new(name: cname, filename: name, part: segment, path: pathname.to_s, full_path: full_path, specificity: "default")
+          end
+        end
+        Chef::Log.error("Cookbook file #{full_path} not under cookbook root paths #{root_paths.inspect}.")
+        raise "Cookbook file #{full_path} not under cookbook root paths #{root_paths.inspect}."
+      end
+
+    end
+  end
+end
diff --git a/lib/chef/cookbook_manifest/manifest_v0.rb b/lib/chef/cookbook_manifest/manifest_v0.rb
index 4664195da2..8e79d70931 100644
--- a/lib/chef/cookbook_manifest/manifest_v0.rb
+++ b/lib/chef/cookbook_manifest/manifest_v0.rb
@@ -49,9 +49,8 @@ class Chef
           if COOKBOOK_SEGMENTS.include?(parent)
             memo[parent] ||= []
             files[parent].each do |file|
-              file["name"] = file["name"].split("/")[1] unless parent == "root_files"
-              file.delete("full_path")
-              memo[parent] << file
+              v0 = { name: file.filename, specificity: file.specificity, checksum: file.checksum, path: file.path }
+              memo[parent] << v0
             end
           end
         end
diff --git a/lib/chef/cookbook_uploader.rb b/lib/chef/cookbook_uploader.rb
index cc74644f27..3ffbf13eb9 100644
--- a/lib/chef/cookbook_uploader.rb
+++ b/lib/chef/cookbook_uploader.rb
@@ -120,7 +120,7 @@ class Chef
         # but we need the base64 encoding for the content-md5
         # header
         checksum64 = Base64.encode64([checksum].pack("H*")).strip
-        file_contents = File.open(file, "rb") { |f| f.read }
+        file_contents = File.open(file[:full_path], "rb") { |f| f.read }
 
         # Custom headers. 'content-type' disables JSON serialization of the request body.
         headers = { "content-type" => "application/x-binary", "content-md5" => checksum64, "accept" => "application/json" }
diff --git a/lib/chef/cookbook_version.rb b/lib/chef/cookbook_version.rb
index dcb8c97548..face83e19b 100644
--- a/lib/chef/cookbook_version.rb
+++ b/lib/chef/cookbook_version.rb
@@ -25,6 +25,7 @@ require "chef/cookbook/metadata"
 require "chef/version_class"
 require "chef/digester"
 require "chef/cookbook_manifest"
+require "chef/cookbook_manifest/file"
 require "chef/server_api"
 
 class Chef
@@ -39,13 +40,8 @@ class Chef
     include Comparable
     extend Forwardable
 
-    def_delegator :@cookbook_manifest, :files_for
-    def_delegator :@cookbook_manifest, :each_file
-
     COOKBOOK_SEGMENTS = [ :resources, :providers, :recipes, :definitions, :libraries, :attributes, :files, :templates, :root_files ]
 
-    attr_reader :all_files
-
     attr_accessor :root_paths
     attr_accessor :name
 
@@ -74,9 +70,39 @@ class Chef
       root_paths[0]
     end
 
+    def all_files
+      @all_files.map { |f| f[:full_path] }
+    end
+
     def all_files=(files)
-      @all_files = Array(files)
-      cookbook_manifest.reset!
+      @all_files = Array(files).each_with_object([]) do |file, memo|
+        memo << CookbookManifest::File.from_full_path(file, root_paths)
+      end
+    end
+
+    def manifest_records_by_path
+      @manifest_records_by_path ||= @all_files.each_with_object({}) do |file, memo|
+        memo[file[:path]] = file
+      end
+    end
+
+    def root_files
+      files_for("root_files")
+    end
+
+    def files_for(part)
+      @all_files.select do |file|
+        part.to_s == file.part
+      end
+    end
+
+    def each_file(excluded_parts: [], &block)
+      excluded_parts = Array(excluded_parts).map { |p| p.to_s }
+
+      @all_files.each do |file|
+        next if excluded_parts.include?(file.part)
+        yield file if block_given?
+      end
     end
 
     # This is the one and only method that knows how cookbook files'
@@ -101,8 +127,6 @@ class Chef
       @root_paths = root_paths
       @frozen = false
 
-      @all_files = Array.new
-
       @file_vendor = nil
       @cookbook_manifest = Chef::CookbookManifest.new(self)
       @metadata = Chef::Cookbook::Metadata.new
@@ -174,15 +198,10 @@ class Chef
       cookbook_manifest.checksums
     end
 
-    def manifest_records_by_path
-      cookbook_manifest.manifest_records_by_path
-    end
-
     # Return recipe names in the form of cookbook_name::recipe_name
     def fully_qualified_recipe_names
       files_for("recipes").inject([]) do |memo, recipe|
-        rname = recipe[:name].split("/")[1]
-        rname = File.basename(rname, ".rb")
+        rname = File.basename(recipe[:filename], ".rb")
         memo << "#{name}::#{rname}"
         memo
       end
@@ -414,7 +433,11 @@ class Chef
       output["frozen?"] = frozen_version?
       output["metadata"] = metadata.to_hash
       output["version"] = version
-      output.merge(cookbook_manifest.by_parent_directory)
+      files = cookbook_manifest.by_parent_directory.each_with_object({}) do |(part, file), memo|
+        memo[part] = file.map(&:to_hash)
+      end
+
+      output.merge(files)
     end
 
     def self.from_hash(o)
@@ -459,7 +482,7 @@ class Chef
     end
 
     def self.chef_server_rest
-      Chef::ServerAPI.new(Chef::Config[:chef_server_url], { version_class: Chef::CookbookManifestVersions })
+      Chef::ServerAPI.new(Chef::Config[:chef_server_url], { version_class: Chef::CookbookManifest::Versions })
     end
 
     def destroy
diff --git a/spec/unit/cookbook/synchronizer_spec.rb b/spec/unit/cookbook/synchronizer_spec.rb
index 77e64482da..e713b961c9 100644
--- a/spec/unit/cookbook/synchronizer_spec.rb
+++ b/spec/unit/cookbook/synchronizer_spec.rb
@@ -118,6 +118,7 @@ describe Chef::CookbookSynchronizer do
 
   let(:synchronizer) do
     Chef::Config[:no_lazy_load] = no_lazy_load
+    Chef::Config[:file_cache_path] = "/file-cache"
     Chef::CookbookSynchronizer.new(cookbook_manifest, events)
   end
 
diff --git a/spec/unit/cookbook_manifest_spec.rb b/spec/unit/cookbook_manifest_spec.rb
index a28eaff3d3..1ae471c15e 100644
--- a/spec/unit/cookbook_manifest_spec.rb
+++ b/spec/unit/cookbook_manifest_spec.rb
@@ -126,12 +126,13 @@ describe Chef::CookbookManifest do
                  relative_path
                end
 
-        {
+        Mash.new({
           "name"        => name,
           "path"        => relative_path,
-          "checksum"    => Chef::Digester.generate_md5_checksum_for_file(path),
           "specificity" => "default",
-        }.tap do |fp|
+          "checksum"    => Chef::Digester.generate_md5_checksum_for_file(path),
+          "url" => nil,
+        }).tap do |fp|
           if full
             fp["full_path"] = path
           end
@@ -168,14 +169,17 @@ describe Chef::CookbookManifest do
     end
 
     context ".each_file" do
+      let(:manifest_files) do
+        map_to_file_specs(all_files)
+      end
+
       it "yields all the files" do
-        files = map_to_file_specs(all_files, full: true)
-        expect(cookbook_manifest.to_enum(:each_file)).to match_array(files)
+        expect(cookbook_manifest.to_enum(:each_file).map(&:to_hash)).to match_array(manifest_files)
       end
 
       it "excludes certain file parts" do
-        files = map_to_file_specs(all_files, full: true).reject { |f| seg = f["name"].split("/")[0]; %w{ files templates }.include?(seg) }
-        expect(cookbook_manifest.to_enum(:each_file, excluded_parts: %w{ files templates })).to match_array(files)
+        files = manifest_files.reject { |f| seg = f["name"].split("/")[0]; %w{ files templates }.include?(seg) }
+        expect(cookbook_manifest.to_enum(:each_file, excluded_parts: %w{ files templates }).map(&:to_hash)).to match_array(files)
       end
     end
   end
diff --git a/spec/unit/cookbook_uploader_spec.rb b/spec/unit/cookbook_uploader_spec.rb
index e992c11ef7..4fb8e30e2e 100644
--- a/spec/unit/cookbook_uploader_spec.rb
+++ b/spec/unit/cookbook_uploader_spec.rb
@@ -101,7 +101,7 @@ describe Chef::CookbookUploader do
         }
 
         expect(http_client).to receive(:put).
-          with(url_for(md5), IO.binread(file_path), upload_headers)
+          with(url_for(md5), IO.binread(file_path.full_path), upload_headers)
 
       end
     end
diff --git a/spec/unit/knife/cookbook_show_spec.rb b/spec/unit/knife/cookbook_show_spec.rb
index 1e8ea836d7..9848194754 100644
--- a/spec/unit/knife/cookbook_show_spec.rb
+++ b/spec/unit/knife/cookbook_show_spec.rb
@@ -130,6 +130,7 @@ describe Chef::Knife::CookbookShow do
           [{ "name" => "recipes/default.rb",
              "path" => "recipes/default.rb",
              "checksum" => "1234",
+             "specificity" => nil,
              "url" => "http://example.org/files/default.rb" }],
         }
       end