Add PG trigger to maintain has_external_wiki

We cache whether a project has an External Wiki integration enabled in
the `has_external_wiki` column on `projects`. It will be `TRUE` if the
project has the "External Wiki" integration (service) enabled.

This was previously maintained with application code.

This cache is easy to fall out of consistency when we fail to maintain
it during bulk operations (including PostgreSQL cascading deletes
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48163/) as
mentioned in:

- https://gitlab.com/gitlab-org/gitlab/-/issues/273574#note_457006686
- https://gitlab.com/gitlab-org/gitlab/-/issues/289798

As Ecosystem is increasingly changing integration data using bulk
operations this has been changed to be maintained through a PostgreSQL
trigger.

https://gitlab.com/gitlab-org/gitlab/-/issues/290715
https://gitlab.com/gitlab-org/gitlab/-/issues/289798

app/models/project.rb

@@ -1333,19 +1333,11 @@ class Project < ApplicationRecord
   end
 
   def external_wiki
-    if has_external_wiki.nil?
-      cache_has_external_wiki
-    end
+    cache_has_external_wiki if has_external_wiki.nil?
 
-    if has_external_wiki
-      @external_wiki ||= services.external_wikis.first
-    else
-      nil
-    end
-  end
+    return unless has_external_wiki?
 
-  def cache_has_external_wiki
-    update_column(:has_external_wiki, services.external_wikis.any?) if Gitlab::Database.read_write?
+    @external_wiki ||= services.external_wikis.first
   end
 
   def find_or_initialize_services
@@ -2707,6 +2699,10 @@ class Project < ApplicationRecord
       objects.each_batch { |relation| out.concat(relation.pluck(:oid)) }
     end
   end
+
+  def cache_has_external_wiki
+    update_column(:has_external_wiki, services.external_wikis.any?) if Gitlab::Database.read_write?
+  end
 end
 
 Project.prepend_if_ee('EE::Project')

app/models/service.rb

@@ -48,7 +48,6 @@ class Service < ApplicationRecord
 
   after_commit :reset_updated_properties
   after_commit :cache_project_has_external_issue_tracker
-  after_commit :cache_project_has_external_wiki
 
   belongs_to :project, inverse_of: :services
   belongs_to :group, inverse_of: :services
@@ -469,12 +468,6 @@ class Service < ApplicationRecord
     end
   end
 
-  def cache_project_has_external_wiki
-    if project && !project.destroyed?
-      project.cache_has_external_wiki
-    end
-  end
-
   def valid_recipients?
     activated? && !importing?
   end

app/services/bulk_create_integration_service.rb

@@ -38,10 +38,6 @@ class BulkCreateIntegrationService
     if integration.external_issue_tracker?
       Project.where(id: batch.select(:id)).update_all(has_external_issue_tracker: true)
     end
-
-    if integration.external_wiki?
-      Project.where(id: batch.select(:id)).update_all(has_external_wiki: true)
-    end
   end
   # rubocop: enable CodeReuse/ActiveRecord
 

changelogs/unreleased/290715-has_external_wiki_trigger.yml

+---
+title: Add PostgreSQL trigger to maintain projects.has_external_wiki
+merge_request: 49916
+author:
+type: changed

db/migrate/20201214032220_add_has_external_wiki_trigger.rb

+# frozen_string_literal: true
+
+class AddHasExternalWikiTrigger < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::SchemaHelpers
+
+  DOWNTIME = false
+  FUNCTION_NAME = 'set_has_external_wiki'.freeze
+  TRIGGER_ON_INSERT_NAME = 'trigger_has_external_wiki_on_insert'.freeze
+  TRIGGER_ON_UPDATE_NAME = 'trigger_has_external_wiki_on_update'.freeze
+  TRIGGER_ON_DELETE_NAME = 'trigger_has_external_wiki_on_delete'.freeze
+
+  def up
+    create_trigger_function(FUNCTION_NAME, replace: true) do
+      <<~SQL
+        UPDATE projects SET has_external_wiki = COALESCE(NEW.active, FALSE)
+        WHERE projects.id = COALESCE(NEW.project_id, OLD.project_id);
+        RETURN NULL;
+      SQL
+    end
+
+    execute(<<~SQL)
+      CREATE TRIGGER #{TRIGGER_ON_INSERT_NAME}
+      AFTER INSERT ON services
+      FOR EACH ROW
+      WHEN (NEW.active = TRUE AND NEW.type = 'ExternalWikiService' AND NEW.project_id IS NOT NULL)
+      EXECUTE FUNCTION #{FUNCTION_NAME}();
+    SQL
+
+    execute(<<~SQL)
+      CREATE TRIGGER #{TRIGGER_ON_UPDATE_NAME}
+      AFTER UPDATE ON services
+      FOR EACH ROW
+      WHEN (NEW.type = 'ExternalWikiService' AND OLD.active != NEW.active AND NEW.project_id IS NOT NULL)
+      EXECUTE FUNCTION #{FUNCTION_NAME}();
+    SQL
+
+    execute(<<~SQL)
+      CREATE TRIGGER #{TRIGGER_ON_DELETE_NAME}
+      AFTER DELETE ON services
+      FOR EACH ROW
+      WHEN (OLD.type = 'ExternalWikiService' AND OLD.project_id IS NOT NULL)
+      EXECUTE FUNCTION #{FUNCTION_NAME}();
+    SQL
+  end
+
+  def down
+    drop_trigger(:services, TRIGGER_ON_INSERT_NAME)
+    drop_trigger(:services, TRIGGER_ON_UPDATE_NAME)
+    drop_trigger(:services, TRIGGER_ON_DELETE_NAME)
+    drop_function(FUNCTION_NAME)
+  end
+end

db/schema_migrations/20201214032220

+db23b5315386ad5d5fec5a14958769cc1e62a0a89ec3246edb9fc024607e917b
\ No newline at end of file

db/structure.sql

@@ -10,6 +10,17 @@ CREATE EXTENSION IF NOT EXISTS btree_gist;
 
 CREATE EXTENSION IF NOT EXISTS pg_trgm;
 
+CREATE FUNCTION set_has_external_wiki() RETURNS trigger
+    LANGUAGE plpgsql
+    AS $$
+BEGIN
+UPDATE projects SET has_external_wiki = COALESCE(NEW.active, FALSE)
+WHERE projects.id = COALESCE(NEW.project_id, OLD.project_id);
+RETURN NULL;
+
+END
+$$;
+
 CREATE FUNCTION table_sync_function_2be879775d() RETURNS trigger
     LANGUAGE plpgsql
     AS $$
@@ -23559,6 +23570,12 @@ ALTER INDEX product_analytics_events_experimental_pkey ATTACH PARTITION gitlab_p
 
 CREATE TRIGGER table_sync_trigger_ee39a25f9d AFTER INSERT OR DELETE OR UPDATE ON audit_events FOR EACH ROW EXECUTE PROCEDURE table_sync_function_2be879775d();
 
+CREATE TRIGGER trigger_has_external_wiki_on_delete AFTER DELETE ON services FOR EACH ROW WHEN ((((old.type)::text = 'ExternalWikiService'::text) AND (old.project_id IS NOT NULL))) EXECUTE PROCEDURE set_has_external_wiki();
+
+CREATE TRIGGER trigger_has_external_wiki_on_insert AFTER INSERT ON services FOR EACH ROW WHEN (((new.active = true) AND ((new.type)::text = 'ExternalWikiService'::text) AND (new.project_id IS NOT NULL))) EXECUTE PROCEDURE set_has_external_wiki();
+
+CREATE TRIGGER trigger_has_external_wiki_on_update AFTER UPDATE ON services FOR EACH ROW WHEN ((((new.type)::text = 'ExternalWikiService'::text) AND (old.active <> new.active) AND (new.project_id IS NOT NULL))) EXECUTE PROCEDURE set_has_external_wiki();
+
 ALTER TABLE ONLY chat_names
     ADD CONSTRAINT fk_00797a2bf9 FOREIGN KEY (service_id) REFERENCES services(id) ON DELETE CASCADE;
 

spec/helpers/projects_helper_spec.rb

@@ -459,6 +459,7 @@ RSpec.describe ProjectsHelper do
     context 'when project has external wiki' do
       it 'includes external wiki tab' do
         project.create_external_wiki_service(active: true, properties: { 'external_wiki_url' => 'https://gitlab.com' })
+        project.reload
 
         is_expected.to include(:external_wiki)
       end

spec/migrations/add_has_external_wiki_trigger_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require_migration!
+
+RSpec.describe AddHasExternalWikiTrigger do
+  let(:migration) { described_class.new }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:services) { table(:services) }
+
+  before do
+    @namespace = namespaces.create!(name: 'foo', path: 'foo')
+    @project = projects.create!(namespace_id: @namespace.id)
+  end
+
+  describe '#up' do
+    before do
+      migrate!
+    end
+
+    describe 'INSERT trigger' do
+      it 'sets `has_external_wiki` to true when active `ExternalWikiService` is inserted' do
+        expect do
+          services.create!(type: 'ExternalWikiService', active: true, project_id: @project.id)
+        end.to change { @project.reload.has_external_wiki }.to(true)
+      end
+
+      it 'does not set `has_external_wiki` to true when service is for a different project' do
+        different_project = projects.create!(namespace_id: @namespace.id)
+
+        expect do
+          services.create!(type: 'ExternalWikiService', active: true, project_id: different_project.id)
+        end.not_to change { @project.reload.has_external_wiki }
+      end
+
+      it 'does not set `has_external_wiki` to true when inactive `ExternalWikiService` is inserted' do
+        expect do
+          services.create!(type: 'ExternalWikiService', active: false, project_id: @project.id)
+        end.not_to change { @project.reload.has_external_wiki }
+      end
+
+      it 'does not set `has_external_wiki` to true when active other service is inserted' do
+        expect do
+          services.create!(type: 'MyService', active: true, project_id: @project.id)
+        end.not_to change { @project.reload.has_external_wiki }
+      end
+    end
+
+    describe 'UPDATE trigger' do
+      it 'sets `has_external_wiki` to true when `ExternalWikiService` is made active' do
+        service = services.create!(type: 'ExternalWikiService', active: false, project_id: @project.id)
+
+        expect do
+          service.update!(active: true)
+        end.to change { @project.reload.has_external_wiki }.to(true)
+      end
+
+      it 'sets `has_external_wiki` to false when `ExternalWikiService` is made inactive' do
+        service = services.create!(type: 'ExternalWikiService', active: true, project_id: @project.id)
+
+        expect do
+          service.update!(active: false)
+        end.to change { @project.reload.has_external_wiki }.to(false)
+      end
+
+      it 'does not change `has_external_wiki` when service is for a different project' do
+        different_project = projects.create!(namespace_id: @namespace.id)
+        service = services.create!(type: 'ExternalWikiService', active: false, project_id: different_project.id)
+
+        expect do
+          service.update!(active: true)
+        end.not_to change { @project.reload.has_external_wiki }
+      end
+    end
+
+    describe 'DELETE trigger' do
+      it 'sets `has_external_wiki` to false when `ExternalWikiService` is deleted' do
+        service = services.create!(type: 'ExternalWikiService', active: true, project_id: @project.id)
+
+        expect do
+          service.delete
+        end.to change { @project.reload.has_external_wiki }.to(false)
+      end
+
+      it 'does not change `has_external_wiki` when service is for a different project' do
+        different_project = projects.create!(namespace_id: @namespace.id)
+        service = services.create!(type: 'ExternalWikiService', active: true, project_id: different_project.id)
+
+        expect do
+          service.delete
+        end.not_to change { @project.reload.has_external_wiki }
+      end
+    end
+  end
+
+  describe '#down' do
+    before do
+      migration.up
+      migration.down
+    end
+
+    it 'drops the INSERT trigger' do
+      expect do
+        services.create!(type: 'ExternalWikiService', active: true, project_id: @project.id)
+      end.not_to change { @project.reload.has_external_wiki }
+    end
+
+    it 'drops the UPDATE trigger' do
+      service = services.create!(type: 'ExternalWikiService', active: false, project_id: @project.id)
+      @project.update!(has_external_wiki: false)
+
+      expect do
+        service.update!(active: true)
+      end.not_to change { @project.reload.has_external_wiki }
+    end
+
+    it 'drops the DELETE trigger' do
+      service = services.create!(type: 'ExternalWikiService', active: true, project_id: @project.id)
+      @project.update!(has_external_wiki: true)
+
+      expect do
+        service.delete
+      end.not_to change { @project.reload.has_external_wiki }
+    end
+  end
+end

spec/models/project_spec.rb

@@ -1067,36 +1067,6 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
-  describe '#cache_has_external_wiki' do
-    let_it_be(:project) { create(:project, has_external_wiki: nil) }
-
-    it 'stores true if there is any external_wikis' do
-      services = double(:service, external_wikis: [ExternalWikiService.new])
-      expect(project).to receive(:services).and_return(services)
-
-      expect do
-        project.cache_has_external_wiki
-      end.to change { project.has_external_wiki}.to(true)
-    end
-
-    it 'stores false if there is no external_wikis' do
-      services = double(:service, external_wikis: [])
-      expect(project).to receive(:services).and_return(services)
-
-      expect do
-        project.cache_has_external_wiki
-      end.to change { project.has_external_wiki}.to(false)
-    end
-
-    it 'does not cache data when in a read-only GitLab instance' do
-      allow(Gitlab::Database).to receive(:read_only?) { true }
-
-      expect do
-        project.cache_has_external_wiki
-      end.not_to change { project.has_external_wiki }
-    end
-  end
-
   describe '#has_wiki?' do
     let(:no_wiki_project)       { create(:project, :wiki_disabled, has_external_wiki: false) }
     let(:wiki_enabled_project)  { create(:project) }
@@ -1136,52 +1106,64 @@ RSpec.describe Project, factory_default: :keep do
   describe '#external_wiki' do
     let_it_be(:project) { create(:project) }
 
-    context 'with an active external wiki' do
-      before do
-        create(:service, project: project, type: 'ExternalWikiService', active: true)
-        project.external_wiki
-      end
+    def subject
+      project.reload.external_wiki
+    end
 
-      it 'sets :has_external_wiki as true' do
-        expect(project.has_external_wiki).to be(true)
-      end
+    it 'returns an active external wiki' do
+      create(:service, project: project, type: 'ExternalWikiService', active: true)
 
-      it 'sets :has_external_wiki as false if an external wiki service is destroyed later' do
-        expect(project.has_external_wiki).to be(true)
+      is_expected.to be_kind_of(ExternalWikiService)
+    end
 
-        project.services.external_wikis.first.destroy
+    it 'does not return an inactive external wiki' do
+      create(:service, project: project, type: 'ExternalWikiService', active: false)
 
-        expect(project.has_external_wiki).to be(false)
-      end
+      is_expected.to eq(nil)
     end
 
-    context 'with an inactive external wiki' do
-      before do
-        create(:service, project: project, type: 'ExternalWikiService', active: false)
-      end
+    it 'sets Project#has_external_wiki when it is nil' do
+      create(:service, project: project, type: 'ExternalWikiService', active: true)
+      project.update_column(:has_external_wiki, nil)
 
-      it 'sets :has_external_wiki as false' do
-        expect(project.has_external_wiki).to be(false)
-      end
+      expect { subject }.to change { project.has_external_wiki }.from(nil).to(true)
     end
+  end
 
-    context 'with no external wiki' do
-      before do
-        project.external_wiki
-      end
+  describe '#has_external_wiki' do
+    let_it_be(:project) { create(:project) }
 
-      it 'sets :has_external_wiki as false' do
-        expect(project.has_external_wiki).to be(false)
-      end
+    def subject
+      project.reload.has_external_wiki
+    end
 
-      it 'sets :has_external_wiki as true if an external wiki service is created later' do
-        expect(project.has_external_wiki).to be(false)
+    specify { is_expected.to eq(false) }
 
+    context 'when there is an active external wiki service' do
+      let!(:service) do
         create(:service, project: project, type: 'ExternalWikiService', active: true)
+      end
+
+      specify { is_expected.to eq(true) }
+
+      it 'becomes false if the external wiki service is destroyed' do
+        expect do
+          Service.find(service.id).delete
+        end.to change { subject }.to(false)
+      end
 
-        expect(project.has_external_wiki).to be(true)
+      it 'becomes false if the external wiki service becomes inactive' do
+        expect do
+          service.update_column(:active, false)
+        end.to change { subject }.to(false)
       end
     end
+
+    it 'is false when external wiki service is not active' do
+      create(:service, project: project, type: 'ExternalWikiService', active: false)
+
+      is_expected.to eq(false)
+    end
   end
 
   describe '#star_count' do