Merge branch 'fj-refactor-repo-type' into 'master'

Refactor GlRepository to include suffix

See merge request gitlab-org/gitlab!23494

app/models/project.rb

@@ -2207,7 +2207,7 @@ class Project < ApplicationRecord
   end
 
   def reference_counter(type: Gitlab::GlRepository::PROJECT)
-    Gitlab::ReferenceCounter.new(type.identifier_for_repositorable(self))
+    Gitlab::ReferenceCounter.new(type.identifier_for_container(self))
   end
 
   def badges

app/models/project_wiki.rb

@@ -65,7 +65,7 @@ class ProjectWiki
   # Returns the Gitlab::Git::Wiki object.
   def wiki
     @wiki ||= begin
-      gl_repository = Gitlab::GlRepository::WIKI.identifier_for_repositorable(project)
+      gl_repository = Gitlab::GlRepository::WIKI.identifier_for_container(project)
       raw_repository = Gitlab::Git::Repository.new(project.repository_storage, disk_path + '.git', gl_repository, full_path)
 
       create_repo!(raw_repository) unless raw_repository.exists?

app/models/repository.rb

@@ -1183,7 +1183,7 @@ class Repository
   def initialize_raw_repository
     Gitlab::Git::Repository.new(project.repository_storage,
                                 disk_path + '.git',
-                                repo_type.identifier_for_repositorable(project),
+                                repo_type.identifier_for_container(project),
                                 project.full_path)
   end
 end

ee/app/controllers/ee/repositories/git_http_controller.rb

@@ -20,7 +20,7 @@ module EE
         if ::Gitlab::Geo.primary?
           # This ID is used by the /internal/post_receive API call
           gl_id = ::Gitlab::GlId.gl_id(user)
-          gl_repository = repo_type.identifier_for_repositorable(project)
+          gl_repository = repo_type.identifier_for_container(project)
           node_id = params["geo_node_id"]
           ::Gitlab::Geo::GitPushHttp.new(gl_id, gl_repository).cache_referrer_node(node_id)
         end

ee/lib/ee/gitlab/gl_repository.rb

@@ -9,7 +9,8 @@ module EE
       DESIGN = ::Gitlab::GlRepository::RepoType.new(
         name: :design,
         access_checker_class: ::Gitlab::GitAccessDesign,
-        repository_accessor: -> (project) { ::DesignManagement::Repository.new(project) }
+        repository_resolver: -> (project) { ::DesignManagement::Repository.new(project) },
+        suffix: :design
       )
 
       EE_TYPES = {

ee/spec/lib/ee/gitlab/gl_repository_spec.rb

@@ -8,7 +8,7 @@ describe ::EE::Gitlab::GlRepository do
     end
 
     it "builds a design repository" do
-      expect(described_class::DESIGN.repository_accessor.call(create(:project)))
+      expect(described_class::DESIGN.repository_resolver.call(create(:project)))
         .to be_a(::DesignManagement::Repository)
     end
   end

ee/spec/lib/gitlab/gl_repository/repo_type_spec.rb

@@ -2,19 +2,27 @@
 require 'spec_helper'
 
 describe Gitlab::GlRepository::RepoType do
-  set(:project) { create(:project) }
+  let_it_be(:project) { create(:project) }
 
   describe Gitlab::GlRepository::DESIGN do
     it_behaves_like 'a repo type' do
       let(:expected_identifier) { "design-#{project.id}" }
       let(:expected_id) { project.id.to_s }
-      let(:expected_suffix) { ".design" }
+      let(:expected_suffix) { '.design' }
       let(:expected_repository) { project.design_repository }
+      let(:expected_container) { project }
     end
 
-    it "knows its type" do
+    it 'knows its type' do
       expect(described_class).to be_design
       expect(described_class).not_to be_project
+      expect(described_class).not_to be_wiki
+    end
+
+    it 'checks if repository path is valid' do
+      expect(described_class.valid?(project.design_repository.full_path)).to be_truthy
+      expect(described_class.valid?(project.repository.full_path)).to be_falsey
+      expect(described_class.valid?(project.wiki.repository.full_path)).to be_falsey
     end
   end
 end

ee/spec/requests/api/internal/base_spec.rb

@@ -118,7 +118,7 @@ describe API::Internal::Base do
 
     context "for design repositories" do
       set(:project) { create(:project) }
-      let(:gl_repository) { EE::Gitlab::GlRepository::DESIGN.identifier_for_repositorable(project) }
+      let(:gl_repository) { EE::Gitlab::GlRepository::DESIGN.identifier_for_container(project) }
 
       it "does not allow access" do
         post(api("/internal/allowed"),

ee/spec/requests/git_http_geo_spec.rb

@@ -385,7 +385,7 @@ describe "Git HTTP requests (Geo)", :geo do
               it 'returns a 200' do
                 is_expected.to have_gitlab_http_status(:ok)
                 expect(json_response['GL_ID']).to match("user-#{user.id}")
-                expect(json_response['GL_REPOSITORY']).to match(Gitlab::GlRepository::PROJECT.identifier_for_repositorable(project))
+                expect(json_response['GL_REPOSITORY']).to match(Gitlab::GlRepository::PROJECT.identifier_for_container(project))
               end
             end
           end

ee/spec/services/design_management/delete_designs_service_spec.rb

@@ -96,7 +96,7 @@ describe DesignManagement::DeleteDesignsService do
         end
 
         it 'calls repository#log_geo_updated_event' do
-          design_repository = EE::Gitlab::GlRepository::DESIGN.repository_accessor.call(project)
+          design_repository = EE::Gitlab::GlRepository::DESIGN.repository_resolver.call(project)
           allow_any_instance_of(described_class).to receive(:repository).and_return(design_repository)
 
           expect(design_repository).to receive(:log_geo_updated_event)

ee/spec/services/design_management/save_designs_service_spec.rb

@@ -7,7 +7,7 @@ describe DesignManagement::SaveDesignsService do
   let(:developer) { create(:user) }
   let(:user) { developer }
   let(:files) { [rails_sample] }
-  let(:design_repository) { EE::Gitlab::GlRepository::DESIGN.repository_accessor.call(project) }
+  let(:design_repository) { EE::Gitlab::GlRepository::DESIGN.repository_resolver.call(project) }
   let(:rails_sample_name) { 'rails_sample.jpg' }
   let(:rails_sample) { sample_image(rails_sample_name) }
   let(:dk_png) { sample_image('dk.png') }

lib/api/helpers/internal_helpers.rb

@@ -116,7 +116,7 @@ module API
       # Project id to pass between components that don't share/don't have
       # access to the same filesystem mounts
       def gl_repository
-        repo_type.identifier_for_repositorable(project)
+        repo_type.identifier_for_container(project)
       end
 
       def gl_project_path

lib/gitlab/gl_repository.rb

@@ -7,12 +7,13 @@ module Gitlab
     PROJECT = RepoType.new(
       name: :project,
       access_checker_class: Gitlab::GitAccess,
-      repository_accessor: -> (project) { project.repository }
+      repository_resolver: -> (project) { project.repository }
     ).freeze
     WIKI = RepoType.new(
       name: :wiki,
       access_checker_class: Gitlab::GitAccessWiki,
-      repository_accessor: -> (project) { project.wiki.repository }
+      repository_resolver: -> (project) { project.wiki.repository },
+      suffix: :wiki
     ).freeze
 
     TYPES = {
@@ -27,15 +28,14 @@ module Gitlab
     def self.parse(gl_repository)
       type_name, _id = gl_repository.split('-').first
       type = types[type_name]
-      subject_id = type&.fetch_id(gl_repository)
 
-      unless subject_id
+      unless type
         raise ArgumentError, "Invalid GL Repository \"#{gl_repository}\""
       end
 
-      project = Project.find_by_id(subject_id)
+      container = type.fetch_container!(gl_repository)
 
-      [project, type]
+      [container, type]
     end
 
     def self.default_type

lib/gitlab/gl_repository/repo_type.rb

@@ -5,16 +5,25 @@ module Gitlab
     class RepoType
       attr_reader :name,
                   :access_checker_class,
-                  :repository_accessor
+                  :repository_resolver,
+                  :container_resolver,
+                  :suffix
 
-      def initialize(name:, access_checker_class:, repository_accessor:)
+      def initialize(
+        name:,
+        access_checker_class:,
+        repository_resolver:,
+        container_resolver: default_container_resolver,
+        suffix: nil)
         @name = name
         @access_checker_class = access_checker_class
-        @repository_accessor = repository_accessor
+        @repository_resolver = repository_resolver
+        @container_resolver = container_resolver
+        @suffix = suffix
       end
 
-      def identifier_for_repositorable(repositorable)
-        "#{name}-#{repositorable.id}"
+      def identifier_for_container(container)
+        "#{name}-#{container.id}"
       end
 
       def fetch_id(identifier)
@@ -22,6 +31,14 @@ module Gitlab
         match[:id] if match
       end
 
+      def fetch_container!(identifier)
+        id = fetch_id(identifier)
+
+        raise ArgumentError, "Invalid GL Repository \"#{identifier}\"" unless id
+
+        container_resolver.call(id)
+      end
+
       def wiki?
         self == WIKI
       end
@@ -31,11 +48,21 @@ module Gitlab
       end
 
       def path_suffix
-        project? ? "" : ".#{name}"
+        suffix ? ".#{suffix}" : ''
       end
 
-      def repository_for(repositorable)
-        repository_accessor.call(repositorable)
+      def repository_for(container)
+        repository_resolver.call(container)
+      end
+
+      def valid?(repository_path)
+        repository_path.end_with?(path_suffix)
+      end
+
+      private
+
+      def default_container_resolver
+        -> (id) { Project.find_by_id(id) }
       end
     end
   end

lib/gitlab/repo_path.rb

@@ -4,8 +4,9 @@ module Gitlab
   module RepoPath
     NotFoundError = Class.new(StandardError)
 
-    def self.parse(repo_path)
-      project_path = repo_path.sub(/\.git\z/, '').sub(%r{\A/}, '')
+    def self.parse(path)
+      repo_path = path.sub(/\.git\z/, '').sub(%r{\A/}, '')
+      redirected_path = nil
 
       # Detect the repo type based on the path, the first one tried is the project
       # type, which does not have a suffix.
@@ -14,10 +15,13 @@ module Gitlab
         # type.
         # We'll always try to find a project with an empty suffix (for the
         # `Gitlab::GlRepository::PROJECT` type.
-        next unless project_path.end_with?(type.path_suffix)
+        next unless type.valid?(repo_path)
 
-        project, was_redirected = find_project(project_path.chomp(type.path_suffix))
-        redirected_path = project_path if was_redirected
+        # Removing the suffix (.wiki, .design, ...) from the project path
+        full_path = repo_path.chomp(type.path_suffix)
+
+        project, was_redirected = find_project(full_path)
+        redirected_path = repo_path if was_redirected
 
         # If we found a matching project, then the type was matched, no need to
         # continue looking.

lib/gitlab/workhorse.rb

@@ -24,7 +24,7 @@ module Gitlab
 
         attrs = {
           GL_ID: Gitlab::GlId.gl_id(user),
-          GL_REPOSITORY: repo_type.identifier_for_repositorable(repository.project),
+          GL_REPOSITORY: repo_type.identifier_for_container(repository.project),
           GL_USERNAME: user&.username,
           ShowAllRefs: show_all_refs,
           Repository: repository.gitaly_repository.to_h,

spec/lib/gitlab/gl_repository/repo_type_spec.rb

@@ -2,33 +2,45 @@
 require 'spec_helper'
 
 describe Gitlab::GlRepository::RepoType do
-  set(:project) { create(:project) }
+  let_it_be(:project) { create(:project) }
 
   describe Gitlab::GlRepository::PROJECT do
     it_behaves_like 'a repo type' do
       let(:expected_identifier) { "project-#{project.id}" }
       let(:expected_id) { project.id.to_s }
-      let(:expected_suffix) { "" }
+      let(:expected_suffix) { '' }
       let(:expected_repository) { project.repository }
+      let(:expected_container) { project }
     end
 
-    it "knows its type" do
+    it 'knows its type' do
       expect(described_class).not_to be_wiki
       expect(described_class).to be_project
     end
+
+    it 'checks if repository path is valid' do
+      expect(described_class.valid?(project.repository.full_path)).to be_truthy
+      expect(described_class.valid?(project.wiki.repository.full_path)).to be_truthy
+    end
   end
 
   describe Gitlab::GlRepository::WIKI do
     it_behaves_like 'a repo type' do
       let(:expected_identifier) { "wiki-#{project.id}" }
       let(:expected_id) { project.id.to_s }
-      let(:expected_suffix) { ".wiki" }
+      let(:expected_suffix) { '.wiki' }
       let(:expected_repository) { project.wiki.repository }
+      let(:expected_container) { project }
     end
 
-    it "knows its type" do
+    it 'knows its type' do
       expect(described_class).to be_wiki
       expect(described_class).not_to be_project
     end
+
+    it 'checks if repository path is valid' do
+      expect(described_class.valid?(project.repository.full_path)).to be_falsey
+      expect(described_class.valid?(project.wiki.repository.full_path)).to be_truthy
+    end
   end
 end

spec/lib/gitlab/gl_repository_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 describe ::Gitlab::GlRepository do
   describe '.parse' do
-    set(:project) { create(:project, :repository) }
+    let_it_be(:project) { create(:project, :repository) }
 
     it 'parses a project gl_repository' do
       expect(described_class.parse("project-#{project.id}")).to eq([project, Gitlab::GlRepository::PROJECT])
@@ -14,8 +14,12 @@ describe ::Gitlab::GlRepository do
       expect(described_class.parse("wiki-#{project.id}")).to eq([project, Gitlab::GlRepository::WIKI])
     end
 
-    it 'throws an argument error on an invalid gl_repository' do
+    it 'throws an argument error on an invalid gl_repository type' do
       expect { described_class.parse("badformat-#{project.id}") }.to raise_error(ArgumentError)
     end
+
+    it 'throws an argument error on an invalid gl_repository id' do
+      expect { described_class.parse("project-foo") }.to raise_error(ArgumentError)
+    end
   end
 end

spec/models/project_spec.rb

@@ -3992,7 +3992,7 @@ describe Project do
       end
 
       it 'schedules HashedStorage::ProjectMigrateWorker with delayed start when the project repo is in use' do
-        Gitlab::ReferenceCounter.new(Gitlab::GlRepository::PROJECT.identifier_for_repositorable(project)).increase
+        Gitlab::ReferenceCounter.new(Gitlab::GlRepository::PROJECT.identifier_for_container(project)).increase
 
         expect(HashedStorage::ProjectMigrateWorker).to receive(:perform_in)
 
@@ -4000,7 +4000,7 @@ describe Project do
       end
 
       it 'schedules HashedStorage::ProjectMigrateWorker with delayed start when the wiki repo is in use' do
-        Gitlab::ReferenceCounter.new(Gitlab::GlRepository::WIKI.identifier_for_repositorable(project)).increase
+        Gitlab::ReferenceCounter.new(Gitlab::GlRepository::WIKI.identifier_for_container(project)).increase
 
         expect(HashedStorage::ProjectMigrateWorker).to receive(:perform_in)
 

spec/requests/api/internal/base_spec.rb

@@ -268,7 +268,7 @@ describe API::Internal::Base do
       end
 
       context 'with env passed as a JSON' do
-        let(:gl_repository) { Gitlab::GlRepository::WIKI.identifier_for_repositorable(project) }
+        let(:gl_repository) { Gitlab::GlRepository::WIKI.identifier_for_container(project) }
 
         it 'sets env in RequestStore' do
           obj_dir_relative = './objects'
@@ -1054,9 +1054,9 @@ describe API::Internal::Base do
   def gl_repository_for(project_or_wiki)
     case project_or_wiki
     when ProjectWiki
-      Gitlab::GlRepository::WIKI.identifier_for_repositorable(project_or_wiki.project)
+      Gitlab::GlRepository::WIKI.identifier_for_container(project_or_wiki.project)
     when Project
-      Gitlab::GlRepository::PROJECT.identifier_for_repositorable(project_or_wiki)
+      Gitlab::GlRepository::PROJECT.identifier_for_container(project_or_wiki)
     else
       nil
     end

spec/support/shared_examples/lib/gitlab/repo_type_shared_examples.rb

 # frozen_string_literal: true
 
 RSpec.shared_examples 'a repo type' do
-  describe "#identifier_for_repositorable" do
-    subject { described_class.identifier_for_repositorable(project) }
+  describe '#identifier_for_container' do
+    subject { described_class.identifier_for_container(project) }
 
     it { is_expected.to eq(expected_identifier) }
   end
 
-  describe "#fetch_id" do
-    it "finds an id match in the identifier" do
+  describe '#fetch_id' do
+    it 'finds an id match in the identifier' do
       expect(described_class.fetch_id(expected_identifier)).to eq(expected_id)
     end
 
     it 'does not break on other identifiers' do
-      expect(described_class.fetch_id("wiki-noid")).to eq(nil)
+      expect(described_class.fetch_id('wiki-noid')).to eq(nil)
     end
   end
 
-  describe "#path_suffix" do
+  describe '#fetch_container!' do
+    it 'returns the container' do
+      expect(described_class.fetch_container!(expected_identifier)).to eq expected_container
+    end
+
+    it 'raises an exception if the identifier is invalid' do
+      expect { described_class.fetch_container!('project-noid') }.to raise_error ArgumentError
+    end
+  end
+
+  describe '#path_suffix' do
     subject { described_class.path_suffix }
 
     it { is_expected.to eq(expected_suffix) }
   end
 
-  describe "#repository_for" do
-    it "finds the repository for the repo type" do
+  describe '#repository_for' do
+    it 'finds the repository for the repo type' do
       expect(described_class.repository_for(project)).to eq(expected_repository)
     end
   end