diff --git a/changelogs/unreleased/290715-change-project-has_external_wiki-to-be-not-null.yml b/changelogs/unreleased/290715-change-project-has_external_wiki-to-be-not-null.yml new file mode 100644 index 0000000000000000000000000000000000000000..b2534a7fc8cbeaf533cf190b0be1eefba7a96ce9 --- /dev/null +++ b/changelogs/unreleased/290715-change-project-has_external_wiki-to-be-not-null.yml @@ -0,0 +1,6 @@ +--- +title: Change projects.has_external_wiki column to be not null and default to false, + and cleanup all null and incorrect data +merge_request: 50916 +author: +type: changed diff --git a/db/migrate/20210105025900_add_default_projects_has_external_wiki.rb b/db/migrate/20210105025900_add_default_projects_has_external_wiki.rb new file mode 100644 index 0000000000000000000000000000000000000000..cd74208d58c2e54e5a12137cc48a9320ab2ea897 --- /dev/null +++ b/db/migrate/20210105025900_add_default_projects_has_external_wiki.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddDefaultProjectsHasExternalWiki < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + with_lock_retries do + change_column_default(:projects, :has_external_wiki, from: nil, to: false) + end + end + + def down + with_lock_retries do + change_column_default(:projects, :has_external_wiki, from: false, to: nil) + end + end +end diff --git a/db/post_migrate/20210105025903_add_not_null_constraint_to_projects_has_external_wiki.rb b/db/post_migrate/20210105025903_add_not_null_constraint_to_projects_has_external_wiki.rb new file mode 100644 index 0000000000000000000000000000000000000000..0560258592f7ef4d8c0910cfb868c03a8624f8ac --- /dev/null +++ b/db/post_migrate/20210105025903_add_not_null_constraint_to_projects_has_external_wiki.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddNotNullConstraintToProjectsHasExternalWiki < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_not_null_constraint :projects, :has_external_wiki, validate: false + end + + def down + remove_not_null_constraint :projects, :has_external_wiki + end +end diff --git a/db/post_migrate/20210105030124_cleanup_projects_with_null_has_external_wiki.rb b/db/post_migrate/20210105030124_cleanup_projects_with_null_has_external_wiki.rb new file mode 100644 index 0000000000000000000000000000000000000000..7995ac6a39394e6da702622daf4ad6922cecb7e8 --- /dev/null +++ b/db/post_migrate/20210105030124_cleanup_projects_with_null_has_external_wiki.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +class CleanupProjectsWithNullHasExternalWiki < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + TMP_INDEX_NAME = 'tmp_index_projects_on_id_where_has_external_wiki_is_true_null'.freeze + BATCH_SIZE = 100 + + disable_ddl_transaction! + + class Service < ActiveRecord::Base + include EachBatch + belongs_to :project + + self.table_name = 'services' + self.inheritance_column = :_type_disabled + end + + class Project < ActiveRecord::Base + include EachBatch + + self.table_name = 'projects' + end + + def up + update_projects_with_active_external_wikis + update_projects_without_active_external_wikis + end + + def down + # no-op : can't go back to `NULL` without first dropping the `NOT NULL` constraint + end + + private + + def update_projects_with_active_external_wikis + # 11 projects are scoped in this query on GitLab.com. + scope = Service.where(active: true, type: 'ExternalWikiService').where.not(project_id: nil) + + scope.each_batch(of: BATCH_SIZE) do |relation| + scope_with_projects = relation + .joins(:project) + .select('project_id') + .merge(Project.where(has_external_wiki: [false, nil]).where(pending_delete: false).where(archived: false)) + + execute(<<~SQL) + WITH project_ids_to_update (id) AS ( + #{scope_with_projects.to_sql} + ) + UPDATE projects SET has_external_wiki = true WHERE id IN (SELECT id FROM project_ids_to_update) + SQL + end + end + + def update_projects_without_active_external_wikis + # Add a temporary index to speed up the scoping of projects. + index_where = <<~SQL + ( + "projects"."has_external_wiki" = TRUE + OR "projects"."has_external_wiki" IS NULL + ) + AND "projects"."pending_delete" = FALSE + AND "projects"."archived" = FALSE + SQL + + add_concurrent_index(:projects, :id, where: index_where, name: TMP_INDEX_NAME) + + services_sub_query = Service + .select('1') + .where('services.project_id = projects.id') + .where(type: 'ExternalWikiService') + .where(active: true) + + # 322 projects are scoped in this query on GitLab.com. + Project.where(index_where).each_batch(of: BATCH_SIZE) do |relation| + relation_with_exists_query = relation.where('NOT EXISTS (?)', services_sub_query) + execute(<<~SQL) + WITH project_ids_to_update (id) AS ( + #{relation_with_exists_query.select(:id).to_sql} + ) + UPDATE projects SET has_external_wiki = false WHERE id IN (SELECT id FROM project_ids_to_update) + SQL + end + + # Drop the temporary index. + remove_concurrent_index_by_name(:projects, TMP_INDEX_NAME) + end +end diff --git a/db/schema_migrations/20210105025900 b/db/schema_migrations/20210105025900 new file mode 100644 index 0000000000000000000000000000000000000000..200ee5b7ff2eafbee823792cfcc60cfd57208310 --- /dev/null +++ b/db/schema_migrations/20210105025900 @@ -0,0 +1 @@ +047dd77352eda8134e55047e2d3fab07bbcd6eb41600cefb9b581d32e441fb68 \ No newline at end of file diff --git a/db/schema_migrations/20210105025903 b/db/schema_migrations/20210105025903 new file mode 100644 index 0000000000000000000000000000000000000000..6680ba6bbacf9c831f8a53fc8310e18d26cb4c12 --- /dev/null +++ b/db/schema_migrations/20210105025903 @@ -0,0 +1 @@ +339d828635107f77116ffd856abcb8f8f64985cabb82c0c34ab76056f5756d2e \ No newline at end of file diff --git a/db/schema_migrations/20210105030124 b/db/schema_migrations/20210105030124 new file mode 100644 index 0000000000000000000000000000000000000000..6fa3d1b147432c33ca5f60c7490fce10faedfc68 --- /dev/null +++ b/db/schema_migrations/20210105030124 @@ -0,0 +1 @@ +30f3cbc0f96848f72a188616503eb80d38c33769c8bebf86a5922b374750d066 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index c042e83cdb6bcc9b12ccfd37b29158a73126cb49..fe70020a3f6ffdbd9f396fe2ceb1fdd5b84bb8c2 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -16105,7 +16105,7 @@ CREATE TABLE projects ( repository_storage character varying DEFAULT 'default'::character varying NOT NULL, repository_read_only boolean, request_access_enabled boolean DEFAULT true NOT NULL, - has_external_wiki boolean, + has_external_wiki boolean DEFAULT false, ci_config_path character varying, lfs_enabled boolean, description_html text, @@ -19618,6 +19618,9 @@ ALTER TABLE ONLY chat_teams ALTER TABLE vulnerability_scanners ADD CONSTRAINT check_37608c9db5 CHECK ((char_length(vendor) <= 255)) NOT VALID; +ALTER TABLE projects + ADD CONSTRAINT check_421d399b70 CHECK ((has_external_wiki IS NOT NULL)) NOT VALID; + ALTER TABLE group_import_states ADD CONSTRAINT check_cda75c7c3f CHECK ((user_id IS NOT NULL)) NOT VALID; diff --git a/spec/migrations/cleanup_projects_with_null_has_external_wiki_spec.rb b/spec/migrations/cleanup_projects_with_null_has_external_wiki_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..c3be936f0f163b35a66c1391a2a1e8cf084f3e73 --- /dev/null +++ b/spec/migrations/cleanup_projects_with_null_has_external_wiki_spec.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require_migration! + +RSpec.describe CleanupProjectsWithNullHasExternalWiki, :migration, schema: 20210105025900 do + let(:namespace) { table(:namespaces).create!(name: 'foo', path: 'bar') } + let(:projects) { table(:projects) } + let(:services) { table(:services) } + let(:constraint_name) { 'check_421d399b70' } + + def create_projects!(num) + Array.new(num) do + projects.create!(namespace_id: namespace.id) + end + end + + def create_active_external_wiki_integrations!(*projects) + projects.each do |project| + services.create!(type: 'ExternalWikiService', project_id: project.id, active: true) + end + end + + def create_disabled_external_wiki_integrations!(*projects) + projects.each do |project| + services.create!(type: 'ExternalWikiService', project_id: project.id, active: false) + end + end + + def create_active_other_integrations!(*projects) + projects.each do |project| + services.create!(type: 'NotAnExternalWikiService', project_id: project.id, active: true) + end + end + + it 'sets `projects.has_external_wiki` correctly' do + allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(false) + + project_with_external_wiki_1, + project_with_external_wiki_2, + project_with_external_wiki_3, + project_with_disabled_external_wiki_1, + project_with_disabled_external_wiki_2, + project_with_disabled_external_wiki_3, + project_without_external_wiki_1, + project_without_external_wiki_2, + project_without_external_wiki_3 = create_projects!(9) + + create_active_external_wiki_integrations!( + project_with_external_wiki_1, + project_with_external_wiki_2, + project_with_external_wiki_3 + ) + + create_disabled_external_wiki_integrations!( + project_with_disabled_external_wiki_1, + project_with_disabled_external_wiki_2, + project_with_disabled_external_wiki_3 + ) + + create_active_other_integrations!( + project_without_external_wiki_1, + project_without_external_wiki_2, + project_without_external_wiki_3 + ) + + # PG triggers on the services table added in a previous migration + # will have set the `has_external_wiki` columns to correct data when + # the services records were created above. + # + # We set the `has_external_wiki` columns for projects to NULL or incorrect + # data manually below to emulate projects in a state before the PG + # triggers were added. + project_with_external_wiki_1.update!(has_external_wiki: nil) + project_with_external_wiki_2.update!(has_external_wiki: false) + + project_with_disabled_external_wiki_1.update!(has_external_wiki: nil) + project_with_disabled_external_wiki_2.update!(has_external_wiki: true) + + project_without_external_wiki_1.update!(has_external_wiki: nil) + project_without_external_wiki_2.update!(has_external_wiki: true) + + migrate! + + expected_true = [ + project_with_external_wiki_1, + project_with_external_wiki_2, + project_with_external_wiki_3 + ].each(&:reload).map(&:has_external_wiki) + + expected_false = [ + project_without_external_wiki_1, + project_without_external_wiki_2, + project_without_external_wiki_3, + project_with_disabled_external_wiki_1, + project_with_disabled_external_wiki_2, + project_with_disabled_external_wiki_3 + ].each(&:reload).map(&:has_external_wiki) + + expect(expected_true).to all(eq(true)) + expect(expected_false).to all(eq(false)) + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 59464c89dc141735723f5a0260b54faf31a1995f..7a7b7af8afeb78d77bddfd51f3a85274e909cc6f 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1145,11 +1145,32 @@ def subject is_expected.to eq(nil) end - it 'sets Project#has_external_wiki when it is nil' do + it 'calls Project#cache_has_external_wiki when `has_external_wiki` is nil' do + project = build(:project, has_external_wiki: nil) + + expect(project).to receive(:cache_has_external_wiki) + + project.external_wiki + end + + it 'does not call Project#cache_has_external_wiki when `has_external_wiki` is not nil' do + project = build(:project) + + expect(project).not_to receive(:cache_has_external_wiki) + + project.external_wiki + end + end + + describe '#cache_has_external_wiki (private method)' do + it 'sets Project#has_external_wiki correctly, affecting Project#external_wiki' do + project = create(:project) create(:service, project: project, type: 'ExternalWikiService', active: true) - project.update_column(:has_external_wiki, nil) + project.update_column(:has_external_wiki, false) - expect { subject }.to change { project.has_external_wiki }.from(nil).to(true) + expect { project.send(:cache_has_external_wiki) } + .to change { project.has_external_wiki }.from(false).to(true) + .and(change { project.external_wiki }.from(nil).to(kind_of(ExternalWikiService))) end end