Add project namespace delete trigger on project delete

- Add to post migration
- Add tests to test destruction sync both ways

app/models/namespace.rb

@@ -57,7 +57,7 @@ class Namespace < ApplicationRecord
   has_one :admin_note, inverse_of: :namespace
   accepts_nested_attributes_for :admin_note, update_only: true
 
-  validates :owner, presence: true, unless: ->(n) { n.type == "Group" || n.type == 'Namespaces::ProjectNamespace' }
+  validates :owner, presence: true, if: ->(n) { n.type.nil? }
   validates :name,
     presence: true,
     length: { maximum: 255 }

app/models/project.rb

@@ -161,6 +161,8 @@ class Project < ApplicationRecord
   belongs_to :creator, class_name: 'User'
   belongs_to :group, -> { where(type: 'Group') }, foreign_key: 'namespace_id'
   belongs_to :namespace
+  # Sync deletion via DB Trigger to ensure we do not have
+  # a project without a project_namespace (or vice-versa)
   belongs_to :project_namespace, class_name: 'Namespaces::ProjectNamespace', foreign_key: 'project_namespace_id', inverse_of: :project
   alias_method :parent, :namespace
   alias_attribute :parent_id, :namespace_id

db/migrate/20210906100021_delete_project_namespace_trigger.rb

+# frozen_string_literal: true
+
+class DeleteProjectNamespaceTrigger < Gitlab::Database::Migration[1.0]
+  include Gitlab::Database::SchemaHelpers
+
+  TRIGGER_NAME = "trigger_delete_project_namespace_on_project_delete"
+  FUNCTION_NAME = 'delete_associated_project_namespace'
+
+  def up
+    create_trigger_function(FUNCTION_NAME, replace: true) do
+      <<~SQL
+        DELETE FROM namespaces
+        WHERE namespaces.id = OLD.project_namespace_id AND
+        namespaces.type = 'Project';
+        RETURN NULL;
+      SQL
+    end
+
+    execute(<<~SQL.squish)
+      CREATE TRIGGER #{TRIGGER_NAME}
+        AFTER DELETE ON projects FOR EACH ROW
+        WHEN (OLD.project_namespace_id IS NOT NULL)
+        EXECUTE FUNCTION #{FUNCTION_NAME}();
+    SQL
+  end
+
+  def down
+    drop_trigger(:projects, TRIGGER_NAME)
+    drop_function(FUNCTION_NAME)
+  end
+end

db/schema_migrations/20210906100021

+74f6800c968f80e18aa000df42fd23086404fdd7a863237e453541b7eeb4eb7f
\ No newline at end of file

db/structure.sql

@@ -10,6 +10,18 @@ CREATE EXTENSION IF NOT EXISTS btree_gist;
 
 CREATE EXTENSION IF NOT EXISTS pg_trgm;
 
+CREATE FUNCTION delete_associated_project_namespace() RETURNS trigger
+    LANGUAGE plpgsql
+    AS $$
+BEGIN
+DELETE FROM namespaces
+WHERE namespaces.id = OLD.project_namespace_id AND
+namespaces.type = 'Project';
+RETURN NULL;
+
+END
+$$;
+
 CREATE FUNCTION integrations_set_type_new() RETURNS trigger
     LANGUAGE plpgsql
     AS $$
@@ -27319,6 +27331,8 @@ CREATE TRIGGER trigger_91dc388a5fe6 BEFORE INSERT OR UPDATE ON dep_ci_build_trac
 
 CREATE TRIGGER trigger_aebe8b822ad3 BEFORE INSERT OR UPDATE ON taggings FOR EACH ROW EXECUTE FUNCTION trigger_aebe8b822ad3();
 
+CREATE TRIGGER trigger_delete_project_namespace_on_project_delete AFTER DELETE ON projects FOR EACH ROW WHEN ((old.project_namespace_id IS NOT NULL)) EXECUTE FUNCTION delete_associated_project_namespace();
+
 CREATE TRIGGER trigger_has_external_issue_tracker_on_delete AFTER DELETE ON integrations FOR EACH ROW WHEN ((((old.category)::text = 'issue_tracker'::text) AND (old.active = true) AND (old.project_id IS NOT NULL))) EXECUTE FUNCTION set_has_external_issue_tracker();
 
 CREATE TRIGGER trigger_has_external_issue_tracker_on_insert AFTER INSERT ON integrations FOR EACH ROW WHEN ((((new.category)::text = 'issue_tracker'::text) AND (new.active = true) AND (new.project_id IS NOT NULL))) EXECUTE FUNCTION set_has_external_issue_tracker();

ee/spec/services/projects/destroy_service_spec.rb

@@ -96,4 +96,15 @@ RSpec.describe Projects::DestroyService do
       expect { subject.execute }.to change(AuditEvent, :count)
     end
   end
+
+  context 'when project has an associated ProjectNamespace' do
+    let!(:project_namespace) { create(:project_namespace, project: project) }
+
+    it 'destroys the associated ProjectNamespace also' do
+      subject.execute
+
+      expect { project_namespace.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      expect { project.reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
 end

spec/factories/namespaces/project_namespace.rb → spec/factories/namespaces/project_namespaces.rb

@@ -3,10 +3,9 @@
 FactoryBot.define do
   factory :project_namespace, class: 'Namespaces::ProjectNamespace' do
     project
-    sequence(:name) { |n| "project_namespace#{n}" }
-    path { name.downcase.gsub(/\s/, '_') }
-    type { 'Namespaces::ProjectNamespace' }
+    name { project.name }
+    path { project.path }
+    type { Namespaces::ProjectNamespace.sti_name }
     owner { nil }
-    project_creation_level { ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS }
   end
 end

spec/factories_spec.rb

@@ -74,6 +74,7 @@ RSpec.describe 'factories' do
     milestone_release
     namespace
     project_broken_repo
+    project_namespace
     project_repository
     prometheus_alert
     prometheus_alert_event

spec/models/namespaces/project_namespace_spec.rb

@@ -6,4 +6,18 @@ RSpec.describe Namespaces::ProjectNamespace, type: :model do
   describe 'relationships' do
     it { is_expected.to have_one(:project).with_foreign_key(:project_namespace_id).inverse_of(:project_namespace) }
   end
+
+  context 'when deleting project namespace' do
+    # using delete rather than destroy due to `delete` skipping AR hooks/callbacks
+    # so it's ensured to work at the DB level. Uses ON DELETE CASCADE on foreign key
+    let_it_be(:project) { create(:project) }
+    let_it_be(:project_namespace) { create(:project_namespace, project: project) }
+
+    it 'also deletes the associated project' do
+      project_namespace.delete
+
+      expect { project_namespace.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      expect { project.reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
 end

spec/models/project_spec.rb

@@ -185,13 +185,16 @@ RSpec.describe Project, factory_default: :keep do
       end
     end
 
-    context 'when destroying project' do
+    context 'when deleting project' do
+      # using delete rather than destroy due to `delete` skipping AR hooks/callbacks
+      # so it's ensured to work at the DB level. Uses AFTER DELETE trigger.
       let_it_be(:project) { create(:project) }
       let_it_be(:project_namespace) { create(:project_namespace, project: project) }
 
-      it 'also destroys the associated ProjectNamespace' do
-        project.destroy!
+      it 'also deletes the associated ProjectNamespace' do
+        project.delete
 
+        expect { project.reload }.to raise_error(ActiveRecord::RecordNotFound)
         expect { project_namespace.reload }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end