Moving away from the "extend" based factory to a more traditional one.

Using `extend` dynamically can lead to bad performance as it
invalidates the method's cache.

5 files changed, 54 insertions, 36 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 3e26c4b2e0f..5f510412015 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -32,6 +32,8 @@ class Project < ActiveRecord::Base
            :merge_requests_enabled?, :issues_enabled?, to: :project_feature,
                                                        allow_nil: true
 
+  delegate :base_dir, :disk_path, :ensure_storage_path_exist, :rename_repo, to: :storage
+
   default_value_for :archived, false
   default_value_for :visibility_level, gitlab_config_features.visibility_level
   default_value_for :container_registry_enabled, gitlab_config_features.container_registry
@@ -59,7 +61,7 @@ class Project < ActiveRecord::Base
   after_validation :check_pending_delete
 
   # Storage specific hooks
-  after_initialize :load_storage
+  after_initialize :use_hashed_storage
   after_create :ensure_storage_path_exist
   after_save :ensure_storage_path_exist, if: :namespace_id_changed?
 
@@ -1424,16 +1426,20 @@ class Project < ActiveRecord::Base
 
   private
 
-  def load_storage
-    return unless has_attribute?(:storage_version)
+  def storage
+    @storage ||=
+      if !has_attribute?(:storage_version) # during migration
+        Storage::LegacyProject.new(self)
+      elsif self.storage_version && self.storage_version >= 1
+        Storage::HashedProject.new(self)
+      else
+        Storage::LegacyProject.new(self)
+      end
+  end
 
-    if self.storage_version && self.storage_version >= 1
-      self.extend Storage::HashedProject
-    elsif !self.persisted? && current_application_settings.hashed_storage_enabled
+  def use_hashed_storage
+    if !self.persisted? && current_application_settings.hashed_storage_enabled
       self.storage_version = LATEST_STORAGE_VERSION
-      self.extend Storage::HashedProject
-    else
-      self.extend Storage::LegacyProject
     end
   end
 
diff --git a/app/models/concerns/storage/hashed_project.rb b/app/models/storage/hashed_project.rb
index 292a73903b5..364794a4eef 100644
--- a/app/models/concerns/storage/hashed_project.rb
+++ b/app/models/storage/hashed_project.rb
@@ -1,6 +1,11 @@
 module Storage
-  module HashedProject
-    extend ActiveSupport::Concern
+  class HashedProject
+    attr_accessor :project
+    delegate :namespace, :gitlab_shell, :repository_storage_path, to: :project
+
+    def initialize(project)
+      @project = project
+    end
 
     # Base directory
     #
@@ -22,13 +27,13 @@ module Storage
 
     def rename_repo
       # TODO: We cannot wipe most of this method until we provide migration path for Container Registries
-      path_was = previous_changes['path'].first
+      path_was = project.previous_changes['path'].first
       old_path_with_namespace = File.join(namespace.full_path, path_was)
-      new_path_with_namespace = File.join(namespace.full_path, path)
+      new_path_with_namespace = File.join(namespace.full_path, project.path)
 
       Rails.logger.error "Attempting to rename #{old_path_with_namespace} -> #{new_path_with_namespace}"
 
-      if has_container_registry_tags?
+      if project.has_container_registry_tags?
         Rails.logger.error "Project #{old_path_with_namespace} cannot be renamed because container registry tags are present!"
 
         # we currently doesn't support renaming repository if it contains images in container registry
@@ -37,14 +42,14 @@ module Storage
 
       begin
         # TODO: we can avoid cache expiration if cache is based on UUID or just project_id
-        expire_caches_before_rename(old_path_with_namespace)
-        expires_full_path_cache
+        project.expire_caches_before_rename(old_path_with_namespace)
+        project.expires_full_path_cache
 
-        send_move_instructions(old_path_with_namespace)
+        project.send_move_instructions(old_path_with_namespace)
 
         @old_path_with_namespace = old_path_with_namespace
 
-        SystemHooksService.new.execute_hooks_for(self, :rename)
+        SystemHooksService.new.execute_hooks_for(project, :rename)
 
         @repository = nil
       rescue => e
@@ -57,8 +62,8 @@ module Storage
       Gitlab::AppLogger.info "Project was renamed: #{old_path_with_namespace} -> #{new_path_with_namespace}"
 
       # TODO: When we move Uploads and Pages to use UUID we can disable this transfers as well
-      Gitlab::UploadsTransfer.new.rename_project(path_was, path, namespace.full_path)
-      Gitlab::PagesTransfer.new.rename_project(path_was, path, namespace.full_path)
+      Gitlab::UploadsTransfer.new.rename_project(path_was, project.path, namespace.full_path)
+      Gitlab::PagesTransfer.new.rename_project(path_was, project.path, namespace.full_path)
     end
 
     private
@@ -66,7 +71,7 @@ module Storage
     # Generates the hash for the project path and name on disk
     # If you need to refer to the repository on disk, use the `#disk_path`
     def disk_hash
-      @disk_hash ||= Digest::SHA2.hexdigest(self.id.to_s) if self.id
+      @disk_hash ||= Digest::SHA2.hexdigest(project.id.to_s) if project.id
     end
   end
 end
diff --git a/app/models/concerns/storage/legacy_project.rb b/app/models/storage/legacy_project.rb
index 839bbcc76ea..57c340075ba 100644
--- a/app/models/concerns/storage/legacy_project.rb
+++ b/app/models/storage/legacy_project.rb
@@ -1,6 +1,11 @@
 module Storage
-  module LegacyProject
-    extend ActiveSupport::Concern
+  class LegacyProject
+    attr_accessor :project
+    delegate :namespace, :gitlab_shell, :repository_storage_path, to: :project
+
+    def initialize(project)
+      @project = project
+    end
 
     # Base directory
     #
@@ -13,28 +18,30 @@ module Storage
     #
     # @return [String] combination of base_dir and the repository own name without `.git` or `.wiki.git` extensions
     def disk_path
-      full_path
+      project.full_path
     end
 
     def ensure_storage_path_exist
+      return unless namespace
+
       gitlab_shell.add_namespace(repository_storage_path, base_dir)
     end
 
     def rename_repo
-      path_was = previous_changes['path'].first
-      old_path_with_namespace = File.join(namespace.full_path, path_was)
-      new_path_with_namespace = File.join(namespace.full_path, path)
+      path_was = project.previous_changes['path'].first
+      old_path_with_namespace = File.join(base_dir, path_was)
+      new_path_with_namespace = File.join(base_dir, project.path)
 
       Rails.logger.error "Attempting to rename #{old_path_with_namespace} -> #{new_path_with_namespace}"
 
-      if has_container_registry_tags?
+      if project.has_container_registry_tags?
         Rails.logger.error "Project #{old_path_with_namespace} cannot be renamed because container registry tags are present!"
 
         # we currently doesn't support renaming repository if it contains images in container registry
         raise StandardError.new('Project cannot be renamed, because images are present in its container registry')
       end
 
-      expire_caches_before_rename(old_path_with_namespace)
+      project.expire_caches_before_rename(old_path_with_namespace)
 
       if gitlab_shell.mv_repository(repository_storage_path, old_path_with_namespace, new_path_with_namespace)
         # If repository moved successfully we need to send update instructions to users.
@@ -42,12 +49,12 @@ module Storage
         # So we basically we mute exceptions in next actions
         begin
           gitlab_shell.mv_repository(repository_storage_path, "#{old_path_with_namespace}.wiki", "#{new_path_with_namespace}.wiki")
-          send_move_instructions(old_path_with_namespace)
-          expires_full_path_cache
+          project.send_move_instructions(old_path_with_namespace)
+          project.expires_full_path_cache
 
           @old_path_with_namespace = old_path_with_namespace
 
-          SystemHooksService.new.execute_hooks_for(self, :rename)
+          SystemHooksService.new.execute_hooks_for(project, :rename)
 
           @repository = nil
         rescue => e
@@ -66,8 +73,8 @@ module Storage
 
       Gitlab::AppLogger.info "Project was renamed: #{old_path_with_namespace} -> #{new_path_with_namespace}"
 
-      Gitlab::UploadsTransfer.new.rename_project(path_was, path, namespace.full_path)
-      Gitlab::PagesTransfer.new.rename_project(path_was, path, namespace.full_path)
+      Gitlab::UploadsTransfer.new.rename_project(path_was, project.path, base_dir)
+      Gitlab::PagesTransfer.new.rename_project(path_was, project.path, base_dir)
     end
   end
 end
diff --git a/db/migrate/20170802013652_add_storage_fields_to_project.rb b/db/migrate/20170802013652_add_storage_fields_to_project.rb
index a0815da0fcd..e99ae53ef11 100644
--- a/db/migrate/20170802013652_add_storage_fields_to_project.rb
+++ b/db/migrate/20170802013652_add_storage_fields_to_project.rb
@@ -8,7 +8,6 @@ class AddStorageFieldsToProject < ActiveRecord::Migration
   disable_ddl_transaction!
 
   def up
-    # rubocop:disable Migration/AddColumnWithDefaultToLargeTable
     add_column :projects, :storage_version, :integer, limit: 2
     add_concurrent_index :projects, :storage_version
   end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index e6fde833c6b..8ff25a6cf6b 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2395,11 +2395,12 @@ describe Project do
   end
 
   context 'hashed storage' do
-    let(:project) { create(:project, :repository, :hashed) }
+    let(:project) { create(:project, :repository) }
     let(:gitlab_shell) { Gitlab::Shell.new }
     let(:hash) { '6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b' }
 
     before do
+      stub_application_setting(hashed_storage_enabled: true)
       allow(Digest::SHA2).to receive(:hexdigest) { hash }
     end