From fbb0dbd5849d38b5cb4f040d1b658fffef6967c2 Mon Sep 17 00:00:00 2001 From: Radamanthus Batnag Date: Fri, 26 Sep 2025 17:54:54 +0800 Subject: [PATCH] Refactor Maven Virtual Registry RegistryUpstream model When we introduced ::VirtualRegistries::Container::RegistryUpstream, we made it as a child class of ::VirtualRegistries::RegistryUpstream. We can remove duplicate code and make ::VirtualRegistries::Packages::Maven::RegistryUpstream inherit from ::VirtualRegistries::RegistryUpstream EE: true --- .../packages/maven/registry_upstream.rb | 62 +----- .../packages/maven/registry_upstream_spec.rb | 202 +----------------- 2 files changed, 4 insertions(+), 260 deletions(-) diff --git a/ee/app/models/virtual_registries/packages/maven/registry_upstream.rb b/ee/app/models/virtual_registries/packages/maven/registry_upstream.rb index 9ff55ec14a442b..465c2c8f413c7e 100644 --- a/ee/app/models/virtual_registries/packages/maven/registry_upstream.rb +++ b/ee/app/models/virtual_registries/packages/maven/registry_upstream.rb @@ -3,75 +3,15 @@ module VirtualRegistries module Packages module Maven - class RegistryUpstream < ApplicationRecord + class RegistryUpstream < ::VirtualRegistries::RegistryUpstream MAX_UPSTREAMS_COUNT = 20 - belongs_to :group belongs_to :registry, class_name: 'VirtualRegistries::Packages::Maven::Registry', inverse_of: :registry_upstreams belongs_to :upstream, class_name: 'VirtualRegistries::Packages::Maven::Upstream', inverse_of: :registry_upstreams - - validates :upstream_id, uniqueness: { scope: :registry_id }, if: :upstream_id? - validates :registry_id, uniqueness: { scope: [:position] } - - validates :group, top_level_group: true, presence: true - validates :position, - numericality: { only_integer: true, greater_than_or_equal_to: 1, less_than_or_equal_to: MAX_UPSTREAMS_COUNT }, - presence: true - - before_validation :set_group, :set_position, on: :create - - def self.sync_higher_positions(registry_upstreams) - subquery = registry_upstreams.select(:registry_id, :position) - - joins("INNER JOIN (#{subquery.to_sql}) AS subquery ON #{table_name}.registry_id = subquery.registry_id") - .where("#{table_name}.position > subquery.position") - .update_all(position: Arel.sql('position - 1')) - end - - def sync_higher_positions - return if position == MAX_UPSTREAMS_COUNT - - self.class - .where(registry_id: registry_id, position: (position + 1)..) - .update_all(position: Arel.sql('position - 1')) - end - - def update_position(new_position) - return if position == new_position - - relation = self.class.where(registry_id:) - - capped_pos = [new_position, relation.maximum(:position)].min - - return if position == capped_pos - - arel_id = self.class.arel_table[:id] - arel_pos = self.class.arel_table[:position] - - case_clause = Arel::Nodes::Case.new.when(arel_id.eq(id)).then(capped_pos) - - case_clause = if capped_pos > position - case_clause.when(arel_pos.between((position + 1)..capped_pos)).then(arel_pos - 1) - else - case_clause.when(arel_pos.between(capped_pos..(position - 1))).then(arel_pos + 1) - end.else(arel_pos) - - relation.update_all(position: case_clause) - end - - private - - def set_group - self.group ||= (registry || upstream).group - end - - def set_position - self.position = self.class.where(registry:, group:).maximum(:position).to_i + 1 - end end end end diff --git a/ee/spec/models/virtual_registries/packages/maven/registry_upstream_spec.rb b/ee/spec/models/virtual_registries/packages/maven/registry_upstream_spec.rb index e4f632d12c648a..ad3011d27f36be 100644 --- a/ee/spec/models/virtual_registries/packages/maven/registry_upstream_spec.rb +++ b/ee/spec/models/virtual_registries/packages/maven/registry_upstream_spec.rb @@ -49,203 +49,7 @@ end end - describe '#update_position' do - let_it_be(:registry) { create(:virtual_registries_packages_maven_registry) } - - let_it_be(:registry_upstreams) do - create_list(:virtual_registries_packages_maven_registry_upstream, 4, registry:) - end - - context 'when position is unchanged' do - it 'does not update any positions' do - expect { registry_upstreams[0].update_position(1) }.not_to change { reload_positions } - end - end - - context 'when moving to a lower position' do - it 'updates the position of the target and increments positions of items in between' do - registry_upstreams[0].update_position(3) - - expect(reload_positions).to eq({ - registry_upstreams[0].id => 3, - registry_upstreams[1].id => 1, - registry_upstreams[2].id => 2, - registry_upstreams[3].id => 4 - }) - end - - it 'handles moving to the lowest position' do - registry_upstreams[0].update_position(4) - - expect(reload_positions).to eq({ - registry_upstreams[0].id => 4, - registry_upstreams[1].id => 1, - registry_upstreams[2].id => 2, - registry_upstreams[3].id => 3 - }) - end - end - - context 'when moving to a higher position' do - it 'updates the position of the target and decrements positions of items in between' do - registry_upstreams[3].update_position(2) - - expect(reload_positions).to eq({ - registry_upstreams[0].id => 1, - registry_upstreams[1].id => 3, - registry_upstreams[2].id => 4, - registry_upstreams[3].id => 2 - }) - end - - it 'handles moving to the highest position' do - registry_upstreams[3].update_position(1) - - expect(reload_positions).to eq({ - registry_upstreams[0].id => 2, - registry_upstreams[1].id => 3, - registry_upstreams[2].id => 4, - registry_upstreams[3].id => 1 - }) - end - end - - context 'when moving to a position beyond the maximum' do - it 'caps the position at the maximum existing position' do - registry_upstreams[1].update_position(10) - - expect(reload_positions).to eq({ - registry_upstreams[0].id => 1, - registry_upstreams[1].id => 4, - registry_upstreams[2].id => 2, - registry_upstreams[3].id => 3 - }) - end - end - - context 'when there are multiple registries' do - let_it_be(:other_registry) { create(:virtual_registries_packages_maven_registry) } - let_it_be_with_reload(:other_registry_upstreams) do - create_list(:virtual_registries_packages_maven_registry_upstream, 2, registry: other_registry) - end - - it 'only updates positions within the same registry' do - registry_upstreams[0].update_position(3) - - # Positions in the original registry should be updated - expect(reload_positions).to eq({ - registry_upstreams[0].id => 3, - registry_upstreams[1].id => 1, - registry_upstreams[2].id => 2, - registry_upstreams[3].id => 4 - }) - - # Positions in the other registry should remain unchanged - expect(other_registry_upstreams[0].position).to eq(1) - expect(other_registry_upstreams[1].position).to eq(2) - end - end - end - - describe '.sync_higher_positions' do - let_it_be(:registry) { create(:virtual_registries_packages_maven_registry) } - - let_it_be_with_refind(:registry_upstreams) do - create_list(:virtual_registries_packages_maven_registry_upstream, 4, registry:) - end - - it 'decrements positions of all registry upstreams with higher positions' do - expect(reload_positions).to eq({ - registry_upstreams[0].id => 1, - registry_upstreams[1].id => 2, - registry_upstreams[2].id => 3, - registry_upstreams[3].id => 4 - }) - - described_class.sync_higher_positions(registry_upstreams[1].upstream.registry_upstreams) - registry_upstreams[1].destroy! - - expect(reload_positions).to eq({ - registry_upstreams[0].id => 1, - registry_upstreams[2].id => 2, - registry_upstreams[3].id => 3 - }) - end - - context 'when there are shared upstreams' do - let_it_be(:other_registry) do - create(:virtual_registries_packages_maven_registry, group: registry.group, name: 'other') - end - - let_it_be(:registry_upstream_1) do - create(:virtual_registries_packages_maven_registry_upstream, registry: other_registry, - upstream: registry_upstreams[0].upstream) - end - - let_it_be(:registry_upstream_2) do - create(:virtual_registries_packages_maven_registry_upstream, registry: other_registry, - upstream: registry_upstreams[1].upstream) - end - - it 'correctly updates positions in all registries' do - expect(reload_positions).to eq({ - registry_upstreams[0].id => 1, - registry_upstreams[1].id => 2, - registry_upstreams[2].id => 3, - registry_upstreams[3].id => 4 - }) - - expect(reload_positions(other_registry)).to eq({ - registry_upstream_1.id => 1, - registry_upstream_2.id => 2 - }) - - described_class.sync_higher_positions( - described_class.where(upstream_id: [registry_upstreams[1].upstream_id, registry_upstream_1.upstream_id]) - ) - registry_upstreams[1].destroy! - registry_upstream_1.destroy! - - expect(reload_positions).to eq({ - registry_upstreams[0].id => 1, - registry_upstreams[2].id => 2, - registry_upstreams[3].id => 3 - }) - - expect(reload_positions(other_registry)).to eq({ - registry_upstream_2.id => 1 - }) - end - end - end - - describe '#sync_higher_positions' do - let_it_be(:registry) { create(:virtual_registries_packages_maven_registry) } - - let_it_be(:registry_upstreams) do - create_list(:virtual_registries_packages_maven_registry_upstream, 4, registry:) - end - - it 'decrements positions of all registry upstreams with higher positions' do - expect(reload_positions).to eq({ - registry_upstreams[0].id => 1, - registry_upstreams[1].id => 2, - registry_upstreams[2].id => 3, - registry_upstreams[3].id => 4 - }) - - registry_upstreams[1].destroy! - registry_upstreams[1].sync_higher_positions - - expect(reload_positions).to eq({ - registry_upstreams[0].id => 1, - registry_upstreams[2].id => 2, - registry_upstreams[3].id => 3 - }) - end - end - - def reload_positions(registry = registry_upstreams[0].registry) - described_class.where(registry:).pluck(:id, :position).to_h - end + it_behaves_like 'registry upstream position sync', + registry_factory: :virtual_registries_packages_maven_registry, + registry_upstream_factory: :virtual_registries_packages_maven_registry_upstream end -- GitLab