Commit a28f25b5 authored by Kamil Trzciński's avatar Kamil Trzciński

Fix direct_upload when records with null file_store are used

Old records have a null value of file_store column.
This causes the problems with current direct_upload implementation,
as this makes it to choose Store::REMOTE instead of Store::LOCAL.

This change moves the store save when change saving the object.
parent 2c7c8db6
...@@ -7,14 +7,15 @@ module Ci ...@@ -7,14 +7,15 @@ module Ci
belongs_to :project belongs_to :project
belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id
before_save :update_file_store mount_uploader :file, JobArtifactUploader
before_save :set_size, if: :file_changed? before_save :set_size, if: :file_changed?
after_save :update_project_statistics_after_save, if: :size_changed? after_save :update_project_statistics_after_save, if: :size_changed?
after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed? after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed?
scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) } after_save :update_file_store
mount_uploader :file, JobArtifactUploader scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) }
delegate :exists?, :open, to: :file delegate :exists?, :open, to: :file
...@@ -25,7 +26,9 @@ module Ci ...@@ -25,7 +26,9 @@ module Ci
} }
def update_file_store def update_file_store
self.file_store = file.object_store # The file.object_store is set during `uploader.store!`
# which happens after object is inserted/updated
self.update_column(:file_store, file.object_store)
end end
def self.artifacts_size_for(project) def self.artifacts_size_for(project)
......
...@@ -11,10 +11,12 @@ class LfsObject < ActiveRecord::Base ...@@ -11,10 +11,12 @@ class LfsObject < ActiveRecord::Base
mount_uploader :file, LfsObjectUploader mount_uploader :file, LfsObjectUploader
before_save :update_file_store after_save :update_file_store
def update_file_store def update_file_store
self.file_store = file.object_store # The file.object_store is set during `uploader.store!`
# which happens after object is inserted/updated
self.update_column(:file_store, file.object_store)
end end
def project_allowed_access?(project) def project_allowed_access?(project)
......
...@@ -183,14 +183,6 @@ module ObjectStorage ...@@ -183,14 +183,6 @@ module ObjectStorage
StoreURL: connection.put_object_url(remote_store_path, upload_path, expire_at, options) StoreURL: connection.put_object_url(remote_store_path, upload_path, expire_at, options)
} }
end end
def default_object_store
if self.object_store_enabled? && self.direct_upload_enabled?
Store::REMOTE
else
Store::LOCAL
end
end
end end
# allow to configure and overwrite the filename # allow to configure and overwrite the filename
...@@ -211,12 +203,13 @@ module ObjectStorage ...@@ -211,12 +203,13 @@ module ObjectStorage
end end
def object_store def object_store
@object_store ||= model.try(store_serialization_column) || self.class.default_object_store # We use Store::LOCAL as null value indicates the local storage
@object_store ||= model.try(store_serialization_column) || Store::LOCAL
end end
# rubocop:disable Gitlab/ModuleWithInstanceVariables # rubocop:disable Gitlab/ModuleWithInstanceVariables
def object_store=(value) def object_store=(value)
@object_store = value || self.class.default_object_store @object_store = value || Store::LOCAL
@storage = storage_for(object_store) @storage = storage_for(object_store)
end end
# rubocop:enable Gitlab/ModuleWithInstanceVariables # rubocop:enable Gitlab/ModuleWithInstanceVariables
...@@ -302,6 +295,15 @@ module ObjectStorage ...@@ -302,6 +295,15 @@ module ObjectStorage
super super
end end
def store!(new_file = nil)
# when direct upload is enabled, always store on remote storage
if self.class.object_store_enabled? && self.class.direct_upload_enabled?
self.object_store = Store::REMOTE
end
super
end
private private
def schedule_background_upload? def schedule_background_upload?
......
---
title: Fix direct_upload when records with null file_store are used
merge_request:
author:
type: fixed
...@@ -168,4 +168,43 @@ describe Ci::JobArtifact do ...@@ -168,4 +168,43 @@ describe Ci::JobArtifact do
end end
end end
end end
describe 'file is being stored' do
subject { create(:ci_job_artifact, :archive) }
context 'when object has nil store' do
before do
subject.update_column(:file_store, nil)
subject.reload
end
it 'is stored locally' do
expect(subject.file_store).to be(nil)
expect(subject.file).to be_file_storage
expect(subject.file.object_store).to eq(ObjectStorage::Store::LOCAL)
end
end
context 'when existing object has local store' do
it 'is stored locally' do
expect(subject.file_store).to be(ObjectStorage::Store::LOCAL)
expect(subject.file).to be_file_storage
expect(subject.file.object_store).to eq(ObjectStorage::Store::LOCAL)
end
end
context 'when direct upload is enabled' do
before do
stub_artifacts_object_storage(direct_upload: true)
end
context 'when file is stored' do
it 'is stored remotely' do
expect(subject.file_store).to eq(ObjectStorage::Store::REMOTE)
expect(subject.file).not_to be_file_storage
expect(subject.file.object_store).to eq(ObjectStorage::Store::REMOTE)
end
end
end
end
end end
...@@ -81,5 +81,44 @@ describe LfsObject do ...@@ -81,5 +81,44 @@ describe LfsObject do
end end
end end
end end
describe 'file is being stored' do
let(:lfs_object) { create(:lfs_object, :with_file) }
context 'when object has nil store' do
before do
lfs_object.update_column(:file_store, nil)
lfs_object.reload
end
it 'is stored locally' do
expect(lfs_object.file_store).to be(nil)
expect(lfs_object.file).to be_file_storage
expect(lfs_object.file.object_store).to eq(ObjectStorage::Store::LOCAL)
end
end
context 'when existing object has local store' do
it 'is stored locally' do
expect(lfs_object.file_store).to be(ObjectStorage::Store::LOCAL)
expect(lfs_object.file).to be_file_storage
expect(lfs_object.file.object_store).to eq(ObjectStorage::Store::LOCAL)
end
end
context 'when direct upload is enabled' do
before do
stub_lfs_object_storage(direct_upload: true)
end
context 'when file is stored' do
it 'is stored remotely' do
expect(lfs_object.file_store).to eq(ObjectStorage::Store::REMOTE)
expect(lfs_object.file).not_to be_file_storage
expect(lfs_object.file.object_store).to eq(ObjectStorage::Store::REMOTE)
end
end
end
end
end end
end end
...@@ -75,36 +75,8 @@ describe ObjectStorage do ...@@ -75,36 +75,8 @@ describe ObjectStorage do
expect(object).to receive(:file_store).and_return(nil) expect(object).to receive(:file_store).and_return(nil)
end end
context 'when object storage is enabled' do it "uses Store::LOCAL" do
context 'when direct uploads are enabled' do is_expected.to eq(described_class::Store::LOCAL)
before do
stub_uploads_object_storage(uploader_class, enabled: true, direct_upload: true)
end
it "uses Store::REMOTE" do
is_expected.to eq(described_class::Store::REMOTE)
end
end
context 'when direct uploads are disabled' do
before do
stub_uploads_object_storage(uploader_class, enabled: true, direct_upload: false)
end
it "uses Store::LOCAL" do
is_expected.to eq(described_class::Store::LOCAL)
end
end
end
context 'when object storage is disabled' do
before do
stub_uploads_object_storage(uploader_class, enabled: false)
end
it "uses Store::LOCAL" do
is_expected.to eq(described_class::Store::LOCAL)
end
end end
end end
...@@ -537,6 +509,72 @@ describe ObjectStorage do ...@@ -537,6 +509,72 @@ describe ObjectStorage do
end end
end end
context 'when local file is used' do
let(:temp_file) { Tempfile.new("test") }
before do
FileUtils.touch(temp_file)
end
after do
FileUtils.rm_f(temp_file)
end
context 'when valid file is used' do
context 'when valid file is specified' do
let(:uploaded_file) { temp_file }
context 'when object storage and direct upload is specified' do
before do
stub_uploads_object_storage(uploader_class, enabled: true, direct_upload: true)
end
context 'when file is stored' do
subject do
uploader.store!(uploaded_file)
end
it 'file to be remotely stored in permament location' do
subject
expect(uploader).to be_exists
expect(uploader).not_to be_cached
expect(uploader).not_to be_file_storage
expect(uploader.path).not_to be_nil
expect(uploader.path).not_to include('tmp/upload')
expect(uploader.path).not_to include('tmp/cache')
expect(uploader.object_store).to eq(described_class::Store::REMOTE)
end
end
end
context 'when object storage and direct upload is not used' do
before do
stub_uploads_object_storage(uploader_class, enabled: true, direct_upload: false)
end
context 'when file is stored' do
subject do
uploader.store!(uploaded_file)
end
it 'file to be remotely stored in permament location' do
subject
expect(uploader).to be_exists
expect(uploader).not_to be_cached
expect(uploader).to be_file_storage
expect(uploader.path).not_to be_nil
expect(uploader.path).not_to include('tmp/upload')
expect(uploader.path).not_to include('tmp/cache')
expect(uploader.object_store).to eq(described_class::Store::LOCAL)
end
end
end
end
end
end
context 'when remote file is used' do context 'when remote file is used' do
let(:temp_file) { Tempfile.new("test") } let(:temp_file) { Tempfile.new("test") }
...@@ -590,9 +628,9 @@ describe ObjectStorage do ...@@ -590,9 +628,9 @@ describe ObjectStorage do
expect(uploader).to be_exists expect(uploader).to be_exists
expect(uploader).to be_cached expect(uploader).to be_cached
expect(uploader).not_to be_file_storage
expect(uploader.path).not_to be_nil expect(uploader.path).not_to be_nil
expect(uploader.path).not_to include('tmp/cache') expect(uploader.path).not_to include('tmp/cache')
expect(uploader.url).not_to be_nil
expect(uploader.path).not_to include('tmp/cache') expect(uploader.path).not_to include('tmp/cache')
expect(uploader.object_store).to eq(described_class::Store::REMOTE) expect(uploader.object_store).to eq(described_class::Store::REMOTE)
end end
...@@ -607,6 +645,7 @@ describe ObjectStorage do ...@@ -607,6 +645,7 @@ describe ObjectStorage do
expect(uploader).to be_exists expect(uploader).to be_exists
expect(uploader).not_to be_cached expect(uploader).not_to be_cached
expect(uploader).not_to be_file_storage
expect(uploader.path).not_to be_nil expect(uploader.path).not_to be_nil
expect(uploader.path).not_to include('tmp/upload') expect(uploader.path).not_to include('tmp/upload')
expect(uploader.path).not_to include('tmp/cache') expect(uploader.path).not_to include('tmp/cache')
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment