From 0637f1be5d4df9cf3fdef4af4c0c4dbf8b9ccdd3 Mon Sep 17 00:00:00 2001 From: Nicolas Dular Date: Fri, 17 Oct 2025 17:02:49 +0200 Subject: [PATCH] Fix gfm reference for namespaces with the same path When we create a system note reference when an issue gets mentioned in an epic, we created a relative reference, like "mentioned in gitlab#X". However, when the group and the issue have the same namespace path name, it was an ambigious reference because the reference parser is looking first to resolve namespace paths within the current parent namespace. In the example above, when there is a group and a project called `gitlab`, we'd create the system note on the `gitlab` project with `mentioned in gitlab#X`. When looking up a namespace with the name `gitlab` in the parent group of the issue, we'd find the group and would reference some issue that has the IID 123. This is not correct, since we wanted to reference the group level issue/work item. By generating an absolute reference in the case above `mentioned in /gitlab#X` we remove the ambiguity as the reference parser can find the correct group work item. Changelog: fixed EE: true --- app/models/issue.rb | 34 +++++++++++++++++-- .../ee/system_notes/issuables_service_spec.rb | 33 ++++++++++++++++++ spec/models/issue_spec.rb | 19 +++++++++++ 3 files changed, 83 insertions(+), 3 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index 3fc57757d361ab..4d3b2e644fb791 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -575,10 +575,10 @@ def blocked_for_repositioning? end # `from` argument can be a Namespace or Project. - def to_reference(from = nil, full: false) + def to_reference(from = nil, full: false, absolute_path: false) reference = "#{self.class.reference_prefix}#{iid}" - "#{namespace.to_reference_base(from, full: full)}#{reference}" + "#{namespace.to_reference_base(from, full: full, absolute_path: absolute_path)}#{reference}" end def suggested_branch_name @@ -817,7 +817,35 @@ def hook_attrs override :gfm_reference def gfm_reference(from = nil) - "#{work_item_type_with_default.name.underscore} #{to_reference(from)}" + # References can be ambiguous when two namespaces have the same path names. This is why we need to use absolute + # paths when cross-referencing between projects and groups. + # + # Example setup: + # - `gitlab` group + # - `gitlab` project within the `gitlab` group + # - In a project issue, we reference an epic with `epic gitlab#123` for a system note + # + # When resolving this system note, we would look for a namespace within the `gitlab` project's parent. + # This would be incorrect, as it would resolve to an ISSUE on the `gitlab` PROJECT, not the `gitlab` GROUP. + # + # This problem only occurs when there is a project with the same name as the group. Otherwise, our reference + # code attempts to resolve it on the group. + # + # Since the reference parser has no information about where each reference originated, we need to fix this in + # the reference itself by providing an absolute path. + # + # In the example above, the `from` argument is the Issue on the Project, and `self` is the item on the Group. + # Every time we reference from a project to a group, we need to use an absolute path. + # So we need to reference it as `epic /gitlab#123`. + # + # We could always use absolute paths to remove the ambiguity, but this would lead to longer references everywhere + # that are harder to read. + params = {} + if (from.is_a?(Project) || from.is_a?(Namespaces::ProjectNamespace)) && group_level? + params = { full: true, absolute_path: true } + end + + "#{work_item_type_with_default.name.underscore} #{to_reference(from, **params)}" end def skip_metrics? diff --git a/ee/spec/services/ee/system_notes/issuables_service_spec.rb b/ee/spec/services/ee/system_notes/issuables_service_spec.rb index 2019261c0884d1..1c860f75fb3c56 100644 --- a/ee/spec/services/ee/system_notes/issuables_service_spec.rb +++ b/ee/spec/services/ee/system_notes/issuables_service_spec.rb @@ -316,6 +316,8 @@ describe '#cross_reference' do let(:mentioned_in) { create(:issue, project: project) } + let(:new_note) { service.cross_reference(mentioned_in) } + subject { service.cross_reference(mentioned_in) } context 'when noteable is an epic' do @@ -333,6 +335,37 @@ end end + context 'with project and group having the same path name' do + using RSpec::Parameterized::TableSyntax + + let_it_be_with_reload(:group) { create(:group, path: 'same-name') } + let_it_be_with_reload(:project) { create(:project, path: 'same-name', group: group) } + let_it_be_with_reload(:noteable) { create(:issue, project: project) } + + let_it_be(:issue) { create(:issue, project: project, iid: 100) } + let_it_be(:issue_2) { create(:issue, project: project, iid: 101) } + let_it_be(:epic) { create(:epic, group: group, iid: 100) } + let_it_be(:epic_work_item) { create(:work_item, :epic, namespace: group, iid: 101) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, iid: 100) } + + let(:service) { described_class.new(noteable: noteable, container: group, author: author) } + + where(:mentioned_in, :notable, :type) do + ref(:epic) | ref(:issue) | 'epic' + ref(:epic) | ref(:merge_request) | 'epic' + ref(:issue) | ref(:merge_request) | 'issue' + ref(:epic_work_item) | ref(:issue) | 'epic' + ref(:epic_work_item) | ref(:merge_request) | 'epic' + ref(:merge_request) | ref(:epic_work_item) | 'merge request' + end + + with_them do + it 'has the correct link' do + expect(new_note.note_html).to include(::Gitlab::UrlBuilder.build(mentioned_in, only_path: true)) + end + end + end + context 'when notable is not an epic' do it 'does not tracks epic cross reference event in usage ping' do expect(::Gitlab::UsageDataCounters::EpicActivityUniqueCounter).not_to receive(:track_epic_cross_referenced) diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 5585e799761d1d..928c1d51a9f65b 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -1862,6 +1862,25 @@ expect(issue.gfm_reference).to eq("#{expected_name} #{issue.to_reference}") end end + + context 'when referencing a group level issue' do + let_it_be(:group) { create(:group) } + let_it_be(:group_issue) { create(:issue, :group_level, namespace: group) } + let_it_be(:group_issue2) { create(:issue, :group_level, namespace: group) } + let_it_be(:issue) { create(:issue) } + + it 'uses uses an absolute path when reference comes from a project' do + expect(group_issue.gfm_reference(issue.project)).to eq("issue /#{group.full_path}##{group_issue.iid}") + end + + it 'uses uses an absolute path when reference comes from a project namespace' do + expect(group_issue.gfm_reference(issue.namespace)).to eq("issue /#{group.full_path}##{group_issue.iid}") + end + + it 'does not use an absolute path when reference comes from a group' do + expect(group_issue.gfm_reference(group)).to eq("issue ##{group_issue.iid}") + end + end end describe '#has_widget?' do -- GitLab