Merge branch '37335-conan-name-validation-fixes' into 'master'

Revert revert of "Update naming validation for Conan recipes"

Closes #37335

See merge request gitlab-org/gitlab!24692

changelogs/unreleased/37335-conan-name-validation-fixes.yml

+---
+title: Conan packages are validated based on full recipe instead of name/version alone
+merge_request: 24692
+author:
+type: changed

db/migrate/20200207182131_replace_conan_metadata_index.rb

+# frozen_string_literal: true
+
+class ReplaceConanMetadataIndex < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  OLD_INDEX = 'index_packages_conan_metadata_on_package_id'
+  NEW_INDEX = 'index_packages_conan_metadata_on_package_id_username_channel'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :packages_conan_metadata,
+                         [:package_id, :package_username, :package_channel],
+                         unique: true, name: NEW_INDEX
+
+    remove_concurrent_index_by_name :packages_conan_metadata, OLD_INDEX
+  end
+
+  def down
+    add_concurrent_index :packages_conan_metadata, :package_id, name: OLD_INDEX
+
+    remove_concurrent_index_by_name :packages_conan_metadata, NEW_INDEX
+  end
+end

db/schema.rb

@@ -2958,7 +2958,7 @@ ActiveRecord::Schema.define(version: 2020_02_12_052620) do
     t.datetime_with_timezone "updated_at", null: false
     t.string "package_username", limit: 255, null: false
     t.string "package_channel", limit: 255, null: false
-    t.index ["package_id"], name: "index_packages_conan_metadata_on_package_id", unique: true
+    t.index ["package_id", "package_username", "package_channel"], name: "index_packages_conan_metadata_on_package_id_username_channel", unique: true
   end
 
   create_table "packages_dependencies", force: :cascade do |t|

ee/app/models/packages/package.rb

@@ -23,8 +23,9 @@ class Packages::Package < ApplicationRecord
     format: { with: Gitlab::Regex.package_name_regex }
 
   validates :name,
-    uniqueness: { scope: %i[project_id version package_type] }
+    uniqueness: { scope: %i[project_id version package_type] }, unless: :conan?
 
+  validate :valid_conan_package_recipe, if: :conan?
   validate :valid_npm_package_name, if: :npm?
   validate :package_already_taken, if: :npm?
 
@@ -39,6 +40,10 @@ class Packages::Package < ApplicationRecord
     joins(:conan_metadatum).where(packages_conan_metadata: { package_channel: package_channel })
   end
 
+  scope :with_conan_username, ->(package_username) do
+    joins(:conan_metadatum).where(packages_conan_metadata: { package_username: package_username })
+  end
+
   scope :has_version, -> { where.not(version: nil) }
   scope :processed, -> do
     where.not(package_type: :nuget).or(
@@ -104,6 +109,20 @@ class Packages::Package < ApplicationRecord
 
   private
 
+  def valid_conan_package_recipe
+    recipe_exists = project.packages
+                           .conan
+                           .includes(:conan_metadatum)
+                           .with_name(name)
+                           .with_version(version)
+                           .with_conan_channel(conan_metadatum.package_channel)
+                           .with_conan_username(conan_metadatum.package_username)
+                           .id_not_in(id)
+                           .exists?
+
+    errors.add(:base, 'Package recipe already exists') if recipe_exists
+  end
+
   def valid_npm_package_name
     return unless project&.root_namespace
 

ee/spec/factories/packages.rb

@@ -66,6 +66,10 @@ FactoryBot.define do
         create :conan_package_file, :conan_package_manifest, package: package
         create :conan_package_file, :conan_package, package: package
       end
+
+      trait(:without_loaded_metadatum) do
+        conan_metadatum { build(:conan_metadatum, package: nil) }
+      end
     end
   end
 
@@ -187,7 +191,7 @@ FactoryBot.define do
   end
 
   factory :conan_metadatum, class: 'Packages::ConanMetadatum' do
-    association :package, package_type: :conan
+    association :package, factory: [:conan_package, :without_loaded_metadatum]
     package_username { 'username' }
     package_channel { 'stable' }
   end

ee/spec/models/packages/package_spec.rb

@@ -47,7 +47,23 @@ RSpec.describe Packages::Package, type: :model do
       end
     end
 
-    Packages::Package.package_types.keys.each do |pt|
+    context "recipe uniqueness for conan packages" do
+      let!(:package) { create('conan_package') }
+
+      it "will allow a conan package with same project, name, version and package_type" do
+        new_package = build('conan_package', project: package.project, name: package.name, version: package.version)
+        new_package.conan_metadatum.package_channel = 'beta'
+        expect(new_package).to be_valid
+      end
+
+      it "will not allow a conan package with same recipe (name, version, metadatum.package_channel, metadatum.package_username, and package_type)" do
+        new_package = build('conan_package', project: package.project, name: package.name, version: package.version)
+        expect(new_package).not_to be_valid
+        expect(new_package.errors.to_a).to include("Package recipe already exists")
+      end
+    end
+
+    Packages::Package.package_types.keys.without('conan').each do |pt|
       context "project id, name, version and package type uniqueness for package type #{pt}" do
         let(:package) { create("#{pt}_package") }
 
@@ -118,25 +134,29 @@ RSpec.describe Packages::Package, type: :model do
         is_expected.to match_array([package2, package3])
       end
     end
+  end
 
-    describe '.with_conan_channel' do
-      let!(:package) { create(:conan_package) }
+  context 'conan scopes' do
+    let!(:package) { create(:conan_package) }
 
+    describe '.with_conan_channel' do
       subject { described_class.with_conan_channel('stable') }
 
       it 'includes only packages with specified version' do
-        is_expected.to match_array([package])
+        is_expected.to include(package)
       end
     end
-  end
-
-  describe '.with_conan_channel' do
-    let!(:package) { create(:conan_package) }
 
-    subject { described_class.with_conan_channel('stable') }
+    describe '.with_conan_username' do
+      subject do
+        described_class.with_conan_username(
+          Packages::ConanMetadatum.package_username_from(full_path: package.project.full_path)
+        )
+      end
 
-    it 'includes only packages with specified version' do
-      is_expected.to eq([package])
+      it 'includes only packages with specified version' do
+        is_expected.to match_array([package])
+      end
     end
   end
 

ee/spec/services/packages/nuget/metadata_extraction_service_spec.rb

@@ -21,7 +21,7 @@ describe Packages::Nuget::MetadataExtractionService do
 
     context 'linked to a non nuget package' do
       before do
-        package_file.package.conan!
+        package_file.package.maven!
       end
 
       it { expect { subject }.to raise_error(::Packages::Nuget::MetadataExtractionService::ExtractionError, 'invalid package file') }