Update external wiki triggers to use new columns

Update the triggers for the functions that maintain the external wiki
flag on the project table so that they check the new column values for
the STI `type/type_new`.

The migration ensures that there is no point at which one of the
triggers is not installed, ensuring we do not allow rows to be inserted,
updated or deleted without triggering one or other of the two versions
of the trigger.

A new temporary trigger is added, which handles the fact that we first
insert rows with `type` set, and then later set `type_new` as part of
the STI migration for the `integrations` table.

Changelog: changed

Fix project spec

This uses a factory to ensure that the type/type_new columns are set
correctly, and renames 'service' to 'integration' where appropriate.

https://gitlab.com/gitlab-org/gitlab/-/issues/334127

db/post_migrate/20210813151908_replace_external_wiki_triggers.rb

+# frozen_string_literal: true
+
+class ReplaceExternalWikiTriggers < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::SchemaHelpers
+
+  def up
+    replace_triggers('type_new', 'Integrations::ExternalWiki')
+
+    # we need an extra trigger to handle when type_new is updated by the
+    # `integrations_set_type_new` trigger.
+    # This can be removed when this trigger has been removed.
+    execute(<<~SQL.squish)
+      CREATE TRIGGER #{trigger_name(:type_new_updated)}
+        AFTER UPDATE OF type_new ON integrations FOR EACH ROW
+        WHEN ((new.type_new)::text = 'Integrations::ExternalWiki'::text AND new.project_id IS NOT NULL)
+        EXECUTE FUNCTION set_has_external_wiki();
+    SQL
+  end
+
+  def down
+    execute("DROP TRIGGER IF EXISTS #{trigger_name(:type_new_updated)} ON integrations;")
+    replace_triggers('type', 'ExternalWikiService')
+  end
+
+  private
+
+  def replace_triggers(column_name, value)
+    triggers(column_name, value).each do |event, condition|
+      trigger = trigger_name(event)
+
+      # create duplicate trigger, using the defined condition
+      execute(<<~SQL.squish)
+      CREATE TRIGGER #{trigger}_new AFTER #{event.upcase} ON integrations FOR EACH ROW
+        WHEN (#{condition})
+        EXECUTE FUNCTION set_has_external_wiki();
+      SQL
+
+      # Swap the triggers in place, so that the new trigger has the canonical name
+      execute("ALTER TRIGGER #{trigger} ON integrations RENAME TO #{trigger}_old;")
+      execute("ALTER TRIGGER #{trigger}_new ON integrations RENAME TO #{trigger};")
+
+      # remove the old, now redundant trigger
+      execute("DROP TRIGGER IF EXISTS #{trigger}_old ON integrations;")
+    end
+  end
+
+  def trigger_name(event)
+    "trigger_has_external_wiki_on_#{event}"
+  end
+
+  def triggers(column_name, value)
+    {
+      delete: "#{matches_value('old', column_name, value)} AND #{project_not_null('old')}",
+      insert: "(new.active = true) AND #{matches_value('new', column_name, value)} AND #{project_not_null('new')}",
+      update: "#{matches_value('new', column_name, value)} AND (old.active <> new.active) AND #{project_not_null('new')}"
+    }
+  end
+
+  def project_not_null(row)
+    "(#{row}.project_id IS NOT NULL)"
+  end
+
+  def matches_value(row, column_name, value)
+    "((#{row}.#{column_name})::text = '#{value}'::text)"
+  end
+end

db/schema_migrations/20210813151908

+fdb6dd20c1cd5feaf0efd8eb94a4d61fc4812f1142572433ae397cd5f27bf603
\ No newline at end of file

db/structure.sql

@@ -26111,11 +26111,13 @@ CREATE TRIGGER trigger_has_external_issue_tracker_on_insert AFTER INSERT ON inte
 
 CREATE TRIGGER trigger_has_external_issue_tracker_on_update AFTER UPDATE ON integrations FOR EACH ROW WHEN ((((new.category)::text = 'issue_tracker'::text) AND (old.active <> new.active) AND (new.project_id IS NOT NULL))) EXECUTE FUNCTION set_has_external_issue_tracker();
 
-CREATE TRIGGER trigger_has_external_wiki_on_delete AFTER DELETE ON integrations FOR EACH ROW WHEN ((((old.type)::text = 'ExternalWikiService'::text) AND (old.project_id IS NOT NULL))) EXECUTE FUNCTION set_has_external_wiki();
+CREATE TRIGGER trigger_has_external_wiki_on_delete AFTER DELETE ON integrations FOR EACH ROW WHEN (((old.type_new = 'Integrations::ExternalWiki'::text) AND (old.project_id IS NOT NULL))) EXECUTE FUNCTION set_has_external_wiki();
 
-CREATE TRIGGER trigger_has_external_wiki_on_insert AFTER INSERT ON integrations FOR EACH ROW WHEN (((new.active = true) AND ((new.type)::text = 'ExternalWikiService'::text) AND (new.project_id IS NOT NULL))) EXECUTE FUNCTION set_has_external_wiki();
+CREATE TRIGGER trigger_has_external_wiki_on_insert AFTER INSERT ON integrations FOR EACH ROW WHEN (((new.active = true) AND (new.type_new = 'Integrations::ExternalWiki'::text) AND (new.project_id IS NOT NULL))) EXECUTE FUNCTION set_has_external_wiki();
 
-CREATE TRIGGER trigger_has_external_wiki_on_update AFTER UPDATE ON integrations FOR EACH ROW WHEN ((((new.type)::text = 'ExternalWikiService'::text) AND (old.active <> new.active) AND (new.project_id IS NOT NULL))) EXECUTE FUNCTION set_has_external_wiki();
+CREATE TRIGGER trigger_has_external_wiki_on_type_new_updated AFTER UPDATE OF type_new ON integrations FOR EACH ROW WHEN (((new.type_new = 'Integrations::ExternalWiki'::text) AND (new.project_id IS NOT NULL))) EXECUTE FUNCTION set_has_external_wiki();
+
+CREATE TRIGGER trigger_has_external_wiki_on_update AFTER UPDATE ON integrations FOR EACH ROW WHEN (((new.type_new = 'Integrations::ExternalWiki'::text) AND (old.active <> new.active) AND (new.project_id IS NOT NULL))) EXECUTE FUNCTION set_has_external_wiki();
 
 CREATE TRIGGER trigger_type_new_on_insert AFTER INSERT ON integrations FOR EACH ROW EXECUTE FUNCTION integrations_set_type_new();
 

spec/migrations/replace_external_wiki_triggers_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require_migration!
+
+RSpec.describe ReplaceExternalWikiTriggers do
+  let(:migration) { described_class.new }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:integrations) { table(:integrations) }
+
+  before do
+    @namespace = namespaces.create!(name: 'foo', path: 'foo')
+    @project = projects.create!(namespace_id: @namespace.id)
+  end
+
+  def create_external_wiki_integration(**attrs)
+    attrs.merge!(type_info)
+
+    integrations.create!(**attrs)
+  end
+
+  def has_external_wiki
+    !!@project.reload.has_external_wiki
+  end
+
+  shared_examples 'external wiki triggers' do
+    describe 'INSERT trigger' do
+      it 'sets `has_external_wiki` to true when active external wiki integration is inserted' do
+        expect do
+          create_external_wiki_integration(active: true, project_id: @project.id)
+        end.to change { has_external_wiki }.to(true)
+      end
+
+      it 'does not set `has_external_wiki` to true when integration is for a different project' do
+        different_project = projects.create!(namespace_id: @namespace.id)
+
+        expect do
+          create_external_wiki_integration(active: true, project_id: different_project.id)
+        end.not_to change { has_external_wiki }
+      end
+
+      it 'does not set `has_external_wiki` to true when inactive external wiki integration is inserted' do
+        expect do
+          create_external_wiki_integration(active: false, project_id: @project.id)
+        end.not_to change { has_external_wiki }
+      end
+
+      it 'does not set `has_external_wiki` to true when active other service is inserted' do
+        expect do
+          integrations.create!(type_new: 'Integrations::MyService', type: 'MyService', active: true, project_id: @project.id)
+        end.not_to change { has_external_wiki }
+      end
+    end
+
+    describe 'UPDATE trigger' do
+      it 'sets `has_external_wiki` to true when `ExternalWikiService` is made active' do
+        service = create_external_wiki_integration(active: false, project_id: @project.id)
+
+        expect do
+          service.update!(active: true)
+        end.to change { has_external_wiki }.to(true)
+      end
+
+      it 'sets `has_external_wiki` to false when integration is made inactive' do
+        service = create_external_wiki_integration(active: true, project_id: @project.id)
+
+        expect do
+          service.update!(active: false)
+        end.to change { has_external_wiki }.to(false)
+      end
+
+      it 'does not change `has_external_wiki` when integration is for a different project' do
+        different_project = projects.create!(namespace_id: @namespace.id)
+        service = create_external_wiki_integration(active: false, project_id: different_project.id)
+
+        expect do
+          service.update!(active: true)
+        end.not_to change { has_external_wiki }
+      end
+    end
+
+    describe 'DELETE trigger' do
+      it 'sets `has_external_wiki` to false when integration is deleted' do
+        service = create_external_wiki_integration(active: true, project_id: @project.id)
+
+        expect do
+          service.delete
+        end.to change { has_external_wiki }.to(false)
+      end
+
+      it 'does not change `has_external_wiki` when integration is for a different project' do
+        different_project = projects.create!(namespace_id: @namespace.id)
+        service = create_external_wiki_integration(active: true, project_id: different_project.id)
+
+        expect do
+          service.delete
+        end.not_to change { has_external_wiki }
+      end
+    end
+  end
+
+  describe '#up' do
+    before do
+      migrate!
+    end
+
+    context 'when integrations are created with the new STI value' do
+      let(:type_info) { { type_new: 'Integrations::ExternalWiki' } }
+
+      it_behaves_like 'external wiki triggers'
+    end
+
+    context 'when integrations are created with the old STI value' do
+      let(:type_info) { { type: 'ExternalWikiService' } }
+
+      it_behaves_like 'external wiki triggers'
+    end
+  end
+
+  describe '#down' do
+    before do
+      migration.up
+      migration.down
+    end
+
+    let(:type_info) { { type: 'ExternalWikiService' } }
+
+    it_behaves_like 'external wiki triggers'
+  end
+end

spec/models/project_spec.rb

@@ -1257,19 +1257,19 @@ RSpec.describe Project, factory_default: :keep do
     end
 
     it 'returns an active external wiki' do
-      create(:service, project: project, type: 'ExternalWikiService', active: true)
+      create(:external_wiki_integration, project: project, active: true)
 
       is_expected.to be_kind_of(Integrations::ExternalWiki)
     end
 
     it 'does not return an inactive external wiki' do
-      create(:service, project: project, type: 'ExternalWikiService', active: false)
+      create(:external_wiki_integration, project: project, active: false)
 
       is_expected.to eq(nil)
     end
 
     it 'sets Project#has_external_wiki when it is nil' do
-      create(:service, project: project, type: 'ExternalWikiService', active: true)
+      create(:external_wiki_integration, project: project, active: true)
       project.update_column(:has_external_wiki, nil)
 
       expect { subject }.to change { project.has_external_wiki }.from(nil).to(true)
@@ -1279,36 +1279,40 @@ RSpec.describe Project, factory_default: :keep do
   describe '#has_external_wiki' do
     let_it_be(:project) { create(:project) }
 
-    def subject
+    def has_external_wiki
       project.reload.has_external_wiki
     end
 
-    specify { is_expected.to eq(false) }
+    specify { expect(has_external_wiki).to eq(false) }
 
-    context 'when there is an active external wiki service' do
-      let!(:service) do
-        create(:service, project: project, type: 'ExternalWikiService', active: true)
+    context 'when there is an active external wiki integration' do
+      let(:active) { true }
+
+      let!(:integration) do
+        create(:external_wiki_integration, project: project, active: active)
       end
 
-      specify { is_expected.to eq(true) }
+      specify { expect(has_external_wiki).to eq(true) }
 
       it 'becomes false if the external wiki service is destroyed' do
         expect do
-          Integration.find(service.id).delete
-        end.to change { subject }.to(false)
+          Integration.find(integration.id).delete
+        end.to change { has_external_wiki }.to(false)
       end
 
       it 'becomes false if the external wiki service becomes inactive' do
         expect do
-          service.update_column(:active, false)
-        end.to change { subject }.to(false)
+          integration.update_column(:active, false)
+        end.to change { has_external_wiki }.to(false)
       end
-    end
 
-    it 'is false when external wiki service is not active' do
-      create(:service, project: project, type: 'ExternalWikiService', active: false)
+      context 'when created as inactive' do
+        let(:active) { false }
 
-      is_expected.to eq(false)
+        it 'is false' do
+          expect(has_external_wiki).to eq(false)
+        end
+      end
     end
   end