diff --git a/app/models/project.rb b/app/models/project.rb index fdb16640cb421607a05f7bc22840d2d19e9a70c7..8939b2ee84dadb24dd62d20dced86c49a413b540 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1333,19 +1333,11 @@ def has_wiki? 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 @@ 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/app/models/service.rb b/app/models/service.rb index 57c099d6f04839f8d1348f3393ab39834693cf4c..9f17279d0a3eb9ff641a61d515d683de87548951 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 61c5565db606f00bde58b84061ead036d497b1e0..df78c3645c74eac399a38bc5966dccdd588fbee2 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 0000000000000000000000000000000000000000..fcc635e53f95be2b9c1d08304915be24dd37365a --- /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 0000000000000000000000000000000000000000..f6e066b75daac56e1d921e8c9837211f8cb993e2 --- /dev/null +++ b/db/migrate/20201214032220_add_has_external_wiki_trigger.rb @@ -0,0 +1,52 @@ +# 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 diff --git a/db/schema_migrations/20201214032220 b/db/schema_migrations/20201214032220 new file mode 100644 index 0000000000000000000000000000000000000000..ec14b260583699d8a262e81fd852cc5737c8390e --- /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 315cb9fe5c318f2d99d89df96e5ca4c99eb800da..4d1c265a05f8ad9c54b5c8dcd6c33c2cb96068a2 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 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; diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index d28d5ecda1b69b4aae33af3e513aa090da6d88ee..3c527b850229c2c2d8db2add38f01276dcf94296 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 0000000000000000000000000000000000000000..10c6888c87e48fca79be3c7cf0ae74dd26a6e679 --- /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.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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 7c3eed14bcf33e2b2f2ad86dd0b0ead967dcbc8c..b6875e3de6e5bca8fc4a5eb1301f927c1ec174e9 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,64 @@ 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