Merge pull request #5622 from chef/chef-ad/3888

Knife: cookbook site share doesn't work with some json metadata

2 files changed, 118 insertions, 115 deletions

diff --git a/lib/chef/cookbook/metadata.rb b/lib/chef/cookbook/metadata.rb
index 02f9831d70..b2f3db9b8e 100644
--- a/lib/chef/cookbook/metadata.rb
+++ b/lib/chef/cookbook/metadata.rb
@@ -641,8 +641,8 @@ class Chef
         VERSION_CONSTRAINTS.each do |dependency_type, hash_key|
           if dependency_group = o[hash_key]
             dependency_group.each do |cb_name, constraints|
-              if metadata.respond_to?(method_name)
-                metadata.public_send(method_name, cb_name, *Array(constraints))
+              if metadata.respond_to?(dependency_type)
+                metadata.public_send(dependency_type, cb_name, *Array(constraints))
               end
             end
           end
diff --git a/spec/unit/knife/cookbook_metadata_spec.rb b/spec/unit/knife/cookbook_metadata_spec.rb
index 4b405d0842..c19fc5ae2d 100644
--- a/spec/unit/knife/cookbook_metadata_spec.rb
+++ b/spec/unit/knife/cookbook_metadata_spec.rb
@@ -19,63 +19,96 @@
 require "spec_helper"
 
 describe Chef::Knife::CookbookMetadata do
+  let(:knife) do
+    knife = Chef::Knife::CookbookMetadata.new
+    knife.name_args = ["foobar"]
+    knife
+  end
+
+  let(:cookbook_dir) { Dir.mktmpdir }
+
+  let(:stdout) { StringIO.new }
+
+  let(:stderr) { StringIO.new }
+
   before(:each) do
-    @knife = Chef::Knife::CookbookMetadata.new
-    @knife.name_args = ["foobar"]
-    @cookbook_dir = Dir.mktmpdir
-    @json_data = '{ "version": "1.0.0" }'
-    @stdout = StringIO.new
-    @stderr = StringIO.new
-    allow(@knife.ui).to receive(:stdout).and_return(@stdout)
-    allow(@knife.ui).to receive(:stderr).and_return(@stderr)
+    allow(knife.ui).to receive(:stdout).and_return(stdout)
+    allow(knife.ui).to receive(:stderr).and_return(stderr)
+  end
+
+  def create_metadata_rb(**kwargs)
+    name = kwargs[:name]
+    Dir.mkdir("#{cookbook_dir}/#{name}")
+    File.open("#{cookbook_dir}/#{name}/metadata.rb", "w+") do |f|
+      kwargs.each do |key, value|
+        if value.is_a?(Array)
+          f.puts "#{key} #{value.map { |v| "\"#{v}\"" }.join(", ")}"
+        else
+          f.puts "#{key} \"#{value}\""
+        end
+      end
+    end
+  end
+
+  def create_metadata_json(**kwargs)
+    name = kwargs[:name]
+    Dir.mkdir("#{cookbook_dir}/#{name}")
+    File.open("#{cookbook_dir}/#{name}/metadata.json", "w+") do |f|
+      f.write(FFI_Yajl::Encoder.encode(kwargs))
+    end
+  end
+
+  def create_invalid_json
+    Dir.mkdir("#{cookbook_dir}/foobar")
+    File.open("#{cookbook_dir}/foobar/metadata.json", "w+") do |f|
+      f.write <<-EOH
+      { "version": "1.0.0", {ImInvalid}}
+      EOH
+    end
   end
 
   describe "run" do
     it "should print an error and exit if a cookbook name was not provided" do
-      @knife.name_args = []
-      expect(@knife.ui).to receive(:error).with(/you must specify the cookbook.+use the --all/i)
-      expect { @knife.run }.to raise_error(SystemExit)
+      knife.name_args = []
+      expect(knife.ui).to receive(:error).with(/you must specify the cookbook.+use the --all/i)
+      expect { knife.run }.to raise_error(SystemExit)
     end
 
     it "should print an error and exit if an empty cookbook name was provided" do
-      @knife.name_args = [""]
-      expect(@knife.ui).to receive(:error).with(/you must specify the cookbook.+use the --all/i)
-      expect { @knife.run }.to raise_error(SystemExit)
+      knife.name_args = [""]
+      expect(knife.ui).to receive(:error).with(/you must specify the cookbook.+use the --all/i)
+      expect { knife.run }.to raise_error(SystemExit)
     end
 
     it "should generate the metadata for the cookbook" do
-      expect(@knife).to receive(:generate_metadata).with("foobar")
-      @knife.run
+      expect(knife).to receive(:generate_metadata).with("foobar")
+      knife.run
     end
 
     describe "with -a or --all" do
       before(:each) do
-        @knife.config[:all] = true
-        @foo = Chef::CookbookVersion.new("foo", "/tmp/blah")
-        @foo.version = "1.0.0"
-        @bar = Chef::CookbookVersion.new("bar", "/tmp/blah")
-        @bar.version = "2.0.0"
-        @cookbook_loader = {
-          "foo" => @foo,
-          "bar" => @bar,
-        }
-        expect(@cookbook_loader).to receive(:load_cookbooks).and_return(@cookbook_loader)
-        expect(@knife).to receive(:generate_metadata).with("foo")
-        expect(@knife).to receive(:generate_metadata).with("bar")
+        Chef::Config[:cookbook_path] = cookbook_dir
+        knife.config[:all] = true
+        create_metadata_rb(name: "foo", version: "1.0.0")
+        create_metadata_rb(name: "bar", version: "2.0.0")
+        expect(knife).to receive(:generate_metadata).with("foo").and_call_original
+        expect(knife).to receive(:generate_metadata).with("bar").and_call_original
       end
 
       it "should generate the metadata for each cookbook" do
-        Chef::Config[:cookbook_path] = @cookbook_dir
-        expect(Chef::CookbookLoader).to receive(:new).with(@cookbook_dir).and_return(@cookbook_loader)
-        @knife.run
+        expect(Chef::CookbookLoader).to receive(:new).with(cookbook_dir).and_call_original
+        knife.run
+        expect(stderr.string).to match /generating metadata for foo from #{cookbook_dir}\/foo\/metadata\.rb/im
+        expect(stderr.string).to match /generating metadata for bar from #{cookbook_dir}\/bar\/metadata\.rb/im
       end
 
-      describe "and with -o or --cookbook-path" do
-        it "should look in the provided path and generate cookbook metadata" do
-          @knife.config[:cookbook_path] = "/opt/chef/cookbooks"
-          expect(Chef::CookbookLoader).to receive(:new).with("/opt/chef/cookbooks").and_return(@cookbook_loader)
-          @knife.run
-        end
+      it "with -o or --cookbook_path should look in the provided path and generate cookbook metadata" do
+        Chef::Config[:cookbook_path] = "/dev/null"
+        knife.config[:cookbook_path] = cookbook_dir
+        expect(Chef::CookbookLoader).to receive(:new).with(cookbook_dir).and_call_original
+        knife.run
+        expect(stderr.string).to match /generating metadata for foo from #{cookbook_dir}\/foo\/metadata\.rb/im
+        expect(stderr.string).to match /generating metadata for bar from #{cookbook_dir}\/bar\/metadata\.rb/im
       end
     end
 
@@ -83,97 +116,67 @@ describe Chef::Knife::CookbookMetadata do
 
   describe "generate_metadata" do
     before(:each) do
-      @knife.config[:cookbook_path] = @cookbook_dir
-      allow(File).to receive(:expand_path).with("#{@cookbook_dir}/foobar/metadata.rb").
-        and_return("#{@cookbook_dir}/foobar/metadata.rb")
+      Chef::Config[:cookbook_path] = cookbook_dir
     end
 
     it "should generate the metadata from metadata.rb if it exists" do
-      expect(File).to receive(:exists?).with("#{@cookbook_dir}/foobar/metadata.rb").
-        and_return(true)
-      expect(@knife).to receive(:generate_metadata_from_file).with("foobar", "#{@cookbook_dir}/foobar/metadata.rb")
-      @knife.run
+      create_metadata_rb(name: "foobar", version: "1.0.0")
+      expect(knife).to receive(:generate_metadata_from_file).with("foobar", "#{cookbook_dir}/foobar/metadata.rb").and_call_original
+      knife.run
+      expect(File.exist?("#{cookbook_dir}/foobar/metadata.json")).to be true
+      json = FFI_Yajl::Parser.parse(IO.read("#{cookbook_dir}/foobar/metadata.json"))
+      expect(json["name"]).to eql("foobar")
+      expect(json["version"]).to eql("1.0.0")
     end
 
     it "should validate the metadata json if metadata.rb does not exist" do
-      expect(File).to receive(:exists?).with("#{@cookbook_dir}/foobar/metadata.rb").
-        and_return(false)
-      expect(@knife).to receive(:validate_metadata_json).with(@cookbook_dir, "foobar")
-      @knife.run
+      create_metadata_json(name: "foobar", version: "1.0.0")
+      expect(knife).to receive(:validate_metadata_json).with(cookbook_dir, "foobar").and_call_original
+      knife.run
     end
   end
 
-  describe "generate_metadata_from_file" do
+  describe "validation errors" do
     before(:each) do
-      @metadata_mock = double("metadata")
-      @json_file_mock = double("json_file")
-    end
-
-    it "should generate the metatdata json from metatdata.rb" do
-      allow(Chef::Cookbook::Metadata).to receive(:new).and_return(@metadata_mock)
-      expect(@metadata_mock).to receive(:name).with("foobar")
-      expect(@metadata_mock).to receive(:from_file).with("#{@cookbook_dir}/foobar/metadata.rb")
-      expect(File).to receive(:open).with("#{@cookbook_dir}/foobar/metadata.json", "w").
-        and_yield(@json_file_mock)
-      expect(@json_file_mock).to receive(:write).with(@json_data)
-      expect(Chef::JSONCompat).to receive(:to_json_pretty).with(@metadata_mock).
-        and_return(@json_data)
-      @knife.generate_metadata_from_file("foobar", "#{@cookbook_dir}/foobar/metadata.rb")
-      expect(@stderr.string).to match /generating metadata for foobar from #{@cookbook_dir}\/foobar\/metadata\.rb/im
-    end
-
-    { Chef::Exceptions::ObsoleteDependencySyntax => "obsolote dependency",
-      Chef::Exceptions::InvalidVersionConstraint => "invalid version constraint",
-    }.each_pair do |klass, description|
-      it "should print an error and exit when an #{description} syntax exception is encountered" do
-        exception = klass.new("#{description} blah")
-        allow(Chef::Cookbook::Metadata).to receive(:new).and_raise(exception)
-        expect do
-          @knife.generate_metadata_from_file("foobar", "#{@cookbook_dir}/foobar/metadata.rb")
-        end.to raise_error(SystemExit)
-        expect(@stderr.string).to match /error: the cookbook 'foobar' contains invalid or obsolete metadata syntax/im
-        expect(@stderr.string).to match /in #{@cookbook_dir}\/foobar\/metadata\.rb/im
-        expect(@stderr.string).to match /#{description} blah/im
-      end
+      Chef::Config[:cookbook_path] = cookbook_dir
     end
-  end
 
-  describe "validate_metadata_json" do
-    it "should validate the metadata json" do
-      expect(File).to receive(:exist?).with("#{@cookbook_dir}/foobar/metadata.json").
-        and_return(true)
-      expect(IO).to receive(:read).with("#{@cookbook_dir}/foobar/metadata.json").
-        and_return(@json_data)
-      expect(Chef::Cookbook::Metadata).to receive(:validate_json).with(@json_data)
-      @knife.validate_metadata_json(@cookbook_dir, "foobar")
+    it "should fail for obsolete operators in metadata.rb" do
+      create_metadata_rb(name: "foobar", version: "1.0.0", depends: [ "foo:bar", ">> 0.2" ])
+      expect(Chef::Cookbook::Metadata).not_to receive(:validate_json)
+      expect { knife.run }.to raise_error(SystemExit)
+      expect(stderr.string).to match /error: the cookbook 'foobar' contains invalid or obsolete metadata syntax/im
     end
 
-    it "should not try to validate the metadata json if the file does not exist" do
-      expect(File).to receive(:exist?).with("#{@cookbook_dir}/foobar/metadata.json").
-        and_return(false)
-      expect(IO).not_to receive(:read)
+    it "should fail for obsolete format in metadata.rb (sadly)" do
+      create_metadata_rb(name: "foobar", version: "1.0.0", depends: [ "foo:bar", "> 0.2", "< 1.0" ])
       expect(Chef::Cookbook::Metadata).not_to receive(:validate_json)
-      @knife.validate_metadata_json(@cookbook_dir, "foobar")
-    end
-
-    { Chef::Exceptions::ObsoleteDependencySyntax => "obsolote dependency",
-      Chef::Exceptions::InvalidVersionConstraint => "invalid version constraint",
-    }.each_pair do |klass, description|
-      it "should print an error and exit when an #{description} syntax exception is encountered" do
-        expect(File).to receive(:exist?).with("#{@cookbook_dir}/foobar/metadata.json").
-          and_return(true)
-        expect(IO).to receive(:read).with("#{@cookbook_dir}/foobar/metadata.json").
-          and_return(@json_data)
-        exception = klass.new("#{description} blah")
-        allow(Chef::Cookbook::Metadata).to receive(:validate_json).and_raise(exception)
-        expect do
-          @knife.validate_metadata_json(@cookbook_dir, "foobar")
-        end.to raise_error(SystemExit)
-        expect(@stderr.string).to match /error: the cookbook 'foobar' contains invalid or obsolete metadata syntax/im
-        expect(@stderr.string).to match /in #{@cookbook_dir}\/foobar\/metadata\.json/im
-        expect(@stderr.string).to match /#{description} blah/im
-      end
+      expect { knife.run }.to raise_error(SystemExit)
+      expect(stderr.string).to match /error: the cookbook 'foobar' contains invalid or obsolete metadata syntax/im
+    end
+
+    it "should fail for obsolete operators in metadata.json" do
+      create_metadata_json(name: "foobar", version: "1.0.0", dependencies: { "foo:bar" => ">> 0.2" })
+      expect { knife.run }.to raise_error(SystemExit)
+      expect(stderr.string).to match /error: the cookbook 'foobar' contains invalid or obsolete metadata syntax/im
     end
-  end
 
+    it "should not fail for unknown field in metadata.rb" do
+      create_metadata_rb(name: "sounders", version: "2.0.0", beats: "toronto")
+      expect(Chef::Cookbook::Metadata).not_to receive(:validate_json)
+      expect { knife.run }.not_to raise_error
+      expect(stderr.string).to eql("")
+    end
+
+    it "should not fail for unknown field in metadata.json" do
+      create_metadata_json(name: "sounders", version: "2.0.0", beats: "toronto")
+      expect { knife.run }.not_to raise_error
+      expect(stderr.string).to eql("")
+    end
+
+    it "should fail on unparsable json" do
+      create_invalid_json
+      expect { knife.run }.to raise_error(Chef::Exceptions::JSON::ParseError)
+    end
+  end
 end