From a79da7326c651dd54144ab7d9c8cab86367f30e2 Mon Sep 17 00:00:00 2001 From: Mike Kozono Date: Mon, 11 Aug 2025 20:29:38 -1000 Subject: [PATCH 1/5] Geo: Add selective sync by organizations Only the backend portion. Behind a feature flag "geo_selective_sync_by_organizations". --- config/gitlab_loose_foreign_keys.yml | 6 +- db/docs/geo_node_organization_links.yml | 11 ++ ...0653_create_geo_node_organization_links.rb | 17 +++ db/schema_migrations/20250811140653 | 1 + db/structure.sql | 29 +++++ .../controllers/admin/geo/nodes_controller.rb | 10 +- ee/app/helpers/ee/geo_helper.rb | 7 ++ ee/app/models/concerns/geo/selective_sync.rb | 12 +- ee/app/models/ee/upload.rb | 4 + .../models/geo/geo_node_organization_link.rb | 10 ++ ee/app/models/geo_node.rb | 2 + ee/app/services/geo/node_update_service.rb | 1 + .../geo_selective_sync_by_organizations.yml | 10 ++ ee/lib/api/geo_nodes.rb | 16 +++ ee/lib/api/geo_sites.rb | 10 ++ ee/lib/ee/api/entities/geo_node.rb | 2 + ee/lib/ee/api/entities/geo_site.rb | 2 + .../admin/geo/nodes_controller_spec.rb | 79 ++++++++++++ .../geo/geo_node_organization_links.rb | 8 ++ .../api/schemas/public_api/v4/geo_node.json | 10 ++ .../api/schemas/public_api/v4/geo_site.json | 10 ++ ee/spec/models/ee/project_spec.rb | 8 ++ .../geo/geo_node_organization_link_spec.rb | 24 ++++ ee/spec/models/geo_node_spec.rb | 106 ++++++++++++++++ ee/spec/requests/api/geo_nodes_spec.rb | 118 +++++++++++++++++- ee/spec/requests/api/geo_sites_spec.rb | 109 ++++++++++++++++ .../services/geo/node_update_service_spec.rb | 45 +++++++ locale/gitlab.pot | 3 + 28 files changed, 666 insertions(+), 4 deletions(-) create mode 100644 db/docs/geo_node_organization_links.yml create mode 100644 db/migrate/20250811140653_create_geo_node_organization_links.rb create mode 100644 db/schema_migrations/20250811140653 create mode 100644 ee/app/models/geo/geo_node_organization_link.rb create mode 100644 ee/config/feature_flags/wip/geo_selective_sync_by_organizations.yml create mode 100644 ee/spec/factories/geo/geo_node_organization_links.rb create mode 100644 ee/spec/models/geo/geo_node_organization_link_spec.rb diff --git a/config/gitlab_loose_foreign_keys.yml b/config/gitlab_loose_foreign_keys.yml index 93fb42f8505f13..471475461b223f 100644 --- a/config/gitlab_loose_foreign_keys.yml +++ b/config/gitlab_loose_foreign_keys.yml @@ -415,6 +415,10 @@ geo_node_namespace_links: - table: namespaces column: namespace_id on_delete: async_delete +geo_node_organization_links: + - table: organizations + column: organization_id + on_delete: async_delete group_push_rules: - table: namespaces column: group_id @@ -467,7 +471,7 @@ merge_request_diff_commits: - table: merge_request_diffs column: merge_request_diff_id on_delete: async_delete - worker_class: 'LooseForeignKeys::MergeRequestDiffCommitCleanupWorker' + worker_class: LooseForeignKeys::MergeRequestDiffCommitCleanupWorker merge_request_metrics: - table: p_ci_pipelines column: pipeline_id diff --git a/db/docs/geo_node_organization_links.yml b/db/docs/geo_node_organization_links.yml new file mode 100644 index 00000000000000..d0ae91e208af7b --- /dev/null +++ b/db/docs/geo_node_organization_links.yml @@ -0,0 +1,11 @@ +--- +table_name: geo_node_organization_links +classes: +- Geo::GeoNodeOrganizationLink +feature_categories: +- geo_replication +description: Passthrough table for geo_nodes many-to-many organizations relation. +introduced_by_url: +milestone: '18.4' +gitlab_schema: gitlab_main_cell_local +table_size: small diff --git a/db/migrate/20250811140653_create_geo_node_organization_links.rb b/db/migrate/20250811140653_create_geo_node_organization_links.rb new file mode 100644 index 00000000000000..a5c95d0c74f2cc --- /dev/null +++ b/db/migrate/20250811140653_create_geo_node_organization_links.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class CreateGeoNodeOrganizationLinks < Gitlab::Database::Migration[2.3] + # Because index_geo_node_organization_links_on_geo_node_id_and_organization_id is over the 63 char limit + INDEX_NAME = :index_geo_node_organization_links_on_geo_node_id_and_org_id + + milestone '18.4' + + def change + create_table :geo_node_organization_links do |t| + t.references :geo_node, null: false, foreign_key: { on_delete: :cascade }, index: false + t.references :organization, null: false, foreign_key: nil + t.timestamps_with_timezone null: false + t.index [:geo_node_id, :organization_id], unique: true, name: INDEX_NAME + end + end +end diff --git a/db/schema_migrations/20250811140653 b/db/schema_migrations/20250811140653 new file mode 100644 index 00000000000000..fd99aba55316ed --- /dev/null +++ b/db/schema_migrations/20250811140653 @@ -0,0 +1 @@ +001c35a1cd0ab35c3cd2429b45de5502b634697de261221f6ea68c695a6a8fa2 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index e8c24387f37041..5a98fcfec225b5 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -16319,6 +16319,23 @@ CREATE SEQUENCE geo_node_namespace_links_id_seq ALTER SEQUENCE geo_node_namespace_links_id_seq OWNED BY geo_node_namespace_links.id; +CREATE TABLE geo_node_organization_links ( + id bigint NOT NULL, + geo_node_id bigint NOT NULL, + organization_id bigint NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL +); + +CREATE SEQUENCE geo_node_organization_links_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE geo_node_organization_links_id_seq OWNED BY geo_node_organization_links.id; + CREATE TABLE geo_node_statuses ( id bigint NOT NULL, geo_node_id bigint NOT NULL, @@ -29801,6 +29818,8 @@ ALTER TABLE ONLY geo_events ALTER COLUMN id SET DEFAULT nextval('geo_events_id_s ALTER TABLE ONLY geo_node_namespace_links ALTER COLUMN id SET DEFAULT nextval('geo_node_namespace_links_id_seq'::regclass); +ALTER TABLE ONLY geo_node_organization_links ALTER COLUMN id SET DEFAULT nextval('geo_node_organization_links_id_seq'::regclass); + ALTER TABLE ONLY geo_node_statuses ALTER COLUMN id SET DEFAULT nextval('geo_node_statuses_id_seq'::regclass); ALTER TABLE ONLY geo_nodes ALTER COLUMN id SET DEFAULT nextval('geo_nodes_id_seq'::regclass); @@ -32600,6 +32619,9 @@ ALTER TABLE ONLY geo_events ALTER TABLE ONLY geo_node_namespace_links ADD CONSTRAINT geo_node_namespace_links_pkey PRIMARY KEY (id); +ALTER TABLE ONLY geo_node_organization_links + ADD CONSTRAINT geo_node_organization_links_pkey PRIMARY KEY (id); + ALTER TABLE ONLY geo_node_statuses ADD CONSTRAINT geo_node_statuses_pkey PRIMARY KEY (id); @@ -38695,6 +38717,10 @@ CREATE UNIQUE INDEX index_geo_node_namespace_links_on_geo_node_id_and_namespace_ CREATE INDEX index_geo_node_namespace_links_on_namespace_id ON geo_node_namespace_links USING btree (namespace_id); +CREATE UNIQUE INDEX index_geo_node_organization_links_on_geo_node_id_and_org_id ON geo_node_organization_links USING btree (geo_node_id, organization_id); + +CREATE INDEX index_geo_node_organization_links_on_organization_id ON geo_node_organization_links USING btree (organization_id); + CREATE UNIQUE INDEX index_geo_node_statuses_on_geo_node_id ON geo_node_statuses USING btree (geo_node_id); CREATE INDEX index_geo_nodes_on_access_key ON geo_nodes USING btree (access_key); @@ -50245,6 +50271,9 @@ ALTER TABLE ONLY gpg_signatures ALTER TABLE ONLY virtual_registries_container_upstreams ADD CONSTRAINT fk_rails_c97afd8bbd FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE; +ALTER TABLE ONLY geo_node_organization_links + ADD CONSTRAINT fk_rails_c9bfa510d9 FOREIGN KEY (geo_node_id) REFERENCES geo_nodes(id) ON DELETE CASCADE; + ALTER TABLE ONLY board_group_recent_visits ADD CONSTRAINT fk_rails_ca04c38720 FOREIGN KEY (board_id) REFERENCES boards(id) ON DELETE CASCADE; diff --git a/ee/app/controllers/admin/geo/nodes_controller.rb b/ee/app/controllers/admin/geo/nodes_controller.rb index b7176c09608ed2..b0a3bb23bb6d37 100644 --- a/ee/app/controllers/admin/geo/nodes_controller.rb +++ b/ee/app/controllers/admin/geo/nodes_controller.rb @@ -45,6 +45,14 @@ def update private def geo_node_params + permitted_hash_params = { + selective_sync_shards: [] + } + + if Feature.enabled?(:geo_selective_sync_by_organizations, :instance) + permitted_hash_params[:organization_ids] = [] + end + params.require(:geo_node).permit( :name, :url, @@ -58,7 +66,7 @@ def geo_node_params :minimum_reverification_interval, :container_repositories_max_capacity, :sync_object_storage, - selective_sync_shards: [] + **permitted_hash_params ) end diff --git a/ee/app/helpers/ee/geo_helper.rb b/ee/app/helpers/ee/geo_helper.rb index b383d69496adc5..c3033daa8349d2 100644 --- a/ee/app/helpers/ee/geo_helper.rb +++ b/ee/app/helpers/ee/geo_helper.rb @@ -40,6 +40,13 @@ def selective_sync_types_json } } + if ::Feature.enabled?(:geo_selective_sync_by_organizations, :instance) + options[:ORGANIZATIONS] = { + label: s_('Geo|Projects in certain organizations'), + value: 'organizations' + } + end + options.to_json end diff --git a/ee/app/models/concerns/geo/selective_sync.rb b/ee/app/models/concerns/geo/selective_sync.rb index bc6706c1663929..a65c9c59160733 100644 --- a/ee/app/models/concerns/geo/selective_sync.rb +++ b/ee/app/models/concerns/geo/selective_sync.rb @@ -4,7 +4,7 @@ module Geo module SelectiveSync extend ActiveSupport::Concern - SELECTIVE_SYNC_TYPES = %w[namespaces shards].freeze + SELECTIVE_SYNC_TYPES = %w[namespaces shards organizations].freeze def selective_sync? validate_selective_sync_type! @@ -24,6 +24,12 @@ def selective_sync_by_shards? selective_sync_type == 'shards' end + def selective_sync_by_organizations? + validate_selective_sync_type! + + selective_sync_type == 'organizations' + end + def validate_selective_sync_type! return if selective_sync_type.blank? return if SELECTIVE_SYNC_TYPES.include?(selective_sync_type) @@ -53,6 +59,10 @@ def namespaces_for_group_owned_replicables selected_namespaces_and_descendants elsif selective_sync_by_shards? selected_leaf_namespaces_and_ancestors + elsif selective_sync_by_organizations? + # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/534201 or other sibling issues that need this + # This should return all namespaces owned by the selected orgs. + Namespace.none else raise 'This scope should not be needed without selective sync' end diff --git a/ee/app/models/ee/upload.rb b/ee/app/models/ee/upload.rb index 96f935b41216cb..f0c4553f817f84 100644 --- a/ee/app/models/ee/upload.rb +++ b/ee/app/models/ee/upload.rb @@ -64,6 +64,10 @@ def search(query) override :selective_sync_scope def selective_sync_scope(node, **_params) if node.selective_sync? + # TODO: Implement selective sync by organizations + # https://gitlab.com/gitlab-org/gitlab/-/issues/534193 + return none if node.selective_sync_by_organizations? + group_attachments(node).or(project_attachments(node)).or(other_attachments) else all diff --git a/ee/app/models/geo/geo_node_organization_link.rb b/ee/app/models/geo/geo_node_organization_link.rb new file mode 100644 index 00000000000000..7d6aed01e1dcc5 --- /dev/null +++ b/ee/app/models/geo/geo_node_organization_link.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +module Geo + class GeoNodeOrganizationLink < ApplicationRecord + belongs_to :geo_node, inverse_of: :organizations + belongs_to :organization, class_name: 'Organizations::Organization' + + validates :organization_id, presence: true, uniqueness: { scope: [:geo_node_id] } + end +end diff --git a/ee/app/models/geo_node.rb b/ee/app/models/geo_node.rb index 48ea4257235a12..94468eab75eaaf 100644 --- a/ee/app/models/geo_node.rb +++ b/ee/app/models/geo_node.rb @@ -15,6 +15,8 @@ class GeoNode < ApplicationRecord has_many :geo_node_namespace_links has_many :namespaces, through: :geo_node_namespace_links + has_many :geo_node_organization_links, class_name: 'Geo::GeoNodeOrganizationLink' + has_many :organizations, through: :geo_node_organization_links, class_name: 'Organizations::Organization' has_one :status, class_name: 'GeoNodeStatus' validates :name, presence: true, uniqueness: { case_sensitive: false }, length: { maximum: 255 } diff --git a/ee/app/services/geo/node_update_service.rb b/ee/app/services/geo/node_update_service.rb index d2ad1f81768cd2..612c034dbaa7af 100644 --- a/ee/app/services/geo/node_update_service.rb +++ b/ee/app/services/geo/node_update_service.rb @@ -8,6 +8,7 @@ def initialize(geo_node, params) @geo_node = geo_node @params = params.dup @params[:namespace_ids] = @params[:namespace_ids].to_s.split(',') if @params[:namespace_ids].is_a? String + @params[:organization_ids] = @params[:organization_ids].to_s.split(',') if @params[:organization_ids].is_a? String end def execute diff --git a/ee/config/feature_flags/wip/geo_selective_sync_by_organizations.yml b/ee/config/feature_flags/wip/geo_selective_sync_by_organizations.yml new file mode 100644 index 00000000000000..3b60c97ba525f6 --- /dev/null +++ b/ee/config/feature_flags/wip/geo_selective_sync_by_organizations.yml @@ -0,0 +1,10 @@ +--- +name: geo_selective_sync_by_organizations +description: +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/514251 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/201213 +rollout_issue_url: +milestone: '18.4' +group: group::geo +type: wip +default_enabled: false diff --git a/ee/lib/api/geo_nodes.rb b/ee/lib/api/geo_nodes.rb index 5dda5673a5db8c..448c64a89324f8 100644 --- a/ee/lib/api/geo_nodes.rb +++ b/ee/lib/api/geo_nodes.rb @@ -70,6 +70,11 @@ def update_geo_nodes_endpoint? optional :selective_sync_namespace_ids, as: :namespace_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IDs of groups that should be synced, if `selective_sync_type` == `namespaces`' + optional :selective_sync_organization_ids, + as: :organization_ids, + type: Array[Integer], + coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, + desc: 'The IDs of organizations that should be synced, if `selective_sync_type` == `organizations`' optional :minimum_reverification_interval, type: Integer, desc: 'The interval (in days) in which the repository verification is valid. Once expired, it will be ' \ 'reverified. This has no effect when set on a secondary node.' @@ -77,6 +82,8 @@ def update_geo_nodes_endpoint? post do create_params = declared_params(include_missing: false) + create_params.delete(:organization_ids) unless Feature.enabled?(:geo_selective_sync_by_organizations, :instance) + new_geo_node = ::Geo::NodeCreateService.new(create_params).execute if new_geo_node.persisted? @@ -256,6 +263,11 @@ def geo_node_status optional :selective_sync_namespace_ids, as: :namespace_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IDs of groups that should be synced, if `selective_sync_type` == `namespaces`' + optional :selective_sync_organization_ids, + as: :organization_ids, + type: Array[Integer], + coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, + desc: 'The IDs of organizations that should be synced, if `selective_sync_type` == `organizations`' optional :minimum_reverification_interval, type: Integer, desc: 'The interval (in days) in which the repository verification is valid. Once expired, it will be ' \ 'reverified. This has no effect when set on a secondary node.' @@ -265,6 +277,10 @@ def geo_node_status update_params = declared_params(include_missing: false) + unless Feature.enabled?(:geo_selective_sync_by_organizations, :instance) + update_params.delete(:organization_ids) + end + updated_geo_node = ::Geo::NodeUpdateService.new(geo_node, update_params).execute if updated_geo_node diff --git a/ee/lib/api/geo_sites.rb b/ee/lib/api/geo_sites.rb index 0b00fbf6e63a61..eab7a885526db9 100644 --- a/ee/lib/api/geo_sites.rb +++ b/ee/lib/api/geo_sites.rb @@ -70,6 +70,11 @@ def update_geo_sites_endpoint? optional :selective_sync_namespace_ids, as: :namespace_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IDs of groups that should be synced, if `selective_sync_type` == `namespaces`' + optional :selective_sync_organization_ids, + as: :organization_ids, + type: Array[Integer], + coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, + desc: 'The IDs of organizations that should be synced, if `selective_sync_type` == `organizations`' optional :minimum_reverification_interval, type: Integer, desc: 'The interval (in days) in which the repository verification is valid. Once expired, it ' \ 'will be reverified. This has no effect when set on a secondary site.' @@ -256,6 +261,11 @@ def geo_site_status optional :selective_sync_namespace_ids, as: :namespace_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IDs of groups that should be synced, if `selective_sync_type` == `namespaces`' + optional :selective_sync_organization_ids, + as: :organization_ids, + type: Array[Integer], + coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, + desc: 'The IDs of organizations that should be synced, if `selective_sync_type` == `organizations`' optional :minimum_reverification_interval, type: Integer, desc: 'The interval (in days) in which the repository verification is valid. Once expired, it ' \ 'will be reverified. This has no effect when set on a secondary site.' diff --git a/ee/lib/ee/api/entities/geo_node.rb b/ee/lib/ee/api/entities/geo_node.rb index 0bb527974204d6..10657cd64ffaa9 100644 --- a/ee/lib/ee/api/entities/geo_node.rb +++ b/ee/lib/ee/api/entities/geo_node.rb @@ -22,6 +22,8 @@ class GeoNode < Grape::Entity expose :selective_sync_type expose :selective_sync_shards expose :namespace_ids, as: :selective_sync_namespace_ids + expose :organization_ids, as: :selective_sync_organization_ids, + if: ->(_, _) { ::Feature.enabled?(:geo_selective_sync_by_organizations, :instance) } expose :minimum_reverification_interval expose :sync_object_storage, if: ->(geo_node, _) { geo_node.secondary? } diff --git a/ee/lib/ee/api/entities/geo_site.rb b/ee/lib/ee/api/entities/geo_site.rb index a506a2f35e7b50..4ca4d45d99e880 100644 --- a/ee/lib/ee/api/entities/geo_site.rb +++ b/ee/lib/ee/api/entities/geo_site.rb @@ -22,6 +22,8 @@ class GeoSite < Grape::Entity expose :selective_sync_type expose :selective_sync_shards expose :namespace_ids, as: :selective_sync_namespace_ids + expose :organization_ids, as: :selective_sync_organization_ids, + if: ->(_, _) { ::Feature.enabled?(:geo_selective_sync_by_organizations, :instance) } expose :minimum_reverification_interval expose :sync_object_storage, if: ->(geo_site, _) { geo_site.secondary? } diff --git a/ee/spec/controllers/admin/geo/nodes_controller_spec.rb b/ee/spec/controllers/admin/geo/nodes_controller_spec.rb index 8a8038941d9933..ad67683d7416f4 100644 --- a/ee/spec/controllers/admin/geo/nodes_controller_spec.rb +++ b/ee/spec/controllers/admin/geo/nodes_controller_spec.rb @@ -108,6 +108,42 @@ def go expect(response).to redirect_to(admin_geo_nodes_path) end end + + context 'with organization_ids parameter' do + let(:organization1) { create(:organization) } + let(:organization2) { create(:organization) } + let(:geo_node_attributes) { { url: 'http://example.com', organization_ids: [organization1.id.to_s, organization2.id.to_s] } } + + context 'when geo_selective_sync_by_organizations feature flag is enabled' do + it 'includes organization_ids in the parameters passed to the service' do + expected_params = ActionController::Parameters.new(geo_node_attributes).permit! + + expect(Geo::NodeCreateService).to receive(:new) + .with(expected_params) + .and_call_original + + go + end + end + + context 'when geo_selective_sync_by_organizations feature flag is disabled' do + before do + stub_feature_flags(geo_selective_sync_by_organizations: false) + end + + it 'excludes organization_ids in the parameters passed to the service' do + expected_params = geo_node_attributes.dup + expected_params.delete(:organization_ids) + expected_params = ActionController::Parameters.new(expected_params).permit! + + expect(Geo::NodeCreateService).to receive(:new) + .with(expected_params) + .and_call_original + + go + end + end + end end end @@ -156,6 +192,49 @@ def go go end + + context 'with organization_ids parameter' do + let(:organization1) { create(:organization) } + let(:organization2) { create(:organization) } + let(:geo_node_attributes) do + { + url: 'http://example.com', + internal_url: 'http://internal-url.com', + selective_sync_shards: %w[foo bar], + organization_ids: [organization1.id.to_s, organization2.id.to_s] + } + end + + context 'when geo_selective_sync_by_organizations feature flag is enabled' do + it 'includes organization_ids in the parameters passed to the service' do + expected_params = ActionController::Parameters.new(geo_node_attributes).permit! + + expect(Geo::NodeUpdateService).to receive(:new) + .with(geo_node, expected_params) + .and_call_original + + go + end + end + + context 'when geo_selective_sync_by_organizations feature flag is disabled' do + before do + stub_feature_flags(geo_selective_sync_by_organizations: false) + end + + it 'excludes organization_ids in the parameters passed to the service' do + expected_params = geo_node_attributes.dup + expected_params.delete(:organization_ids) + expected_params = ActionController::Parameters.new(expected_params).permit! + + expect(Geo::NodeUpdateService).to receive(:new) + .with(geo_node, expected_params) + .and_call_original + + go + end + end + end end end end diff --git a/ee/spec/factories/geo/geo_node_organization_links.rb b/ee/spec/factories/geo/geo_node_organization_links.rb new file mode 100644 index 00000000000000..6fa27827875fdb --- /dev/null +++ b/ee/spec/factories/geo/geo_node_organization_links.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :geo_node_organization_link, class: "Geo::GeoNodeOrganizationLink" do + geo_node + organization + end +end diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/geo_node.json b/ee/spec/fixtures/api/schemas/public_api/v4/geo_node.json index b385397672bacd..236788648453f6 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/geo_node.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/geo_node.json @@ -14,6 +14,7 @@ "selective_sync_type", "selective_sync_shards", "selective_sync_namespace_ids", + "selective_sync_organization_ids", "minimum_reverification_interval", "clone_protocol", "web_edit_url", @@ -86,6 +87,15 @@ "type": "integer" } }, + "selective_sync_organization_ids": { + "type": [ + "array", + "null" + ], + "items": { + "type": "integer" + } + }, "minimum_reverification_interval": { "type": "integer" }, diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/geo_site.json b/ee/spec/fixtures/api/schemas/public_api/v4/geo_site.json index 3187aa6b92fe9e..1aed068d44ab60 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/geo_site.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/geo_site.json @@ -14,6 +14,7 @@ "selective_sync_type", "selective_sync_shards", "selective_sync_namespace_ids", + "selective_sync_organization_ids", "minimum_reverification_interval", "web_edit_url", "_links" @@ -85,6 +86,15 @@ "type": "integer" } }, + "selective_sync_organization_ids": { + "type": [ + "array", + "null" + ], + "items": { + "type": "integer" + } + }, "minimum_reverification_interval": { "type": "integer" }, diff --git a/ee/spec/models/ee/project_spec.rb b/ee/spec/models/ee/project_spec.rb index a5764e55ff7b97..73888f3bfde43e 100644 --- a/ee/spec/models/ee/project_spec.rb +++ b/ee/spec/models/ee/project_spec.rb @@ -5463,6 +5463,14 @@ def stub_default_url_options(host) expect(described_class.selective_sync_scope(node)).to match_array([project_1, project_2]) end + it 'returns none with selective sync by organizations, for now' do + organization_1 = create(:organization) + organization_2 = create(:organization) + node.update!(selective_sync_type: 'organizations', organizations: [organization_1, organization_2]) + + expect(described_class.selective_sync_scope(node)).to be_empty + end + it 'raises if an unrecognised selective sync type is used' do node.update_attribute(:selective_sync_type, 'unknown') diff --git a/ee/spec/models/geo/geo_node_organization_link_spec.rb b/ee/spec/models/geo/geo_node_organization_link_spec.rb new file mode 100644 index 00000000000000..328c63a213ad8f --- /dev/null +++ b/ee/spec/models/geo/geo_node_organization_link_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Geo::GeoNodeOrganizationLink, :models, feature_category: :geo_replication do + describe 'relationships' do + it { is_expected.to belong_to(:geo_node) } + it { is_expected.to belong_to(:organization).class_name('Organizations::Organization') } + end + + describe 'validations' do + let!(:geo_node_organization_link) { create(:geo_node_organization_link) } + + it { is_expected.to validate_presence_of(:organization_id) } + it { is_expected.to validate_uniqueness_of(:organization_id).scoped_to(:geo_node_id) } + end + + context 'with loose foreign key on geo_node_organization_links.organization_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let_it_be(:parent) { create(:organization) } + let_it_be(:model) { create(:geo_node_organization_link, organization: parent) } + end + end +end diff --git a/ee/spec/models/geo_node_spec.rb b/ee/spec/models/geo_node_spec.rb index 42a0c738c0f14c..283aa6fbd9f30b 100644 --- a/ee/spec/models/geo_node_spec.rb +++ b/ee/spec/models/geo_node_spec.rb @@ -22,6 +22,8 @@ it { is_expected.to have_many(:geo_node_namespace_links) } it { is_expected.to have_many(:namespaces).through(:geo_node_namespace_links) } + it { is_expected.to have_many(:geo_node_organization_links) } + it { is_expected.to have_many(:organizations).through(:geo_node_organization_links).class_name('Organizations::Organization') } end it_behaves_like 'encrypted attribute', :secret_access_key, :db_key_base_32 do @@ -42,6 +44,12 @@ it { is_expected.to validate_uniqueness_of(:name).case_insensitive } it { is_expected.to validate_length_of(:name).is_at_most(255) } + context 'when checking SELECTIVE_SYNC_TYPES constant' do + it 'has expected sync types' do + expect(GeoNode::SELECTIVE_SYNC_TYPES).to match_array(%w[namespaces shards organizations]) + end + end + context 'when validating primary node' do it 'cannot be disabled' do primary_node.enabled = false @@ -748,6 +756,12 @@ is_expected.to be_truthy end + it 'returns true when selective sync is by organizations' do + node.update!(selective_sync_type: 'organizations') + + is_expected.to be_truthy + end + it 'returns false when selective sync is disabled' do node.update!( selective_sync_type: '', @@ -771,6 +785,90 @@ end end + describe '#selective_sync_by_organizations?' do + subject { node.selective_sync_by_organizations? } + + context 'when selective_sync_type is organizations' do + before do + node.update!(selective_sync_type: 'organizations') + end + + it 'returns true' do + is_expected.to be_truthy + end + end + + context 'when selective_sync_type is namespaces' do + before do + node.update!(selective_sync_type: 'namespaces') + end + + it 'returns false' do + is_expected.to be_falsy + end + end + + context 'when selective_sync_type is shards' do + before do + node.update!(selective_sync_type: 'shards') + end + + it 'returns false' do + is_expected.to be_falsy + end + end + + context 'when selective_sync_type is nil' do + before do + node.update!(selective_sync_type: nil) + end + + it 'returns false' do + is_expected.to be_falsy + end + end + + context 'when selective_sync_type is empty string' do + before do + node.update!(selective_sync_type: '') + end + + it 'returns false' do + is_expected.to be_falsy + end + end + + context 'when selective_sync_type is an invalid value' do + before do + node.update_column(:selective_sync_type, 'invalid') + end + + it 'returns false' do + is_expected.to be_falsy + end + end + + context 'when selective_sync_type has mixed case' do + before do + node.update_column(:selective_sync_type, 'Organizations') + end + + it 'returns false due to case sensitivity' do + is_expected.to be_falsy + end + end + + context 'when selective_sync_type has extra whitespace' do + before do + node.update_column(:selective_sync_type, ' organizations ') + end + + it 'returns false due to exact string match requirement' do + is_expected.to be_falsy + end + end + end + describe '#namespaces_for_group_owned_replicables' do subject(:namespace_ids_for_group_owned_replicables) { node.namespaces_for_group_owned_replicables.pluck(:id) } @@ -800,6 +898,14 @@ is_expected.to contain_exactly(selected_namespace.id, child_namespace.id) end end + + context 'when selective sync is by organization' do + it 'returns none' do + node.update!(selective_sync_type: 'organizations', organizations: []) + + is_expected.to be_empty + end + end end describe '#name=' do diff --git a/ee/spec/requests/api/geo_nodes_spec.rb b/ee/spec/requests/api/geo_nodes_spec.rb index 3d0c102d9b2979..7500a8238073b0 100644 --- a/ee/spec/requests/api/geo_nodes_spec.rb +++ b/ee/spec/requests/api/geo_nodes_spec.rb @@ -36,10 +36,65 @@ selective_sync_namespace_ids: group_to_sync.id, minimum_reverification_interval: 10 } - expect_any_instance_of(Geo::NodeCreateService).to receive(:execute).once.and_call_original + post api('/geo_nodes', admin, admin_mode: true), params: geo_node_params expect(response).to have_gitlab_http_status(:created) end + + context 'with organization_ids parameter' do + let(:organization1) { create(:organization) } + let(:organization2) { create(:organization) } + + context 'when geo_selective_sync_by_organizations feature flag is enabled' do + it 'accepts organization_ids parameter' do + geo_node_params = { + name: 'Test Node with Organizations', + url: 'http://example.com', + selective_sync_type: "organizations", + selective_sync_organization_ids: [organization1.id, organization2.id] + } + + post api('/geo_nodes', admin, admin_mode: true), params: geo_node_params + + expect(response).to have_gitlab_http_status(:created) + end + + it 'includes organization_ids in the response' do + geo_node_params = { + name: 'Test Node with Organizations', + url: 'http://example.com', + selective_sync_type: "organizations", + selective_sync_organization_ids: [organization1.id, organization2.id] + } + + post api('/geo_nodes', admin, admin_mode: true), params: geo_node_params + + expect(response).to have_gitlab_http_status(:created) + expect(json_response).to include('selective_sync_type' => 'organizations') + expect(json_response).to include('selective_sync_organization_ids' => [organization1.id, organization2.id]) + end + end + + context 'when geo_selective_sync_by_organizations feature flag is disabled' do + before do + stub_feature_flags(geo_selective_sync_by_organizations: false) + end + + it 'ignores organization_ids parameter when feature flag is disabled' do + geo_node_params = { + name: 'Test Node without Organizations', + url: 'http://example.com', + selective_sync_type: "organizations", + selective_sync_organization_ids: [organization1.id, organization2.id] + } + + post api('/geo_nodes', admin, admin_mode: true), params: geo_node_params + + expect(response).to have_gitlab_http_status(:created) + expect(json_response).not_to include('organization_ids') + end + end + end end describe 'GET /geo_nodes' do @@ -259,6 +314,67 @@ expect(json_response).to include(params) end + context 'with organization_ids parameter' do + let(:organization1) { create(:organization) } + let(:organization2) { create(:organization) } + + context 'when geo_selective_sync_by_organizations feature flag is enabled' do + it 'updates organization_ids parameter' do + params = { + selective_sync_type: "organizations", + selective_sync_organization_ids: [organization1.id, organization2.id] + } + + put api("/geo_nodes/#{secondary.id}", admin, admin_mode: true), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/geo_node', dir: 'ee') + expect(json_response).to include('selective_sync_type' => 'organizations') + expect(json_response).to include('selective_sync_organization_ids' => [organization1.id, organization2.id]) + end + + it 'can clear organization_ids by setting to empty array' do + # First set organizations + secondary.update!(selective_sync_type: 'organizations', organization_ids: [organization1.id]) + + params = { + selective_sync_type: "organizations", + selective_sync_organization_ids: [] + } + + put api("/geo_nodes/#{secondary.id}", admin, admin_mode: true), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to include('selective_sync_type' => 'organizations') + expect(json_response).to include('selective_sync_organization_ids' => []) + end + end + + context 'when geo_selective_sync_by_organizations feature flag is disabled' do + before do + stub_feature_flags(geo_selective_sync_by_organizations: false) + end + + it 'ignores organization_ids parameter' do + # Set some existing organizations + secondary.update!(selective_sync_type: 'organizations', organization_ids: [organization1.id]) + + params = { + selective_sync_type: "namespaces", + selective_sync_organization_ids: [organization2.id] + } + + put api("/geo_nodes/#{secondary.id}", admin, admin_mode: true), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to include('selective_sync_type' => 'namespaces') + expect(json_response).not_to include('organization_ids') + # Should preserve existing organization_ids since parameter was ignored + expect(secondary.reload.organization_ids).to eq([organization1.id]) + end + end + end + it 'can update primary' do params = { url: 'https://updated.example.com/' diff --git a/ee/spec/requests/api/geo_sites_spec.rb b/ee/spec/requests/api/geo_sites_spec.rb index 279aedd7397e71..d014255a23d346 100644 --- a/ee/spec/requests/api/geo_sites_spec.rb +++ b/ee/spec/requests/api/geo_sites_spec.rb @@ -61,6 +61,61 @@ expect(json_response).to include({ 'message' => { 'enabled' => ['Geo primary node cannot be disabled'], 'primary' => ['node already exists'] } }) end + + context 'with organization_ids parameter' do + let(:organization1) { create(:organization) } + let(:organization2) { create(:organization) } + + context 'when geo_selective_sync_by_organizations feature flag is enabled' do + it 'accepts organization_ids parameter' do + geo_node_params = { + name: 'Test Node with Organizations', + url: 'http://example.com', + selective_sync_type: "organizations", + selective_sync_organization_ids: [organization1.id, organization2.id] + } + + post api('/geo_nodes', admin, admin_mode: true), params: geo_node_params + + expect(response).to have_gitlab_http_status(:created) + end + + it 'includes organization_ids in the response' do + geo_node_params = { + name: 'Test Node with Organizations', + url: 'http://example.com', + selective_sync_type: "organizations", + selective_sync_organization_ids: [organization1.id, organization2.id] + } + + post api('/geo_nodes', admin, admin_mode: true), params: geo_node_params + + expect(response).to have_gitlab_http_status(:created) + expect(json_response).to include('selective_sync_type' => 'organizations') + expect(json_response).to include('selective_sync_organization_ids' => [organization1.id, organization2.id]) + end + end + + context 'when geo_selective_sync_by_organizations feature flag is disabled' do + before do + stub_feature_flags(geo_selective_sync_by_organizations: false) + end + + it 'ignores organization_ids parameter when feature flag is disabled' do + geo_node_params = { + name: 'Test Node without Organizations', + url: 'http://example.com', + selective_sync_type: "organizations", + selective_sync_organization_ids: [organization1.id, organization2.id] + } + + post api('/geo_nodes', admin, admin_mode: true), params: geo_node_params + + expect(response).to have_gitlab_http_status(:created) + expect(json_response).not_to include('organization_ids') + end + end + end end describe 'GET /geo_sites' do @@ -294,6 +349,60 @@ expect(json_response).to include(params) end + context 'with organization_ids parameter' do + let(:organization1) { create(:organization) } + let(:organization2) { create(:organization) } + + it 'updates organization_ids parameter' do + params = { + selective_sync_type: "organizations", + selective_sync_organization_ids: [organization1.id, organization2.id] + } + + put api("/geo_sites/#{secondary.id}", admin, admin_mode: true), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/geo_site', dir: 'ee') + expect(json_response).to include('selective_sync_type' => 'organizations') + expect(json_response).to include('selective_sync_organization_ids' => [organization1.id, organization2.id]) + end + + it 'can clear organization_ids by setting to empty array' do + # First set organizations + secondary.update!(selective_sync_type: 'organizations', organization_ids: [organization1.id]) + + params = { + selective_sync_type: "organizations", + selective_sync_organization_ids: [] + } + + put api("/geo_sites/#{secondary.id}", admin, admin_mode: true), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to include('selective_sync_type' => 'organizations') + expect(json_response).to include('selective_sync_organization_ids' => []) + end + + context 'when geo_selective_sync_by_organizations feature flag is disabled' do + before do + stub_feature_flags(geo_selective_sync_by_organizations: false) + end + + it 'does not include selective_sync_organization_ids in the response' do + # Set some organizations in the DB + secondary.update!(organization_ids: [organization1.id]) + + params = { files_max_capacity: 123 } + + put api("/geo_sites/#{secondary.id}", admin, admin_mode: true), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to include('files_max_capacity' => 123) + expect(json_response).not_to include('selective_sync_organization_ids') + end + end + end + it 'can update primary' do params = { url: 'https://updated.example.com/' diff --git a/ee/spec/services/geo/node_update_service_spec.rb b/ee/spec/services/geo/node_update_service_spec.rb index dcbf0f2bcf8d73..a6579039cfc62d 100644 --- a/ee/spec/services/geo/node_update_service_spec.rb +++ b/ee/spec/services/geo/node_update_service_spec.rb @@ -36,5 +36,50 @@ expect(service.execute).to eq false end + + context 'when params includes organization_ids' do + context 'when organization_ids is a string of comma-separated integers' do + it 'updates the organization links' do + organization_1 = create(:organization) + organization_2 = create(:organization) + + service = described_class.new(geo_node, { organization_ids: "#{organization_1.id},#{organization_2.id}" }) + expect(service.execute).to be true + geo_node.reload + + expect(geo_node.organizations).to match_array([organization_1, organization_2]) + end + end + + context 'when organization_ids is an empty string' do + it 'successfully removes organization links' do + organization = create(:organization) + geo_node.organizations << organization + geo_node.save! + expect(geo_node.organizations).not_to be_empty + + service = described_class.new(geo_node, { organization_ids: '' }) + expect(service.execute).to be true + geo_node.reload + + expect(geo_node.organizations).to be_empty + end + end + + context 'when organization_ids is an empty Array' do + it 'successfully removes organization links' do + organization = create(:organization) + geo_node.organizations << organization + geo_node.save! + expect(geo_node.organizations).not_to be_empty + + service = described_class.new(geo_node, { organization_ids: [] }) + expect(service.execute).to be true + geo_node.reload + + expect(geo_node.organizations).to be_empty + end + end + end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index b9a730112868eb..97f4a46ef95d94 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -29354,6 +29354,9 @@ msgstr "" msgid "Geo|Projects in certain groups" msgstr "" +msgid "Geo|Projects in certain organizations" +msgstr "" + msgid "Geo|Projects in certain storage shards" msgstr "" -- GitLab From afd00fe739c061b22e41d2ea8477cd08cd5b8fd9 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 3 Sep 2025 15:47:56 -1000 Subject: [PATCH 2/5] Disabling the feature flag syncs all --- ee/app/controllers/admin/geo/nodes_controller.rb | 4 +--- ee/app/helpers/ee/geo_helper.rb | 2 +- ee/app/models/concerns/geo/selective_sync.rb | 10 +++++++++- ee/lib/api/geo_nodes.rb | 6 ++---- ee/lib/ee/api/entities/geo_node.rb | 2 +- ee/lib/ee/api/entities/geo_site.rb | 2 +- ee/lib/gitlab/geo.rb | 4 ++++ 7 files changed, 19 insertions(+), 11 deletions(-) diff --git a/ee/app/controllers/admin/geo/nodes_controller.rb b/ee/app/controllers/admin/geo/nodes_controller.rb index b0a3bb23bb6d37..20bf4e5be18187 100644 --- a/ee/app/controllers/admin/geo/nodes_controller.rb +++ b/ee/app/controllers/admin/geo/nodes_controller.rb @@ -49,9 +49,7 @@ def geo_node_params selective_sync_shards: [] } - if Feature.enabled?(:geo_selective_sync_by_organizations, :instance) - permitted_hash_params[:organization_ids] = [] - end + permitted_hash_params[:organization_ids] = [] if ::Gitlab::Geo.geo_selective_sync_by_organizations_enabled? params.require(:geo_node).permit( :name, diff --git a/ee/app/helpers/ee/geo_helper.rb b/ee/app/helpers/ee/geo_helper.rb index c3033daa8349d2..2f01fd88c9e58c 100644 --- a/ee/app/helpers/ee/geo_helper.rb +++ b/ee/app/helpers/ee/geo_helper.rb @@ -40,7 +40,7 @@ def selective_sync_types_json } } - if ::Feature.enabled?(:geo_selective_sync_by_organizations, :instance) + if ::Gitlab::Geo.geo_selective_sync_by_organizations_enabled? options[:ORGANIZATIONS] = { label: s_('Geo|Projects in certain organizations'), value: 'organizations' diff --git a/ee/app/models/concerns/geo/selective_sync.rb b/ee/app/models/concerns/geo/selective_sync.rb index a65c9c59160733..f3634e557b2333 100644 --- a/ee/app/models/concerns/geo/selective_sync.rb +++ b/ee/app/models/concerns/geo/selective_sync.rb @@ -9,7 +9,14 @@ module SelectiveSync def selective_sync? validate_selective_sync_type! - selective_sync_type.present? + types = SELECTIVE_SYNC_TYPES + + # If someone enables the FF, tries selective sync by org, and finally disables the FF: + # 1. We won't raise unknown selective sync type error + # 2. But we will assume that they want selective sync to be disabled (sync everything). + types -= 'organizations' unless ::Gitlab::Geo.geo_selective_sync_by_organizations_enabled? + + types.include?(selective_sync_type) end def selective_sync_by_namespaces? @@ -26,6 +33,7 @@ def selective_sync_by_shards? def selective_sync_by_organizations? validate_selective_sync_type! + return false unless ::Gitlab::Geo.geo_selective_sync_by_organizations_enabled? selective_sync_type == 'organizations' end diff --git a/ee/lib/api/geo_nodes.rb b/ee/lib/api/geo_nodes.rb index 448c64a89324f8..32b96524890f8f 100644 --- a/ee/lib/api/geo_nodes.rb +++ b/ee/lib/api/geo_nodes.rb @@ -82,7 +82,7 @@ def update_geo_nodes_endpoint? post do create_params = declared_params(include_missing: false) - create_params.delete(:organization_ids) unless Feature.enabled?(:geo_selective_sync_by_organizations, :instance) + create_params.delete(:organization_ids) unless ::Gitlab::Geo.geo_selective_sync_by_organizations_enabled? new_geo_node = ::Geo::NodeCreateService.new(create_params).execute @@ -277,9 +277,7 @@ def geo_node_status update_params = declared_params(include_missing: false) - unless Feature.enabled?(:geo_selective_sync_by_organizations, :instance) - update_params.delete(:organization_ids) - end + update_params.delete(:organization_ids) unless ::Gitlab::Geo.geo_selective_sync_by_organizations_enabled? updated_geo_node = ::Geo::NodeUpdateService.new(geo_node, update_params).execute diff --git a/ee/lib/ee/api/entities/geo_node.rb b/ee/lib/ee/api/entities/geo_node.rb index 10657cd64ffaa9..7fab4f1ac4f6e8 100644 --- a/ee/lib/ee/api/entities/geo_node.rb +++ b/ee/lib/ee/api/entities/geo_node.rb @@ -23,7 +23,7 @@ class GeoNode < Grape::Entity expose :selective_sync_shards expose :namespace_ids, as: :selective_sync_namespace_ids expose :organization_ids, as: :selective_sync_organization_ids, - if: ->(_, _) { ::Feature.enabled?(:geo_selective_sync_by_organizations, :instance) } + if: ->(_, _) { ::Gitlab::Geo.geo_selective_sync_by_organizations_enabled? } expose :minimum_reverification_interval expose :sync_object_storage, if: ->(geo_node, _) { geo_node.secondary? } diff --git a/ee/lib/ee/api/entities/geo_site.rb b/ee/lib/ee/api/entities/geo_site.rb index 4ca4d45d99e880..f9ca9a0106e798 100644 --- a/ee/lib/ee/api/entities/geo_site.rb +++ b/ee/lib/ee/api/entities/geo_site.rb @@ -23,7 +23,7 @@ class GeoSite < Grape::Entity expose :selective_sync_shards expose :namespace_ids, as: :selective_sync_namespace_ids expose :organization_ids, as: :selective_sync_organization_ids, - if: ->(_, _) { ::Feature.enabled?(:geo_selective_sync_by_organizations, :instance) } + if: ->(_, _) { ::Gitlab::Geo.geo_selective_sync_by_organizations_enabled? } expose :minimum_reverification_interval expose :sync_object_storage, if: ->(geo_site, _) { geo_site.secondary? } diff --git a/ee/lib/gitlab/geo.rb b/ee/lib/gitlab/geo.rb index 409d0ddaa8461d..a51659e7de5516 100644 --- a/ee/lib/gitlab/geo.rb +++ b/ee/lib/gitlab/geo.rb @@ -359,5 +359,9 @@ def self.postgresql_replication_agnostic_enabled? def self.org_mover_extend_selective_sync_to_primary_checksumming? Feature.enabled?(:org_mover_extend_selective_sync_to_primary_checksumming, type: :ops) # rubocop:disable Gitlab/FeatureFlagWithoutActor -- Checksumming is instance wide end + + def self.geo_selective_sync_by_organizations_enabled? + ::Feature.enabled?(:geo_selective_sync_by_organizations, :instance) + end end end -- GitLab From d8b9b144ecba8b1c58d5be1b4b04e29cd217d449 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 4 Sep 2025 14:55:16 -1000 Subject: [PATCH 3/5] Fix selective_sync_by_organizations? tests --- ee/spec/models/geo_node_spec.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ee/spec/models/geo_node_spec.rb b/ee/spec/models/geo_node_spec.rb index 283aa6fbd9f30b..1710fc51abab29 100644 --- a/ee/spec/models/geo_node_spec.rb +++ b/ee/spec/models/geo_node_spec.rb @@ -786,7 +786,7 @@ end describe '#selective_sync_by_organizations?' do - subject { node.selective_sync_by_organizations? } + subject(:selective_sync_by_organizations?) { node.selective_sync_by_organizations? } context 'when selective_sync_type is organizations' do before do @@ -843,8 +843,8 @@ node.update_column(:selective_sync_type, 'invalid') end - it 'returns false' do - is_expected.to be_falsy + it 'raises an error' do + expect { selective_sync_by_organizations? }.to raise_error(Geo::Errors::UnknownSelectiveSyncType) end end @@ -853,8 +853,8 @@ node.update_column(:selective_sync_type, 'Organizations') end - it 'returns false due to case sensitivity' do - is_expected.to be_falsy + it 'raises an error' do + expect { selective_sync_by_organizations? }.to raise_error(Geo::Errors::UnknownSelectiveSyncType) end end @@ -863,8 +863,8 @@ node.update_column(:selective_sync_type, ' organizations ') end - it 'returns false due to exact string match requirement' do - is_expected.to be_falsy + it 'raises an error' do + expect { selective_sync_by_organizations? }.to raise_error(Geo::Errors::UnknownSelectiveSyncType) end end end -- GitLab From 9c33c51e8b3d01dabc2c949f96ebed6c2fe68d15 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 9 Sep 2025 15:43:16 -1000 Subject: [PATCH 4/5] Bump migration timestamp And remove unnecessary foreign_key: nil option. --- ....rb => 20250909140653_create_geo_node_organization_links.rb} | 2 +- db/schema_migrations/20250811140653 | 1 - db/schema_migrations/20250909140653 | 1 + 3 files changed, 2 insertions(+), 2 deletions(-) rename db/migrate/{20250811140653_create_geo_node_organization_links.rb => 20250909140653_create_geo_node_organization_links.rb} (90%) delete mode 100644 db/schema_migrations/20250811140653 create mode 100644 db/schema_migrations/20250909140653 diff --git a/db/migrate/20250811140653_create_geo_node_organization_links.rb b/db/migrate/20250909140653_create_geo_node_organization_links.rb similarity index 90% rename from db/migrate/20250811140653_create_geo_node_organization_links.rb rename to db/migrate/20250909140653_create_geo_node_organization_links.rb index a5c95d0c74f2cc..930912a5a30864 100644 --- a/db/migrate/20250811140653_create_geo_node_organization_links.rb +++ b/db/migrate/20250909140653_create_geo_node_organization_links.rb @@ -9,7 +9,7 @@ class CreateGeoNodeOrganizationLinks < Gitlab::Database::Migration[2.3] def change create_table :geo_node_organization_links do |t| t.references :geo_node, null: false, foreign_key: { on_delete: :cascade }, index: false - t.references :organization, null: false, foreign_key: nil + t.references :organization, null: false t.timestamps_with_timezone null: false t.index [:geo_node_id, :organization_id], unique: true, name: INDEX_NAME end diff --git a/db/schema_migrations/20250811140653 b/db/schema_migrations/20250811140653 deleted file mode 100644 index fd99aba55316ed..00000000000000 --- a/db/schema_migrations/20250811140653 +++ /dev/null @@ -1 +0,0 @@ -001c35a1cd0ab35c3cd2429b45de5502b634697de261221f6ea68c695a6a8fa2 \ No newline at end of file diff --git a/db/schema_migrations/20250909140653 b/db/schema_migrations/20250909140653 new file mode 100644 index 00000000000000..ec0215fa182d45 --- /dev/null +++ b/db/schema_migrations/20250909140653 @@ -0,0 +1 @@ +f05ee8a621716852e7a3cf203439b13eff1c830156abe8a9456a015e394c0f31 \ No newline at end of file -- GitLab From e68e1b766a7e69f656d7615c3267617a35e561be Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 9 Sep 2025 15:51:24 -1000 Subject: [PATCH 5/5] Align validations better with DB constraints --- ee/app/models/geo/geo_node_organization_link.rb | 6 +++--- ee/spec/models/geo/geo_node_organization_link_spec.rb | 5 ++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/ee/app/models/geo/geo_node_organization_link.rb b/ee/app/models/geo/geo_node_organization_link.rb index 7d6aed01e1dcc5..890d12c199c374 100644 --- a/ee/app/models/geo/geo_node_organization_link.rb +++ b/ee/app/models/geo/geo_node_organization_link.rb @@ -2,9 +2,9 @@ module Geo class GeoNodeOrganizationLink < ApplicationRecord - belongs_to :geo_node, inverse_of: :organizations - belongs_to :organization, class_name: 'Organizations::Organization' + belongs_to :geo_node, inverse_of: :organizations, optional: false + belongs_to :organization, class_name: 'Organizations::Organization', optional: false - validates :organization_id, presence: true, uniqueness: { scope: [:geo_node_id] } + validates :organization_id, uniqueness: { scope: [:geo_node_id] } end end diff --git a/ee/spec/models/geo/geo_node_organization_link_spec.rb b/ee/spec/models/geo/geo_node_organization_link_spec.rb index 328c63a213ad8f..5d629f71474f4e 100644 --- a/ee/spec/models/geo/geo_node_organization_link_spec.rb +++ b/ee/spec/models/geo/geo_node_organization_link_spec.rb @@ -4,14 +4,13 @@ RSpec.describe Geo::GeoNodeOrganizationLink, :models, feature_category: :geo_replication do describe 'relationships' do - it { is_expected.to belong_to(:geo_node) } - it { is_expected.to belong_to(:organization).class_name('Organizations::Organization') } + it { is_expected.to belong_to(:geo_node).required } + it { is_expected.to belong_to(:organization).class_name('Organizations::Organization').required } end describe 'validations' do let!(:geo_node_organization_link) { create(:geo_node_organization_link) } - it { is_expected.to validate_presence_of(:organization_id) } it { is_expected.to validate_uniqueness_of(:organization_id).scoped_to(:geo_node_id) } end -- GitLab