Merge branch '198116-project-to_reference-does-not-produce-a-valid-reference' into 'master'

Resolve "Project#to_reference does not produce a valid reference"

Closes #198116

See merge request gitlab-org/gitlab!23452

app/models/commit.rb

@@ -484,10 +484,10 @@ class Commit
   end
 
   def commit_reference(from, referable_commit_id, full: false)
-    reference = project.to_reference(from, full: full)
+    base = project.to_reference_base(from, full: full)
 
-    if reference.present?
-      "#{reference}#{self.class.reference_prefix}#{referable_commit_id}"
+    if base.present?
+      "#{base}#{self.class.reference_prefix}#{referable_commit_id}"
     else
       referable_commit_id
     end

app/models/commit_range.rb

@@ -92,7 +92,7 @@ class CommitRange
   alias_method :id, :to_s
 
   def to_reference(from = nil, full: false)
-    project_reference = project.to_reference(from, full: full)
+    project_reference = project.to_reference_base(from, full: full)
 
     if project_reference.present?
       project_reference + self.class.reference_prefix + self.id
@@ -102,7 +102,7 @@ class CommitRange
   end
 
   def reference_link_text(from = nil)
-    project_reference = project.to_reference(from)
+    project_reference = project.to_reference_base(from)
     reference         = ref_from + notation + ref_to
 
     if project_reference.present?

app/models/concerns/referable.rb

@@ -23,6 +23,14 @@ module Referable
     ''
   end
 
+  # If this referable object can serve as the base for the
+  # reference of child objects (e.g. projects are the base of
+  # issues), but it is formatted differently, then you may wish
+  # to override this method.
+  def to_reference_base(from = nil, full:)
+    to_reference(from, full: full)
+  end
+
   def reference_link_text(from = nil)
     to_reference(from)
   end

app/models/issue.rb

@@ -173,7 +173,7 @@ class Issue < ApplicationRecord
   def to_reference(from = nil, full: false)
     reference = "#{self.class.reference_prefix}#{iid}"
 
-    "#{project.to_reference(from, full: full)}#{reference}"
+    "#{project.to_reference_base(from, full: full)}#{reference}"
   end
 
   def suggested_branch_name

app/models/label.rb

@@ -225,7 +225,7 @@ class Label < ApplicationRecord
     reference = "#{self.class.reference_prefix}#{format_reference}"
 
     if from
-      "#{from.to_reference(target_project, full: full)}#{reference}"
+      "#{from.to_reference_base(target_project, full: full)}#{reference}"
     else
       reference
     end

app/models/merge_request.rb

@@ -396,7 +396,7 @@ class MergeRequest < ApplicationRecord
   def to_reference(from = nil, full: false)
     reference = "#{self.class.reference_prefix}#{iid}"
 
-    "#{project.to_reference(from, full: full)}#{reference}"
+    "#{project.to_reference_base(from, full: full)}#{reference}"
   end
 
   def commits(limit: nil)

app/models/milestone.rb

@@ -228,7 +228,7 @@ class Milestone < ApplicationRecord
     reference = "#{self.class.reference_prefix}#{format_reference}"
 
     if project
-      "#{project.to_reference(from, full: full)}#{reference}"
+      "#{project.to_reference_base(from, full: full)}#{reference}"
     else
       reference
     end

app/models/project.rb

@@ -1068,12 +1068,19 @@ class Project < ApplicationRecord
     end
   end
 
-  def to_reference_with_postfix
-    "#{to_reference(full: true)}#{self.class.reference_postfix}"
+  # Produce a valid reference (see Referable#to_reference)
+  #
+  # NB: For projects, all references are 'full' - i.e. they all include the
+  # full_path, rather than just the project name. For this reason, we ignore
+  # the value of `full:` passed to this method, which is part of the Referable
+  # interface.
+  def to_reference(from = nil, full: false)
+    base = to_reference_base(from, full: true)
+    "#{base}#{self.class.reference_postfix}"
   end
 
   # `from` argument can be a Namespace or Project.
-  def to_reference(from = nil, full: false)
+  def to_reference_base(from = nil, full: false)
     if full || cross_namespace_reference?(from)
       full_path
     elsif cross_project_reference?(from)

app/models/snippet.rb

@@ -180,7 +180,7 @@ class Snippet < ApplicationRecord
     reference = "#{self.class.reference_prefix}#{id}"
 
     if project.present?
-      "#{project.to_reference(from, full: full)}#{reference}"
+      "#{project.to_reference_base(from, full: full)}#{reference}"
     else
       reference
     end

ee/spec/requests/api/issue_links_spec.rb

@@ -141,7 +141,7 @@ describe API::IssueLinks do
 
         it 'returns 201 when sending full path of target project' do
           post api("/projects/#{project.id}/issues/#{issue.iid}/links", user),
-               params: { target_project_id: project.to_reference(full: true), target_issue_iid: target_issue.iid }
+               params: { target_project_id: project.full_path, target_issue_iid: target_issue.iid }
 
           expect_link_response
         end

lib/banzai/filter/milestone_reference_filter.rb

@@ -121,7 +121,7 @@ module Banzai
 
       def object_link_text(object, matches)
         milestone_link = escape_once(super)
-        reference = object.project&.to_reference(project)
+        reference = object.project&.to_reference_base(project)
 
         if reference.present?
           "#{milestone_link} <i>in #{reference}</i>".html_safe

lib/banzai/filter/project_reference_filter.rb

@@ -104,7 +104,7 @@ module Banzai
       def link_to_project(project, link_content: nil)
         url = urls.project_url(project, only_path: context[:only_path])
         data = data_attribute(project: project.id)
-        content = link_content || project.to_reference_with_postfix
+        content = link_content || project.to_reference
 
         link_tag(url, data, content, project.name)
       end

spec/features/issues/move_spec.rb

@@ -32,7 +32,7 @@ describe 'issue move to another project' do
     let(:new_project) { create(:project) }
     let(:new_project_search) { create(:project) }
     let(:text) { "Text with #{mr.to_reference}" }
-    let(:cross_reference) { old_project.to_reference(new_project) }
+    let(:cross_reference) { old_project.to_reference_base(new_project) }
 
     before do
       old_project.add_reporter(user)

spec/lib/banzai/filter/commit_range_reference_filter_spec.rb

@@ -229,10 +229,10 @@ describe Banzai::Filter::CommitRangeReferenceFilter do
     end
 
     it 'ignores invalid commit IDs on the referenced project' do
-      exp = act = "Fixed #{project2.to_reference}@#{commit1.id.reverse}...#{commit2.id}"
+      exp = act = "Fixed #{project2.to_reference_base}@#{commit1.id.reverse}...#{commit2.id}"
       expect(reference_filter(act).to_html).to eq exp
 
-      exp = act = "Fixed #{project2.to_reference}@#{commit1.id}...#{commit2.id.reverse}"
+      exp = act = "Fixed #{project2.to_reference_base}@#{commit1.id}...#{commit2.id.reverse}"
       expect(reference_filter(act).to_html).to eq exp
     end
   end

spec/lib/banzai/filter/label_reference_filter_spec.rb

@@ -369,7 +369,7 @@ describe Banzai::Filter::LabelReferenceFilter do
     end
 
     context 'with project reference' do
-      let(:reference) { "#{project.to_reference}#{group_label.to_reference(format: :name)}" }
+      let(:reference) { "#{project.to_reference_base}#{group_label.to_reference(format: :name)}" }
 
       it 'links to a valid reference' do
         doc = reference_filter("See #{reference}", project: project)
@@ -385,7 +385,7 @@ describe Banzai::Filter::LabelReferenceFilter do
       end
 
       it 'ignores invalid label names' do
-        exp = act = %(Label #{project.to_reference}#{Label.reference_prefix}"#{group_label.name.reverse}")
+        exp = act = %(Label #{project.to_reference_base}#{Label.reference_prefix}"#{group_label.name.reverse}")
 
         expect(reference_filter(act).to_html).to eq exp
       end

spec/lib/banzai/filter/milestone_reference_filter_spec.rb

@@ -367,15 +367,17 @@ describe Banzai::Filter::MilestoneReferenceFilter do
       expect(doc.css('a').first.text).to eq(urls.milestone_url(milestone))
     end
 
-    it 'does not support cross-project references' do
+    it 'does not support cross-project references', :aggregate_failures do
       another_group = create(:group)
       another_project = create(:project, :public, group: group)
-      project_reference = another_project.to_reference(project)
+      project_reference = another_project.to_reference_base(project)
+      input_text = "See #{project_reference}#{reference}"
 
       milestone.update!(group: another_group)
 
-      doc = reference_filter("See #{project_reference}#{reference}")
+      doc = reference_filter(input_text)
 
+      expect(input_text).to match(Milestone.reference_pattern)
       expect(doc.css('a')).to be_empty
     end
 

spec/lib/banzai/filter/project_reference_filter_spec.rb

@@ -10,7 +10,7 @@ describe Banzai::Filter::ProjectReferenceFilter do
   end
 
   def get_reference(project)
-    project.to_reference_with_postfix
+    project.to_reference
   end
 
   let(:project) { create(:project, :public) }

spec/lib/gitlab/gfm/reference_rewriter_spec.rb

@@ -8,7 +8,7 @@ describe Gitlab::Gfm::ReferenceRewriter do
   let(:new_project) { create(:project, name: 'new-project', group: group) }
   let(:user) { create(:user) }
 
-  let(:old_project_ref) { old_project.to_reference(new_project) }
+  let(:old_project_ref) { old_project.to_reference_base(new_project) }
   let(:text) { 'some text' }
 
   before do

spec/models/project_spec.rb

@@ -131,23 +131,19 @@ describe Project do
     end
 
     context 'when creating a new project' do
-      it 'automatically creates a CI/CD settings row' do
-        project = create(:project)
+      let_it_be(:project) { create(:project) }
 
+      it 'automatically creates a CI/CD settings row' do
         expect(project.ci_cd_settings).to be_an_instance_of(ProjectCiCdSetting)
         expect(project.ci_cd_settings).to be_persisted
       end
 
       it 'automatically creates a container expiration policy row' do
-        project = create(:project)
-
         expect(project.container_expiration_policy).to be_an_instance_of(ContainerExpirationPolicy)
         expect(project.container_expiration_policy).to be_persisted
       end
 
       it 'automatically creates a Pages metadata row' do
-        project = create(:project)
-
         expect(project.pages_metadatum).to be_an_instance_of(ProjectPagesMetadatum)
         expect(project.pages_metadatum).to be_persisted
       end
@@ -532,111 +528,114 @@ describe Project do
     it { is_expected.to delegate_method(:last_pipeline).to(:commit).with_arguments(allow_nil: true) }
   end
 
-  describe '#to_reference_with_postfix' do
-    it 'returns the full path with reference_postfix' do
-      namespace = create(:namespace, path: 'sample-namespace')
-      project = create(:project, path: 'sample-project', namespace: namespace)
-
-      expect(project.to_reference_with_postfix).to eq 'sample-namespace/sample-project>'
-    end
-  end
+  describe 'reference methods' do
+    let_it_be(:owner)     { create(:user, name: 'Gitlab') }
+    let_it_be(:namespace) { create(:namespace, name: 'Sample namespace', path: 'sample-namespace', owner: owner) }
+    let_it_be(:project)   { create(:project, name: 'Sample project', path: 'sample-project', namespace: namespace) }
+    let_it_be(:group)     { create(:group, name: 'Group', path: 'sample-group') }
+    let_it_be(:another_project) { create(:project, namespace: namespace) }
+    let_it_be(:another_namespace_project) { create(:project, name: 'another-project') }
 
-  describe '#to_reference' do
-    let(:owner)     { create(:user, name: 'Gitlab') }
-    let(:namespace) { create(:namespace, path: 'sample-namespace', owner: owner) }
-    let(:project)   { create(:project, path: 'sample-project', namespace: namespace) }
-    let(:group)     { create(:group, name: 'Group', path: 'sample-group') }
+    describe '#to_reference' do
+      it 'returns the path with reference_postfix' do
+        expect(project.to_reference).to eq("#{project.full_path}>")
+      end
 
-    context 'when nil argument' do
-      it 'returns nil' do
-        expect(project.to_reference).to be_nil
+      it 'returns the path with reference_postfix when arg is self' do
+        expect(project.to_reference(project)).to eq("#{project.full_path}>")
       end
-    end
 
-    context 'when full is true' do
-      it 'returns complete path to the project' do
-        expect(project.to_reference(full: true)).to          eq 'sample-namespace/sample-project'
-        expect(project.to_reference(project, full: true)).to eq 'sample-namespace/sample-project'
-        expect(project.to_reference(group, full: true)).to   eq 'sample-namespace/sample-project'
+      it 'returns the full_path with reference_postfix when full' do
+        expect(project.to_reference(full: true)).to eq("#{project.full_path}>")
       end
-    end
 
-    context 'when same project argument' do
-      it 'returns nil' do
-        expect(project.to_reference(project)).to be_nil
+      it 'returns the full_path with reference_postfix when cross-project' do
+        expect(project.to_reference(build_stubbed(:project))).to eq("#{project.full_path}>")
       end
     end
 
-    context 'when cross namespace project argument' do
-      let(:another_namespace_project) { create(:project, name: 'another-project') }
-
-      it 'returns complete path to the project' do
-        expect(project.to_reference(another_namespace_project)).to eq 'sample-namespace/sample-project'
+    describe '#to_reference_base' do
+      context 'when nil argument' do
+        it 'returns nil' do
+          expect(project.to_reference_base).to be_nil
+        end
       end
-    end
 
-    context 'when same namespace / cross-project argument' do
-      let(:another_project) { create(:project, namespace: namespace) }
+      context 'when full is true' do
+        it 'returns complete path to the project', :aggregate_failures do
+          be_full_path = eq('sample-namespace/sample-project')
 
-      it 'returns path to the project' do
-        expect(project.to_reference(another_project)).to eq 'sample-project'
+          expect(project.to_reference_base(full: true)).to be_full_path
+          expect(project.to_reference_base(project, full: true)).to be_full_path
+          expect(project.to_reference_base(group, full: true)).to be_full_path
+        end
       end
-    end
 
-    context 'when different namespace / cross-project argument' do
-      let(:another_namespace) { create(:namespace, path: 'another-namespace', owner: owner) }
-      let(:another_project)   { create(:project, path: 'another-project', namespace: another_namespace) }
+      context 'when same project argument' do
+        it 'returns nil' do
+          expect(project.to_reference_base(project)).to be_nil
+        end
+      end
 
-      it 'returns full path to the project' do
-        expect(project.to_reference(another_project)).to eq 'sample-namespace/sample-project'
+      context 'when cross namespace project argument' do
+        it 'returns complete path to the project' do
+          expect(project.to_reference_base(another_namespace_project)).to eq 'sample-namespace/sample-project'
+        end
       end
-    end
 
-    context 'when argument is a namespace' do
-      context 'with same project path' do
+      context 'when same namespace / cross-project argument' do
         it 'returns path to the project' do
-          expect(project.to_reference(namespace)).to eq 'sample-project'
+          expect(project.to_reference_base(another_project)).to eq 'sample-project'
         end
       end
 
-      context 'with different project path' do
+      context 'when different namespace / cross-project argument with same owner' do
+        let(:another_namespace_same_owner) { create(:namespace, path: 'another-namespace', owner: owner) }
+        let(:another_project_same_owner)   { create(:project, path: 'another-project', namespace: another_namespace_same_owner) }
+
         it 'returns full path to the project' do
-          expect(project.to_reference(group)).to eq 'sample-namespace/sample-project'
+          expect(project.to_reference_base(another_project_same_owner)).to eq 'sample-namespace/sample-project'
         end
       end
-    end
-  end
 
-  describe '#to_human_reference' do
-    let(:owner) { create(:user, name: 'Gitlab') }
-    let(:namespace) { create(:namespace, name: 'Sample namespace', owner: owner) }
-    let(:project) { create(:project, name: 'Sample project', namespace: namespace) }
+      context 'when argument is a namespace' do
+        context 'with same project path' do
+          it 'returns path to the project' do
+            expect(project.to_reference_base(namespace)).to eq 'sample-project'
+          end
+        end
 
-    context 'when nil argument' do
-      it 'returns nil' do
-        expect(project.to_human_reference).to be_nil
+        context 'with different project path' do
+          it 'returns full path to the project' do
+            expect(project.to_reference_base(group)).to eq 'sample-namespace/sample-project'
+          end
+        end
       end
     end
 
-    context 'when same project argument' do
-      it 'returns nil' do
-        expect(project.to_human_reference(project)).to be_nil
+    describe '#to_human_reference' do
+      context 'when nil argument' do
+        it 'returns nil' do
+          expect(project.to_human_reference).to be_nil
+        end
       end
-    end
-
-    context 'when cross namespace project argument' do
-      let(:another_namespace_project) { create(:project, name: 'another-project') }
 
-      it 'returns complete name with namespace of the project' do
-        expect(project.to_human_reference(another_namespace_project)).to eq 'Gitlab / Sample project'
+      context 'when same project argument' do
+        it 'returns nil' do
+          expect(project.to_human_reference(project)).to be_nil
+        end
       end
-    end
 
-    context 'when same namespace / cross-project argument' do
-      let(:another_project) { create(:project, namespace: namespace) }
+      context 'when cross namespace project argument' do
+        it 'returns complete name with namespace of the project' do
+          expect(project.to_human_reference(another_namespace_project)).to eq 'Gitlab / Sample project'
+        end
+      end
 
-      it 'returns name of the project' do
-        expect(project.to_human_reference(another_project)).to eq 'Sample project'
+      context 'when same namespace / cross-project argument' do
+        it 'returns name of the project' do
+          expect(project.to_human_reference(another_project)).to eq 'Sample project'
+        end
       end
     end
   end