Merge pull request #7820 from chef/lcg/chef-15-cleanup-cookbook-loader

More cookbook loader cleanup and documentation

4 files changed, 110 insertions, 77 deletions

diff --git a/lib/chef/cookbook/cookbook_version_loader.rb b/lib/chef/cookbook/cookbook_version_loader.rb
index c864c30505..96bc4aa303 100644
--- a/lib/chef/cookbook/cookbook_version_loader.rb
+++ b/lib/chef/cookbook/cookbook_version_loader.rb
@@ -7,12 +7,21 @@ require "find"
 
 class Chef
   class Cookbook
+    # This class is only used drectly from the Chef::CookbookLoader and from chef-fs,
+    # so it only affects legacy-mode chef-client runs and knife.  It is not used by
+    # server or zolo/zero modes.
+    #
+    # This seems to be mostly a glorified factory method for creating CookbookVersion
+    # objects now, with creating Metadata objects bolted onto the side?  It used
+    # to be also responsible for the merging of multiple objects when creating
+    # shadowed/merged cookbook versions from multiple sources.  It also handles
+    # Chefignore files.
+    #
     class CookbookVersionLoader
 
       UPLOADED_COOKBOOK_VERSION_FILE = ".uploaded-cookbook-version.json".freeze
 
       attr_reader :cookbook_settings
-      attr_reader :cookbook_paths
       attr_reader :frozen
       attr_reader :uploaded_cookbook_version_file
 
@@ -25,13 +34,11 @@ class Chef
 
       def initialize(path, chefignore = nil)
         @cookbook_path = File.expand_path( path ) # cookbook_path from which this was loaded
-        # We keep a list of all cookbook paths that have been merged in
-        @cookbook_paths = [ cookbook_path ]
 
         @inferred_cookbook_name = File.basename( path )
         @chefignore = chefignore
         @metadata = nil
-        @relative_path = /#{Regexp.escape(@cookbook_path)}\/(.+)$/
+        @relative_path = /#{Regexp.escape(cookbook_path)}\/(.+)$/
         @metadata_loaded = false
         @cookbook_settings = {
           all_files: {},
@@ -68,53 +75,22 @@ class Chef
         if empty?
           Chef::Log.warn "Found a directory #{cookbook_name} in the cookbook path, but it contains no cookbook files. skipping."
         end
-        @cookbook_settings
+        cookbook_settings
       end
 
       alias :load_cookbooks :load
 
-      def metadata_filenames
-        return @metadata_filenames unless @metadata_filenames.empty?
-        if File.exists?(File.join(cookbook_path, UPLOADED_COOKBOOK_VERSION_FILE))
-          @uploaded_cookbook_version_file = File.join(cookbook_path, UPLOADED_COOKBOOK_VERSION_FILE)
-        end
-
-        if File.exists?(File.join(cookbook_path, "metadata.json"))
-          @metadata_filenames << File.join(cookbook_path, "metadata.json")
-        elsif File.exists?(File.join(cookbook_path, "metadata.rb"))
-          @metadata_filenames << File.join(cookbook_path, "metadata.rb")
-        elsif @uploaded_cookbook_version_file
-          @metadata_filenames << @uploaded_cookbook_version_file
-        end
-
-        # Set frozen based on .uploaded-cookbook-version.json
-        set_frozen
-        @metadata_filenames
-      end
-
       def cookbook_version
         return nil if empty?
 
-        Chef::CookbookVersion.new(cookbook_name, *cookbook_paths).tap do |c|
+        Chef::CookbookVersion.new(cookbook_name, cookbook_path).tap do |c|
           c.all_files            = cookbook_settings[:all_files].values
           c.metadata             = metadata
 
-          c.freeze_version if @frozen
+          c.freeze_version if frozen
         end
       end
 
-      def cookbook_name
-        # The `name` attribute is now required in metadata, so
-        # inferred_cookbook_name generally should not be used. Per CHEF-2923,
-        # we have to not raise errors in cookbook metadata immediately, so that
-        # users can still `knife cookbook upload some-cookbook` when an
-        # unrelated cookbook has an error in its metadata.  This situation
-        # could prevent us from reading the `name` attribute from the metadata
-        # entirely, but the name is used as a hash key in CookbookLoader, so we
-        # fall back to the inferred name here.
-        (metadata.name || @inferred_cookbook_name).to_sym
-      end
-
       # Generates the Cookbook::Metadata object
       def metadata
         return @metadata unless @metadata.nil?
@@ -125,7 +101,7 @@ class Chef
           case metadata_file
           when /\.rb$/
             apply_ruby_metadata(metadata_file)
-          when @uploaded_cookbook_version_file
+          when uploaded_cookbook_version_file
             apply_json_cookbook_version_metadata(metadata_file)
           when /\.json$/
             apply_json_metadata(metadata_file)
@@ -146,13 +122,46 @@ class Chef
         @metadata
       end
 
+      def cookbook_name
+        # The `name` attribute is now required in metadata, so
+        # inferred_cookbook_name generally should not be used. Per CHEF-2923,
+        # we have to not raise errors in cookbook metadata immediately, so that
+        # users can still `knife cookbook upload some-cookbook` when an
+        # unrelated cookbook has an error in its metadata.  This situation
+        # could prevent us from reading the `name` attribute from the metadata
+        # entirely, but the name is used as a hash key in CookbookLoader, so we
+        # fall back to the inferred name here.
+        (metadata.name || inferred_cookbook_name).to_sym
+      end
+
+      private
+
+      def metadata_filenames
+        return @metadata_filenames unless @metadata_filenames.empty?
+        if File.exists?(File.join(cookbook_path, UPLOADED_COOKBOOK_VERSION_FILE))
+          @uploaded_cookbook_version_file = File.join(cookbook_path, UPLOADED_COOKBOOK_VERSION_FILE)
+        end
+
+        if File.exists?(File.join(cookbook_path, "metadata.json"))
+          @metadata_filenames << File.join(cookbook_path, "metadata.json")
+        elsif File.exists?(File.join(cookbook_path, "metadata.rb"))
+          @metadata_filenames << File.join(cookbook_path, "metadata.rb")
+        elsif uploaded_cookbook_version_file
+          @metadata_filenames << uploaded_cookbook_version_file
+        end
+
+        # Set frozen based on .uploaded-cookbook-version.json
+        set_frozen
+        @metadata_filenames
+      end
+
       def raise_metadata_error!
-        raise @metadata_error unless @metadata_error.nil?
+        raise metadata_error unless metadata_error.nil?
         # Metadata won't be valid if the cookbook is empty. If the cookbook is
         # actually empty, a metadata error here would be misleading, so don't
         # raise it (if called by #load!, a different error is raised).
         if !empty? && !metadata.valid?
-          message = "Cookbook loaded at path(s) [#{@cookbook_paths.join(', ')}] has invalid metadata: #{metadata.errors.join('; ')}"
+          message = "Cookbook loaded at path [#{cookbook_path}] has invalid metadata: #{metadata.errors.join('; ')}"
           raise Exceptions::MetadataNotValid, message
         end
         false
@@ -162,18 +171,6 @@ class Chef
         cookbook_settings.values.all? { |files_hash| files_hash.empty? } && metadata_filenames.size == 0
       end
 
-      def merge!(other_cookbook_loader)
-        other_cookbook_settings = other_cookbook_loader.cookbook_settings
-        cookbook_settings.each do |file_type, file_list|
-          file_list.merge!(other_cookbook_settings[file_type])
-        end
-        metadata_filenames.concat(other_cookbook_loader.metadata_filenames)
-        @cookbook_paths += other_cookbook_loader.cookbook_paths
-        @frozen = true if other_cookbook_loader.frozen
-        @metadata = nil # reset metadata so it gets reloaded and all metadata files applied.
-        self
-      end
-
       def chefignore
         @chefignore ||= Chefignore.new(File.basename(cookbook_path))
       end
@@ -223,14 +220,14 @@ class Chef
       def apply_ruby_metadata(file)
         @metadata.from_file(file)
       rescue Chef::Exceptions::JSON::ParseError
-        Chef::Log.error("Error evaluating metadata.rb for #{@inferred_cookbook_name} in " + file)
+        Chef::Log.error("Error evaluating metadata.rb for #{inferred_cookbook_name} in " + file)
         raise
       end
 
       def apply_json_metadata(file)
         @metadata.from_json(IO.read(file))
       rescue Chef::Exceptions::JSON::ParseError
-        Chef::Log.error("Couldn't parse cookbook metadata JSON for #{@inferred_cookbook_name} in " + file)
+        Chef::Log.error("Couldn't parse cookbook metadata JSON for #{inferred_cookbook_name} in " + file)
         raise
       end
 
@@ -247,7 +244,7 @@ class Chef
         # metadata contains a name key.
         @metadata.name(data["cookbook_name"]) unless data["metadata"].key?("name")
       rescue Chef::Exceptions::JSON::ParseError
-        Chef::Log.error("Couldn't parse cookbook metadata JSON for #{@inferred_cookbook_name} in " + file)
+        Chef::Log.error("Couldn't parse cookbook metadata JSON for #{inferred_cookbook_name} in " + file)
         raise
       end
 
@@ -257,7 +254,7 @@ class Chef
             data = Chef::JSONCompat.parse(IO.read(uploaded_cookbook_version_file))
             @frozen = data["frozen?"]
           rescue Chef::Exceptions::JSON::ParseError
-            Chef::Log.error("Couldn't parse cookbook metadata JSON for #{@inferred_cookbook_name} in #{uploaded_cookbook_version_file}")
+            Chef::Log.error("Couldn't parse cookbook metadata JSON for #{inferred_cookbook_name} in #{uploaded_cookbook_version_file}")
             raise
           end
         end
diff --git a/lib/chef/cookbook_loader.rb b/lib/chef/cookbook_loader.rb
index ed6f9342df..1a7dec8b03 100644
--- a/lib/chef/cookbook_loader.rb
+++ b/lib/chef/cookbook_loader.rb
@@ -25,42 +25,72 @@ require "chef/cookbook_version"
 require "chef/cookbook/chefignore"
 require "chef/cookbook/metadata"
 
-#
-# CookbookLoader class loads the cookbooks lazily as read
-#
 class Chef
+  # This class is used by knife, cheffs and legacy chef-solo modes.  It is not used by the server mode
+  # of chef-client or zolo/zero modes.
+  #
+  # This class implements orchestration around producing a single cookbook_version for a cookbook or
+  # loading a Mash of all cookbook_versions, using the cookbook_version_loader class, and doing
+  # lazy-access and memoization to only load each cookbook once on demand.
+  #
+  # This implements a key-value style each which makes it appear to be a Hash of String => CookbookVersion
+  # pairs where the String is the cookbook name.  The use of Enumerable combined with the Hash-style
+  # each is likely not entirely sane.
+  #
+  # This object is also passed and injected into the CookbookCollection object where it is converted
+  # to a Mash that looks almost exactly like the cookbook_by_name Mash in this object.
+  #
   class CookbookLoader
-    # FIXME: doc public api
-    attr_reader :cookbook_paths
+    # @return [Array<String>] the array of repo paths containing cookbook dirs
+    attr_reader :repo_paths
 
+    # XXX: this is highly questionable combined with the Hash-style each method
     include Enumerable
 
+    # @param repo_paths [Array<String>] the array of repo paths containing cookbook dirs
     def initialize(*repo_paths)
       @repo_paths = repo_paths.flatten.map { |p| File.expand_path(p) }
-      raise ArgumentError, "You must specify at least one cookbook repo path" if @repo_paths.empty?
+      raise ArgumentError, "You must specify at least one cookbook repo path" if repo_paths.empty?
     end
 
+    # The primary function of this class is to build this Mash mapping cookbook names as a string to
+    # the CookbookVersion objects for them.  Callers must call "load_cookbooks" first.
+    #
+    # @return [Mash<String, Chef::CookbookVersion>]
     def cookbooks_by_name
       @cookbooks_by_name ||= Mash.new
     end
 
+    # This class also builds a mapping of cookbook names to their Metadata objects.  Callers must call
+    # "load_cookbooks" first.
+    #
+    # @return [Mash<String, Chef::Cookbook::Metadata>]
     def metadata
       @metadata ||= Mash.new
     end
 
+    # Loads all cookbooks across all repo_paths
+    #
+    # @return [Mash<String, Chef::CookbookVersion>] the cookbooks_by_name Mash
     def load_cookbooks
-      cookbook_loaders.each_key do |cookbook_name|
+      cookbook_version_loaders.each_key do |cookbook_name|
         load_cookbook(cookbook_name)
       end
       cookbooks_by_name
     end
 
+    # Loads a single cookbook by its name.
+    #
+    # @param [String]
+    # @return [Chef::CookbookVersion]
     def load_cookbook(cookbook_name)
-      return nil unless cookbook_loaders.key?(cookbook_name)
+      unless cookbook_version_loaders.key?(cookbook_name)
+        raise Exceptions::CookbookNotFoundInRepo, "Cannot find a cookbook named #{cookbook_name}; did you forget to add metadata to a cookbook? (https://docs.chef.io/config_rb_metadata.html)"
+      end
 
       return cookbooks_by_name[cookbook_name] if cookbooks_by_name.key?(cookbook_name)
 
-      loader = cookbook_loaders[cookbook_name]
+      loader = cookbook_version_loaders[cookbook_name]
 
       loader.load
 
@@ -71,11 +101,7 @@ class Chef
     end
 
     def [](cookbook)
-      if cookbooks_by_name.key?(cookbook.to_sym) || load_cookbook(cookbook.to_sym)
-        cookbooks_by_name[cookbook.to_sym]
-      else
-        raise Exceptions::CookbookNotFoundInRepo, "Cannot find a cookbook named #{cookbook}; did you forget to add metadata to a cookbook? (https://docs.chef.io/config_rb_metadata.html)"
-      end
+      load_cookbook(cookbook)
     end
 
     alias :fetch :[]
@@ -113,6 +139,11 @@ class Chef
 
     private
 
+    # Helper method to lazily create and remember the chefignore object
+    # for a given repo_path.
+    #
+    # @param [String] repo_path the full path to the cookbook directory of the repo
+    # @return [Chef::Cookbook::Chefignore] the chefignore object for the repo_path
     def chefignore(repo_path)
       @chefignores ||= {}
       @chefignores[repo_path] ||= Cookbook::Chefignore.new(repo_path)
@@ -126,14 +157,17 @@ class Chef
     def all_files_in_repo_paths
       @all_files_in_repo_paths ||=
         begin
-          @repo_paths.inject([]) do |all_children, repo_path|
+          repo_paths.inject([]) do |all_children, repo_path|
             all_children + Dir[File.join(Chef::Util::PathHelper.escape_glob_dir(repo_path), "*")]
           end
         end
     end
 
-    def cookbook_loaders
-      @cookbook_loaders ||=
+    # This method creates a Mash of the CookbookVersionLoaders for each cookbook.
+    #
+    # @return [Mash<String, Cookbook::CookbookVersionLoader>]
+    def cookbook_version_loaders
+      @cookbook_version_loaders ||=
         begin
           mash = Mash.new
           all_directories_in_repo_paths.each do |cookbook_path|
diff --git a/spec/unit/cookbook/cookbook_version_loader_spec.rb b/spec/unit/cookbook/cookbook_version_loader_spec.rb
index 40a054abee..c38af059f5 100644
--- a/spec/unit/cookbook/cookbook_version_loader_spec.rb
+++ b/spec/unit/cookbook/cookbook_version_loader_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Daniel DeLeo (<dan@chef.io>)
-# Copyright:: Copyright 2014-2016, Chef Software, Inc.
+# Copyright:: Copyright 2014-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -173,7 +173,7 @@ describe Chef::Cookbook::CookbookVersionLoader do
       let(:cookbook_path) { File.join(CHEF_SPEC_DATA, "incomplete-metadata-chef-repo/incomplete-metadata") }
 
       let(:error_message) do
-        "Cookbook loaded at path(s) [#{cookbook_path}] has invalid metadata: The `name' attribute is required in cookbook metadata"
+        "Cookbook loaded at path [#{cookbook_path}] has invalid metadata: The `name' attribute is required in cookbook metadata"
       end
 
       it "raises an error when loading with #load!" do
diff --git a/spec/unit/cookbook_loader_spec.rb b/spec/unit/cookbook_loader_spec.rb
index 7c15cc9030..ddb4f00f35 100644
--- a/spec/unit/cookbook_loader_spec.rb
+++ b/spec/unit/cookbook_loader_spec.rb
@@ -196,7 +196,9 @@ describe Chef::CookbookLoader do
     end
 
     it "should not load the cookbook again when accessed" do
-      expect(cookbook_loader).not_to receive("load_cookbook")
+      cookbook_loader.send(:cookbook_version_loaders).each do |cbv_loader|
+        expect(cbv_loader).not_to receive(:load)
+      end
       cookbook_loader["openldap"]
     end