From 1c29f1d15e4d97c4d3b621f9f5a7352ff999b62b Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 6 Jan 2021 12:45:58 +1300 Subject: [PATCH 1/4] Set has_external_wiki to NOT NULL default FALSE This change contains migrations for `projects.has_external_wiki` to become a `NOT NULL` column that defaults to `FALSE`. This is possible after the PG Trigger added in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49916 that always maintains `projects.has_external_wiki` as a boolean and never a `NULL`. It includes a data migration to set all historical `NULL` columns to be either `TRUE` or `FALSE`. The data migration also fixes the historical bad data for `has_external_wiki`: https://gitlab.com/gitlab-org/gitlab/-/issues/273574. https://gitlab.com/gitlab-org/gitlab/-/issues/290715. --- ...oject-has_external_wiki-to-be-not-null.yml | 6 + ..._add_default_projects_has_external_wiki.rb | 19 ++++ ...onstraint_to_projects_has_external_wiki.rb | 16 +++ ...up_projects_with_null_has_external_wiki.rb | 87 +++++++++++++++ db/schema_migrations/20210105025900 | 1 + db/schema_migrations/20210105025903 | 1 + db/schema_migrations/20210105030124 | 1 + db/structure.sql | 5 +- ...ojects_with_null_has_external_wiki_spec.rb | 104 ++++++++++++++++++ 9 files changed, 239 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/290715-change-project-has_external_wiki-to-be-not-null.yml create mode 100644 db/migrate/20210105025900_add_default_projects_has_external_wiki.rb create mode 100644 db/post_migrate/20210105025903_add_not_null_constraint_to_projects_has_external_wiki.rb create mode 100644 db/post_migrate/20210105030124_cleanup_projects_with_null_has_external_wiki.rb create mode 100644 db/schema_migrations/20210105025900 create mode 100644 db/schema_migrations/20210105025903 create mode 100644 db/schema_migrations/20210105030124 create mode 100644 spec/migrations/cleanup_projects_with_null_has_external_wiki_spec.rb 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 00000000000000..b2534a7fc8cbea --- /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 00000000000000..cd74208d58c2e5 --- /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 00000000000000..0560258592f7ef --- /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 00000000000000..22ecd9067fabc9 --- /dev/null +++ b/db/post_migrate/20210105030124_cleanup_projects_with_null_has_external_wiki.rb @@ -0,0 +1,87 @@ +# 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 + + disable_ddl_transaction! + + class Service < ActiveRecord::Base + include EachBatch + belongs_to :project + + self.table_name = 'services' + 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: 100) 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: 1000) 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 00000000000000..200ee5b7ff2eaf --- /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 00000000000000..6680ba6bbacf9c --- /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 00000000000000..6fa3d1b147432c --- /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 c042e83cdb6bcc..fe70020a3f6ffd 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 00000000000000..c3be936f0f163b --- /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 -- GitLab From 58738ac5496ca141fa2bc7cf4c1395b97901077e Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 27 Jan 2021 12:33:00 +1300 Subject: [PATCH 2/4] Add reviewer feedback --- ...10105030124_cleanup_projects_with_null_has_external_wiki.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 index 22ecd9067fabc9..2d025f80009758 100644 --- 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 @@ -13,6 +13,7 @@ class Service < ActiveRecord::Base belongs_to :project self.table_name = 'services' + self.inheritance_column = :_type_disabled end class Project < ActiveRecord::Base @@ -71,7 +72,7 @@ def update_projects_without_active_external_wikis .where(active: true) # 322 projects are scoped in this query on GitLab.com. - Project.where(index_where).each_batch(of: 1000) do |relation| + Project.where(index_where).each_batch(of: 1_000) do |relation| relation_with_exists_query = relation.where('NOT EXISTS (?)', services_sub_query) execute(<<~SQL) WITH project_ids_to_update (id) AS ( -- GitLab From a557d5118cbd545f39d659ea408c20aab4032a24 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 28 Jan 2021 16:02:30 +1300 Subject: [PATCH 3/4] Add reviewer feedback --- ...105030124_cleanup_projects_with_null_has_external_wiki.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 index 2d025f80009758..7995ac6a39394e 100644 --- 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 @@ -5,6 +5,7 @@ class CleanupProjectsWithNullHasExternalWiki < ActiveRecord::Migration[6.0] DOWNTIME = false TMP_INDEX_NAME = 'tmp_index_projects_on_id_where_has_external_wiki_is_true_null'.freeze + BATCH_SIZE = 100 disable_ddl_transaction! @@ -37,7 +38,7 @@ 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: 100) do |relation| + scope.each_batch(of: BATCH_SIZE) do |relation| scope_with_projects = relation .joins(:project) .select('project_id') @@ -72,7 +73,7 @@ def update_projects_without_active_external_wikis .where(active: true) # 322 projects are scoped in this query on GitLab.com. - Project.where(index_where).each_batch(of: 1_000) do |relation| + 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 ( -- GitLab From 39dff8e1334586824d80fecbb111a9849d446bb4 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 28 Jan 2021 17:34:52 +1300 Subject: [PATCH 4/4] Re-work Project#external_wiki test We can no longer have projects with `nil` values for `has_external_wiki` after the migrations added in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/50916. Until we remove the `Project#cache_has_external_wiki` method in https://gitlab.com/gitlab-org/gitlab/-/issues/300247 the spec testing this behaviour has been reworked to first verify that `#external_wiki` can call `#cache_has_external_wiki` will `has_external_wiki` is `nil`, and that `#cache_has_external_wiki` will update `has_external_wiki` correctly. --- spec/models/project_spec.rb | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 59464c89dc1417..7a7b7af8afeb78 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 -- GitLab