Merge branch '9490-record-repository_type-on-lfs_objects_projects' into 'master'

Save repository_type to LfsObjectsProject

See merge request gitlab-org/gitlab-ee!13894

app/models/lfs_object.rb

@@ -5,7 +5,7 @@ class LfsObject < ApplicationRecord
   include ObjectStorage::BackgroundMove
 
   has_many :lfs_objects_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-  has_many :projects, through: :lfs_objects_projects
+  has_many :projects, -> { distinct }, through: :lfs_objects_projects
 
   scope :with_files_stored_locally, -> { where(file_store: LfsObjectUploader::Store::LOCAL) }
 

app/models/lfs_objects_project.rb

@@ -5,11 +5,17 @@ class LfsObjectsProject < ApplicationRecord
   belongs_to :lfs_object
 
   validates :lfs_object_id, presence: true
-  validates :lfs_object_id, uniqueness: { scope: [:project_id], message: "already exists in project" }
+  validates :lfs_object_id, uniqueness: { scope: [:project_id, :repository_type], message: "already exists in repository" }
   validates :project_id, presence: true
 
   after_commit :update_project_statistics, on: [:create, :destroy]
 
+  enum repository_type: {
+    project: 0,
+    wiki: 1,
+    design: 2 ## EE-specific
+  }
+
   private
 
   def update_project_statistics

app/models/project.rb

@@ -222,7 +222,7 @@ class Project < ApplicationRecord
   has_many :starrers, through: :users_star_projects, source: :user
   has_many :releases
   has_many :lfs_objects_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-  has_many :lfs_objects, through: :lfs_objects_projects
+  has_many :lfs_objects, -> { distinct }, through: :lfs_objects_projects
   has_many :lfs_file_locks
   has_many :project_group_links
   has_many :invited_groups, through: :project_group_links, source: :group

app/services/files/create_service.rb

@@ -3,7 +3,7 @@
 module Files
   class CreateService < Files::BaseService
     def create_commit!
-      transformer = Lfs::FileTransformer.new(project, @branch_name)
+      transformer = Lfs::FileTransformer.new(project, repository, @branch_name)
 
       result = transformer.new_file(@file_path, @file_content)
 

app/services/files/multi_service.rb

@@ -5,7 +5,7 @@ module Files
     UPDATE_FILE_ACTIONS = %w(update move delete chmod).freeze
 
     def create_commit!
-      transformer = Lfs::FileTransformer.new(project, @branch_name)
+      transformer = Lfs::FileTransformer.new(project, repository, @branch_name)
 
       actions = actions_after_lfs_transformation(transformer, params[:actions])
       actions = transform_move_actions(actions)

app/services/lfs/file_transformer.rb

@@ -8,17 +8,17 @@ module Lfs
   #        pointer returned. If the file isn't in LFS the untransformed content
   #        is returned to save in the commit.
   #
-  # transformer = Lfs::FileTransformer.new(project, @branch_name)
+  # transformer = Lfs::FileTransformer.new(project, repository, @branch_name)
   # content_or_lfs_pointer = transformer.new_file(file_path, content).content
   # create_transformed_commit(content_or_lfs_pointer)
   #
   class FileTransformer
-    attr_reader :project, :branch_name
+    attr_reader :project, :repository, :repository_type, :branch_name
 
-    delegate :repository, to: :project
-
-    def initialize(project, branch_name)
+    def initialize(project, repository, branch_name)
       @project = project
+      @repository = repository
+      @repository_type = repository.repo_type.name
       @branch_name = branch_name
     end
 
@@ -64,7 +64,11 @@ module Lfs
     # rubocop: enable CodeReuse/ActiveRecord
 
     def link_lfs_object!(lfs_object)
-      project.lfs_objects << lfs_object
+      LfsObjectsProject.safe_find_or_create_by!(
+        project: project,
+        lfs_object: lfs_object,
+        repository_type: repository_type
+      )
     end
 
     def parse_file_content(file_content, encoding: nil)

db/migrate/20190602014139_add_repository_type_to_lfs_objects_project.rb

+# frozen_string_literal: true
+
+class AddRepositoryTypeToLfsObjectsProject < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :lfs_objects_projects, :repository_type, :integer, limit: 2, null: true
+  end
+end

db/migrate/20190606034427_add_lfs_object_id_index_to_lfs_objects_projects.rb

+# frozen_string_literal: true
+
+class AddLfsObjectIdIndexToLfsObjectsProjects < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :lfs_objects_projects, :lfs_object_id
+  end
+
+  def down
+    remove_concurrent_index :lfs_objects_projects, :lfs_object_id
+  end
+end

db/schema.rb

@@ -1769,6 +1769,8 @@ ActiveRecord::Schema.define(version: 20190611161641) do
     t.integer "project_id", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.integer "repository_type", limit: 2
+    t.index ["lfs_object_id"], name: "index_lfs_objects_projects_on_lfs_object_id", using: :btree
     t.index ["project_id"], name: "index_lfs_objects_projects_on_project_id", using: :btree
   end
 

ee/app/services/design_management/save_designs_service.rb

@@ -59,7 +59,7 @@ module DesignManagement
     end
 
     def repository
-      collection.repository
+      project.design_repository
     end
 
     def project
@@ -101,7 +101,7 @@ module DesignManagement
     def file_content(file, full_path)
       return file.to_io if Feature.disabled?(:store_designs_in_lfs)
 
-      transformer = Lfs::FileTransformer.new(project, target_branch)
+      transformer = Lfs::FileTransformer.new(project, repository, target_branch)
       transformer.new_file(full_path, file.to_io).content
     end
 

ee/spec/services/design_management/save_designs_service_spec.rb

@@ -148,6 +148,14 @@ describe DesignManagement::SaveDesignsService do
           it 'saves the design to LFS' do
             expect { service.execute }.to change { LfsObject.count }.by(1)
           end
+
+          it 'saves the repository_type of the LfsObjectsProject as design' do
+            expect do
+              service.execute
+            end.to change { project.lfs_objects_projects.count }.from(0).to(1)
+
+            expect(project.lfs_objects_projects.first.repository_type).to eq('design')
+          end
         end
 
         context 'and the `store_designs_in_lfs` feature is not enabled' do

spec/factories/lfs_objects_projects.rb

@@ -2,5 +2,6 @@ FactoryBot.define do
   factory :lfs_objects_project do
     lfs_object
     project
+    repository_type :project
   end
 end

spec/models/lfs_object_spec.rb

@@ -3,6 +3,20 @@
 require 'spec_helper'
 
 describe LfsObject do
+  it 'has a distinct has_many :projects relation through lfs_objects_projects' do
+    lfs_object = create(:lfs_object)
+    project = create(:project)
+    [:project, :design].each do |repository_type|
+      create(:lfs_objects_project, project: project,
+                                   lfs_object: lfs_object,
+                                   repository_type: repository_type)
+    end
+
+    expect(lfs_object.lfs_objects_projects.size).to eq(2)
+    expect(lfs_object.projects.size).to eq(1)
+    expect(lfs_object.projects.to_a).to eql([project])
+  end
+
   describe '#local_store?' do
     it 'returns true when file_store is equal to LfsObjectUploader::Store::LOCAL' do
       subject.file_store = LfsObjectUploader::Store::LOCAL

spec/models/lfs_objects_project_spec.rb

@@ -20,8 +20,8 @@ describe LfsObjectsProject do
 
     it 'validates object id' do
       is_expected.to validate_uniqueness_of(:lfs_object_id)
-        .scoped_to(:project_id)
-        .with_message("already exists in project")
+        .scoped_to(:project_id, :repository_type)
+        .with_message("already exists in repository")
     end
   end
 

spec/models/project_spec.rb

@@ -103,6 +103,20 @@ describe Project do
       expect(described_class.reflect_on_association(:merge_requests).has_inverse?).to eq(:target_project)
     end
 
+    it 'has a distinct has_many :lfs_objects relation through lfs_objects_projects' do
+      project = create(:project)
+      lfs_object = create(:lfs_object)
+      [:project, :design].each do |repository_type|
+        create(:lfs_objects_project, project: project,
+                                     lfs_object: lfs_object,
+                                     repository_type: repository_type)
+      end
+
+      expect(project.lfs_objects_projects.size).to eq(2)
+      expect(project.lfs_objects.size).to eq(1)
+      expect(project.lfs_objects.to_a).to eql([lfs_object])
+    end
+
     context 'after initialized' do
       it "has a project_feature" do
         expect(described_class.new.project_feature).to be_present

spec/services/lfs/file_transformer_spec.rb

@@ -3,13 +3,13 @@
 require "spec_helper"
 
 describe Lfs::FileTransformer do
-  let(:project) { create(:project, :repository) }
+  let(:project) { create(:project, :repository, :wiki_repo) }
   let(:repository) { project.repository }
   let(:file_content) { 'Test file content' }
   let(:branch_name) { 'lfs' }
   let(:file_path) { 'test_file.lfs' }
 
-  subject { described_class.new(project, branch_name) }
+  subject { described_class.new(project, repository, branch_name) }
 
   describe '#new_file' do
     context 'with lfs disabled' do
@@ -100,6 +100,12 @@ describe Lfs::FileTransformer do
         end.to change { project.lfs_objects.count }.by(1)
       end
 
+      it 'saves the repository_type to LfsObjectsProject' do
+        subject.new_file(file_path, file_content)
+
+        expect(project.lfs_objects_projects.first.repository_type).to eq('project')
+      end
+
       context 'when LfsObject already exists' do
         let(:lfs_pointer) { Gitlab::Git::LfsPointerFile.new(file_content) }
 
@@ -113,6 +119,56 @@ describe Lfs::FileTransformer do
           end.to change { project.lfs_objects.count }.by(1)
         end
       end
+
+      context 'when the LfsObject is already linked to project' do
+        before do
+          subject.new_file(file_path, file_content)
+        end
+
+        shared_examples 'a new LfsObject is not created' do
+          it do
+            expect do
+              second_service.new_file(file_path, file_content)
+            end.not_to change { project.lfs_objects.count }
+          end
+        end
+
+        context 'and the service is called again with the same repository type' do
+          let(:second_service) { described_class.new(project, repository, branch_name) }
+
+          include_examples 'a new LfsObject is not created'
+
+          it 'does not create a new LfsObjectsProject record' do
+            expect do
+              second_service.new_file(file_path, file_content)
+            end.not_to change { project.lfs_objects_projects.count }
+          end
+        end
+
+        context 'and the service is called again with a different repository type' do
+          let(:second_service) { described_class.new(project, project.wiki.repository, branch_name) }
+
+          before do
+            expect(second_service).to receive(:lfs_file?).and_return(true)
+          end
+
+          include_examples 'a new LfsObject is not created'
+
+          it 'creates a new LfsObjectsProject record' do
+            expect do
+              second_service.new_file(file_path, file_content)
+            end.to change { project.lfs_objects_projects.count }.by(1)
+          end
+
+          it 'sets the correct repository_type on the new LfsObjectsProject record' do
+            second_service.new_file(file_path, file_content)
+
+            repository_types = project.lfs_objects_projects.order(:id).pluck(:repository_type)
+
+            expect(repository_types).to eq(%w(project wiki))
+          end
+        end
+      end
     end
   end
 end