Merge branch '344596-ensure-backgroundusernamespace-migration-complete' into 'master'

Cleanup backgroundUserNamespace migration

See merge request gitlab-org/gitlab!73495

db/fixtures/development/02_users.rb

@@ -41,11 +41,12 @@ class Gitlab::Seeder::Users
     relation = User.where(admin: false)
     Gitlab::Seeder.with_mass_insert(relation.count, Namespace) do
       ActiveRecord::Base.connection.execute <<~SQL
-        INSERT INTO namespaces (name, path, owner_id)
+        INSERT INTO namespaces (name, path, owner_id, type)
         SELECT
           username,
           username,
-          id
+          id,
+          'User'
         FROM users WHERE NOT admin
       SQL
     end

db/post_migrate/20211101222614_consume_remaining_user_namespace_jobs.rb

+# frozen_string_literal: true
+
+class ConsumeRemainingUserNamespaceJobs < Gitlab::Database::Migration[1.0]
+  MIGRATION = 'BackfillUserNamespace'
+  BATCH_SIZE = 200
+  DEFAULT_VALUE = 'User'
+
+  disable_ddl_transaction!
+
+  def up
+    Gitlab::BackgroundMigration.steal(MIGRATION)
+
+    # Do a manual update in case we lost BG jobs. The expected record count should be 0 or very low.
+    define_batchable_model('namespaces').where(type: nil).each_batch(of: BATCH_SIZE) do |batch|
+      min, max = batch.pluck('MIN(id), MAX(id)').flatten
+
+      Gitlab::BackgroundMigration::BackfillUserNamespace.new.perform(min, max, :namespaces, :id, BATCH_SIZE, 0)
+    end
+
+    change_column_null :namespaces, :type, false
+  end
+
+  def down
+    change_column_null :namespaces, :type, true
+  end
+end

db/schema_migrations/20211101222614

+a579b14aff1d186d89173e383442f2ffbd69b1baed3f9a4c758fbb001b445139
\ No newline at end of file

db/structure.sql

@@ -16419,7 +16419,7 @@ CREATE TABLE namespaces (
     owner_id integer,
     created_at timestamp without time zone,
     updated_at timestamp without time zone,
-    type character varying DEFAULT 'User'::character varying,
+    type character varying DEFAULT 'User'::character varying NOT NULL,
     description character varying DEFAULT ''::character varying NOT NULL,
     avatar character varying,
     membership_lock boolean DEFAULT false,

ee/spec/helpers/billing_plans_helper_spec.rb

@@ -167,7 +167,7 @@ RSpec.describe BillingPlansHelper, :saas do
   end
 
   describe '#use_new_purchase_flow?' do
-    where type: [Group.sti_name, nil],
+    where type: [Group.sti_name, Namespaces::UserNamespace.sti_name],
       plan: Plan.all_plans,
       trial_active: [true, false]
 
@@ -345,7 +345,7 @@ RSpec.describe BillingPlansHelper, :saas do
     end
   end
 
-  describe "#plan_purchase_or_upgrade_url" do
+  describe '#plan_purchase_or_upgrade_url' do
     let(:plan) { double('Plan') }
 
     it 'is upgradable' do
@@ -364,7 +364,7 @@ RSpec.describe BillingPlansHelper, :saas do
     end
   end
 
-  describe "#plan_purchase_url" do
+  describe '#plan_purchase_url' do
     let_it_be(:group) { create(:group) }
     let_it_be(:user) { create(:user) }
 
@@ -390,7 +390,7 @@ RSpec.describe BillingPlansHelper, :saas do
     end
   end
 
-  describe "#hand_raise_props" do
+  describe '#hand_raise_props' do
     let_it_be(:namespace) { create(:namespace) }
     let_it_be(:user) { create(:user, username: 'Joe', first_name: 'Joe', last_name: 'Doe', organization: 'ACME') }
 

ee/spec/lib/ee/gitlab/background_migration/populate_uuids_for_security_findings_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe ::Gitlab::BackgroundMigration::PopulateUuidsForSecurityFindings,
   let(:fingerprint_4) { Digest::SHA1.hexdigest(SecureRandom.uuid) }
 
   let(:user) { users.create!(email: 'test@gitlab.com', projects_limit: 5) }
-  let(:namespace) { namespaces.create!(name: 'gitlab', path: 'gitlab-org') }
+  let(:namespace) { namespaces.create!(name: 'gitlab', path: 'gitlab-org', type: Namespaces::UserNamespace.sti_name) }
   let(:project) { projects.create!(namespace_id: namespace.id, name: 'foo') }
   let(:ci_pipeline) { ci_pipelines.create!(project_id: project.id, ref: 'master', sha: 'adf43c3a', status: 'success') }
   let(:ci_build_1) { ci_builds.create!(commit_id: ci_pipeline.id, retried: false, type: 'Ci::Build') }

ee/spec/lib/ee/gitlab/background_migration/update_vulnerability_occurrences_location_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::BackgroundMigration::UpdateVulnerabilityOccurrencesLocation, schema: 20211102114802 do
   let_it_be(:namespaces) { table(:namespaces) }
-  let_it_be(:group) { namespaces.create!(name: 'foo', path: 'foo') }
+  let_it_be(:group) { namespaces.create!(name: 'foo', path: 'foo', type: Group.sti_name) }
   let_it_be(:projects) { table(:projects) }
   let_it_be(:findings) { table(:vulnerability_occurrences) }
   let_it_be(:scanners) { table(:vulnerability_scanners) }

ee/spec/migrations/update_vulnerability_occurrences_location_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe UpdateVulnerabilityOccurrencesLocation, :migration do
   let(:migration_name) { 'UpdateVulnerabilityOccurrencesLocation' }
 
   let_it_be(:namespaces) { table(:namespaces) }
-  let_it_be(:group) { namespaces.create!(name: 'foo', path: 'foo') }
+  let_it_be(:group) { namespaces.create!(name: 'foo', path: 'foo', type: Group.sti_name) }
   let_it_be(:projects) { table(:projects) }
   let_it_be(:findings) { table(:vulnerability_occurrences) }
   let_it_be(:scanners) { table(:vulnerability_scanners) }

lib/tasks/gitlab/update_templates.rake

@@ -32,7 +32,7 @@ namespace :gitlab do
 
     tmp_namespace_path = "tmp-project-import-#{Time.now.to_i}"
     puts "Creating temporary namespace #{tmp_namespace_path}"
-    tmp_namespace = Namespace.create!(owner: admin, name: tmp_namespace_path, path: tmp_namespace_path)
+    tmp_namespace = Namespace.create!(owner: admin, name: tmp_namespace_path, path: tmp_namespace_path, type: Namespaces::UserNamespace.sti_name)
 
     templates = if template_names.empty?
                   Gitlab::ProjectTemplate.all

spec/lib/gitlab/background_migration/drop_invalid_security_findings_spec.rb

@@ -2,7 +2,7 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::BackgroundMigration::DropInvalidSecurityFindings, schema: 20211108211434 do
-  let(:namespace) { table(:namespaces).create!(name: 'user', path: 'user') }
+  let(:namespace) { table(:namespaces).create!(name: 'user', path: 'user', type: Namespaces::UserNamespace.sti_name) }
   let(:project) { table(:projects).create!(namespace_id: namespace.id) }
 
   let(:pipelines) { table(:ci_pipelines) }

spec/lib/gitlab/background_migration/remove_vulnerability_finding_links_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Gitlab::BackgroundMigration::RemoveVulnerabilityFindingLinks, :mi
   let(:vulnerability_findings) { table(:vulnerability_occurrences) }
   let(:finding_links) { table(:vulnerability_finding_links) }
 
-  let(:namespace) { table(:namespaces).create!(name: 'user', path: 'user') }
+  let(:namespace) { table(:namespaces).create!(name: 'user', path: 'user', type: Namespaces::UserNamespace.sti_name) }
   let(:project) { table(:projects).create!(namespace_id: namespace.id) }
   let(:scanner) { table(:vulnerability_scanners).create!(project_id: project.id, external_id: 'scanner', name: 'scanner') }
   let(:vulnerability_identifier) do

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -2431,7 +2431,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
     let(:issues)         { table(:issues) }
 
     def setup
-      namespace = namespaces.create!(name: 'foo', path: 'foo')
+      namespace = namespaces.create!(name: 'foo', path: 'foo', type: Namespaces::UserNamespace.sti_name)
       projects.create!(namespace_id: namespace.id)
     end
 

spec/migrations/20211101222614_consume_remaining_user_namespace_jobs_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!
+
+RSpec.describe ConsumeRemainingUserNamespaceJobs do
+  let(:namespaces) { table(:namespaces) }
+  let!(:namespace) { namespaces.create!(name: 'gitlab', path: 'gitlab-org', type: nil) }
+
+  context 'when Namespaces with nil `type` still exist' do
+    it 'steals sidekiq jobs from BackfillUserNamespace background migration' do
+      expect(Gitlab::BackgroundMigration).to receive(:steal).with('BackfillUserNamespace')
+
+      migrate!
+    end
+
+    it 'migrates namespaces without type' do
+      expect { migrate! }.to change { namespaces.where(type: 'User').count }.from(0).to(1)
+    end
+  end
+end

spec/migrations/20211110151350_schedule_drop_invalid_security_findings_spec.rb

@@ -6,7 +6,7 @@ require_migration!
 RSpec.describe ScheduleDropInvalidSecurityFindings, :migration, schema: 20211108211434 do
   let_it_be(:background_migration_jobs) { table(:background_migration_jobs) }
 
-  let_it_be(:namespace) { table(:namespaces).create!(name: 'user', path: 'user') }
+  let_it_be(:namespace) { table(:namespaces).create!(name: 'user', path: 'user', type: Namespaces::UserNamespace.sti_name) }
   let_it_be(:project) { table(:projects).create!(namespace_id: namespace.id) }
 
   let_it_be(:pipelines) { table(:ci_pipelines) }

spec/migrations/20211116091751_change_namespace_type_default_to_user_spec.rb

 # frozen_string_literal: true
 
-require 'spec_helper'
-
-require_migration!
-
-RSpec.describe ChangeNamespaceTypeDefaultToUser do
-  let(:namespaces) { table(:namespaces) }
-
-  it 'defaults type to User' do
-    some_namespace1 = namespaces.create!(name: 'magic namespace1', path: 'magicnamespace1', type: nil)
-
-    expect(some_namespace1.reload.type).to be_nil
-
-    migrate!
-
-    some_namespace2 = namespaces.create!(name: 'magic namespace2', path: 'magicnamespace2', type: nil)
-
-    expect(some_namespace1.reload.type).to be_nil
-    expect(some_namespace2.reload.type).to eq 'User'
-  end
-end
+# With https://gitlab.com/gitlab-org/gitlab/-/merge_requests/73495, we no longer allow
+# a Namespace type to be nil. There is nothing left to test for this migration,
+# but we'll keep this file here as a tombstone.

spec/models/concerns/case_sensitivity_spec.rb

@@ -13,8 +13,8 @@ RSpec.describe CaseSensitivity do
       end
     end
 
-    let_it_be(:model_1) { model.create!(path: 'mOdEl-1', name: 'mOdEl 1') }
-    let_it_be(:model_2) { model.create!(path: 'mOdEl-2', name: 'mOdEl 2') }
+    let_it_be(:model_1) { model.create!(path: 'mOdEl-1', name: 'mOdEl 1', type: Namespaces::UserNamespace.sti_name) }
+    let_it_be(:model_2) { model.create!(path: 'mOdEl-2', name: 'mOdEl 2', type: Group.sti_name) }
 
     it 'finds a single instance by a single attribute regardless of case' do
       expect(model.iwhere(path: 'MODEL-1')).to contain_exactly(model_1)

spec/models/namespace_spec.rb

@@ -259,13 +259,12 @@ RSpec.describe Namespace do
       end
     end
 
-    context 'creating a Namespace with nil type' do
+    context 'unable to create a Namespace with nil type' do
+      let(:namespace) { nil }
       let(:namespace_type) { nil }
 
-      it 'is the correct type of namespace' do
-        expect(Namespace.find(namespace.id)).to be_a(Namespace)
-        expect(namespace.kind).to eq('user')
-        expect(namespace.user_namespace?).to be_truthy
+      it 'raises ActiveRecord::NotNullViolation' do
+        expect { create(:namespace, type: namespace_type, parent: parent) }.to raise_error(ActiveRecord::NotNullViolation)
       end
     end