diff options
author | Douwe Maan <douwe@gitlab.com> | 2017-11-14 12:48:19 +0300 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2017-11-14 12:48:19 +0300 |
commit | c9515ca541ebaa3ce0a3208ab08eb59c580cbed6 (patch) | |
tree | 940a4de00c36470ae7910fd7e15dd0338c2d6472 | |
parent | 58bd04ab75d99fa9be0d9568c442704d400e8b1d (diff) | |
parent | ebd51744729cb1b68754f8ba4d7f9adcec28d58d (diff) |
Merge branch 'jej/fix-lfs-integrity-with-forks' into 'master'
Handle forks in Gitlab::Checks::LfsIntegrity
Closes #39902
See merge request gitlab-org/gitlab-ce!15279
-rw-r--r-- | app/controllers/concerns/lfs_request.rb | 10 | ||||
-rw-r--r-- | app/models/lfs_object.rb | 10 | ||||
-rw-r--r-- | app/models/project.rb | 12 | ||||
-rw-r--r-- | lib/gitlab/checks/lfs_integrity.rb | 5 | ||||
-rw-r--r-- | spec/lib/gitlab/checks/change_access_spec.rb | 43 | ||||
-rw-r--r-- | spec/lib/gitlab/checks/lfs_integrity_spec.rb | 74 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 18 |
7 files changed, 116 insertions, 56 deletions
diff --git a/app/controllers/concerns/lfs_request.rb b/app/controllers/concerns/lfs_request.rb index 6cca9f95618..4311f9d4db9 100644 --- a/app/controllers/concerns/lfs_request.rb +++ b/app/controllers/concerns/lfs_request.rb @@ -92,15 +92,7 @@ module LfsRequest end def storage_project - @storage_project ||= begin - result = project - - # TODO: Make this go to the fork_network root immeadiatly - # dependant on the discussion in: https://gitlab.com/gitlab-org/gitlab-ce/issues/39769 - result = result.fork_source while result.forked? - - result - end + @storage_project ||= project.lfs_storage_project end def objects diff --git a/app/models/lfs_object.rb b/app/models/lfs_object.rb index b7cf96abe83..fc586fa216e 100644 --- a/app/models/lfs_object.rb +++ b/app/models/lfs_object.rb @@ -6,16 +6,8 @@ class LfsObject < ActiveRecord::Base mount_uploader :file, LfsObjectUploader - def storage_project(project) - if project && project.forked? - storage_project(project.forked_from_project) - else - project - end - end - def project_allowed_access?(project) - projects.exists?(storage_project(project).id) + projects.exists?(project.lfs_storage_project.id) end def self.destroy_unreferenced diff --git a/app/models/project.rb b/app/models/project.rb index bae16b6b2af..853f6bc504a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1047,6 +1047,18 @@ class Project < ActiveRecord::Base forked_from_project || fork_network&.root_project end + def lfs_storage_project + @lfs_storage_project ||= begin + result = self + + # TODO: Make this go to the fork_network root immeadiatly + # dependant on the discussion in: https://gitlab.com/gitlab-org/gitlab-ce/issues/39769 + result = result.fork_source while result&.forked? + + result || self + end + end + def personal? !group end diff --git a/lib/gitlab/checks/lfs_integrity.rb b/lib/gitlab/checks/lfs_integrity.rb index 27a95764dc1..f7276a380dc 100644 --- a/lib/gitlab/checks/lfs_integrity.rb +++ b/lib/gitlab/checks/lfs_integrity.rb @@ -15,7 +15,10 @@ module Gitlab return false unless new_lfs_pointers.present? - existing_count = @project.lfs_objects.where(oid: new_lfs_pointers.map(&:lfs_oid)).count + existing_count = @project.lfs_storage_project + .lfs_objects + .where(oid: new_lfs_pointers.map(&:lfs_oid)) + .count existing_count != new_lfs_pointers.count end diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb index 74a24a4424b..c2bca816aae 100644 --- a/spec/lib/gitlab/checks/change_access_spec.rb +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -165,47 +165,16 @@ describe Gitlab::Checks::ChangeAccess do end context 'LFS integrity check' do - let(:blob_object) { project.repository.blob_at_branch('lfs', 'files/lfs/lfs_object.iso') } + it 'fails if any LFS blobs are missing' do + allow_any_instance_of(Gitlab::Checks::LfsIntegrity).to receive(:objects_missing?).and_return(true) - before do - allow_any_instance_of(Gitlab::Git::RevList).to receive(:new_objects) do |&lazy_block| - lazy_block.call([blob_object.id]) - end - end - - context 'with LFS not enabled' do - it 'skips integrity check' do - expect_any_instance_of(Gitlab::Git::RevList).not_to receive(:new_objects) - - subject.exec - end + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /LFS objects are missing/) end - context 'with LFS enabled' do - before do - allow(project).to receive(:lfs_enabled?).and_return(true) - end - - context 'deletion' do - let(:changes) { { oldrev: oldrev, ref: ref } } - - it 'skips integrity check' do - expect_any_instance_of(Gitlab::Git::RevList).not_to receive(:new_objects) + it 'succeeds if LFS objects have already been uploaded' do + allow_any_instance_of(Gitlab::Checks::LfsIntegrity).to receive(:objects_missing?).and_return(false) - subject.exec - end - end - - it 'fails if any LFS blobs are missing' do - expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /LFS objects are missing/) - end - - it 'succeeds if LFS objects have already been uploaded' do - lfs_object = create(:lfs_object, oid: blob_object.lfs_oid) - create(:lfs_objects_project, project: project, lfs_object: lfs_object) - - expect { subject.exec }.not_to raise_error - end + expect { subject.exec }.not_to raise_error end end end diff --git a/spec/lib/gitlab/checks/lfs_integrity_spec.rb b/spec/lib/gitlab/checks/lfs_integrity_spec.rb new file mode 100644 index 00000000000..17756621221 --- /dev/null +++ b/spec/lib/gitlab/checks/lfs_integrity_spec.rb @@ -0,0 +1,74 @@ +require 'spec_helper' + +describe Gitlab::Checks::LfsIntegrity do + include ProjectForksHelper + let(:project) { create(:project, :repository) } + let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' } + + subject { described_class.new(project, newrev) } + + describe '#objects_missing?' do + let(:blob_object) { project.repository.blob_at_branch('lfs', 'files/lfs/lfs_object.iso') } + + before do + allow_any_instance_of(Gitlab::Git::RevList).to receive(:new_objects) do |&lazy_block| + lazy_block.call([blob_object.id]) + end + end + + context 'with LFS not enabled' do + it 'skips integrity check' do + expect_any_instance_of(Gitlab::Git::RevList).not_to receive(:new_objects) + + subject.objects_missing? + end + end + + context 'with LFS enabled' do + before do + allow(project).to receive(:lfs_enabled?).and_return(true) + end + + context 'deletion' do + let(:newrev) { nil } + + it 'skips integrity check' do + expect_any_instance_of(Gitlab::Git::RevList).not_to receive(:new_objects) + + expect(subject.objects_missing?).to be_falsey + end + end + + it 'is true if any LFS blobs are missing' do + expect(subject.objects_missing?).to be_truthy + end + + it 'is false if LFS objects have already been uploaded' do + lfs_object = create(:lfs_object, oid: blob_object.lfs_oid) + create(:lfs_objects_project, project: project, lfs_object: lfs_object) + + expect(subject.objects_missing?).to be_falsey + end + end + + context 'for forked project' do + let(:parent_project) { create(:project, :repository) } + let(:project) { fork_project(parent_project, nil, repository: true) } + + before do + allow(project).to receive(:lfs_enabled?).and_return(true) + end + + it 'is true parent project is missing LFS objects' do + expect(subject.objects_missing?).to be_truthy + end + + it 'is false parent project already conatins LFS objects for the fork' do + lfs_object = create(:lfs_object, oid: blob_object.lfs_oid) + create(:lfs_objects_project, project: parent_project, lfs_object: lfs_object) + + expect(subject.objects_missing?).to be_falsey + end + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 4db417c8793..f7f19d464d1 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1938,6 +1938,24 @@ describe Project do expect(second_fork.fork_source).to eq(project) end end + + describe '#lfs_storage_project' do + it 'returns self for non-forks' do + expect(project.lfs_storage_project).to eq project + end + + it 'returns the fork network root for forks' do + second_fork = fork_project(forked_project) + + expect(second_fork.lfs_storage_project).to eq project + end + + it 'returns self when fork_source is nil' do + expect(forked_project).to receive(:fork_source).and_return(nil) + + expect(forked_project.lfs_storage_project).to eq forked_project + end + end end describe '#pushes_since_gc' do |