From dfdd8145a65c0eb96cc4f3f2ca483fbaed8dcd06 Mon Sep 17 00:00:00 2001 From: hustewart Date: Mon, 22 Sep 2025 11:48:15 -0400 Subject: [PATCH 1/7] Add snippet_repository sharding associations A snippet repository can use either an organization or a project for sharding. This adds that associations to both sides. --- app/models/organizations/organization.rb | 1 + app/models/project.rb | 1 + app/models/snippet_repository.rb | 12 ++++++++++++ spec/models/organizations/organization_spec.rb | 1 + spec/models/project_spec.rb | 1 + spec/models/snippet_repository_spec.rb | 2 ++ 6 files changed, 18 insertions(+) diff --git a/app/models/organizations/organization.rb b/app/models/organizations/organization.rb index f016e47ce110f9..f9b38d179cda7f 100644 --- a/app/models/organizations/organization.rb +++ b/app/models/organizations/organization.rb @@ -25,6 +25,7 @@ class Organization < ApplicationRecord has_many :root_groups, -> { roots }, class_name: 'Group', inverse_of: :organization has_many :projects has_many :snippets + has_many :snippet_repositories, inverse_of: :organization has_many :topics, class_name: "Projects::Topic" has_many :integrations diff --git a/app/models/project.rb b/app/models/project.rb index 74a648a44305a5..6cbc91041b50a3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -347,6 +347,7 @@ def self.integration_association_name(name) has_many :tag_push_hooks_integrations, -> { tag_push_hooks }, class_name: 'Integration' has_many :wiki_page_hooks_integrations, -> { wiki_page_hooks }, class_name: 'Integration' has_many :snippets, class_name: 'ProjectSnippet' + has_many :snippet_repositories, inverse_of: :project has_many :hooks, class_name: 'ProjectHook' has_many :protected_branches has_many :exported_protected_branches diff --git a/app/models/snippet_repository.rb b/app/models/snippet_repository.rb index 6e0e9437903754..b19ebddd45729c 100644 --- a/app/models/snippet_repository.rb +++ b/app/models/snippet_repository.rb @@ -13,6 +13,18 @@ class SnippetRepository < ApplicationRecord belongs_to :snippet, inverse_of: :snippet_repository + belongs_to :organization, + class_name: 'Organizations::Organization', + foreign_key: 'snippet_organization_id', + inverse_of: :snippet_repositories, + optional: true + + belongs_to :project, + class_name: 'Project', + foreign_key: 'snippet_project_id', + inverse_of: :snippet_repositories, + optional: true + delegate :repository, :repository_storage, to: :snippet class << self diff --git a/spec/models/organizations/organization_spec.rb b/spec/models/organizations/organization_spec.rb index c37da0b7344822..a1a594979ae1a4 100644 --- a/spec/models/organizations/organization_spec.rb +++ b/spec/models/organizations/organization_spec.rb @@ -26,6 +26,7 @@ it { is_expected.to have_many(:organization_users).inverse_of(:organization) } it { is_expected.to have_many :projects } it { is_expected.to have_many :snippets } + it { is_expected.to have_many :snippet_repositories } it { is_expected.to have_many :topics } end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 0febda10dfa1e4..7f8c277b3e8a07 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -48,6 +48,7 @@ it { is_expected.to have_many(:namespace_requesters) } it { is_expected.to have_many(:notes).dependent(:destroy) } it { is_expected.to have_many(:snippets).class_name('ProjectSnippet') } + it { is_expected.to have_many(:snippet_repositories) } it { is_expected.to have_many(:deploy_keys_projects) } it { is_expected.to have_many(:deploy_keys) } it { is_expected.to have_many(:hooks) } diff --git a/spec/models/snippet_repository_spec.rb b/spec/models/snippet_repository_spec.rb index 70fed43624e37b..763354a94c7826 100644 --- a/spec/models/snippet_repository_spec.rb +++ b/spec/models/snippet_repository_spec.rb @@ -12,6 +12,8 @@ describe 'associations' do it { is_expected.to belong_to(:shard) } it { is_expected.to belong_to(:snippet) } + it { is_expected.to belong_to(:organization) } + it { is_expected.to belong_to(:project) } end it_behaves_like 'shardable scopes' do -- GitLab From 451d9402f48c09de082cd839c6ffcf4d18ce68b9 Mon Sep 17 00:00:00 2001 From: hustewart Date: Mon, 22 Sep 2025 16:05:19 -0400 Subject: [PATCH 2/7] Ensure and validate snippet repo sharding keys --- app/models/snippet_repository.rb | 25 +++++++ spec/models/snippet_repository_spec.rb | 97 ++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/app/models/snippet_repository.rb b/app/models/snippet_repository.rb index b19ebddd45729c..2a74629c41cf20 100644 --- a/app/models/snippet_repository.rb +++ b/app/models/snippet_repository.rb @@ -27,6 +27,10 @@ class SnippetRepository < ApplicationRecord delegate :repository, :repository_storage, to: :snippet + before_validation :ensure_sharding_keys + + validate :validate_has_exactly_one_sharding_key + class << self def find_snippet(disk_path) find_by(disk_path: disk_path)&.snippet @@ -55,6 +59,27 @@ def multi_files_action(user, files = [], **options) private + def ensure_sharding_keys + return if sharding_keys_present? && !snippet_changed? + + self.organization = snippet&.organization if snippet&.organization_id.present? + self.project = snippet&.project if snippet&.project_id.present? + end + + def sharding_keys_present? + snippet_organization_id.present? || snippet_project_id.present? + end + + def validate_has_exactly_one_sharding_key + sharding_keys = [organization, project].compact + + if sharding_keys.empty? + errors.add(:base, 'must belong to either an organization or a project') + elsif sharding_keys.size > 1 + errors.add(:base, 'cannot belong to both an organization and a project') + end + end + def capture_git_error(&block) yield block rescue Gitlab::Git::Index::IndexError, diff --git a/spec/models/snippet_repository_spec.rb b/spec/models/snippet_repository_spec.rb index 763354a94c7826..177f7fe44701d7 100644 --- a/spec/models/snippet_repository_spec.rb +++ b/spec/models/snippet_repository_spec.rb @@ -16,6 +16,103 @@ it { is_expected.to belong_to(:project) } end + # This is for the cells related sharding keys: + # snippet_project_id and snippet_organization_id + describe 'sharding key validations' do + let_it_be(:organization) { create(:organization) } + let_it_be(:project) { create(:project, organization: organization) } + let_it_be(:personal_snippet) { create(:personal_snippet, organization: organization) } + let_it_be(:project_snippet) { create(:project_snippet, project: project) } + + describe 'validations' do + context 'when both sharding keys are present' do + it 'is invalid' do + snippet_repo = build(:snippet_repository, + snippet: personal_snippet, + organization: organization, + project: project) + + expect(snippet_repo).not_to be_valid + expect(snippet_repo.errors[:base]).to include('cannot belong to both an organization and a project') + end + end + + context 'when neither sharding key is present' do + it 'is invalid' do + personal_snippet.organization_id = nil + + snippet_repo = build(:snippet_repository, + snippet: personal_snippet, + organization: nil, + project: nil) + + expect(snippet_repo).not_to be_valid + expect(snippet_repo.errors[:base]).to include('must belong to either an organization or a project') + end + end + + context 'when only organization is present' do + it 'is valid' do + snippet_repo = build(:snippet_repository, + snippet: personal_snippet, + organization: organization, + project: nil) + + expect(snippet_repo).to be_valid + end + end + + context 'when only project is present' do + it 'is valid' do + snippet_repo = build(:snippet_repository, + snippet: project_snippet, + organization: nil, + project: project) + + expect(snippet_repo).to be_valid + end + end + end + + describe 'before_validation callbacks' do + context 'for personal snippet' do + it 'automatically assigns organization from snippet' do + snippet_repo = build(:snippet_repository, snippet: personal_snippet) + + expect(snippet_repo.organization).to be_nil + snippet_repo.valid? + expect(snippet_repo.organization).to eq(personal_snippet.organization) + end + end + + context 'for project snippet' do + it 'automatically assigns project from snippet' do + snippet_repo = build(:snippet_repository, snippet: project_snippet) + + expect(snippet_repo.project).to be_nil + snippet_repo.valid? + expect(snippet_repo.project).to eq(project_snippet.project) + end + end + + context 'when snippet changes' do + it 'reassigns sharding keys on snippet change' do + new_personal_snippet = create(:personal_snippet, organization: create(:organization)) + snippet_repo = create(:snippet_repository, snippet: project_snippet) + + expect(snippet_repo.project).to eq(project_snippet.project) + expect(snippet_repo.organization).to be_nil + + snippet_repo.snippet = new_personal_snippet + snippet_repo.valid? + + expect(snippet_repo.organization).to eq(new_personal_snippet.organization) + expect(snippet_repo.project).to eq(project_snippet.project) # Should still be set + end + end + end + end + it_behaves_like 'shardable scopes' do let_it_be(:record_1) { create(:snippet_repository) } let_it_be(:record_2, reload: true) { create(:snippet_repository) } -- GitLab From 731e5ccbbac92e330ef2f874d1129c380d938671 Mon Sep 17 00:00:00 2001 From: hustewart Date: Tue, 23 Sep 2025 14:26:57 -0400 Subject: [PATCH 3/7] Update all models yml --- spec/lib/gitlab/import_export/all_models.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 78c0b417a17e9c..0946541d5dba24 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -715,6 +715,7 @@ project: - iterations - notes - snippets +- snippet_repositories - hooks - all_protected_branches - protected_branches -- GitLab From 350bef67fa2757a9fbf452f1cfa04e9a8b7001f6 Mon Sep 17 00:00:00 2001 From: hustewart Date: Tue, 23 Sep 2025 16:41:13 -0400 Subject: [PATCH 4/7] Simplify before validation assignment --- app/models/snippet_repository.rb | 16 +++++++--------- spec/models/snippet_repository_spec.rb | 16 ---------------- 2 files changed, 7 insertions(+), 25 deletions(-) diff --git a/app/models/snippet_repository.rb b/app/models/snippet_repository.rb index 2a74629c41cf20..4a1d6c51fc92d0 100644 --- a/app/models/snippet_repository.rb +++ b/app/models/snippet_repository.rb @@ -59,23 +59,21 @@ def multi_files_action(user, files = [], **options) private + def compact_sharding_keys + [organization, project].compact + end + def ensure_sharding_keys - return if sharding_keys_present? && !snippet_changed? + return if compact_sharding_keys.size == 1 self.organization = snippet&.organization if snippet&.organization_id.present? self.project = snippet&.project if snippet&.project_id.present? end - def sharding_keys_present? - snippet_organization_id.present? || snippet_project_id.present? - end - def validate_has_exactly_one_sharding_key - sharding_keys = [organization, project].compact - - if sharding_keys.empty? + if compact_sharding_keys.empty? errors.add(:base, 'must belong to either an organization or a project') - elsif sharding_keys.size > 1 + elsif compact_sharding_keys.size > 1 errors.add(:base, 'cannot belong to both an organization and a project') end end diff --git a/spec/models/snippet_repository_spec.rb b/spec/models/snippet_repository_spec.rb index 177f7fe44701d7..48d18683f6d79d 100644 --- a/spec/models/snippet_repository_spec.rb +++ b/spec/models/snippet_repository_spec.rb @@ -94,22 +94,6 @@ expect(snippet_repo.project).to eq(project_snippet.project) end end - - context 'when snippet changes' do - it 'reassigns sharding keys on snippet change' do - new_personal_snippet = create(:personal_snippet, organization: create(:organization)) - snippet_repo = create(:snippet_repository, snippet: project_snippet) - - expect(snippet_repo.project).to eq(project_snippet.project) - expect(snippet_repo.organization).to be_nil - - snippet_repo.snippet = new_personal_snippet - snippet_repo.valid? - - expect(snippet_repo.organization).to eq(new_personal_snippet.organization) - expect(snippet_repo.project).to eq(project_snippet.project) # Should still be set - end - end end end -- GitLab From 1de86140f031048f8f5e3371e532d40db3f0b31e Mon Sep 17 00:00:00 2001 From: hustewart Date: Tue, 23 Sep 2025 16:45:30 -0400 Subject: [PATCH 5/7] Add i18n to errors --- app/models/snippet_repository.rb | 4 ++-- locale/gitlab.pot | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/models/snippet_repository.rb b/app/models/snippet_repository.rb index 4a1d6c51fc92d0..8bbe1469e0af0c 100644 --- a/app/models/snippet_repository.rb +++ b/app/models/snippet_repository.rb @@ -72,9 +72,9 @@ def ensure_sharding_keys def validate_has_exactly_one_sharding_key if compact_sharding_keys.empty? - errors.add(:base, 'must belong to either an organization or a project') + errors.add(:base, -'must belong to either an organization or a project') elsif compact_sharding_keys.size > 1 - errors.add(:base, 'cannot belong to both an organization and a project') + errors.add(:base, _('cannot belong to both an organization and a project')) end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5596a4236072c4..062f359969c0d1 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -76910,6 +76910,9 @@ msgstr "" msgid "cannot be used for user namespace" msgstr "" +msgid "cannot belong to both an organization and a project" +msgstr "" + msgid "cannot contain HTML/XML tags, including any word between angle brackets (<,>)." msgstr "" -- GitLab From ba948400eb0a7c84fafe454756a840e3a2ff8fcf Mon Sep 17 00:00:00 2001 From: hustewart Date: Thu, 25 Sep 2025 09:53:47 -0400 Subject: [PATCH 6/7] Fix error message typo --- app/models/snippet_repository.rb | 2 +- locale/gitlab.pot | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/snippet_repository.rb b/app/models/snippet_repository.rb index 8bbe1469e0af0c..ec5f18ca23e773 100644 --- a/app/models/snippet_repository.rb +++ b/app/models/snippet_repository.rb @@ -72,7 +72,7 @@ def ensure_sharding_keys def validate_has_exactly_one_sharding_key if compact_sharding_keys.empty? - errors.add(:base, -'must belong to either an organization or a project') + errors.add(:base, _('must belong to either an organization or a project')) elsif compact_sharding_keys.size > 1 errors.add(:base, _('cannot belong to both an organization and a project')) end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 062f359969c0d1..6de352ce889ba6 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -78418,6 +78418,9 @@ msgstr "" msgid "must belong to either a project or an organization" msgstr "" +msgid "must belong to either an organization or a project" +msgstr "" + msgid "must belong to same project of its requirement object." msgstr "" -- GitLab From e061b40bd870fa4367d77cbfb196d14080b5e79a Mon Sep 17 00:00:00 2001 From: hustewart Date: Thu, 25 Sep 2025 10:16:20 -0400 Subject: [PATCH 7/7] Add edge case testing --- spec/models/snippet_repository_spec.rb | 39 ++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/spec/models/snippet_repository_spec.rb b/spec/models/snippet_repository_spec.rb index 48d18683f6d79d..657f66d4184a2c 100644 --- a/spec/models/snippet_repository_spec.rb +++ b/spec/models/snippet_repository_spec.rb @@ -75,6 +75,9 @@ end describe 'before_validation callbacks' do + let_it_be(:other_organization) { create(:organization) } + let_it_be(:other_project) { create(:project) } + context 'for personal snippet' do it 'automatically assigns organization from snippet' do snippet_repo = build(:snippet_repository, snippet: personal_snippet) @@ -83,6 +86,24 @@ snippet_repo.valid? expect(snippet_repo.organization).to eq(personal_snippet.organization) end + + # This is an edge case because the key would initially be based on the + # snippet's key, but the test exists to prove that we early return if + # the key exists. + context 'when one sharding key is already set' do + it 'retains the same key' do + snippet_repo = build( + :snippet_repository, + snippet: personal_snippet, + organization: other_organization + ) + + expect(snippet_repo.organization).not_to be_nil + snippet_repo.valid? + expect(snippet_repo.organization).not_to eq(personal_snippet.organization) + expect(snippet_repo.organization).to eq(other_organization) + end + end end context 'for project snippet' do @@ -94,6 +115,24 @@ expect(snippet_repo.project).to eq(project_snippet.project) end end + + # This is an edge case because the key would initially be based on the + # snippet's key, but the test exists to prove that we early return if + # the key exists. + context 'when one sharding key is already set' do + it 'retains the same key' do + snippet_repo = build( + :snippet_repository, + snippet: project_snippet, + project: other_project + ) + + expect(snippet_repo.project).not_to be_nil + snippet_repo.valid? + expect(snippet_repo.project).not_to eq(project_snippet.project) + expect(snippet_repo.project).to eq(other_project) + end + end end end -- GitLab