diff options
-rw-r--r-- | app/models/lfs_object.rb | 2 | ||||
-rw-r--r-- | app/models/lfs_objects_project.rb | 8 | ||||
-rw-r--r-- | app/models/project.rb | 2 | ||||
-rw-r--r-- | app/services/files/create_service.rb | 2 | ||||
-rw-r--r-- | app/services/files/multi_service.rb | 2 | ||||
-rw-r--r-- | app/services/lfs/file_transformer.rb | 16 | ||||
-rw-r--r-- | db/migrate/20190602014139_add_repository_type_to_lfs_objects_project.rb | 11 | ||||
-rw-r--r-- | db/migrate/20190606034427_add_lfs_object_id_index_to_lfs_objects_projects.rb | 17 | ||||
-rw-r--r-- | db/schema.rb | 2 | ||||
-rw-r--r-- | spec/factories/lfs_objects_projects.rb | 1 | ||||
-rw-r--r-- | spec/models/lfs_object_spec.rb | 14 | ||||
-rw-r--r-- | spec/models/lfs_objects_project_spec.rb | 4 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 14 | ||||
-rw-r--r-- | spec/services/lfs/file_transformer_spec.rb | 60 |
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 |