Merge branch '290715-has_external_wiki_trigger' into 'master'

Add PG trigger to maintain `projects.has_external_wiki`

See merge request gitlab-org/gitlab!49916

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

@@ -433,6 +433,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,51 +1106,63 @@ 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
-
-      it 'sets :has_external_wiki as true' do
-        expect(project.has_external_wiki).to be(true)
+    def subject
+      project.reload.external_wiki
     end
 
-      it 'sets :has_external_wiki as false if an external wiki service is destroyed later' do
-        expect(project.has_external_wiki).to be(true)
-
-        project.services.external_wikis.first.destroy
+    it 'returns an active external wiki' do
+      create(:service, project: project, type: 'ExternalWikiService', active: true)
 
-        expect(project.has_external_wiki).to be(false)
-      end
+      is_expected.to be_kind_of(ExternalWikiService)
     end
 
-    context 'with an inactive external wiki' do
-      before do
+    it 'does not return an inactive external wiki' do
       create(:service, project: project, type: 'ExternalWikiService', active: false)
+
+      is_expected.to eq(nil)
     end
 
-      it 'sets :has_external_wiki as false' do
-        expect(project.has_external_wiki).to be(false)
+    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)
+
+      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)
+    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