Merge branch 'refactor_push_rules_v2' into 'master'

Refactor push rules v2

See merge request gitlab-org/gitlab!28286

app/models/application_setting.rb

@@ -14,6 +14,7 @@ class ApplicationSetting < ApplicationRecord
   add_authentication_token_field :static_objects_external_storage_auth_token
 
   belongs_to :self_monitoring_project, class_name: "Project", foreign_key: 'instance_administration_project_id'
+  belongs_to :push_rule
   alias_attribute :self_monitoring_project_id, :instance_administration_project_id
 
   belongs_to :instance_administrators_group, class_name: "Group"

app/models/project_setting.rb

@@ -9,3 +9,5 @@ class ProjectSetting < ApplicationRecord
     where(primary_key => safe_find_or_create_by(attrs))
   end
 end
+
+ProjectSetting.prepend_if_ee('EE::ProjectSetting')

changelogs/unreleased/refactor_push_rules.yml

+---
+title: Refactor push rules and add push_rule_id columns in project settings and application settings
+merge_request: 28286
+author:
+type: added

db/migrate/20200325104755_add_push_rules_id_to_project_settings.rb

+# frozen_string_literal: true
+
+class AddPushRulesIdToProjectSettings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :project_settings, :push_rule_id, :bigint
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :project_settings, :push_rule_id
+    end
+  end
+end

db/migrate/20200325104756_add_push_rules_foreign_key_to_project_settings.rb

+# frozen_string_literal: true
+
+class AddPushRulesForeignKeyToProjectSettings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :project_settings, :push_rule_id, unique: true
+    add_concurrent_foreign_key :project_settings, :push_rules, column: :push_rule_id, on_delete: :cascade
+  end
+
+  def down
+    remove_foreign_key_if_exists :project_settings, column: :push_rule_id
+    remove_concurrent_index :project_settings, :push_rule_id
+  end
+end

db/migrate/20200325104833_add_push_rules_id_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddPushRulesIdToApplicationSettings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :application_settings, :push_rule_id, :bigint
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :application_settings, :push_rule_id
+    end
+  end
+end

db/migrate/20200325104834_add_push_rules_foreign_key_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddPushRulesForeignKeyToApplicationSettings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :application_settings, :push_rule_id, unique: true
+    add_concurrent_foreign_key :application_settings, :push_rules, column: :push_rule_id, on_delete: :nullify
+  end
+
+  def down
+    remove_concurrent_index :application_settings, :push_rule_id
+    remove_foreign_key_if_exists :application_settings, column: :push_rule_id
+  end
+end

db/post_migrate/20200325162730_schedule_backfill_push_rules_id_in_projects.rb

+# frozen_string_literal: true
+
+class ScheduleBackfillPushRulesIdInProjects < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  MIGRATION = 'BackfillPushRulesIdInProjects'.freeze
+  BATCH_SIZE = 1_000
+
+  class PushRules < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'push_rules'
+  end
+
+  def up
+    # Update one record that is connected to the instance
+    value_to_be_updated_to = ScheduleBackfillPushRulesIdInProjects::PushRules.find_by(is_sample: true)&.id
+
+    execute "UPDATE application_settings SET push_rule_id = #{value_to_be_updated_to}" if value_to_be_updated_to
+
+    ApplicationSetting.expire
+
+    queue_background_migration_jobs_by_range_at_intervals(ScheduleBackfillPushRulesIdInProjects::PushRules,
+                                                          MIGRATION,
+                                                          5.minutes,
+                                                          batch_size: BATCH_SIZE)
+  end
+
+  def down
+    execute "UPDATE application_settings SET push_rule_id = NULL"
+
+    ApplicationSetting.expire
+  end
+end

db/structure.sql

@@ -399,7 +399,8 @@ CREATE TABLE public.application_settings (
     namespace_storage_size_limit bigint DEFAULT 0 NOT NULL,
     seat_link_enabled boolean DEFAULT true NOT NULL,
     container_expiration_policies_enable_historic_entries boolean DEFAULT false NOT NULL,
-    issues_create_limit integer DEFAULT 300 NOT NULL
+    issues_create_limit integer DEFAULT 300 NOT NULL,
+    push_rule_id bigint
 );
 
 CREATE SEQUENCE public.application_settings_id_seq
@@ -5025,7 +5026,8 @@ ALTER SEQUENCE public.project_repository_states_id_seq OWNED BY public.project_r
 CREATE TABLE public.project_settings (
     project_id integer NOT NULL,
     created_at timestamp with time zone NOT NULL,
-    updated_at timestamp with time zone NOT NULL
+    updated_at timestamp with time zone NOT NULL,
+    push_rule_id bigint
 );
 
 CREATE TABLE public.project_statistics (
@@ -8677,6 +8679,8 @@ CREATE INDEX index_application_settings_on_file_template_project_id ON public.ap
 
 CREATE INDEX index_application_settings_on_instance_administrators_group_id ON public.application_settings USING btree (instance_administrators_group_id);
 
+CREATE UNIQUE INDEX index_application_settings_on_push_rule_id ON public.application_settings USING btree (push_rule_id);
+
 CREATE INDEX index_application_settings_on_usage_stats_set_by_user_id ON public.application_settings USING btree (usage_stats_set_by_user_id);
 
 CREATE INDEX index_applicationsettings_on_instance_administration_project_id ON public.application_settings USING btree (instance_administration_project_id);
@@ -9891,6 +9895,8 @@ CREATE INDEX index_project_repositories_on_shard_id ON public.project_repositori
 
 CREATE UNIQUE INDEX index_project_repository_states_on_project_id ON public.project_repository_states USING btree (project_id);
 
+CREATE UNIQUE INDEX index_project_settings_on_push_rule_id ON public.project_settings USING btree (push_rule_id);
+
 CREATE INDEX index_project_statistics_on_namespace_id ON public.project_statistics USING btree (namespace_id);
 
 CREATE UNIQUE INDEX index_project_statistics_on_project_id ON public.project_statistics USING btree (project_id);
@@ -10636,6 +10642,9 @@ ALTER TABLE ONLY public.epics
 ALTER TABLE ONLY public.ci_pipelines
     ADD CONSTRAINT fk_3d34ab2e06 FOREIGN KEY (pipeline_schedule_id) REFERENCES public.ci_pipeline_schedules(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY public.project_settings
+    ADD CONSTRAINT fk_413a953e20 FOREIGN KEY (push_rule_id) REFERENCES public.push_rules(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.ci_pipeline_schedule_variables
     ADD CONSTRAINT fk_41c35fda51 FOREIGN KEY (pipeline_schedule_id) REFERENCES public.ci_pipeline_schedules(id) ON DELETE CASCADE;
 
@@ -10687,6 +10696,9 @@ ALTER TABLE ONLY public.merge_requests
 ALTER TABLE ONLY public.ci_builds
     ADD CONSTRAINT fk_6661f4f0e8 FOREIGN KEY (resource_group_id) REFERENCES public.ci_resource_groups(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY public.application_settings
+    ADD CONSTRAINT fk_693b8795e4 FOREIGN KEY (push_rule_id) REFERENCES public.push_rules(id) ON DELETE SET NULL;
+
 ALTER TABLE ONLY public.merge_requests
     ADD CONSTRAINT fk_6a5165a692 FOREIGN KEY (milestone_id) REFERENCES public.milestones(id) ON DELETE SET NULL;
 
@@ -13113,9 +13125,14 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200323134519
 20200324093258
 20200324115359
+20200325104755
+20200325104756
+20200325104833
+20200325104834
 20200325111432
 20200325152327
 20200325160952
+20200325162730
 20200325183636
 20200326114443
 20200326122700

ee/app/controllers/admin/push_rules_controller.rb

@@ -14,6 +14,7 @@ class Admin::PushRulesController < Admin::ApplicationController
     @push_rule.update(push_rule_params)
 
     if @push_rule.valid?
+      link_push_rule_to_application_settings
       redirect_to admin_push_rule_path, notice: _('Push Rule updated successfully.')
     else
       render :show
@@ -51,4 +52,10 @@ class Admin::PushRulesController < Admin::ApplicationController
   def set_application_setting
     @application_setting = ApplicationSetting.current_without_cache
   end
+
+  def link_push_rule_to_application_settings
+    return if @application_setting.push_rule_id
+
+    @application_setting.update(push_rule_id: @push_rule.id)
+  end
 end

ee/app/controllers/ee/projects/settings/repository_controller.rb

@@ -17,7 +17,11 @@ module EE
         def push_rule
           return unless project.feature_available?(:push_rules)
 
-          project.create_push_rule unless project.push_rule
+          unless project.push_rule
+            push_rule = project.create_push_rule
+            project.project_setting.update(push_rule_id: push_rule.id)
+          end
+
           @push_rule = project.push_rule # rubocop:disable Gitlab/ModuleWithInstanceVariables
         end
 

ee/app/models/ee/project_setting.rb

+# frozen_string_literal: true
+
+module EE
+  module ProjectSetting
+    extend ActiveSupport::Concern
+
+    prepended do
+      belongs_to :push_rule
+    end
+  end
+end

ee/app/services/ee/projects/create_service.rb

@@ -59,6 +59,7 @@ module EE
         if predefined_push_rule
           push_rule = predefined_push_rule.dup.tap { |gh| gh.is_sample = false }
           project.push_rule = push_rule
+          project.project_setting.update(push_rule: push_rule)
         end
       end
 

ee/spec/controllers/admin/push_rules_controller_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Admin::PushRulesController do
+  include StubENV
+
   let(:admin) { create(:admin) }
 
   before do
@@ -18,6 +20,10 @@ describe Admin::PushRulesController do
       }
     end
 
+    before do
+      stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+    end
+
     it 'updates sample push rule' do
       expect_next_instance_of(PushRule) do |instance|
         expect(instance).to receive(:update).with(ActionController::Parameters.new(params).permit!)
@@ -28,6 +34,12 @@ describe Admin::PushRulesController do
       expect(response).to redirect_to(admin_push_rule_path)
     end
 
+    it 'links push rule with application settings' do
+      patch :update, params: { push_rule: params }
+
+      expect(ApplicationSetting.current.push_rule_id).not_to be_nil
+    end
+
     context 'push rules unlicensed' do
       before do
         stub_licensed_features(push_rules: false)

ee/spec/controllers/projects/settings/repository_controller_spec.rb

@@ -21,6 +21,12 @@ describe Projects::Settings::RepositoryController do
         is_expected.to be_persisted
       end
 
+      it 'is connected to project_settings' do
+        get :show, params: { namespace_id: project.namespace, project_id: project }
+
+        expect(project.project_setting.push_rule).to eq(subject)
+      end
+
       context 'unlicensed' do
         before do
           stub_licensed_features(push_rules: false)

ee/spec/features/admin/admin_interacts_with_push_rules_spec.rb

@@ -3,9 +3,12 @@
 require "spec_helper"
 
 describe "Admin interacts with push rules" do
+  include StubENV
+
   let_it_be(:user) { create(:admin) }
 
   before do
+    stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
     sign_in(user)
   end
 

ee/spec/services/projects/create_service_spec.rb

@@ -186,12 +186,12 @@ describe Projects::CreateService, '#execute' do
     end
   end
 
-  context 'git hook sample' do
+  context 'push rules sample' do
     let!(:sample) { create(:push_rule_sample) }
 
     subject(:push_rule) { create_project(user, opts).push_rule }
 
-    it 'creates git hook from sample' do
+    it 'creates push rule from sample' do
       is_expected.to have_attributes(
         force_push_regex: sample.force_push_regex,
         deny_delete_tag: sample.deny_delete_tag,
@@ -200,6 +200,12 @@ describe Projects::CreateService, '#execute' do
       )
     end
 
+    it 'creates association between project settings and push rule' do
+      project_setting = subject.project.project_setting
+
+      expect(project_setting.push_rule_id).to eq(subject.id)
+    end
+
     context 'push rules unlicensed' do
       before do
         stub_licensed_features(push_rules: false)

lib/gitlab/background_migration/backfill_push_rules_id_in_projects.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # Class that will insert record into project_push_rules
+    # for each existing push_rule
+    class BackfillPushRulesIdInProjects
+      # Temporary AR table for push rules
+      class ProjectSetting < ActiveRecord::Base
+        self.table_name = 'project_settings'
+      end
+
+      def perform(start_id, stop_id)
+        ProjectSetting.connection.execute(<<~SQL)
+          UPDATE project_settings ps1
+          SET push_rule_id = pr.id
+          FROM project_settings ps2
+          INNER JOIN push_rules pr
+          ON ps2.project_id = pr.project_id
+          WHERE pr.is_sample = false
+          AND pr.id BETWEEN #{start_id} AND #{stop_id}
+          AND ps1.project_id = ps2.project_id
+        SQL
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/backfill_push_rules_id_in_projects_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::BackfillPushRulesIdInProjects, :migration, schema: 2020_03_25_162730 do
+  let(:push_rules) { table(:push_rules) }
+  let(:projects) { table(:projects) }
+  let(:project_settings) { table(:project_settings) }
+  let(:namespace) { table(:namespaces).create(name: 'user', path: 'user') }
+
+  subject { described_class.new }
+
+  describe '#perform' do
+    it 'creates new project push_rules for all push rules in the range' do
+      project_1 = projects.create(id: 1, namespace_id: namespace.id)
+      project_2 = projects.create(id: 2, namespace_id: namespace.id)
+      project_3 = projects.create(id: 3, namespace_id: namespace.id)
+      project_settings_1 = project_settings.create(project_id: project_1.id)
+      project_settings_2 = project_settings.create(project_id: project_2.id)
+      project_settings_3 = project_settings.create(project_id: project_3.id)
+      push_rule_1 = push_rules.create(id: 5, is_sample: false, project_id: project_1.id)
+      push_rule_2 = push_rules.create(id: 6, is_sample: false, project_id: project_2.id)
+      push_rules.create(id: 8, is_sample: false, project_id: 3)
+
+      subject.perform(5, 7)
+
+      expect(project_settings_1.reload.push_rule_id).to eq(push_rule_1.id)
+      expect(project_settings_2.reload.push_rule_id).to eq(push_rule_2.id)
+      expect(project_settings_3.reload.push_rule_id).to be_nil
+    end
+  end
+end

spec/migrations/schedule_backfill_push_rules_id_in_projects_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require Rails.root.join('db', 'post_migrate', '20200325162730_schedule_backfill_push_rules_id_in_projects.rb')
+
+describe ScheduleBackfillPushRulesIdInProjects do
+  let(:push_rules) { table(:push_rules) }
+
+  it 'adds global rule association to application settings' do
+    application_settings = table(:application_settings)
+    setting = application_settings.create!
+    sample_rule = push_rules.create!(is_sample: true)
+
+    Sidekiq::Testing.fake! do
+      disable_migrations_output { migrate! }
+    end
+
+    setting.reload
+    expect(setting.push_rule_id).to eq(sample_rule.id)
+  end
+
+  it 'schedules worker to migrate project push rules' do
+    rule_1 = push_rules.create!
+    rule_2 = push_rules.create!
+
+    Sidekiq::Testing.fake! do
+      disable_migrations_output { migrate! }
+
+      expect(BackgroundMigrationWorker.jobs.size).to eq(1)
+      expect(described_class::MIGRATION)
+        .to be_scheduled_delayed_migration(5.minutes, rule_1.id, rule_2.id)
+    end
+  end
+end