Merge branch '202423-validate-foreign-key-on-group-hooks' into 'master'

Validate foreign key on GroupHooks

See merge request gitlab-org/gitlab!60527

changelogs/unreleased/202423-validate-foreign-key-on-group-hooks.yml

+---
+title: Validate foreign key on GroupHooks
+merge_request: 60527
+author:
+type: other

db/migrate/20210428151144_update_invalid_web_hooks.rb

+# frozen_string_literal: true
+
+class UpdateInvalidWebHooks < ActiveRecord::Migration[6.0]
+  disable_ddl_transaction!
+
+  class WebHook < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'web_hooks'
+  end
+
+  def up
+    WebHook.each_batch(of: 10_000, column: :id) do |relation|
+      relation.where(type: 'ProjectHook')
+              .where.not(project_id: nil)
+              .where.not(group_id: nil)
+              .update_all(group_id: nil)
+    end
+  end
+
+  def down
+    # no-op
+  end
+end

db/migrate/20210428151238_validate_foreign_key_on_group_hooks.rb

+# frozen_string_literal: true
+
+class ValidateForeignKeyOnGroupHooks < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  CONSTRAINT_NAME = 'fk_rails_d35697648e'
+
+  def up
+    validate_foreign_key :web_hooks, :group_id, name: CONSTRAINT_NAME
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema_migrations/20210428151144

+468373a97f7bd66197c81f01bebd27256cf96ec8fc226c5d73e579a7ecc3930d
\ No newline at end of file

db/schema_migrations/20210428151238

+3244023441c2afa450ad76345a494975b4a7154892298daf1ec4223d27fb7ca3
\ No newline at end of file

db/structure.sql

@@ -26934,7 +26934,7 @@ ALTER TABLE ONLY pool_repositories
     ADD CONSTRAINT fk_rails_d2711daad4 FOREIGN KEY (source_project_id) REFERENCES projects(id) ON DELETE SET NULL;
 
 ALTER TABLE ONLY web_hooks
-    ADD CONSTRAINT fk_rails_d35697648e FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE NOT VALID;
+    ADD CONSTRAINT fk_rails_d35697648e FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE;
 
 ALTER TABLE ONLY group_group_links
     ADD CONSTRAINT fk_rails_d3a0488427 FOREIGN KEY (shared_group_id) REFERENCES namespaces(id) ON DELETE CASCADE;

ee/app/models/ee/group.rb

@@ -30,7 +30,7 @@ module EE
 
       has_many :ldap_group_links, foreign_key: 'group_id', dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
       has_many :saml_group_links, foreign_key: 'group_id'
-      has_many :hooks, dependent: :destroy, class_name: 'GroupHook' # rubocop:disable Cop/ActiveRecordDependent
+      has_many :hooks, class_name: 'GroupHook'
 
       has_many :allowed_email_domains, -> { order(id: :asc) }, autosave: true
 

spec/migrations/update_invalid_web_hooks_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require_migration!
+
+RSpec.describe UpdateInvalidWebHooks do
+  let(:web_hooks) { table(:web_hooks) }
+  let(:groups) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+
+  before do
+    group = groups.create!(name: 'gitlab', path: 'gitlab-org')
+    project = projects.create!(namespace_id: group.id)
+
+    web_hooks.create!(group_id: group.id, type: 'GroupHook')
+    web_hooks.create!(project_id: project.id, type: 'ProjectHook')
+    web_hooks.create!(group_id: group.id, project_id: project.id, type: 'ProjectHook')
+  end
+
+  it 'clears group_id when ProjectHook type and project_id are present', :aggregate_failures do
+    expect(web_hooks.where.not(group_id: nil).where.not(project_id: nil).count).to eq(1)
+
+    migrate!
+
+    expect(web_hooks.where.not(group_id: nil).where.not(project_id: nil).count).to eq(0)
+    expect(web_hooks.where(type: 'GroupHook').count).to eq(1)
+    expect(web_hooks.where(type: 'ProjectHook').count).to eq(2)
+  end
+end