From 10f0ddb8ef02f58b4c90e69f5d8cd81f62094526 Mon Sep 17 00:00:00 2001 From: Bojan Marjanovic Date: Mon, 29 Sep 2025 16:11:20 +0200 Subject: [PATCH 1/2] Add Unique claims behind FF --- .../gitlab/feature_flag_without_actor.yml | 1 + app/models/concerns/cells/unique.rb | 29 ++--- .../beta/cells_unique_claims.yml | 10 ++ spec/models/concerns/cells/unique_spec.rb | 103 +++++++----------- 4 files changed, 63 insertions(+), 80 deletions(-) create mode 100644 config/feature_flags/beta/cells_unique_claims.yml diff --git a/.rubocop_todo/gitlab/feature_flag_without_actor.yml b/.rubocop_todo/gitlab/feature_flag_without_actor.yml index 4bc8e1c724ba78..57245a6e1fbd53 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 d98cd04c24383d..911649f2bfc518 100644 --- a/app/models/concerns/cells/unique.rb +++ b/app/models/concerns/cells/unique.rb @@ -4,7 +4,6 @@ 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 @@ -12,17 +11,8 @@ module Unique 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, if: -> { Feature.enabled?(:cells_unique_claims) } + before_destroy inspect_destroy_changes, if: -> { Feature.enabled?(:cells_unique_claims) } end class_methods do @@ -48,6 +38,14 @@ def cells_unique_attribute(name, type:) def cells_unique_has_many(name) cells_unique_has_many_associations << name end + + def inspect_save_changes + InspectChanges.new(self).inspect_changes + end + + def inspect_destroy_changes + InspectChanges.new(self, destroy: true).inspect_changes + end end def self.claim_and_commit(changes) @@ -69,12 +67,5 @@ def self.rollback yield end end - - private - - def with_claim_timeout(timeout_ms = CLAIM_TIMEOUT_MS) - connection.exec_query("SET LOCAL statement_timeout = #{timeout_ms}") - yield - 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 00000000000000..a32bd5090bf770 --- /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 83e809381ea0c5..60cebbbf117bf9 100644 --- a/spec/models/concerns/cells/unique_spec.rb +++ b/spec/models/concerns/cells/unique_spec.rb @@ -3,25 +3,17 @@ 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(: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 +53,52 @@ 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 + describe 'after_save callback' do + it 'calls inspect_save_changes after saving' do 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(Cells::InspectChanges).to receive(:new).with(instance).at_least(:once).and_return(inspector) expect(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) + describe 'before_destroy callback' do + before do + instance.save! + end - block_called = false - result = instance.send(:with_claim_timeout) do - block_called = true - :block_result - end + it 'calls inspect_destroy_changes before destroying' do + inspector = instance_double(Cells::InspectChanges) - expect(block_called).to be true - expect(result).to eq(:block_result) - end + expect(Cells::InspectChanges).to receive(:new).with(instance, destroy: true).and_return(inspector) + expect(inspector).to receive(:inspect_changes) - it 'executes query before yielding' do - call_order = [] + instance.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) + instance.destroy! end end end @@ -132,9 +108,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) + instance.destroy! end end end -- GitLab From 596507cc478e8891ef73e7583a60c122faf4a170 Mon Sep 17 00:00:00 2001 From: Bojan Marjanovic Date: Tue, 30 Sep 2025 13:40:45 +0200 Subject: [PATCH 2/2] Fixes failing pipeline --- app/models/concerns/cells/unique.rb | 30 ++++++++++++++--------- spec/models/concerns/cells/unique_spec.rb | 23 +++++++++-------- 2 files changed, 29 insertions(+), 24 deletions(-) diff --git a/app/models/concerns/cells/unique.rb b/app/models/concerns/cells/unique.rb index 911649f2bfc518..db8c3c84f39b7d 100644 --- a/app/models/concerns/cells/unique.rb +++ b/app/models/concerns/cells/unique.rb @@ -9,10 +9,8 @@ module Unique SOURCE_TABLE = Gitlab::Cells::TopologyService::ClaimSource::SourceTable included do - next unless Gitlab.config.cell.enabled - - after_save inspect_save_changes, if: -> { Feature.enabled?(:cells_unique_claims) } - before_destroy inspect_destroy_changes, if: -> { Feature.enabled?(:cells_unique_claims) } + after_save :inspect_save_changes + before_destroy :inspect_destroy_changes end class_methods do @@ -38,14 +36,6 @@ def cells_unique_attribute(name, type:) def cells_unique_has_many(name) cells_unique_has_many_associations << name end - - def inspect_save_changes - InspectChanges.new(self).inspect_changes - end - - def inspect_destroy_changes - InspectChanges.new(self, destroy: true).inspect_changes - end end def self.claim_and_commit(changes) @@ -67,5 +57,21 @@ def self.rollback yield end end + + private + + 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/spec/models/concerns/cells/unique_spec.rb b/spec/models/concerns/cells/unique_spec.rb index 60cebbbf117bf9..d4121c29301125 100644 --- a/spec/models/concerns/cells/unique_spec.rb +++ b/spec/models/concerns/cells/unique_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' RSpec.describe Cells::Unique, feature_category: :cell do + let_it_be(:user) { create(:user) } let(:test_klass) { ::User } describe 'configuration' do @@ -62,27 +63,25 @@ describe 'after_save callback' do it 'calls inspect_save_changes after saving' do - inspector = instance_double(Cells::InspectChanges) + email_inspector = instance_double(Cells::InspectChanges) + user_inspector = instance_double(Cells::InspectChanges) - expect(Cells::InspectChanges).to receive(:new).with(instance).at_least(:once).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.save! end end describe 'before_destroy callback' do - before do - instance.save! - end - it 'calls inspect_destroy_changes before destroying' do inspector = instance_double(Cells::InspectChanges) - expect(Cells::InspectChanges).to receive(:new).with(instance, destroy: true).and_return(inspector) + expect(Cells::InspectChanges).to receive(:new).with(user, destroy: true).and_return(inspector) expect(inspector).to receive(:inspect_changes) - - instance.destroy! + user.destroy! end end @@ -98,7 +97,7 @@ it 'does not call inspect_destroy_changes on destroy' do expect(Cells::InspectChanges).not_to receive(:new) - instance.destroy! + user.destroy! end end end @@ -115,7 +114,7 @@ it 'does not call inspect_destroy_changes on destroy' do expect(Cells::InspectChanges).not_to receive(:new) - instance.destroy! + user.destroy! end end end -- GitLab