From 58914b3c56ab01f56031872e3796a5931e02fa2f Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Mon, 18 Jan 2021 14:35:19 +1300 Subject: [PATCH] Set has_external_issue_tracker to NOT NULL This change contains migrations for `projects.has_external_issue_tracker` 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/51852 that always maintains `projects.has_external_issue_tracker` 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_issue_tracker`: https://gitlab.com/gitlab-org/gitlab/-/issues/273574. https://gitlab.com/gitlab-org/gitlab/-/issues/290715. --- app/models/project.rb | 6 - ..._external_issue_tracker-to-be-not-null.yml | 6 + ...ult_projects_has_external_issue_tracker.rb | 9 ++ ..._to_projects_has_external_issue_tracker.rb | 17 +++ ...ts_with_null_has_external_issue_tracker.rb | 54 +++++++++ db/schema_migrations/20210118011518 | 1 + db/schema_migrations/20210118011623 | 1 + db/schema_migrations/20210118011840 | 1 + db/structure.sql | 5 +- ...th_null_has_external_issue_tracker_spec.rb | 112 ++++++++++++++++++ spec/models/project_spec.rb | 13 -- 11 files changed, 205 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/290715-change-project-has_external_issue_tracker-to-be-not-null.yml create mode 100644 db/migrate/20210118011518_add_default_projects_has_external_issue_tracker.rb create mode 100644 db/post_migrate/20210118011623_add_not_null_constraint_to_projects_has_external_issue_tracker.rb create mode 100644 db/post_migrate/20210118011840_cleanup_projects_with_null_has_external_issue_tracker.rb create mode 100644 db/schema_migrations/20210118011518 create mode 100644 db/schema_migrations/20210118011623 create mode 100644 db/schema_migrations/20210118011840 create mode 100644 spec/migrations/cleanup_projects_with_null_has_external_issue_tracker_spec.rb diff --git a/app/models/project.rb b/app/models/project.rb index bfa85fc9d99b00..8bc4b2a37886f2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1314,8 +1314,6 @@ def default_issues_tracker? end def external_issue_tracker - cache_has_external_issue_tracker if has_external_issue_tracker.nil? - return unless has_external_issue_tracker? @external_issue_tracker ||= services.external_issue_trackers.first @@ -2680,10 +2678,6 @@ def oids(objects, oids: []) def cache_has_external_wiki update_column(:has_external_wiki, services.external_wikis.any?) if Gitlab::Database.read_write? end - - def cache_has_external_issue_tracker - update_column(:has_external_issue_tracker, services.external_issue_trackers.any?) if Gitlab::Database.read_write? - end end Project.prepend_if_ee('EE::Project') diff --git a/changelogs/unreleased/290715-change-project-has_external_issue_tracker-to-be-not-null.yml b/changelogs/unreleased/290715-change-project-has_external_issue_tracker-to-be-not-null.yml new file mode 100644 index 00000000000000..1b1e6388e0cdc3 --- /dev/null +++ b/changelogs/unreleased/290715-change-project-has_external_issue_tracker-to-be-not-null.yml @@ -0,0 +1,6 @@ +--- +title: Change projects.has_external_issue_tracker column to be not null and default + to false, and cleanup all null and incorrect data +merge_request: 51855 +author: +type: changed diff --git a/db/migrate/20210118011518_add_default_projects_has_external_issue_tracker.rb b/db/migrate/20210118011518_add_default_projects_has_external_issue_tracker.rb new file mode 100644 index 00000000000000..74caf211c5fd87 --- /dev/null +++ b/db/migrate/20210118011518_add_default_projects_has_external_issue_tracker.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddDefaultProjectsHasExternalIssueTracker < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + change_column_default(:projects, :has_external_issue_tracker, from: nil, to: false) + end +end diff --git a/db/post_migrate/20210118011623_add_not_null_constraint_to_projects_has_external_issue_tracker.rb b/db/post_migrate/20210118011623_add_not_null_constraint_to_projects_has_external_issue_tracker.rb new file mode 100644 index 00000000000000..443fa020bad280 --- /dev/null +++ b/db/post_migrate/20210118011623_add_not_null_constraint_to_projects_has_external_issue_tracker.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddNotNullConstraintToProjectsHasExternalIssueTracker < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_not_null_constraint :projects, :has_external_issue_tracker, validate: false + end + + def down + remove_not_null_constraint :projects, :has_external_issue_tracker + end +end diff --git a/db/post_migrate/20210118011840_cleanup_projects_with_null_has_external_issue_tracker.rb b/db/post_migrate/20210118011840_cleanup_projects_with_null_has_external_issue_tracker.rb new file mode 100644 index 00000000000000..1e2a6471e6b7d6 --- /dev/null +++ b/db/post_migrate/20210118011840_cleanup_projects_with_null_has_external_issue_tracker.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +class CleanupProjectsWithNullHasExternalIssueTracker < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + # On GitLab.com there will be ~350 projects updated, however, we use + # batching out of abundant caution. + BATCH_SIZE = 1000 + + disable_ddl_transaction! + + class Service < ActiveRecord::Base + self.table_name = 'services' + end + + class Project < ActiveRecord::Base + include EachBatch + + self.table_name = 'projects' + end + + def up + services_sub_query = Service + .select('1') + .where('services.project_id = projects.id') + .where(category: 'issue_tracker') + .where(active: true) + + Project.each_batch(of: BATCH_SIZE) do |relation| + # 11 projects are scoped in this query on GitLab.com. + # Expected run time ~130 ms (cold cache query). + relation + .where('EXISTS (?)', services_sub_query) + .where(has_external_issue_tracker: [false, nil]) # By additionally scoping `false` we also correct historic bad data: https://gitlab.com/gitlab-org/gitlab/-/issues/273574 + .where(pending_delete: false) + .where(archived: false) + .update_all(has_external_issue_tracker: true) + + # 322 projects are scoped in this query on GitLab.com. + # Expected run time ~3 minutes (cold cache query). + relation + .where('NOT EXISTS (?)', services_sub_query) + .where(has_external_issue_tracker: [true, nil]) # By additionally scoping `true` we also correct historic bad data: https://gitlab.com/gitlab-org/gitlab/-/issues/273574 + .where(pending_delete: false) + .where(archived: false) + .update_all(has_external_issue_tracker: false) + end + end + + def down + # no-op : can't go back to `NULL` without first dropping the `NOT NULL` constraint + end +end diff --git a/db/schema_migrations/20210118011518 b/db/schema_migrations/20210118011518 new file mode 100644 index 00000000000000..d99dd40972bcb1 --- /dev/null +++ b/db/schema_migrations/20210118011518 @@ -0,0 +1 @@ +3ddf05cd7a2619e4be1286b87bfab9a6ab5239d3921909b14485719b4f28c1d3 \ No newline at end of file diff --git a/db/schema_migrations/20210118011623 b/db/schema_migrations/20210118011623 new file mode 100644 index 00000000000000..d5375a4fe5798d --- /dev/null +++ b/db/schema_migrations/20210118011623 @@ -0,0 +1 @@ +df697762e3d375fbc9f87697f885f85d47402bf7e46a8353af16cfe3363991ca \ No newline at end of file diff --git a/db/schema_migrations/20210118011840 b/db/schema_migrations/20210118011840 new file mode 100644 index 00000000000000..9d9588a0e26da5 --- /dev/null +++ b/db/schema_migrations/20210118011840 @@ -0,0 +1 @@ +de4246dd62e47d0c376d7c3d1c4ec585d761a7c3b8fd7829b55529ab1669a19c \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 261e9a1e73ba83..7a7ea114e7f993 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -16010,7 +16010,7 @@ CREATE TABLE projects ( last_repository_check_at timestamp without time zone, container_registry_enabled boolean, only_allow_merge_if_pipeline_succeeds boolean DEFAULT false NOT NULL, - has_external_issue_tracker boolean, + has_external_issue_tracker boolean DEFAULT false, repository_storage character varying DEFAULT 'default'::character varying NOT NULL, repository_read_only boolean, request_access_enabled boolean DEFAULT true NOT NULL, @@ -19518,6 +19518,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_38eb20f8ef CHECK ((has_external_issue_tracker 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_issue_tracker_spec.rb b/spec/migrations/cleanup_projects_with_null_has_external_issue_tracker_spec.rb new file mode 100644 index 00000000000000..fe22a71aec8867 --- /dev/null +++ b/spec/migrations/cleanup_projects_with_null_has_external_issue_tracker_spec.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require_migration! + +RSpec.describe CleanupProjectsWithNullHasExternalIssueTracker, :migration do + let_it_be(:namespace) { table(:namespaces).create!(name: 'foo', path: 'foo') } + let(:projects) { table(:projects) } + let(:services) { table(:services) } + let(:constraint_name) { 'check_38eb20f8ef' } + + before do + # In order to insert a row with a NULL to fill. + ActiveRecord::Base.connection.execute "ALTER TABLE projects DROP CONSTRAINT #{constraint_name}" + end + + after do + # Revert DB structure + ActiveRecord::Base.connection.execute "ALTER TABLE projects ADD CONSTRAINT #{constraint_name} CHECK ((has_external_issue_tracker IS NOT NULL)) NOT VALID;" + end + + def create_projects!(num) + Array.new(num) do + projects.create!(namespace_id: namespace.id) + end + end + + def create_active_external_issue_tracker_integrations!(*projects) + projects.each do |project| + services.create!(category: 'issue_tracker', project_id: project.id, active: true) + end + end + + def create_disabled_external_issue_tracker_integrations!(*projects) + projects.each do |project| + services.create!(category: 'issue_tracker', project_id: project.id, active: false) + end + end + + def create_active_other_integrations!(*projects) + projects.each do |project| + services.create!(category: 'not_issue_tracker', project_id: project.id, active: true) + end + end + + it 'sets `projects.has_external_issue_tracker` correctly' do + project_with_external_issue_tracker_1, + project_with_external_issue_tracker_2, + project_with_external_issue_tracker_3, + project_with_disabled_external_issue_tracker_1, + project_with_disabled_external_issue_tracker_2, + project_with_disabled_external_issue_tracker_3, + project_without_external_issue_tracker_1, + project_without_external_issue_tracker_2, + project_without_external_issue_tracker_3 = create_projects!(9) + + create_active_external_issue_tracker_integrations!( + project_with_external_issue_tracker_1, + project_with_external_issue_tracker_2, + project_with_external_issue_tracker_3 + ) + + create_disabled_external_issue_tracker_integrations!( + project_with_disabled_external_issue_tracker_1, + project_with_disabled_external_issue_tracker_2, + project_with_disabled_external_issue_tracker_3 + ) + + create_active_other_integrations!( + project_without_external_issue_tracker_1, + project_without_external_issue_tracker_2, + project_without_external_issue_tracker_3 + ) + + # PG triggers on the services table added in a previous migration + # will have set the `has_external_issue_tracker` columns to correct data when + # the services records were created above. + # + # We set the `has_external_issue_tracker` 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_issue_tracker_1.update!(has_external_issue_tracker: nil) + project_with_external_issue_tracker_2.update!(has_external_issue_tracker: false) + + project_with_disabled_external_issue_tracker_1.update!(has_external_issue_tracker: nil) + project_with_disabled_external_issue_tracker_2.update!(has_external_issue_tracker: true) + + project_without_external_issue_tracker_1.update!(has_external_issue_tracker: nil) + project_without_external_issue_tracker_2.update!(has_external_issue_tracker: true) + + migrate! + + expected_true = [ + project_with_external_issue_tracker_1, + project_with_external_issue_tracker_2, + project_with_external_issue_tracker_3 + ].each(&:reload).map(&:has_external_issue_tracker) + + expected_false = [ + project_without_external_issue_tracker_1, + project_without_external_issue_tracker_2, + project_without_external_issue_tracker_3, + project_with_disabled_external_issue_tracker_1, + project_with_disabled_external_issue_tracker_2, + project_with_disabled_external_issue_tracker_3 + ].each(&:reload).map(&:has_external_issue_tracker) + + 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 8d0b7336955767..555a5dd366e6ab 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1039,19 +1039,6 @@ end describe '#external_issue_tracker' do - it 'sets Project#has_external_issue_tracker when it is nil' do - project_with_no_tracker = create(:project, has_external_issue_tracker: nil) - project_with_tracker = create(:redmine_project, has_external_issue_tracker: nil) - - expect do - project_with_no_tracker.external_issue_tracker - end.to change { project_with_no_tracker.reload.has_external_issue_tracker }.from(nil).to(false) - - expect do - project_with_tracker.external_issue_tracker - end.to change { project_with_tracker.reload.has_external_issue_tracker }.from(nil).to(true) - end - it 'returns nil and does not query services when there is no external issue tracker' do project = create(:project) -- GitLab