From 51f21766e7168848c5a8b6f75c93a07dcb69c1c9 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Mon, 14 Dec 2020 18:21:50 +1300 Subject: [PATCH 1/3] 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 | 14 +- app/models/service.rb | 7 - .../bulk_create_integration_service.rb | 4 - .../290715-has_external_wiki_trigger.yml | 5 + ...214032220_add_has_external_wiki_trigger.rb | 63 +++++++++ db/schema_migrations/20201214032220 | 1 + db/structure.sql | 17 +++ spec/helpers/projects_helper_spec.rb | 1 + .../add_has_external_wiki_trigger_spec.rb | 128 ++++++++++++++++++ spec/models/project_spec.rb | 95 +++++-------- 10 files changed, 252 insertions(+), 83 deletions(-) create mode 100644 changelogs/unreleased/290715-has_external_wiki_trigger.yml create mode 100644 db/migrate/20201214032220_add_has_external_wiki_trigger.rb create mode 100644 db/schema_migrations/20201214032220 create mode 100644 spec/migrations/add_has_external_wiki_trigger_spec.rb diff --git a/app/models/project.rb b/app/models/project.rb index fdb16640cb4216..7e30d70be4c5f3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1333,19 +1333,9 @@ def has_wiki? end def external_wiki - if has_external_wiki.nil? - cache_has_external_wiki - end - - 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 diff --git a/app/models/service.rb b/app/models/service.rb index 57c099d6f04839..9f17279d0a3eb9 100644 --- a/app/models/service.rb +++ b/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 @@ def cache_project_has_external_issue_tracker 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 diff --git a/app/services/bulk_create_integration_service.rb b/app/services/bulk_create_integration_service.rb index 61c5565db606f0..df78c3645c74ea 100644 --- a/app/services/bulk_create_integration_service.rb +++ b/app/services/bulk_create_integration_service.rb @@ -38,10 +38,6 @@ def run_callbacks(batch) 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 diff --git a/changelogs/unreleased/290715-has_external_wiki_trigger.yml b/changelogs/unreleased/290715-has_external_wiki_trigger.yml new file mode 100644 index 00000000000000..fcc635e53f95be --- /dev/null +++ b/changelogs/unreleased/290715-has_external_wiki_trigger.yml @@ -0,0 +1,5 @@ +--- +title: Add PostgreSQL trigger to maintain projects.has_external_wiki +merge_request: 49916 +author: +type: changed diff --git a/db/migrate/20201214032220_add_has_external_wiki_trigger.rb b/db/migrate/20201214032220_add_has_external_wiki_trigger.rb new file mode 100644 index 00000000000000..b5f5016368555b --- /dev/null +++ b/db/migrate/20201214032220_add_has_external_wiki_trigger.rb @@ -0,0 +1,63 @@ +# 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 NEW; + SQL + end + + execute(<<~SQL) + CREATE TRIGGER #{TRIGGER_ON_INSERT_NAME} + AFTER INSERT ON services + FOR EACH ROW + WHEN (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 + execute(<<~SQL) + DROP TRIGGER #{TRIGGER_ON_INSERT_NAME} ON services; + SQL + + execute(<<~SQL) + DROP TRIGGER #{TRIGGER_ON_UPDATE_NAME} ON services; + SQL + + execute(<<~SQL) + DROP TRIGGER #{TRIGGER_ON_DELETE_NAME} ON services; + SQL + + execute(<<~SQL) + DROP FUNCTION #{FUNCTION_NAME}; + SQL + end +end diff --git a/db/schema_migrations/20201214032220 b/db/schema_migrations/20201214032220 new file mode 100644 index 00000000000000..ec14b260583699 --- /dev/null +++ b/db/schema_migrations/20201214032220 @@ -0,0 +1 @@ +db23b5315386ad5d5fec5a14958769cc1e62a0a89ec3246edb9fc024607e917b \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 315cb9fe5c318f..ba21e4201e5cf5 100644 --- a/db/structure.sql +++ b/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 NEW; + +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.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; diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index d28d5ecda1b69b..3c527b850229c2 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -459,6 +459,7 @@ 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 diff --git a/spec/migrations/add_has_external_wiki_trigger_spec.rb b/spec/migrations/add_has_external_wiki_trigger_spec.rb new file mode 100644 index 00000000000000..e32763bfb70045 --- /dev/null +++ b/spec/migrations/add_has_external_wiki_trigger_spec.rb @@ -0,0 +1,128 @@ +# 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.to change { @project.reload.has_external_wiki }.to(false) # (Changes from `nil`) + 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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 7c3eed14bcf33e..04038319beeb5d 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1067,36 +1067,6 @@ 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,57 @@ 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 + end - context 'with an inactive external wiki' do - before do - create(:service, project: project, type: 'ExternalWikiService', active: false) - 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 - context 'with no external wiki' do - before do - project.external_wiki - end + specify { is_expected.to eq(false) } - it 'sets :has_external_wiki as false' do - expect(project.has_external_wiki).to be(false) + context 'when there is an active external wiki service' do + let!(:service) do + create(:service, project: project, type: 'ExternalWikiService', active: true) 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(true) } - create(:service, project: project, type: 'ExternalWikiService', active: 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 -- GitLab From 6deddf9cb545915d958815b070db78dee7e0d288 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 5 Jan 2021 15:50:06 +1300 Subject: [PATCH 2/3] Re-add support for lazy-caching has_external_wiki https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49916/diffs#note_465796664 --- app/models/project.rb | 6 ++++++ spec/models/project_spec.rb | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/app/models/project.rb b/app/models/project.rb index 7e30d70be4c5f3..8939b2ee84dadb 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1333,6 +1333,8 @@ def has_wiki? end def external_wiki + cache_has_external_wiki if has_external_wiki.nil? + return unless has_external_wiki? @external_wiki ||= services.external_wikis.first @@ -2697,6 +2699,10 @@ def oids(objects, oids: []) 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') diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 04038319beeb5d..b6875e3de6e5bc 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1121,6 +1121,13 @@ def subject 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) + project.update_column(:has_external_wiki, nil) + + expect { subject }.to change { project.has_external_wiki }.from(nil).to(true) + end end describe '#has_external_wiki' do -- GitLab From 48cea8dd08d98ba855605e21f87c9a697f0c067a Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Mon, 11 Jan 2021 14:41:51 +1300 Subject: [PATCH 3/3] Add reviewer feedback --- ...214032220_add_has_external_wiki_trigger.rb | 23 +++++-------------- db/structure.sql | 4 ++-- .../add_has_external_wiki_trigger_spec.rb | 2 +- 3 files changed, 9 insertions(+), 20 deletions(-) diff --git a/db/migrate/20201214032220_add_has_external_wiki_trigger.rb b/db/migrate/20201214032220_add_has_external_wiki_trigger.rb index b5f5016368555b..f6e066b75daac5 100644 --- a/db/migrate/20201214032220_add_has_external_wiki_trigger.rb +++ b/db/migrate/20201214032220_add_has_external_wiki_trigger.rb @@ -14,7 +14,7 @@ def up <<~SQL UPDATE projects SET has_external_wiki = COALESCE(NEW.active, FALSE) WHERE projects.id = COALESCE(NEW.project_id, OLD.project_id); - RETURN NEW; + RETURN NULL; SQL end @@ -22,7 +22,7 @@ def up CREATE TRIGGER #{TRIGGER_ON_INSERT_NAME} AFTER INSERT ON services FOR EACH ROW - WHEN (NEW.type = 'ExternalWikiService' AND NEW.project_id IS NOT NULL) + WHEN (NEW.active = TRUE AND NEW.type = 'ExternalWikiService' AND NEW.project_id IS NOT NULL) EXECUTE FUNCTION #{FUNCTION_NAME}(); SQL @@ -44,20 +44,9 @@ def up end def down - execute(<<~SQL) - DROP TRIGGER #{TRIGGER_ON_INSERT_NAME} ON services; - SQL - - execute(<<~SQL) - DROP TRIGGER #{TRIGGER_ON_UPDATE_NAME} ON services; - SQL - - execute(<<~SQL) - DROP TRIGGER #{TRIGGER_ON_DELETE_NAME} ON services; - SQL - - execute(<<~SQL) - DROP FUNCTION #{FUNCTION_NAME}; - SQL + 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 diff --git a/db/structure.sql b/db/structure.sql index ba21e4201e5cf5..4d1c265a05f8ad 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -16,7 +16,7 @@ CREATE FUNCTION set_has_external_wiki() RETURNS trigger BEGIN UPDATE projects SET has_external_wiki = COALESCE(NEW.active, FALSE) WHERE projects.id = COALESCE(NEW.project_id, OLD.project_id); -RETURN NEW; +RETURN NULL; END $$; @@ -23572,7 +23572,7 @@ CREATE TRIGGER table_sync_trigger_ee39a25f9d AFTER INSERT OR DELETE OR UPDATE ON 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.type)::text = 'ExternalWikiService'::text) AND (new.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(); diff --git a/spec/migrations/add_has_external_wiki_trigger_spec.rb b/spec/migrations/add_has_external_wiki_trigger_spec.rb index e32763bfb70045..10c6888c87e48f 100644 --- a/spec/migrations/add_has_external_wiki_trigger_spec.rb +++ b/spec/migrations/add_has_external_wiki_trigger_spec.rb @@ -38,7 +38,7 @@ 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.to change { @project.reload.has_external_wiki }.to(false) # (Changes from `nil`) + 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 -- GitLab