Commit a0038605 authored by Douglas Barbosa Alexandre's avatar Douglas Barbosa Alexandre Committed by Igor Drozdov

Fix Gitlab::Geo::Replication::BaseTransfer class

The uploader attribute was introduced is only used
to upload the file to Object Storage when the option
to sync object storage is enabled.

The problem is that we're setting this value during
the object initialization. It raises an error for
orphaned records while building the uploader because
model is nil.
parent e5d7d61b
---
title: Geo - Fix Gitlab::Geo::Replication::BaseTransfer for orphaned registries
merge_request: 34292
author:
type: fixed
...@@ -6,15 +6,15 @@ module Gitlab ...@@ -6,15 +6,15 @@ module Gitlab
class BaseTransfer class BaseTransfer
include LogHelpers include LogHelpers
attr_reader :file_type, :file_id, :filename, :uploader, :expected_checksum, :request_data attr_reader :file_type, :file_id, :filename, :expected_checksum, :request_data, :resource
TEMP_PREFIX = 'tmp_'.freeze TEMP_PREFIX = 'tmp_'.freeze
def initialize(file_type:, file_id:, request_data:, expected_checksum: nil, filename: nil, uploader: nil) def initialize(resource:, file_type:, file_id:, request_data:, expected_checksum: nil, filename: nil)
@resource = resource
@file_type = file_type @file_type = file_type
@file_id = file_id @file_id = file_id
@filename = filename @filename = filename
@uploader = uploader
@expected_checksum = expected_checksum @expected_checksum = expected_checksum
@request_data = request_data @request_data = request_data
end end
...@@ -85,6 +85,10 @@ module Gitlab ...@@ -85,6 +85,10 @@ module Gitlab
private private
def uploader
raise NotImplementedError, "#{self.class} does not implement #{__method__}"
end
def skipped_result def skipped_result
Result.new(success: false, bytes_downloaded: 0, skipped: true) Result.new(success: false, bytes_downloaded: 0, skipped: true)
end end
......
...@@ -9,7 +9,6 @@ module Gitlab ...@@ -9,7 +9,6 @@ module Gitlab
# * Returning a detailed Result object # * Returning a detailed Result object
class FileTransfer < BaseTransfer class FileTransfer < BaseTransfer
# Initialize a transfer service for a specified Upload # Initialize a transfer service for a specified Upload
#
# @param [Symbol] file_type # @param [Symbol] file_type
# @param [Upload] upload # @param [Upload] upload
def initialize(file_type, upload) def initialize(file_type, upload)
...@@ -25,12 +24,16 @@ module Gitlab ...@@ -25,12 +24,16 @@ module Gitlab
private private
def uploader
resource.retrieve_uploader
end
def local_file_attributes(file_type, upload) def local_file_attributes(file_type, upload)
{ {
resource: upload,
file_type: file_type, file_type: file_type,
file_id: upload.id, file_id: upload.id,
filename: upload.absolute_path, filename: upload.absolute_path,
uploader: upload.retrieve_uploader,
expected_checksum: upload.checksum, expected_checksum: upload.checksum,
request_data: build_request_data(file_type, upload) request_data: build_request_data(file_type, upload)
} }
...@@ -38,9 +41,9 @@ module Gitlab ...@@ -38,9 +41,9 @@ module Gitlab
def remote_file_attributes(file_type, upload) def remote_file_attributes(file_type, upload)
{ {
resource: upload,
file_type: file_type, file_type: file_type,
file_id: upload.id, file_id: upload.id,
uploader: upload.retrieve_uploader,
request_data: build_request_data(file_type, upload) request_data: build_request_data(file_type, upload)
} }
end end
......
...@@ -21,12 +21,16 @@ module Gitlab ...@@ -21,12 +21,16 @@ module Gitlab
private private
def uploader
resource.file
end
def local_job_artifact_attributes(job_artifact) def local_job_artifact_attributes(job_artifact)
{ {
resource: job_artifact,
file_type: :job_artifact, file_type: :job_artifact,
file_id: job_artifact.id, file_id: job_artifact.id,
filename: job_artifact.file.path, filename: job_artifact.file.path,
uploader: job_artifact.file,
expected_checksum: job_artifact.file_sha256, expected_checksum: job_artifact.file_sha256,
request_data: job_artifact_request_data(job_artifact) request_data: job_artifact_request_data(job_artifact)
} }
...@@ -34,9 +38,9 @@ module Gitlab ...@@ -34,9 +38,9 @@ module Gitlab
def remote_job_artifact_attributes(job_artifact) def remote_job_artifact_attributes(job_artifact)
{ {
resource: job_artifact,
file_type: :job_artifact, file_type: :job_artifact,
file_id: job_artifact.id, file_id: job_artifact.id,
uploader: job_artifact.file,
request_data: job_artifact_request_data(job_artifact) request_data: job_artifact_request_data(job_artifact)
} }
end end
......
...@@ -21,12 +21,16 @@ module Gitlab ...@@ -21,12 +21,16 @@ module Gitlab
private private
def uploader
resource.file
end
def local_lfs_object_attributes(lfs_object) def local_lfs_object_attributes(lfs_object)
{ {
resource: lfs_object,
file_type: :lfs, file_type: :lfs,
file_id: lfs_object.id, file_id: lfs_object.id,
filename: lfs_object.file.path, filename: lfs_object.file.path,
uploader: lfs_object.file,
expected_checksum: lfs_object.oid, expected_checksum: lfs_object.oid,
request_data: lfs_request_data(lfs_object) request_data: lfs_request_data(lfs_object)
} }
...@@ -34,9 +38,9 @@ module Gitlab ...@@ -34,9 +38,9 @@ module Gitlab
def remote_lfs_object_attributes(lfs_object) def remote_lfs_object_attributes(lfs_object)
{ {
resource: lfs_object,
file_type: :lfs, file_type: :lfs,
file_id: lfs_object.id, file_id: lfs_object.id,
uploader: lfs_object.file,
expected_checksum: lfs_object.oid, expected_checksum: lfs_object.oid,
request_data: lfs_request_data(lfs_object) request_data: lfs_request_data(lfs_object)
} }
......
...@@ -10,8 +10,9 @@ RSpec.describe Gitlab::Geo::Replication::BaseTransfer do ...@@ -10,8 +10,9 @@ RSpec.describe Gitlab::Geo::Replication::BaseTransfer do
describe '#resource_url' do describe '#resource_url' do
subject do subject do
described_class.new(file_type: 'design_management/design_v432x230', file_id: 1, described_class.new(file_type: 'design_management/design_v432x230',
filename: Tempfile.new, expected_checksum: nil, request_data: nil) file_id: 1, filename: Tempfile.new, expected_checksum: nil,
request_data: nil, resource: nil)
end end
context 'when file type contains /' do context 'when file type contains /' do
...@@ -25,7 +26,7 @@ RSpec.describe Gitlab::Geo::Replication::BaseTransfer do ...@@ -25,7 +26,7 @@ RSpec.describe Gitlab::Geo::Replication::BaseTransfer do
describe '#can_transfer?' do describe '#can_transfer?' do
subject do subject do
described_class.new(file_type: :avatar, file_id: 1, filename: Tempfile.new, described_class.new(file_type: :avatar, file_id: 1, filename: Tempfile.new,
expected_checksum: nil, request_data: nil) expected_checksum: nil, request_data: nil, resource: nil)
end end
before do before do
...@@ -51,7 +52,7 @@ RSpec.describe Gitlab::Geo::Replication::BaseTransfer do ...@@ -51,7 +52,7 @@ RSpec.describe Gitlab::Geo::Replication::BaseTransfer do
context 'when destination filename is a directory' do context 'when destination filename is a directory' do
it 'returns false' do it 'returns false' do
subject = described_class.new(file_type: :avatar, file_id: 1, filename: Dir::Tmpname.tmpdir, subject = described_class.new(file_type: :avatar, file_id: 1, filename: Dir::Tmpname.tmpdir,
expected_checksum: nil, request_data: nil) expected_checksum: nil, request_data: nil, resource: nil)
expect(subject.can_transfer?).to be_falsey expect(subject.can_transfer?).to be_falsey
end end
...@@ -59,8 +60,8 @@ RSpec.describe Gitlab::Geo::Replication::BaseTransfer do ...@@ -59,8 +60,8 @@ RSpec.describe Gitlab::Geo::Replication::BaseTransfer do
context 'when no filename is informed' do context 'when no filename is informed' do
it 'returns true' do it 'returns true' do
subject = described_class.new(file_type: :avatar, file_id: 1, subject = described_class.new(file_type: :avatar, file_id: 1, expected_checksum: nil,
expected_checksum: nil, request_data: nil) request_data: nil, resource: nil)
expect(subject.can_transfer?).to be_truthy expect(subject.can_transfer?).to be_truthy
end end
......
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