diff --git a/app/models/application_record.rb b/app/models/application_record.rb index 7cec15efa8d46869d451f4a4997f0c969c55aaf0..8195ee8d5f08f1648a6f890fd7bb213181093430 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -165,6 +165,10 @@ def self.sharding_keys @sharding_keys ||= Gitlab::Database::Dictionary.entry(table_name).sharding_key || {} end + def sharding_keys + @sharding_keys ||= self.class.sharding_keys.transform_keys(&method(:public_send)).invert + end + def readable_by?(user) Ability.allowed?(user, :"read_#{to_ability_name}", self) end diff --git a/app/models/concerns/cells/inspect_changes.rb b/app/models/concerns/cells/inspect_changes.rb index 8a9cffe011905242e2a6fc1fc3e435a01532059c..0d4c96d8d34f930153d13bbd7d12d86db0d61e50 100644 --- a/app/models/concerns/cells/inspect_changes.rb +++ b/app/models/concerns/cells/inspect_changes.rb @@ -2,6 +2,18 @@ module Cells class InspectChanges + class Visit + extend Forwardable + + attr_reader :record + def_delegators :@map, :[], :[]=, :filter_map + + def initialize(new_record) + @record = new_record + @map = {} + end + end + attr_reader :record def initialize(new_record, destroy: false) @@ -43,7 +55,7 @@ def inspected end def prepare_inspecting - visited[inspect_changes_key] = {} + visited[inspect_changes_key] = Visit.new(record) end def inspect_record diff --git a/app/models/concerns/cells/transaction_callback.rb b/app/models/concerns/cells/transaction_callback.rb index 69047adbeac27455fdbc3a4e3589aadf47bb7130..74b1f7462770aac9413c10f1b50b4fe7fe144b3f 100644 --- a/app/models/concerns/cells/transaction_callback.rb +++ b/app/models/concerns/cells/transaction_callback.rb @@ -8,6 +8,10 @@ def self.visited visited_stack.first # Flatten to the first end + def self.last_visited + visited_stack.last + end + def self.visited_stack Thread.current.thread_variable_get(THREAD_VARIABLE_NAME) || Thread.current.thread_variable_set(THREAD_VARIABLE_NAME, []) @@ -29,7 +33,7 @@ def commit_db_transaction # and when that happens, we'll roll it back, and we need to look at # the stack to know if it's a unbalanced ROLLBACK or not, therefore # we only pop after successful COMMIT or ROLLBACk. - Unique.claim_and_commit(TransactionCallback.visited_stack.last) do + Unique.claim_and_commit(TransactionCallback.last_visited.values) do super TransactionCallback.visited_stack.pop end diff --git a/app/models/concerns/cells/unique.rb b/app/models/concerns/cells/unique.rb index db8c3c84f39b7df4195bf58af409c1a2818173d6..f68a5290755ce5a8619f450fad6d62f543d654f5 100644 --- a/app/models/concerns/cells/unique.rb +++ b/app/models/concerns/cells/unique.rb @@ -11,10 +11,11 @@ module Unique included do after_save :inspect_save_changes before_destroy :inspect_destroy_changes + cells_config end class_methods do - attr_reader :cells_owner_type, :cells_source_table + attr_reader :cells_type, :cells_source_table def cells_unique_attributes @cells_unique_attributes ||= {} @@ -24,8 +25,10 @@ def cells_unique_has_many_associations @cells_unique_has_many_associations ||= [] end - def cells_config(owner_type:, source_table:) - @cells_owner_type = owner_type + def cells_config( + type: OWNER_TYPE::UNSPECIFIED, + source_table: SOURCE_TABLE::UNSPECIFIED) + @cells_type = type @cells_source_table = source_table end diff --git a/app/models/email.rb b/app/models/email.rb index 7edbd89dc21dc7d9148fa3bcf010d0afe19bb4cc..677b273c3ef8796861e4e5467ff6c7dfddc828f5 100644 --- a/app/models/email.rb +++ b/app/models/email.rb @@ -7,7 +7,7 @@ class Email < ApplicationRecord cells_unique_attribute :email, type: BUCKET_TYPE::UNSPECIFIED # TODO: EMAIL? - cells_config owner_type: OWNER_TYPE::USER, source_table: SOURCE_TABLE::UNSPECIFIED + cells_config source_table: SOURCE_TABLE::UNSPECIFIED belongs_to :user, optional: false belongs_to :banned_user, class_name: '::Users::BannedUser', foreign_key: 'user_id', inverse_of: 'emails' diff --git a/app/models/gpg_key.rb b/app/models/gpg_key.rb index 4fa06af9af70a050a9cb341e67bfd73fe9b61d28..a01cd55c69df01390417d9a19f3a1051749bed17 100644 --- a/app/models/gpg_key.rb +++ b/app/models/gpg_key.rb @@ -11,7 +11,7 @@ class GpgKey < ApplicationRecord cells_unique_attribute :fingerprint, type: BUCKET_TYPE::UNSPECIFIED cells_unique_attribute :primary_keyid, type: BUCKET_TYPE::UNSPECIFIED - cells_config owner_type: OWNER_TYPE::UNSPECIFIED, source_table: SOURCE_TABLE::UNSPECIFIED + cells_config source_table: SOURCE_TABLE::UNSPECIFIED cells_unique_has_many :subkeys diff --git a/app/models/gpg_key_subkey.rb b/app/models/gpg_key_subkey.rb index 03cdff865730e3d21a3da7fe61656139fe102286..3c041a6e6b6f79563cd5ec13de9e19ddcc187ee1 100644 --- a/app/models/gpg_key_subkey.rb +++ b/app/models/gpg_key_subkey.rb @@ -5,7 +5,7 @@ class GpgKeySubkey < ApplicationRecord include Cells::Unique cells_unique_attribute :fingerprint, type: BUCKET_TYPE::UNSPECIFIED - cells_config owner_type: OWNER_TYPE::UNSPECIFIED, source_table: SOURCE_TABLE::UNSPECIFIED + cells_config source_table: SOURCE_TABLE::UNSPECIFIED sha_attribute :keyid sha_attribute :fingerprint diff --git a/app/models/key.rb b/app/models/key.rb index 8e2581876a65d76aa1ac4fbf73ffc7ea37e15296..418c1316afbf38e944263d1a0c7072df5c8792e1 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -13,7 +13,7 @@ class Key < ApplicationRecord cells_unique_attribute :key, type: BUCKET_TYPE::UNSPECIFIED # TODO: KEY? cells_unique_attribute :fingerprint_sha256, type: BUCKET_TYPE::UNSPECIFIED # TODO: KEY_FINGERPRINT? - cells_config owner_type: OWNER_TYPE::UNSPECIFIED, source_table: SOURCE_TABLE::ORGANIZATIONS + cells_config source_table: SOURCE_TABLE::UNSPECIFIED sha256_attribute :fingerprint_sha256 diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 53e5260549fb081c14583cebd498a9ffea4a6c15..de0ca2239a5636cd7c82f6a454284c50791d66dd 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -21,6 +21,7 @@ class Namespace < ApplicationRecord include UseSqlFunctionForPrimaryKeyLookups include SafelyChangeColumnDefault include Todoable + include Cells::Unique extend Gitlab::Utils::Override diff --git a/app/models/organizations/organization.rb b/app/models/organizations/organization.rb index 7fdbce80c6d9b9634f4cce525949b5b8f7ddfc94..4843ade7a41029d177e0275cec3554d05606c263 100644 --- a/app/models/organizations/organization.rb +++ b/app/models/organizations/organization.rb @@ -10,7 +10,7 @@ class Organization < ApplicationRecord cells_unique_attribute :path, type: BUCKET_TYPE::UNSPECIFIED # TODO: PATH? - cells_config owner_type: OWNER_TYPE::ORGANIZATION, source_table: SOURCE_TABLE::ORGANIZATIONS + cells_config type: OWNER_TYPE::ORGANIZATION, source_table: SOURCE_TABLE::ORGANIZATIONS DEFAULT_ORGANIZATION_ID = 1 diff --git a/app/models/route.rb b/app/models/route.rb index aab42fd78f8de444d19c9288bc460c9a30ce0a33..700c8b5e212c5dd5115df99b6868f0cfcb35c088 100644 --- a/app/models/route.rb +++ b/app/models/route.rb @@ -8,7 +8,7 @@ class Route < ApplicationRecord cells_unique_attribute :path, type: BUCKET_TYPE::ROUTES - cells_config owner_type: OWNER_TYPE::UNSPECIFIED, source_table: SOURCE_TABLE::ROUTES + cells_config source_table: SOURCE_TABLE::ROUTES belongs_to :source, polymorphic: true, inverse_of: :route # rubocop:disable Cop/PolymorphicAssociations belongs_to :namespace, inverse_of: :namespace_route diff --git a/app/models/user.rb b/app/models/user.rb index daaf0ada131801965cf5fe5c6d84018d5d67da11..eafe3bea9dac986d49131217caa3ec956c40c4c0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -42,7 +42,7 @@ class User < ApplicationRecord cells_unique_has_many :gpg_keys cells_unique_has_many :emails - cells_config owner_type: OWNER_TYPE::USER, source_table: SOURCE_TABLE::UNSPECIFIED + cells_config type: OWNER_TYPE::USER, source_table: SOURCE_TABLE::UNSPECIFIED ignore_column %i[role skype], remove_after: '2025-09-18', remove_with: '18.4' diff --git a/lib/gitlab/topology_service_client/claim_service.rb b/lib/gitlab/topology_service_client/claim_service.rb index 258f795729cdbdb27f02e5e512bd03538becf183..654e2742d338d8a45ee61500c54c5f31abb11abc 100644 --- a/lib/gitlab/topology_service_client/claim_service.rb +++ b/lib/gitlab/topology_service_client/claim_service.rb @@ -111,58 +111,57 @@ def build_claim_request(changes) end def build_create_records(changes) - changes.flat_map do |(table, id), columns_values| - columns_values.filter_map do |column, (_old_value, new_value)| + changes.flat_map do |visit| + visit.filter_map do |column, (_old_value, new_value)| if new_value # rubocop:disable Style/IfUnlessModifier -- I think this is more clear - build_claim_data(table, id, column, new_value) + build_claim_data(visit.record, column, new_value) end end end end - def build_claim_data(table, id, column, new_value) + def build_claim_data(record, column, new_value) Gitlab::Cells::TopologyService::ClaimData.new( - bucket: build_bucket(table, column, new_value), - owner: build_owner(table, id), - source: build_source(table, id) + bucket: build_bucket(record, column, new_value), + owner: build_owner(record), + source: build_source(record) ) end - def build_bucket(table, column, value) + def build_bucket(record, column, value) Gitlab::Cells::TopologyService::ClaimBucket.new( { - bucket_type: table.classify.constantize.cells_unique_attributes.dig(column.to_sym, :type), + bucket_type: record.class.cells_unique_attributes.dig(column.to_sym, :type), bucket_value: value } ) end - # TODO: How do we tell the owner, when the only thing we have is a - # list of changed tables and columns? We need to somehow retain the - # hierarchy to really tell - def build_owner(table, id) + # TODO: What if this record has more than one sharding keys? + def build_owner(record) + table, id = record.sharding_keys.first Gitlab::Cells::TopologyService::ClaimOwner.new( { - owner_type: table.classify.constantize.cells_owner_type, + owner_type: determine_owner_type_from_table(table), owner_id: id } ) end - def build_source(table, id) + def build_source(record) Gitlab::Cells::TopologyService::ClaimSource.new( { - source_table: table.classify.constantize.cells_source_table, - source_record_id: id + source_table: record.class.cells_source_table, + source_record_id: record.id } ) end def build_destroy_records(changes) - changes.flat_map do |(table, _id), columns_values| - columns_values.filter_map do |column, (old_value, _new_value)| + changes.flat_map do |visit| + visit.filter_map do |column, (old_value, _new_value)| if old_value.present? # rubocop:disable Style/IfUnlessModifier -- I think this is more clear - build_bucket(table, column, old_value) + build_bucket(visit.record, column, old_value) end end end @@ -175,6 +174,16 @@ def build_claim_lease(uuid) ) end + def determine_owner_type_from_table(table) + model_name = table.classify + + if Object.const_defined?(model_name) + model_name.constantize + else + table.capitalize.constantize.const_get(model_name) + end.cells_type + end + def service_class Gitlab::Cells::TopologyService::ClaimService::Stub end diff --git a/spec/models/concerns/cells/unique_spec.rb b/spec/models/concerns/cells/unique_spec.rb index d4121c2930112511510dc0655b90e27980261fb3..b92dab4b9593f99b189aa21725f903b701cc330f 100644 --- a/spec/models/concerns/cells/unique_spec.rb +++ b/spec/models/concerns/cells/unique_spec.rb @@ -8,11 +8,11 @@ describe 'configuration' do it 'sets and retrieves cell configuration' do - test_klass.cells_config(owner_type: :user, source_table: :users) + test_klass.cells_config(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_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 diff --git a/spec/models/email_spec.rb b/spec/models/email_spec.rb index e4d820552a7df55d198b35fc295809b123a101fe..e0fb9df7e0f8ad56e01530f16aea8cfa2dac00bb 100644 --- a/spec/models/email_spec.rb +++ b/spec/models/email_spec.rb @@ -6,7 +6,7 @@ let_it_be(:user) { create(:user) } it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::USER, + type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, source_table: Cells::Unique::SOURCE_TABLE::UNSPECIFIED, unique_attributes: [:email] diff --git a/spec/models/gpg_key_spec.rb b/spec/models/gpg_key_spec.rb index b71b3bf4b7eb7f32d3ef60b2e3f0ce338aca7a67..acad9633d120c7cb1505dcfcf8bc95b9e7e8d2de 100644 --- a/spec/models/gpg_key_spec.rb +++ b/spec/models/gpg_key_spec.rb @@ -6,7 +6,7 @@ subject { build(:gpg_key) } it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, + type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, source_table: Cells::Unique::SOURCE_TABLE::UNSPECIFIED, unique_attributes: [:key, :fingerprint, :primary_keyid] diff --git a/spec/models/gpg_key_subkey_spec.rb b/spec/models/gpg_key_subkey_spec.rb index 295f0712cec53200ecdbb5e74d7b9d5487605246..e9d8a17cce7b6ee17b9c524b536fe5232ad1e996 100644 --- a/spec/models/gpg_key_subkey_spec.rb +++ b/spec/models/gpg_key_subkey_spec.rb @@ -16,7 +16,7 @@ end it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, + type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, source_table: Cells::Unique::SOURCE_TABLE::UNSPECIFIED, unique_attributes: [:fingerprint] end diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index 66ca553c308736e402c939370fdd6eadb833d56f..3b6a7b142f24148d8e0bb95bbee36ddd90661d87 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -6,7 +6,7 @@ it_behaves_like 'having unique enum values' it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, + type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, source_table: Cells::Unique::SOURCE_TABLE::ORGANIZATIONS, unique_attributes: [:key, :fingerprint_sha256] diff --git a/spec/models/organizations/organization_spec.rb b/spec/models/organizations/organization_spec.rb index d79b53274fae47c26f05a8e6fabc8cfa4ff5c1ae..52486b630273c158ad24e5d162115527074495a9 100644 --- a/spec/models/organizations/organization_spec.rb +++ b/spec/models/organizations/organization_spec.rb @@ -6,7 +6,7 @@ let_it_be_with_refind(:organization) { create(:organization) } it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::ORGANIZATION, + type: Cells::Unique::OWNER_TYPE::ORGANIZATION, source_table: Cells::Unique::SOURCE_TABLE::ORGANIZATIONS, unique_attributes: [:path] diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb index 2207e72b2d8745939dce9976af9e1a9afd92a846..4db4bb72cba4634dff388c549aeffa05defca653 100644 --- a/spec/models/route_spec.rb +++ b/spec/models/route_spec.rb @@ -7,7 +7,7 @@ let(:route) { group.route } it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, + type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, source_table: Cells::Unique::SOURCE_TABLE::ROUTES, unique_attributes: [:path] diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 08050de6faf322e7c2840ce9119dd7ae3671bd35..aad954fb2fd6225b266f325582b42dc6e0b2cd00 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -50,7 +50,7 @@ it_behaves_like 'having unique enum values' it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::USER, + type: Cells::Unique::OWNER_TYPE::USER, source_table: Cells::Unique::SOURCE_TABLE::UNSPECIFIED, unique_attributes: [:username] diff --git a/spec/support/shared_examples/cells/unique_shared_examples.rb b/spec/support/shared_examples/cells/unique_shared_examples.rb index f5f152f52e705ad95acff1748b24b14b95c6896d..17a13d413936fef5511c7e4970c88ba6b623d7a6 100644 --- a/spec/support/shared_examples/cells/unique_shared_examples.rb +++ b/spec/support/shared_examples/cells/unique_shared_examples.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true -RSpec.shared_examples 'cells unique model' do |owner_type:, source_table:, unique_attributes:| +RSpec.shared_examples 'cells unique model' do |type:, source_table:, unique_attributes:| it 'has the expected owner_type' do - expect(described_class.cells_owner_type).to eq(owner_type) + expect(described_class.cells_type).to eq(type) end it 'has the expected source_table' do