diff --git a/src/supermarket/Gemfile b/src/supermarket/Gemfile index 84f3db258..bdeabeb4f 100644 --- a/src/supermarket/Gemfile +++ b/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' diff --git a/src/supermarket/Gemfile.lock b/src/supermarket/Gemfile.lock index ae088a4e6..8fa955e75 100644 --- a/src/supermarket/Gemfile.lock +++ b/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) diff --git a/src/supermarket/app/lib/active_model/validations/chef_version_validator.rb b/src/supermarket/app/lib/active_model/validations/chef_version_validator.rb new file mode 100644 index 000000000..2e80494f6 --- /dev/null +++ b/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 diff --git a/src/supermarket/app/models/cookbook.rb b/src/supermarket/app/models/cookbook.rb index 76f3c473c..ae405d2eb 100644 --- a/src/supermarket/app/models/cookbook.rb +++ b/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] 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 diff --git a/src/supermarket/app/models/cookbook_version.rb b/src/supermarket/app/models/cookbook_version.rb index ab7ea71d6..602a1dc41 100644 --- a/src/supermarket/app/models/cookbook_version.rb +++ b/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 diff --git a/src/supermarket/spec/models/cookbook_upload/parameters_spec.rb b/src/supermarket/spec/models/cookbook_upload/parameters_spec.rb index bf8bc4317..7867d5405 100644 --- a/src/supermarket/spec/models/cookbook_upload/parameters_spec.rb +++ b/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) diff --git a/src/supermarket/spec/models/cookbook_upload_spec.rb b/src/supermarket/spec/models/cookbook_upload_spec.rb index 4b1025743..5cff02ab5 100644 --- a/src/supermarket/spec/models/cookbook_upload_spec.rb +++ b/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) diff --git a/src/supermarket/spec/models/cookbook_version_spec.rb b/src/supermarket/spec/models/cookbook_version_spec.rb index 74528d8f6..aecd2ba26 100644 --- a/src/supermarket/spec/models/cookbook_version_spec.rb +++ b/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 diff --git a/src/supermarket/spec/support/api_spec_helpers.rb b/src/supermarket/spec/support/api_spec_helpers.rb index 66d3bb8f7..1c4ba5690 100644 --- a/src/supermarket/spec/support/api_spec_helpers.rb +++ b/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