Merge branch '290715-change-project-has_external_wiki-to-be-not-null' into 'master'

Set `projects.has_external_wiki` to `DEFAULT false` and `NOT NULL`

See merge request gitlab-org/gitlab!50916

changelogs/unreleased/290715-change-project-has_external_wiki-to-be-not-null.yml

+---
+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

db/migrate/20210105025900_add_default_projects_has_external_wiki.rb

+# 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

db/post_migrate/20210105025903_add_not_null_constraint_to_projects_has_external_wiki.rb

+# 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

db/post_migrate/20210105030124_cleanup_projects_with_null_has_external_wiki.rb

+# 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

db/schema_migrations/20210105025900

+047dd77352eda8134e55047e2d3fab07bbcd6eb41600cefb9b581d32e441fb68
\ No newline at end of file

db/schema_migrations/20210105025903

+339d828635107f77116ffd856abcb8f8f64985cabb82c0c34ab76056f5756d2e
\ No newline at end of file

db/schema_migrations/20210105030124

+30f3cbc0f96848f72a188616503eb80d38c33769c8bebf86a5922b374750d066
\ No newline at end of file

db/structure.sql

@@ -16129,7 +16129,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,
@@ -19662,6 +19662,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;
 

spec/migrations/cleanup_projects_with_null_has_external_wiki_spec.rb

+# 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

spec/models/project_spec.rb

@@ -1145,11 +1145,32 @@ RSpec.describe Project, factory_default: :keep do
       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