validate a cookbook version against chef's required format

A version for a cookbook must be either X.Y.Z or X.Y.[1] Up until this
change, the validation for a CookbookVersion's version was against
SemVer rules. SemVer allows for release labels or build info to appear
after the version number itself. Chef does not!

Here, we add a new ChefVersionValidator that reuses the existing version
validation logic of the Chef gem in ::Chef::Version. Supermarket is
already relying on the Chef gem to validate version constraints set for
cookbook dependencies and supported platform.

Fixes #1714 to return an error to any client if a cookbook is uploaded
with an invalid version.

[1] https://docs.chef.io/cookbook_versions.html#constraints

Signed-off-by: Robb Kidd <[email protected]>

src/supermarket/app/lib/active_model/validations/chef_version_validator.rb

@@ -0,0 +1,43 @@
+require 'active_model'
+require 'chef/version_class'
+require 'chef/exceptions'
+
+module ActiveModel
+  module Validations
+    #
+    # Validates that strings are formatted as Chef version constraints.
+    #
+    class ChefVersionValidator < ActiveModel::EachValidator
+      #
+      # Create a new validator. Called implicitly when
+      # a +chef_version+ validation is added to an attribute.
+      #
+      # @param options [Hash] validation options
+      # @option options [String] :message
+      #   ('is not a valid Chef version') a custom validation message
+      #
+      def initialize(options)
+        options.fetch(:message) do
+          options.store(:message, 'is not a valid Chef version')
+        end
+
+        super(options)
+      end
+
+      #
+      # Add an error to +attribute+ of +record+ if the given +value+ is not
+      # a valid Chef version constraint
+      #
+      # @param record [ActiveModel::Model]
+      # @param attribute [Symbol]
+      # @param value
+      #
+      def validate_each(record, attribute, value)
+        Chef::Version.new(value)
+      rescue Chef::Exceptions::InvalidCookbookVersion => e
+        msg = "#{options.fetch(:message)}. #{e.class}: #{e.message}"
+        record.errors.add(attribute, msg, value: value)
+      end
+    end
+  end
+end

src/supermarket/app/models/cookbook_version.rb

@@ -1,3 +1,5 @@
+require 'active_model/validations/chef_version_validator'
+
 class CookbookVersion < ApplicationRecord
   include SeriousErrors
 
@@ -20,8 +22,7 @@ class CookbookVersion < ApplicationRecord
   validates :license, presence: true, length: { maximum: 255 }
   validates :description, presence: true
   validates :readme, presence: true
-  validates :version, presence: true, uniqueness: { scope: :cookbook }
-  validate :semantic_version
+  validates :version, presence: true, uniqueness: { scope: :cookbook }, chef_version: true
   validates_attachment(
     :tarball,
     presence: true,
@@ -95,18 +96,4 @@ def metric_result_pass_rate
       '-'
     end
   end
-
-  private
-
-  #
-  # Ensure that the version string we have been given conforms to semantic
-  # versioning at http://semver.org
-  #
-  def semantic_version
-    begin
-      Semverse::Version.new(version)
-    rescue Semverse::InvalidVersionFormat
-      errors.add(:version, 'is formatted incorrectly')
-    end
-  end
 end

src/supermarket/spec/models/cookbook_upload_spec.rb

@@ -122,6 +122,19 @@
       expect(version).to be_present
     end
 
+    it 'yields an error if the version number is not a valid Chef version' do
+      tarball = build_cookbook_tarball('invalid_version') do |tar|
+        tar.file('metadata.json') { JSON.dump(name: 'invalid_version', version: '1.2.3-rc4') }
+        tar.file('README.md') { "# Check for a bad version" }
+      end
+
+      upload = CookbookUpload.new(user, cookbook: '{}' , tarball: tarball)
+      errors = upload.finish { |e, _| e }
+
+      expect(errors.full_messages).
+        to include( a_string_matching('not a valid Chef version') )
+    end
+
     it 'yields an error if the cookbook is not valid JSON' do
       upload = CookbookUpload.new(user, cookbook: 'ack!', tarball: 'tarball')
       errors = upload.finish { |e, _| e }

src/supermarket/spec/models/cookbook_version_spec.rb

@@ -11,37 +11,49 @@
     it { should validate_presence_of(:description) }
     it { should validate_presence_of(:version) }
     it { should validate_presence_of(:readme) }
-    it "validates uniqueness of version for a cookbook" do
-      cookbook = create(:cookbook)
-      create(:cookbook_version, cookbook: cookbook)
-      should validate_uniqueness_of(:version).scoped_to(:cookbook_id)
-    end
     it { should validate_length_of(:license).is_at_most(255) }
 
-    it 'seriously validates the uniqueness of cookbook version numbers' do
-      cookbook = create(:cookbook)
-      cookbook_version = create(:cookbook_version, cookbook: cookbook)
+    context 'for version number' do
+      let(:cookbook) { create(:cookbook) }
+      let(:cookbook_version) { create(:cookbook_version, cookbook: cookbook) }
 
-      duplicate_version = CookbookVersion.new(
-        cookbook: cookbook,
-        license: cookbook_version.license,
-        tarball: cookbook_version.tarball,
-        version: cookbook_version.version
-      )
+      it 'passes for a valid version number format x.y.z' do
+        cookbook_version.version = '1.2.3'
+        expect(cookbook_version).to be_valid
+        expect(cookbook_version.errors[:version]).to be_empty
+      end
 
-      expect do
-        duplicate_version.save(validate: false)
-      end.to raise_error(ActiveRecord::RecordNotUnique)
-    end
+      it "validates uniqueness of version for a cookbook" do
+        cookbook_version
+        should validate_uniqueness_of(:version).scoped_to(:cookbook_id)
+      end
 
-    it 'validates that the version number is semantically correct' do
-      cookbook = create(:cookbook)
-      cookbook_version = build(:cookbook_version, cookbook: cookbook, version: 'hahano')
-      expect(cookbook_version).to_not be_valid
-      expect(cookbook_version.errors[:version]).to_not be_empty
-      cookbook_version.version = '8.7.6'
-      expect(cookbook_version).to be_valid
-      expect(cookbook_version.errors[:version]).to be_empty
+      it 'seriously validates the uniqueness of cookbook version numbers' do
+        duplicate_version = CookbookVersion.new(
+          cookbook: cookbook,
+          license: cookbook_version.license,
+          tarball: cookbook_version.tarball,
+          version: cookbook_version.version
+        )
+
+        expect do
+          duplicate_version.save(validate: false)
+        end.to raise_error(ActiveRecord::RecordNotUnique)
+      end
+
+      context 'if not a valid chef version' do
+        it 'fails on just a string of silliness' do
+          cookbook_version.version = 'hahahano'
+          expect(cookbook_version).to_not be_valid
+          expect(cookbook_version.errors[:version]).to include( a_string_matching('not a valid Chef version') )
+        end
+
+        it 'fails on a dashed extra info like RC or pre' do
+          cookbook_version.version = '1.2.3-RC1'
+          expect(cookbook_version).to_not be_valid
+          expect(cookbook_version.errors[:version]).to include( a_string_matching('not a valid Chef version') )
+        end
+      end
     end
 
     context 'when something that is not a tarball is given' do