Merge pull request #1715 from chef/robb/validate-cookbook-version-aga…

…inst-chef

Fix version validation against chef's required format

src/supermarket/Gemfile

@@ -43,7 +43,6 @@ gem 'rinku', require: 'rails_rinku'
 gem 'rollout'
 gem 'sass-globbing'
 gem 'sass-rails'
-gem 'semverse'
 gem 'sentry-raven', require: false
 gem 'sitemap_generator'
 gem 'sprockets'
@@ -67,7 +66,6 @@ group :development do
   gem 'guard-rspec', require: false
   gem 'guard-rubocop', require: false
   gem 'license_finder'
-  gem 'pry-rails'
   gem 'spring'
   gem 'spring-commands-rspec'
 end
@@ -92,6 +90,7 @@ group :development, :test do
   gem 'byebug'
   gem 'launchy'
   gem 'mail_view'
+  gem 'pry-rails'
   gem 'rspec-rails'
   gem 'rubocop', '>= 0.23.0'
 

src/supermarket/Gemfile.lock

@@ -524,7 +524,6 @@ GEM
     sawyer (0.8.1)
       addressable (>= 2.3.5, < 2.6)
       faraday (~> 0.8, < 1.0)
-    semverse (2.0.0)
     sentry-raven (2.4.0)
       faraday (>= 0.7.6, < 1.0)
     serverspec (2.38.1)
@@ -670,7 +669,6 @@ DEPENDENCIES
   rubocop (>= 0.23.0)
   sass-globbing
   sass-rails
-  semverse
   sentry-raven
   shoulda-matchers (~> 2.8)
   sidekiq (~> 4.2)

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.rb

@@ -1,3 +1,5 @@
+require 'chef/version_class'
+
 class Cookbook < ApplicationRecord
   include PgSearch
   include Badgeable
@@ -135,7 +137,7 @@ def self.total_download_count
   # @return [Array<CookbookVersion>] the sorted CookbookVersion records
   #
   def sorted_cookbook_versions
-    @sorted_cookbook_versions ||= cookbook_versions.sort_by { |v| Semverse::Version.new(v.version) }.reverse
+    @sorted_cookbook_versions ||= cookbook_versions.sort_by { |v| Chef::Version.new(v.version) }.reverse
   end
 
   #
@@ -335,7 +337,7 @@ def contingents
                       .sort_by do |cd|
       [
         cd.cookbook_version.cookbook.name,
-        Semverse::Version.new(cd.cookbook_version.version)
+        Chef::Version.new(cd.cookbook_version.version)
       ]
     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/parameters_spec.rb

@@ -38,18 +38,13 @@ def params(hash)
     end
 
     it 'is extracted from the top-level metadata.json' do
-      tarball = Tempfile.new('multiple-metadata', 'tmp').tap do |file|
-        io = AndFeathers.build('multiple-metadata') do |base|
-          base.file('metadata.json') do
-            JSON.dump(name: 'multiple')
-          end
-          base.file('PaxHeader/metadata.json') do
-            JSON.dump(name: 'PaxHeader-multiple')
-          end
-        end.to_io(AndFeathers::GzippedTarball, :reverse_each)
-
-        file.write(io.read)
-        file.rewind
+      tarball = build_cookbook_tarball('multiple-metadata') do |base|
+        base.file('metadata.json') do
+          JSON.dump(name: 'multiple')
+        end
+        base.file('PaxHeader/metadata.json') do
+          JSON.dump(name: 'PaxHeader-multiple')
+        end
       end
 
       params = params(cookbook: '{}', tarball: tarball)
@@ -99,18 +94,13 @@ def params(hash)
     end
 
     it 'is extracted from the top-level README' do
-      tarball = Tempfile.new('multiple-readme', 'tmp').tap do |file|
-        io = AndFeathers.build('multiple-readme') do |base|
-          base.file('metadata.json') { JSON.dump(name: 'multiple-readme') }
-          base.file('README') { 'readme' }
-          base.file('PaxHeader/metadata.json') do
-            JSON.dump(name: 'multiple-readme')
-          end
-          base.file('PaxHeader/README') { 'impostor readme' }
-        end.to_io(AndFeathers::GzippedTarball, :reverse_each)
-
-        file.write(io.read)
-        file.rewind
+      tarball = build_cookbook_tarball('multiple-readme') do |base|
+        base.file('metadata.json') { JSON.dump(name: 'multiple-readme') }
+        base.file('README') { 'readme' }
+        base.file('PaxHeader/metadata.json') do
+          JSON.dump(name: 'multiple-readme')
+        end
+        base.file('PaxHeader/README') { 'impostor readme' }
       end
 
       params = params(cookbook: '{}', tarball: tarball)

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 }
@@ -199,13 +212,8 @@
     end
 
     it 'yields an error if the metadata.json has a malformed platforms hash' do
-      tarball = Tempfile.new('bad_platforms', 'tmp').tap do |file|
-        io = AndFeathers.build('cookbook') do |cookbook|
-          cookbook.file('metadata.json') { JSON.dump(platforms: '') }
-        end.to_io(AndFeathers::GzippedTarball)
-
-        file.write(io.read)
-        file.rewind
+      tarball = build_cookbook_tarball('bad_platforms') do |tar|
+        tar.file('metadata.json') { JSON.dump(name: 'bad_platforms', platforms: '') }
       end
 
       upload = CookbookUpload.new(user, cookbook: '{}', tarball: tarball)
@@ -216,13 +224,8 @@
     end
 
     it 'yields an error if the metadata.json has a malformed dependencies hash' do
-      tarball = Tempfile.new('bad_dependencies', 'tmp').tap do |file|
-        io = AndFeathers.build('cookbook') do |cookbook|
-          cookbook.file('metadata.json') { JSON.dump(dependencies: '') }
-        end.to_io(AndFeathers::GzippedTarball)
-
-        file.write(io.read)
-        file.rewind
+      tarball = build_cookbook_tarball('bad_dependencies') do |tar|
+        tar.file('metadata.json') { JSON.dump(name: 'bad_dependencies', dependencies: '') }
       end
 
       upload = CookbookUpload.new(user, cookbook: '{}', tarball: tarball)

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

src/supermarket/spec/support/api_spec_helpers.rb

@@ -1,6 +1,4 @@
-require 'and_feathers'
-require 'and_feathers/gzipped_tarball'
-require 'tempfile'
+require_relative 'tarball_helpers'
 require 'mixlib/authentication/signedheaderauth'
 
 module ApiSpecHelpers
@@ -140,16 +138,11 @@ def cookbook_upload(cookbook_name, opts = {})
           }
         }.merge(custom_metadata)
 
-        tarball = Tempfile.new([cookbook_name, '.tgz'], 'tmp').tap do |file|
-          io = AndFeathers.build(cookbook_name) do |base_dir|
-            base_dir.file('README.md') { '# README' }
-            base_dir.file('metadata.json') do
-              JSON.dump(metadata)
-            end
-          end.to_io(AndFeathers::GzippedTarball)
-
-          file.write(io.read)
-          file.rewind
+        tarball = build_cookbook_tarball(cookbook_name) do |base_dir|
+          base_dir.file('README.md') { '# README' }
+          base_dir.file('metadata.json') do
+            JSON.dump(metadata)
+          end
         end
       end