summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorLuke Duncalfe <lduncalfe@eml.cc>2019-06-06 18:51:50 +1200
committerLuke Duncalfe <lduncalfe@eml.cc>2019-06-17 11:23:06 +1200
commitf7163afb8a036468ccc3f657ac09f3c09b318dab (patch)
treef935d63b744625597467489a7b11ccbb07864c8a
parent82822945d446acf44176a36da38675e98ca17616 (diff)
downloadgitlab-ce-f7163afb8a036468ccc3f657ac09f3c09b318dab.tar.gz
CE backport for changes in EE MR 138949490-record-repository_type-on-lfs_objects_projects-ce
This backports to CE changes that allow the recording of the repository_type in the table lfs_objects_projects. This is in order to allow future pruning of unreferenced LFS objects, as we will need to know which repository to look in for the LFS pointer file. The EE MR that contains the original code and a full explanation of the changes is https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/13894 EE Issue https://gitlab.com/gitlab-org/gitlab-ee/issues/9490 Note that there was a lot of CE code changed in the EE MR because we want to allow the wiki repository to also use LFS. See https://gitlab.com/gitlab-org/gitlab-ce/issues/43721. As the wiki is an unlicensed feature, a full backport is required to enable this.
-rw-r--r--app/models/lfs_object.rb2
-rw-r--r--app/models/lfs_objects_project.rb8
-rw-r--r--app/models/project.rb2
-rw-r--r--app/services/files/create_service.rb2
-rw-r--r--app/services/files/multi_service.rb2
-rw-r--r--app/services/lfs/file_transformer.rb16
-rw-r--r--db/migrate/20190602014139_add_repository_type_to_lfs_objects_project.rb11
-rw-r--r--db/migrate/20190606034427_add_lfs_object_id_index_to_lfs_objects_projects.rb17
-rw-r--r--db/schema.rb2
-rw-r--r--spec/factories/lfs_objects_projects.rb1
-rw-r--r--spec/models/lfs_object_spec.rb14
-rw-r--r--spec/models/lfs_objects_project_spec.rb4
-rw-r--r--spec/models/project_spec.rb14
-rw-r--r--spec/services/lfs/file_transformer_spec.rb60
14 files changed, 140 insertions, 15 deletions
diff --git a/app/models/lfs_object.rb b/app/models/lfs_object.rb
index 5245dbc8d15..79a376ff0fd 100644
--- a/app/models/lfs_object.rb
+++ b/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) }
diff --git a/app/models/lfs_objects_project.rb b/app/models/lfs_objects_project.rb
index f9afb18c1d7..e45c56b6394 100644
--- a/app/models/lfs_objects_project.rb
+++ b/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
diff --git a/app/models/project.rb b/app/models/project.rb
index 9d17d68eee2..d658214c4ef 100644
--- a/app/models/project.rb
+++ b/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
diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb
index fd5442a6c28..f2cd51ef4d0 100644
--- a/app/services/files/create_service.rb
+++ b/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)
diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb
index c1bc26c330a..d8c4e5bc5e8 100644
--- a/app/services/files/multi_service.rb
+++ b/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)
diff --git a/app/services/lfs/file_transformer.rb b/app/services/lfs/file_transformer.rb
index 5239fe1b6e3..d1746399908 100644
--- a/app/services/lfs/file_transformer.rb
+++ b/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)
diff --git a/db/migrate/20190602014139_add_repository_type_to_lfs_objects_project.rb b/db/migrate/20190602014139_add_repository_type_to_lfs_objects_project.rb
new file mode 100644
index 00000000000..6ff64ba12a9
--- /dev/null
+++ b/db/migrate/20190602014139_add_repository_type_to_lfs_objects_project.rb
@@ -0,0 +1,11 @@
+# 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
diff --git a/db/migrate/20190606034427_add_lfs_object_id_index_to_lfs_objects_projects.rb b/db/migrate/20190606034427_add_lfs_object_id_index_to_lfs_objects_projects.rb
new file mode 100644
index 00000000000..fc09fcfae0f
--- /dev/null
+++ b/db/migrate/20190606034427_add_lfs_object_id_index_to_lfs_objects_projects.rb
@@ -0,0 +1,17 @@
+# 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
diff --git a/db/schema.rb b/db/schema.rb
index 4ed7c0cb248..01a00103dd5 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1190,6 +1190,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
diff --git a/spec/factories/lfs_objects_projects.rb b/spec/factories/lfs_objects_projects.rb
index c225387a5de..4804d0bb884 100644
--- a/spec/factories/lfs_objects_projects.rb
+++ b/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
diff --git a/spec/models/lfs_object_spec.rb b/spec/models/lfs_object_spec.rb
index 3d4d4b7d795..85bfc3f1387 100644
--- a/spec/models/lfs_object_spec.rb
+++ b/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
diff --git a/spec/models/lfs_objects_project_spec.rb b/spec/models/lfs_objects_project_spec.rb
index 3e86ee38566..e320f873989 100644
--- a/spec/models/lfs_objects_project_spec.rb
+++ b/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
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index aad08b9d4aa..a4940670377 100644
--- a/spec/models/project_spec.rb
+++ b/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
diff --git a/spec/services/lfs/file_transformer_spec.rb b/spec/services/lfs/file_transformer_spec.rb
index 888eea6e91e..9973d64930b 100644
--- a/spec/services/lfs/file_transformer_spec.rb
+++ b/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