Save repository_type to LfsObjectsProject

This lets us know which repository the LFS pointer file is in, in order
to prune unreferenced LFS objects in the future.

Saving LFS objects is intended to now be supported for repositories
other than the regular 'project' repository; the extra repositories that
will also support LFS objects are 'wiki' and 'design'.

Because an LFS object's oid is determined by the data in the file, it
will be possible for a project to have the same LfsObject in each of the
three different repositories.

Which repository the LFS object lives in will now be
recorded in the has-and-belongs-to-many relation model
LfsObjectsProject. In the situation where the same blob is saved in, for
e.g., the 'project' and 'design' repositories, there will be two
LfsObjectsProject records created for each of the two repository_types.
This means the has_many relationships between lfs_objects and projects
now need a `distinct` scope to limit the records returned to
non-duplicates.

Lfs::FileTransformer will now create LfsObjectsProjects with
a repository_type saved. The DesignManagement::SaveDesignsService is the
first part of the app to pass the repository_type to this class
to have it saved.

There is a technical debt task to have all parts of the app that create
LFS objects save repository_type to LfsObjectsProject in future.

https://gitlab.com/gitlab-org/gitlab-ee/issues/9490 particular this
comment thread
https://gitlab.com/gitlab-org/gitlab-ee/issues/9490#note_155912249

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

@@ -223,7 +223,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/schema.rb

@@ -1769,6 +1769,7 @@ 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 ["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