Merge branch 'ab/projects-without-feature' into 'master'

Make sure project_feature records always exist

See merge request gitlab-org/gitlab!23766

changelogs/unreleased/ab-projects-without-feature.yml

+---
+title: Ensure all Project records have a corresponding ProjectFeature record
+merge_request: 23766
+author:
+type: fixed

db/post_migrate/20200127111840_fix_projects_without_project_feature.rb

+# frozen_string_literal: true
+
+class FixProjectsWithoutProjectFeature < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  BATCH_SIZE = 50_000
+  MIGRATION = 'FixProjectsWithoutProjectFeature'
+
+  disable_ddl_transaction!
+
+  class Project < ActiveRecord::Base
+    include EachBatch
+  end
+
+  def up
+    queue_background_migration_jobs_by_range_at_intervals(Project, MIGRATION, 2.minutes, batch_size: BATCH_SIZE)
+  end
+
+  def down
+    # no-op
+  end
+end

lib/gitlab/background_migration/fix_projects_without_project_feature.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # This migration creates missing project_features records
+    # for the projects within the given range of ids
+    class FixProjectsWithoutProjectFeature
+      def perform(from_id, to_id)
+        if number_of_created_records = create_missing!(from_id, to_id) > 0
+          log(number_of_created_records, from_id, to_id)
+        end
+      end
+
+      private
+
+      def create_missing!(from_id, to_id)
+        result = ActiveRecord::Base.connection.select_one(sql(from_id, to_id))
+        return 0 unless result
+
+        result['number_of_created_records']
+      end
+
+      def sql(from_id, to_id)
+        <<~SQL
+          WITH created_records AS (
+            INSERT INTO project_features (
+              project_id,
+              merge_requests_access_level,
+              issues_access_level,
+              wiki_access_level,
+              snippets_access_level,
+              builds_access_level,
+              repository_access_level,
+              forking_access_level,
+              pages_access_level,
+              created_at,
+              updated_at
+            )
+              SELECT projects.id,
+                20, 20, 20, 20, 20, 20, 20,
+                #{pages_access_level},
+                TIMEZONE('UTC', NOW()), TIMEZONE('UTC', NOW())
+              FROM projects
+              WHERE projects.id BETWEEN #{Integer(from_id)} AND #{Integer(to_id)}
+              AND NOT EXISTS (
+                SELECT 1 FROM project_features
+                WHERE project_features.project_id = projects.id
+              )
+            ON CONFLICT (project_id) DO NOTHING
+            RETURNING *
+          )
+          SELECT COUNT(*) as number_of_created_records
+          FROM created_records
+        SQL
+      end
+
+      def pages_access_level
+        if ::Gitlab::Pages.access_control_is_forced?
+          "10"
+        else
+          "GREATEST(projects.visibility_level, 10)"
+        end
+      end
+
+      def log(count, from_id, to_id)
+        logger = Gitlab::BackgroundMigration::Logger.build
+
+        logger.info(message: "FixProjectsWithoutProjectFeature: created missing project_features for #{count} projects in id=#{from_id}...#{to_id}")
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/fix_projects_without_project_feature_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::FixProjectsWithoutProjectFeature, :migration, schema: 2020_01_27_111840 do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:project_features) { table(:project_features) }
+
+  let(:namespace) { namespaces.create(name: 'foo', path: 'foo') }
+
+  let!(:project) { projects.create!(namespace_id: namespace.id) }
+  let(:private_project_without_feature) { projects.create!(namespace_id: namespace.id, visibility_level: 0) }
+  let(:public_project_without_feature) { projects.create!(namespace_id: namespace.id, visibility_level: 20) }
+  let!(:projects_without_feature) { [private_project_without_feature, public_project_without_feature] }
+
+  before do
+    project_features.create({ project_id: project.id, pages_access_level: 20 })
+  end
+
+  subject { described_class.new.perform(Project.minimum(:id), Project.maximum(:id)) }
+
+  def project_feature_records
+    project_features.order(:project_id).pluck(:project_id)
+  end
+
+  def features(project)
+    project_features.find_by(project_id: project.id)&.attributes
+  end
+
+  it 'creates a ProjectFeature for projects without it' do
+    expect { subject }.to change { project_feature_records }.from([project.id]).to([project.id, *projects_without_feature.map(&:id)])
+  end
+
+  it 'creates ProjectFeature records with default values for a public project' do
+    subject
+
+    expect(features(public_project_without_feature)).to include(
+      {
+        "merge_requests_access_level" => 20,
+        "issues_access_level" => 20,
+        "wiki_access_level" => 20,
+        "snippets_access_level" => 20,
+        "builds_access_level" => 20,
+        "repository_access_level" => 20,
+        "pages_access_level" => 20,
+        "forking_access_level" => 20
+      }
+    )
+  end
+
+  it 'creates ProjectFeature records with default values for a private project' do
+    subject
+
+    expect(features(private_project_without_feature)).to include("pages_access_level" => 10)
+  end
+
+  context 'when access control to pages is forced' do
+    before do
+      allow(::Gitlab::Pages).to receive(:access_control_is_forced?).and_return(true)
+    end
+
+    it 'creates ProjectFeature records with default values for a public project' do
+      subject
+
+      expect(features(public_project_without_feature)).to include("pages_access_level" => 10)
+    end
+  end
+
+  it 'sets created_at/updated_at timestamps' do
+    subject
+
+    expect(project_features.where('created_at IS NULL OR updated_at IS NULL')).to be_empty
+  end
+end

spec/migrations/fix_projects_without_project_feature_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200127111840_fix_projects_without_project_feature.rb')
+
+describe FixProjectsWithoutProjectFeature, :migration do
+  let(:namespace) { table(:namespaces).create(name: 'gitlab', path: 'gitlab-org') }
+
+  let!(:projects) do
+    [
+      table(:projects).create(namespace_id: namespace.id, name: 'foo 1'),
+      table(:projects).create(namespace_id: namespace.id, name: 'foo 2'),
+      table(:projects).create(namespace_id: namespace.id, name: 'foo 3')
+    ]
+  end
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 2)
+  end
+
+  around do |example|
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        example.call
+      end
+    end
+  end
+
+  it 'schedules jobs for ranges of projects' do
+    migrate!
+
+    expect(described_class::MIGRATION)
+      .to be_scheduled_delayed_migration(2.minutes, projects[0].id, projects[1].id)
+
+    expect(described_class::MIGRATION)
+      .to be_scheduled_delayed_migration(4.minutes, projects[2].id, projects[2].id)
+  end
+
+  it 'schedules jobs according to the configured batch size' do
+    expect { migrate! }.to change { BackgroundMigrationWorker.jobs.size }.by(2)
+  end
+end