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

Update naming validation for Conan recipes

Closes #37335

See merge request gitlab-org/gitlab!23467

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

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

db/migrate/20200129234037_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

@@ -2945,7 +2945,7 @@ ActiveRecord::Schema.define(version: 2020_02_06_111847) 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(
@@ -100,6 +105,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/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") }
 
@@ -119,24 +135,28 @@ RSpec.describe Packages::Package, type: :model do
       end
     end
 
-    describe '.with_conan_channel' do
+    context 'conan scopes' do
       let!(:package) { create(:conan_package) }
 
-      subject { described_class.with_conan_channel('stable') }
+      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])
+        it 'includes only packages with specified version' do
+          is_expected.to match_array([package])
+        end
       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
   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') }