Merge branch 'revert-project-has_external_wiki' into 'master'

Revert project has external wiki MR

See merge request gitlab-org/gitlab!53216

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

changelogs/unreleased/revert-project-has_external_wiki.yml

+---
+title: Set projects.has_external_wiki default to null if default value is present
+merge_request: 53216
+author:
+type: other

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 → db/migrate/20210203092540_remove_has_external_wiki_constraint.rb

 # frozen_string_literal: true
 
-class AddNotNullConstraintToProjectsHasExternalWiki < ActiveRecord::Migration[6.0]
+class RemoveHasExternalWikiConstraint < 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
+    # This reverts the following migration: add_not_null_constraint :projects, :has_external_wiki, validate: false
+    remove_not_null_constraint :projects, :has_external_wiki
   end
 
   def down
-    remove_not_null_constraint :projects, :has_external_wiki
+    # no-op
   end
 end

db/migrate/20210203092549_restore_has_external_wiki_default_value.rb

+# frozen_string_literal: true
+
+class RestoreHasExternalWikiDefaultValue < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class TmpProject < ActiveRecord::Base
+    self.table_name = 'projects'
+  end
+
+  # This reverts the following migration: change_column_default(:projects, :has_external_wiki, from: nil, to: false)
+  # We only change the column when the current default value is false
+  def up
+    # Find out the current default value
+    column = TmpProject.columns.find { |c| c.name == 'has_external_wiki' }
+    return unless column
+
+    if column.default == 'false'
+      with_lock_retries do
+        change_column_default(:projects, :has_external_wiki, from: false, to: nil)
+      end
+    end
+  end
+
+  def down
+    # no-op
+  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/schema_migrations/20210203092540

+fa0b69df96069dd29971f69a51ce4eb691480904c5693fa480f6daa83b042e77
\ No newline at end of file

db/schema_migrations/20210203092549

+096d13548e156a13b9ccdbb39ed83b3be7ef64a57df5daea2e3cd870f3e2b448
\ 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 DEFAULT false,
+    has_external_wiki boolean,
     ci_config_path character varying,
     lfs_enabled boolean,
     description_html text,
@@ -19662,9 +19662,6 @@ 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,32 +1145,11 @@ RSpec.describe Project, factory_default: :keep do
       is_expected.to eq(nil)
     end
 
-    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)
+    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, false)
+      project.update_column(:has_external_wiki, nil)
 
-      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)))
+      expect { subject }.to change { project.has_external_wiki }.from(nil).to(true)
     end
   end