diff --git a/.rubocop_todo/gitlab/feature_flag_without_actor.yml b/.rubocop_todo/gitlab/feature_flag_without_actor.yml index 4bc8e1c724ba781b74efe7685a4001bbbdade209..57245a6e1fbd53aa597957e3272928362a4035c5 100644 --- a/.rubocop_todo/gitlab/feature_flag_without_actor.yml +++ b/.rubocop_todo/gitlab/feature_flag_without_actor.yml @@ -21,6 +21,7 @@ Gitlab/FeatureFlagWithoutActor: - 'app/models/clusters/instance.rb' - 'app/models/concerns/counter_attribute.rb' - 'app/models/concerns/reset_on_column_errors.rb' + - 'app/models/concerns/cells/unique.rb' - 'app/models/concerns/web_hooks/auto_disabling.rb' - 'app/models/merge_request.rb' - 'app/models/namespace.rb' diff --git a/app/models/concerns/cells/unique.rb b/app/models/concerns/cells/unique.rb index d98cd04c24383dbe6204af39c433847d3ebcb82f..db8c3c84f39b7df4195bf58af409c1a2818173d6 100644 --- a/app/models/concerns/cells/unique.rb +++ b/app/models/concerns/cells/unique.rb @@ -4,25 +4,13 @@ module Cells module Unique extend ActiveSupport::Concern - CLAIM_TIMEOUT_MS = 10000 BUCKET_TYPE = Gitlab::Cells::TopologyService::ClaimBucket::BucketType OWNER_TYPE = Gitlab::Cells::TopologyService::ClaimOwner::OwnerType SOURCE_TABLE = Gitlab::Cells::TopologyService::ClaimSource::SourceTable included do - next unless Gitlab.config.cell.enabled - - after_save do - with_claim_timeout do - InspectChanges.new(self).inspect_changes - end - end - - before_destroy do - with_claim_timeout do - InspectChanges.new(self, destroy: true).inspect_changes - end - end + after_save :inspect_save_changes + before_destroy :inspect_destroy_changes end class_methods do @@ -72,9 +60,18 @@ def self.rollback private - def with_claim_timeout(timeout_ms = CLAIM_TIMEOUT_MS) - connection.exec_query("SET LOCAL statement_timeout = #{timeout_ms}") - yield + def inspect_save_changes + return unless Gitlab.config.cell.enabled + return unless Feature.enabled?(:cells_unique_claims) + + InspectChanges.new(self).inspect_changes + end + + def inspect_destroy_changes + return unless Gitlab.config.cell.enabled + return unless Feature.enabled?(:cells_unique_claims) + + InspectChanges.new(self, destroy: true).inspect_changes end end end diff --git a/config/feature_flags/beta/cells_unique_claims.yml b/config/feature_flags/beta/cells_unique_claims.yml new file mode 100644 index 0000000000000000000000000000000000000000..a32bd5090bf770e644bb256a754b6a7397987826 --- /dev/null +++ b/config/feature_flags/beta/cells_unique_claims.yml @@ -0,0 +1,10 @@ +--- +name: cells_unique_claims +description: +feature_issue_url: https://gitlab.com/gitlab-com/gl-infra/tenant-scale/cells-infrastructure/team/-/issues/365 +introduced_by_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/572819 +milestone: '18.5' +group: group::cells infrastructure +type: beta +default_enabled: false diff --git a/spec/models/concerns/cells/unique_spec.rb b/spec/models/concerns/cells/unique_spec.rb index 83e809381ea0c5cced0a62a01ab11eb67f58bfc8..d4121c2930112511510dc0655b90e27980261fb3 100644 --- a/spec/models/concerns/cells/unique_spec.rb +++ b/spec/models/concerns/cells/unique_spec.rb @@ -3,25 +3,18 @@ require 'spec_helper' RSpec.describe Cells::Unique, feature_category: :cell do - let(:dummy_class) do - Class.new(ActiveRecord::Base) do - self.table_name = 'users' - include Cells::Unique - end - end + let_it_be(:user) { create(:user) } + let(:test_klass) { ::User } describe 'configuration' do it 'sets and retrieves cell configuration' do - dummy_class.cells_config(owner_type: :user, source_table: :users) - dummy_class.cells_unique_attribute(:email, type: :string) - dummy_class.cells_unique_attribute(:username, type: :string) - - expect(dummy_class.cells_owner_type).to eq(:user) - expect(dummy_class.cells_source_table).to eq(:users) - expect(dummy_class.cells_unique_attributes).to eq( - email: { type: :string }, - username: { type: :string } - ) + test_klass.cells_config(owner_type: :user, source_table: :users) + test_klass.cells_unique_attribute(:email, type: :string) + test_klass.cells_unique_attribute(:username, type: :string) + + expect(test_klass.cells_owner_type).to eq(:user) + expect(test_klass.cells_source_table).to eq(:users) + expect(test_klass.cells_unique_attributes).to eq(email: { type: :string }, username: { type: :string }) end end @@ -61,68 +54,50 @@ end describe 'callbacks' do - let(:instance) { dummy_class.new } - let(:connection) { instance_double(ActiveRecord::ConnectionAdapters::PostgreSQLAdapter) } + let(:instance) { build(:user) } context 'when cells are enabled' do before do allow(Gitlab.config.cell).to receive(:enabled).and_return(true) - allow(instance).to receive(:connection).and_return(connection) end - describe 'before_commit callback' do - it 'inspects changes with timeout' do - inspector = instance_double(Cells::InspectChanges) + describe 'after_save callback' do + it 'calls inspect_save_changes after saving' do + email_inspector = instance_double(Cells::InspectChanges) + user_inspector = instance_double(Cells::InspectChanges) - expect(connection).to receive(:exec_query).with("SET LOCAL statement_timeout = 10000") - expect(Cells::InspectChanges).to receive(:new).with(instance).and_return(inspector) - expect(inspector).to receive(:inspect_changes) + expect(Cells::InspectChanges).to receive(:new).with(an_instance_of(Email)).and_return(email_inspector) + expect(Cells::InspectChanges).to receive(:new).with(instance).and_return(user_inspector) + expect(email_inspector).to receive(:inspect_changes) + expect(user_inspector).to receive(:inspect_changes) - instance.run_callbacks(:before_commit) + instance.save! end end - describe 'instance methods' do - describe '#with_claim_timeout (private)' do - it 'sets the statement timeout to default value' do - expect(connection).to receive(:exec_query).with("SET LOCAL statement_timeout = 10000") - - instance.send(:with_claim_timeout) { :result } - end - - it 'sets the statement timeout to custom value' do - expect(connection).to receive(:exec_query).with("SET LOCAL statement_timeout = 5000") - - instance.send(:with_claim_timeout, 5000) { :result } - end - - it 'yields to the block' do - allow(connection).to receive(:exec_query) - - block_called = false - result = instance.send(:with_claim_timeout) do - block_called = true - :block_result - end - - expect(block_called).to be true - expect(result).to eq(:block_result) - end + describe 'before_destroy callback' do + it 'calls inspect_destroy_changes before destroying' do + inspector = instance_double(Cells::InspectChanges) - it 'executes query before yielding' do - call_order = [] + expect(Cells::InspectChanges).to receive(:new).with(user, destroy: true).and_return(inspector) + expect(inspector).to receive(:inspect_changes) + user.destroy! + end + end - expect(connection).to receive(:exec_query) do |query| - call_order << :exec_query - expect(query).to eq("SET LOCAL statement_timeout = 10000") - end + context 'when feature flag is disabled' do + before do + stub_feature_flags(cells_unique_claims: false) + end - instance.send(:with_claim_timeout) do - call_order << :block - end + it 'does not call inspect_save_changes on save' do + expect(Cells::InspectChanges).not_to receive(:new) + instance.save! + end - expect(call_order).to eq([:exec_query, :block]) - end + it 'does not call inspect_destroy_changes on destroy' do + expect(Cells::InspectChanges).not_to receive(:new) + user.destroy! end end end @@ -132,9 +107,14 @@ allow(Gitlab.config.cell).to receive(:enabled).and_return(false) end - it 'does not inspect changes' do + it 'does not call inspect_save_changes on save' do + expect(Cells::InspectChanges).not_to receive(:new) + instance.save! + end + + it 'does not call inspect_destroy_changes on destroy' do expect(Cells::InspectChanges).not_to receive(:new) - instance.run_callbacks(:commit) + user.destroy! end end end