Merge branch 'url-builder-refactor' into 'master'

Support more types in UrlBuilder

See merge request gitlab-org/gitlab!28195

app/models/concerns/has_repository.rb

@@ -110,7 +110,7 @@ module HasRepository
   end
 
   def web_url(only_path: nil)
-    raise NotImplementedError
+    Gitlab::UrlBuilder.build(self, only_path: only_path)
   end
 
   def repository_size_checker

app/models/group.rb

@@ -172,8 +172,8 @@ class Group < Namespace
     "#{self.class.reference_prefix}#{full_path}"
   end
 
-  def web_url
-    Gitlab::Routing.url_helpers.group_canonical_url(self)
+  def web_url(only_path: nil)
+    Gitlab::UrlBuilder.build(self, only_path: only_path)
   end
 
   def human_name

app/models/personal_snippet.rb

@@ -2,8 +2,4 @@
 
 class PersonalSnippet < Snippet
   include WithUploads
-
-  def web_url(only_path: nil)
-    Gitlab::Routing.url_helpers.snippet_url(self, only_path: only_path)
-  end
 end

app/models/project.rb

@@ -1121,10 +1121,6 @@ class Project < ApplicationRecord
     end
   end
 
-  def web_url(only_path: nil)
-    Gitlab::Routing.url_helpers.project_url(self, only_path: only_path)
-  end
-
   def readme_url
     readme_path = repository.readme_path
     if readme_path

app/models/project_snippet.rb

@@ -5,8 +5,4 @@ class ProjectSnippet < Snippet
 
   validates :project, presence: true
   validates :secret, inclusion: { in: [false] }
-
-  def web_url(only_path: nil)
-    Gitlab::Routing.url_helpers.project_snippet_url(project, self, only_path: only_path)
-  end
 end

app/models/project_wiki.rb

@@ -44,8 +44,8 @@ class ProjectWiki
   # @deprecated use full_path when you need it for an URL route or disk_path when you want to point to the filesystem
   alias_method :path_with_namespace, :full_path
 
-  def web_url
-    Gitlab::Routing.url_helpers.project_wiki_url(@project, :home)
+  def web_url(only_path: nil)
+    Gitlab::UrlBuilder.build(self, only_path: only_path)
   end
 
   def url_to_repo

app/presenters/commit_presenter.rb

@@ -18,7 +18,7 @@ class CommitPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def web_url
-    Gitlab::UrlBuilder.new(commit).url
+    url_builder.build(commit)
   end
 
   def signature_html

app/presenters/issue_presenter.rb

@@ -4,20 +4,14 @@ class IssuePresenter < Gitlab::View::Presenter::Delegated
   presents :issue
 
   def web_url
-    url_builder.url
+    url_builder.build(issue)
   end
 
   def issue_path
-    url_builder.issue_path(issue)
+    url_builder.build(issue, only_path: true)
   end
 
   def subscribed?
     issue.subscribed?(current_user, issue.project)
   end
-
-  private
-
-  def url_builder
-    @url_builder ||= Gitlab::UrlBuilder.new(issue)
-  end
 end

app/presenters/milestone_presenter.rb

@@ -4,12 +4,6 @@ class MilestonePresenter < Gitlab::View::Presenter::Delegated
   presents :milestone
 
   def milestone_path
-    url_builder.milestone_path(milestone)
-  end
-
-  private
-
-  def url_builder
-    @url_builder ||= Gitlab::UrlBuilder.new(milestone)
+    url_builder.build(milestone, only_path: true)
   end
 end

ee/app/presenters/epic_issue_presenter.rb

@@ -11,10 +11,6 @@ class EpicIssuePresenter < Gitlab::View::Presenter::Delegated
 
   private
 
-  def url_builder
-    @url_builder ||= Gitlab::UrlBuilder.new(issue)
-  end
-
   def can_admin_issue_link?(current_user)
     Ability.allowed?(current_user, :admin_epic_issue, issue) && Ability.allowed?(current_user, :admin_epic, issue.epic)
   end

ee/app/presenters/epic_presenter.rb

@@ -14,11 +14,11 @@ class EpicPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def group_epic_path
-    url_builder.group_epic_path(epic.group, epic)
+    url_builder.build(epic, only_path: true)
   end
 
   def group_epic_url
-    url_builder.group_epic_url(epic.group, epic)
+    url_builder.build(epic)
   end
 
   def group_epic_link_path
@@ -146,9 +146,4 @@ class EpicPresenter < Gitlab::View::Presenter::Delegated
       }
     end
   end
-
-  # important for using routing helpers in GraphQL
-  def url_builder
-    @url_builder ||= Gitlab::UrlBuilder.new(epic)
-  end
 end

ee/lib/ee/gitlab/url_builder.rb

@@ -3,45 +3,47 @@
 module EE
   module Gitlab
     module UrlBuilder
-      extend ::Gitlab::Utils::Override
+      extend ActiveSupport::Concern
 
-      override :url
-      def url
-        case object
-        when ::DesignManagement::Design
-          design_url
-        when Epic
-          group_epic_url(object.group, object)
-        when Vulnerability
-          project_security_vulnerability_url(object.project, object)
-        else
-          super
+      class_methods do
+        extend ::Gitlab::Utils::Override
+
+        override :build
+        def build(object, **options)
+          case object.itself
+          when ::DesignManagement::Design
+            design_url(object, **options)
+          when Epic
+            instance.group_epic_url(object.group, object, **options)
+          when Vulnerability
+            instance.project_security_vulnerability_url(object.project, object, **options)
+          else
+            super
+          end
         end
-      end
 
-      override :note_url
-      def note_url
-        noteable = object.noteable
+        override :note_url
+        def note_url(note, **options)
+          noteable = note.noteable
 
-        if object.for_epic?
-          group_epic_url(noteable.group, noteable, anchor: dom_id(object))
-        elsif object.for_vulnerability?
-          project_security_vulnerability_url(noteable.project, noteable, anchor: dom_id(object))
-        else
-          super
+          if note.for_epic?
+            instance.group_epic_url(noteable.group, noteable, anchor: dom_id(note), **options)
+          elsif note.for_vulnerability?
+            instance.project_security_vulnerability_url(noteable.project, noteable, anchor: dom_id(note), **options)
+          else
+            super
+          end
         end
-      end
-
-      private
 
-      def design_url
-        size, ref = opts.values_at(:size, :ref)
-        design = object
+        def design_url(design, **options)
+          size, ref = options.values_at(:size, :ref)
+          options.except!(:size, :ref)
 
-        if size
-          project_design_management_designs_resized_image_url(design.project, design, ref, size)
-        else
-          project_design_management_designs_raw_image_url(design.project, design, ref)
+          if size
+            instance.project_design_management_designs_resized_image_url(design.project, design, ref, size, **options)
+          else
+            instance.project_design_management_designs_raw_image_url(design.project, design, ref, **options)
+          end
         end
       end
     end

ee/spec/lib/ee/gitlab/url_builder_spec.rb

@@ -3,63 +3,40 @@
 require 'spec_helper'
 
 describe Gitlab::UrlBuilder do
-  describe '.build' do
-    context 'when passing a DesignManagement::Design' do
-      it 'returns a proper URL to the raw (unresized) image' do
-        design = build_stubbed(:design)
-
-        url = described_class.build(design, ref: 'master')
-
-        expect(url).to eq "#{Settings.gitlab['url']}/#{design.project.full_path}/-/design_management/designs/#{design.id}/master/raw_image"
-      end
+  subject { described_class }
 
-      it 'returns a proper URL to the resized image' do
-        design = build_stubbed(:design)
+  describe '.build' do
+    using RSpec::Parameterized::TableSyntax
 
-        url = described_class.build(design, ref: 'master', size: 'small')
+    where(:factory, :path_generator) do
+      :design                | ->(design)        { "/#{design.project.full_path}/-/design_management/designs/#{design.id}/raw_image" }
+      :epic                  | ->(epic)          { "/groups/#{epic.group.full_path}/-/epics/#{epic.iid}" }
+      :vulnerability         | ->(vulnerability) { "/#{vulnerability.project.full_path}/-/security/vulnerabilities/#{vulnerability.id}" }
 
-        expect(url).to eq "#{Settings.gitlab['url']}/#{design.project.full_path}/-/design_management/designs/#{design.id}/master/resized_image/small"
-      end
+      :note_on_epic          | ->(note)          { "/groups/#{note.noteable.group.full_path}/-/epics/#{note.noteable.iid}#note_#{note.id}" }
+      :note_on_vulnerability | ->(note)          { "/#{note.project.full_path}/-/security/vulnerabilities/#{note.noteable.id}#note_#{note.id}" }
     end
 
-    context 'when passing an epic' do
-      it 'returns a proper URL' do
-        epic = build_stubbed(:epic, iid: 42)
-
-        url = described_class.build(epic)
+    with_them do
+      let(:object) { build_stubbed(factory) }
+      let(:path) { path_generator.call(object) }
 
-        expect(url).to eq "#{Settings.gitlab['url']}/groups/#{epic.group.full_path}/-/epics/#{epic.iid}"
+      it 'returns the full URL' do
+        expect(subject.build(object)).to eq("#{Settings.gitlab['url']}#{path}")
       end
-    end
-
-    context 'when passing an epic note' do
-      it 'returns a proper URL' do
-        epic = create(:epic)
-        note = build_stubbed(:note_on_epic, noteable: epic)
 
-        url = described_class.build(note)
-
-        expect(url).to eq "#{Settings.gitlab['url']}/groups/#{epic.group.full_path}/-/epics/#{epic.iid}#note_#{note.id}"
+      it 'returns only the path if only_path is set' do
+        expect(subject.build(object, only_path: true)).to eq(path)
       end
     end
 
-    context 'when passing a vulnerability' do
-      it 'returns a proper URL' do
-        vulnerability = build_stubbed(:vulnerability, id: 42)
-
-        url = described_class.build(vulnerability)
-
-        expect(url).to eq "#{Settings.gitlab['url']}/#{vulnerability.project.full_path}/-/security/vulnerabilities/#{vulnerability.id}"
-      end
-    end
+    context 'when passing a DesignManagement::Design' do
+      let(:design) { build_stubbed(:design) }
 
-    context 'when passing a vulnerability note' do
-      it 'returns a proper URL' do
-        vulnerability = create(:vulnerability)
-        note = build_stubbed(:note_on_vulnerability, noteable: vulnerability)
-        url = described_class.build(note)
+      it 'uses the given ref and size in the URL' do
+        url = subject.build(design, ref: 'feature', size: 'small')
 
-        expect(url).to eq "#{Settings.gitlab['url']}/#{vulnerability.project.full_path}/-/security/vulnerabilities/#{vulnerability.id}#note_#{note.id}"
+        expect(url).to eq "#{Settings.gitlab['url']}/#{design.project.full_path}/-/design_management/designs/#{design.id}/feature/resized_image/small"
       end
     end
   end

lib/gitlab/url_builder.rb

@@ -2,76 +2,74 @@
 
 module Gitlab
   class UrlBuilder
+    include Singleton
     include Gitlab::Routing
     include GitlabRoutingHelper
-    include ActionView::RecordIdentifier
 
-    attr_reader :object, :opts
+    delegate :build, to: :class
 
-    def self.build(object, opts = {})
-      new(object, opts).url
-    end
+    class << self
+      include ActionView::RecordIdentifier
 
-    def url
-      # Objects are sometimes wrapped in a BatchLoader instance
-      case object.itself
-      when Commit
-        commit_url
-      when Issue
-        issue_url(object)
-      when MergeRequest
-        merge_request_url(object)
-      when Note
-        note_url
-      when WikiPage
-        wiki_page_url
-      when Snippet
-        opts[:raw].present? ? gitlab_raw_snippet_url(object) : gitlab_snippet_url(object)
-      when Milestone
-        milestone_url(object)
-      when ::Ci::Build
-        project_job_url(object.project, object)
-      when User
-        user_url(object)
-      else
-        raise NotImplementedError.new("No URL builder defined for #{object.inspect}")
+      def build(object, **options)
+        # Objects are sometimes wrapped in a BatchLoader instance
+        case object.itself
+        when ::Ci::Build
+          instance.project_job_url(object.project, object, **options)
+        when Commit
+          commit_url(object, **options)
+        when Group
+          instance.group_canonical_url(object, **options)
+        when Issue
+          instance.issue_url(object, **options)
+        when MergeRequest
+          instance.merge_request_url(object, **options)
+        when Milestone
+          instance.milestone_url(object, **options)
+        when Note
+          note_url(object, **options)
+        when Project
+          instance.project_url(object, **options)
+        when Snippet
+          snippet_url(object, **options)
+        when User
+          instance.user_url(object, **options)
+        when ProjectWiki
+          instance.project_wiki_url(object.project, :home, **options)
+        when WikiPage
+          instance.project_wiki_url(object.wiki.project, object.slug, **options)
+        else
+          raise NotImplementedError.new("No URL builder defined for #{object.inspect}")
+        end
       end
-    end
-
-    private
 
-    def initialize(object, opts = {})
-      @object = object
-      @opts = opts
-    end
-
-    def commit_url(opts = {})
-      return '' if object.project.nil?
+      def commit_url(commit, **options)
+        return '' unless commit.project
 
-      namespace_project_commit_url({
-        namespace_id: object.project.namespace,
-        project_id: object.project,
-        id: object.id
-      }.merge!(opts))
-    end
-
-    def note_url
-      if object.for_commit?
-        commit_url(id: object.commit_id, anchor: dom_id(object))
-
-      elsif object.for_issue?
-        issue_url(object.noteable, anchor: dom_id(object))
+        instance.commit_url(commit, **options)
+      end
 
-      elsif object.for_merge_request?
-        merge_request_url(object.noteable, anchor: dom_id(object))
+      def note_url(note, **options)
+        if note.for_commit?
+          return '' unless note.project
 
-      elsif object.for_snippet?
-        gitlab_snippet_url(object.noteable, anchor: dom_id(object))
+          instance.project_commit_url(note.project, note.commit_id, anchor: dom_id(note), **options)
+        elsif note.for_issue?
+          instance.issue_url(note.noteable, anchor: dom_id(note), **options)
+        elsif note.for_merge_request?
+          instance.merge_request_url(note.noteable, anchor: dom_id(note), **options)
+        elsif note.for_snippet?
+          instance.gitlab_snippet_url(note.noteable, anchor: dom_id(note), **options)
+        end
       end
-    end
 
-    def wiki_page_url
-      project_wiki_url(object.wiki.project, object.slug)
+      def snippet_url(snippet, **options)
+        if options.delete(:raw).present?
+          instance.gitlab_raw_snippet_url(snippet, **options)
+        else
+          instance.gitlab_snippet_url(snippet, **options)
+        end
+      end
     end
   end
 end

lib/gitlab/view/presenter/base.rb

@@ -26,6 +26,10 @@ module Gitlab
           self
         end
 
+        def url_builder
+          Gitlab::UrlBuilder.instance
+        end
+
         class_methods do
           def presenter?
             true

spec/factories/milestones.rb

@@ -25,6 +25,14 @@ FactoryBot.define do
       due_date { Date.new(2000, 1, 30) }
     end
 
+    trait :on_project do
+      project
+    end
+
+    trait :on_group do
+      group
+    end
+
     after(:build, :stub) do |milestone, evaluator|
       if evaluator.group
         milestone.group = evaluator.group
@@ -44,5 +52,7 @@ FactoryBot.define do
 
     factory :active_milestone, traits: [:active]
     factory :closed_milestone, traits: [:closed]
+    factory :project_milestone, traits: [:on_project]
+    factory :group_milestone, traits: [:on_group]
   end
 end

spec/models/personal_snippet_spec.rb

@@ -21,8 +21,6 @@ describe PersonalSnippet do
     let_it_be(:container) { create(:personal_snippet, :repository) }
     let(:stubbed_container) { build_stubbed(:personal_snippet) }
     let(:expected_full_path) { "@snippets/#{container.id}" }
-    let(:expected_repository_klass) { Repository }
-    let(:expected_storage_klass) { Storage::Hashed }
     let(:expected_web_url_path) { "snippets/#{container.id}" }
   end
 end

spec/models/project_snippet_spec.rb

@@ -37,8 +37,6 @@ describe ProjectSnippet do
     let_it_be(:container) { create(:project_snippet, :repository) }
     let(:stubbed_container) { build_stubbed(:project_snippet) }
     let(:expected_full_path) { "#{container.project.full_path}/@snippets/#{container.id}" }
-    let(:expected_repository_klass) { Repository }
-    let(:expected_storage_klass) { Storage::Hashed }
     let(:expected_web_url_path) { "#{container.project.full_path}/snippets/#{container.id}" }
   end
 end

spec/models/project_spec.rb

@@ -116,9 +116,6 @@ describe Project do
       let_it_be(:container) { create(:project, :repository, path: 'somewhere') }
       let(:stubbed_container) { build_stubbed(:project) }
       let(:expected_full_path) { "#{container.namespace.full_path}/somewhere" }
-      let(:expected_repository_klass) { Repository }
-      let(:expected_storage_klass) { Storage::Hashed }
-      let(:expected_web_url_path) { "#{container.namespace.full_path}/somewhere" }
     end
 
     it 'has an inverse relationship with merge requests' do

spec/models/project_wiki_spec.rb

@@ -28,7 +28,7 @@ describe ProjectWiki do
 
   describe '#web_url' do
     it 'returns the full web URL to the wiki' do
-      expect(subject.web_url).to eq("#{Gitlab.config.gitlab.url}/#{project.full_path}/-/wikis/home")
+      expect(subject.web_url).to eq(Gitlab::UrlBuilder.build(subject))
     end
   end
 

spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb

 # frozen_string_literal: true
 
 RSpec.shared_examples 'model with repository' do
+  let(:container) { raise NotImplementedError }
+  let(:stubbed_container) { raise NotImplementedError }
+  let(:expected_full_path) { raise NotImplementedError }
+  let(:expected_web_url_path) { expected_full_path }
+
   describe '#commits_by' do
     let(:commits) { container.repository.commits('HEAD', limit: 3).commits }
     let(:commit_shas) { commits.map(&:id) }
@@ -46,74 +51,33 @@ RSpec.shared_examples 'model with repository' do
     end
   end
 
-  describe '#ssh_url_to_repo' do
-    it 'returns container ssh address' do
-      expect(container.ssh_url_to_repo).to eq container.url_to_repo
+  describe '#url_to_repo' do
+    it 'returns the SSH URL to the repository' do
+      expect(container.url_to_repo).to eq("#{Gitlab.config.gitlab_shell.ssh_path_prefix}#{expected_web_url_path}.git")
     end
   end
 
-  describe '#http_url_to_repo' do
-    subject { container.http_url_to_repo }
-
-    context 'when a custom HTTP clone URL root is not set' do
-      it 'returns the url to the repo without a username' do
-        expect(subject).to eq("#{container.web_url}.git")
-        expect(subject).not_to include('@')
-      end
+  describe '#ssh_url_to_repo' do
+    it 'returns the SSH URL to the repository' do
+      expect(container.ssh_url_to_repo).to eq(container.url_to_repo)
     end
+  end
 
-    context 'when a custom HTTP clone URL root is set' do
-      before do
-        stub_application_setting(custom_http_clone_url_root: custom_http_clone_url_root)
-      end
-
-      context 'when custom HTTP clone URL root has a relative URL root' do
-        context 'when custom HTTP clone URL root ends with a slash' do
-          let(:custom_http_clone_url_root) { 'https://git.example.com:51234/mygitlab/' }
-
-          it 'returns the url to the repo, with the root replaced with the custom one' do
-            expect(subject).to eq("#{custom_http_clone_url_root}#{expected_web_url_path}.git")
-          end
-        end
-
-        context 'when custom HTTP clone URL root does not end with a slash' do
-          let(:custom_http_clone_url_root) { 'https://git.example.com:51234/mygitlab' }
-
-          it 'returns the url to the repo, with the root replaced with the custom one' do
-            expect(subject).to eq("#{custom_http_clone_url_root}/#{expected_web_url_path}.git")
-          end
-        end
-      end
-
-      context 'when custom HTTP clone URL root does not have a relative URL root' do
-        context 'when custom HTTP clone URL root ends with a slash' do
-          let(:custom_http_clone_url_root) { 'https://git.example.com:51234/' }
-
-          it 'returns the url to the repo, with the root replaced with the custom one' do
-            expect(subject).to eq("#{custom_http_clone_url_root}#{expected_web_url_path}.git")
-          end
-        end
-
-        context 'when custom HTTP clone URL root does not end with a slash' do
-          let(:custom_http_clone_url_root) { 'https://git.example.com:51234' }
-
-          it 'returns the url to the repo, with the root replaced with the custom one' do
-            expect(subject).to eq("#{custom_http_clone_url_root}/#{expected_web_url_path}.git")
-          end
-        end
-      end
+  describe '#http_url_to_repo' do
+    it 'returns the HTTP URL to the repository' do
+      expect(container.http_url_to_repo).to eq("#{Gitlab.config.gitlab.url}/#{expected_web_url_path}.git")
     end
   end
 
   describe '#repository' do
     it 'returns valid repo' do
-      expect(container.repository).to be_kind_of(expected_repository_klass)
+      expect(container.repository).to be_kind_of(Repository)
     end
   end
 
   describe '#storage' do
     it 'returns valid storage' do
-      expect(container.storage).to be_kind_of(expected_storage_klass)
+      expect(container.storage).to be_kind_of(Storage::Hashed)
     end
   end