Commit 06f6133e authored by Michael Kozono's avatar Michael Kozono Committed by Igor Drozdov

Use the same keys more consistently

And add a couple convenience methods to help:

* Replicator#replicable_params
* Replicator.for_replicable_params
parent e999b496
......@@ -179,7 +179,7 @@ class GeoNode < ApplicationRecord
#
# @param [String] replicable_name
# @param [Integer] replicable_id
def geo_retrieve_url(replicable_name, replicable_id)
def geo_retrieve_url(replicable_name:, replicable_id:)
geo_api_url("retrieve/#{replicable_name}/#{replicable_id}")
end
......
......@@ -2,15 +2,12 @@
module Geo
class BlobUploadService
attr_reader :replicable_name, :blob_id, :checksum, :replicator
attr_reader :checksum, :replicator
def initialize(replicable_name:, blob_id:, decoded_params:)
@replicable_name = replicable_name
@blob_id = blob_id
def initialize(replicable_name:, replicable_id:, decoded_params:)
@checksum = decoded_params.delete(:checksum)
replicator_klass = Gitlab::Geo::Replicator.for_replicable_name(replicable_name)
@replicator = replicator_klass.new(model_record_id: blob_id)
@replicator = Gitlab::Geo::Replicator.for_replicable_params(replicable_name: replicable_name, replicable_id: replicable_id)
end
def execute
......
......@@ -24,8 +24,7 @@ module Geo
strong_memoize(:replicator) do
model_record_id = payload[:model_record_id]
replicator_class = ::Gitlab::Geo::Replicator.for_replicable_name(replicable_name)
replicator_class.new(model_record_id: model_record_id)
::Gitlab::Geo::Replicator.for_replicable_params(replicable_name: replicable_name, replicable_id: model_record_id)
end
end
......
......@@ -11,8 +11,7 @@ module Geo
idempotent!
def perform(replicable_name, replicable_id)
replicator_class = ::Gitlab::Geo::Replicator.for_replicable_name(replicable_name)
replicator = replicator_class.new(model_record_id: replicable_id)
replicator = ::Gitlab::Geo::Replicator.for_replicable_params(replicable_name: replicable_name, replicable_id: replicable_id)
replicator.calculate_checksum!
rescue ActiveRecord::RecordNotFound
......
......@@ -22,16 +22,15 @@ module API
params do
requires :replicable_name, type: String, desc: 'Replicable name (eg. package_file)'
requires :id, type: Integer, desc: 'The model ID that needs to be transferred'
requires :replicable_id, type: Integer, desc: 'The replicable ID that needs to be transferred'
end
get 'retrieve/:replicable_name/:id' do
get 'retrieve/:replicable_name/:replicable_id' do
check_gitlab_geo_request_ip!
authorize_geo_transfer!(replicable_name: params[:replicable_name], id: params[:id])
params_sym = params.symbolize_keys
authorize_geo_transfer!(params_sym)
decoded_params = geo_jwt_decoder.decode
service = ::Geo::BlobUploadService.new(replicable_name: params[:replicable_name],
blob_id: params[:id],
decoded_params: decoded_params)
service = ::Geo::BlobUploadService.new(**params_sym, decoded_params: decoded_params)
response = service.execute
if response[:code] == :ok
......
......@@ -8,7 +8,7 @@ module Gitlab
attr_reader :replicator
delegate :replicable_name, :model_record, :primary_checksum, :carrierwave_uploader, to: :replicator
delegate :primary_checksum, :carrierwave_uploader, to: :replicator
delegate :file_storage?, to: :carrierwave_uploader
class Result
......@@ -49,7 +49,7 @@ module Gitlab
# @return [String] URL to download the resource from
def resource_url
Gitlab::Geo.primary_node.geo_retrieve_url(replicable_name, model_record.id.to_s)
Gitlab::Geo.primary_node.geo_retrieve_url(**replicator.replicable_params)
end
private
......@@ -62,10 +62,7 @@ module Gitlab
#
# @return [Hash] HTTP request headers
def request_headers
request_data = {
replicable_name: replicable_name,
id: model_record.id
}
request_data = replicator.replicable_params
TransferRequest.new(request_data).headers
end
......@@ -181,7 +178,7 @@ module Gitlab
pathname = Pathname.new(absolute_path)
temp = Tempfile.new(TEMP_PREFIX, pathname.dirname.to_s)
else
temp = Tempfile.new("#{TEMP_PREFIX}-#{replicable_name}-#{model_record.id}")
temp = Tempfile.new("#{TEMP_PREFIX}-#{replicator.replicable_name}-#{replicator.model_record_id}")
end
temp.chmod(default_permissions)
......
......@@ -92,6 +92,18 @@ module Gitlab
raise e
end
# Given the output of a replicator's `replicable_params`, reinstantiate
# the replicator instance
#
# @param [String] replicable_name of a replicator instance
# @param [Integer] replicable_id of a replicator instance
# @return [Geo::Replicator] replicator instance
def self.for_replicable_params(replicable_name:, replicable_id:)
replicator_class = for_replicable_name(replicable_name)
replicator_class.new(model_record_id: replicable_id)
end
def self.checksummed
model.checksummed
end
......@@ -127,7 +139,7 @@ module Gitlab
# @param [Integer] model_record_id
def initialize(model_record: nil, model_record_id: nil)
@model_record = model_record
@model_record_id = model_record_id
@model_record_id = model_record_id || model_record&.id
end
# Instance of the replicable model
......@@ -233,6 +245,14 @@ module Gitlab
end
end
# Return exactly the data needed by `for_replicable_params` to
# reinstantiate this Replicator elsewhere.
#
# @return [Hash] the replicable name and ID
def replicable_params
{ replicable_name: replicable_name, replicable_id: model_record_id }
end
protected
# Store an event on the database
......
......@@ -30,6 +30,10 @@ describe Gitlab::Geo::Replicator do
event :test
event :another_test
def self.model
::DummyModel
end
protected
def consume_event_test(user:, other:)
......@@ -224,7 +228,7 @@ describe Gitlab::Geo::Replicator do
# We cannot infer parent project, so parent_project_id should be overridden.
context 'when model_record does not respond to project_id' do
let(:model_record) { double(:model_record) }
let(:model_record) { double(:model_record, id: 555) }
it 'raises NotImplementedError' do
expect { replicator.parent_project_id }.to raise_error(NotImplementedError)
......@@ -233,12 +237,67 @@ describe Gitlab::Geo::Replicator do
# We assume project_id to be the parent project.
context 'when model_record responds to project_id' do
let(:model_record) { double(:model_record, project_id: 1234) }
let(:model_record) { double(:model_record, id: 555, project_id: 1234) }
it 'does not error' do
expect(replicator.parent_project_id).to eq(1234)
end
end
end
describe '.for_replicable_params' do
it 'returns the corresponding Replicator instance' do
replicator = described_class.for_replicable_params(replicable_name: 'dummy', replicable_id: 123456)
expect(replicator).to be_a(Geo::DummyReplicator)
expect(replicator.model_record_id).to eq(123456)
end
end
describe '.replicable_params' do
it 'returns a Hash of data needed to reinstantiate the Replicator' do
replicator = Geo::DummyReplicator.new(model_record_id: 123456)
expect(replicator.replicable_params).to eq(replicable_name: 'dummy', replicable_id: 123456)
end
end
describe '#initialize' do
subject(:replicator) { Geo::DummyReplicator.new(**args) }
let(:model_record) { double('DummyModel instance', id: 1234) }
context 'given model_record' do
let(:args) { { model_record: model_record } }
it 'sets model_record' do
expect(replicator.model_record).to eq(model_record)
end
it 'sets model_record_id' do
expect(replicator.model_record_id).to eq(1234)
end
end
context 'given model_record_id' do
let(:args) { { model_record_id: 1234 } }
before do
model = double('DummyModel')
# These two stubs are needed because `#model_record` instantiates the
# defined `.model` class.
allow(Geo::DummyReplicator).to receive(:model).and_return(model)
allow(model).to receive(:find).with(1234).and_return(model_record)
end
it 'sets model_record' do
expect(replicator.model_record).to eq(model_record)
end
it 'sets model_record_id' do
expect(replicator.model_record_id).to eq(1234)
end
end
end
end
end
......@@ -536,6 +536,14 @@ describe GeoNode, :request_store, :geo, type: :model do
end
end
describe '#geo_retrieve_url' do
let(:retrieve_url) { "https://localhost:3000/gitlab/api/#{api_version}/geo/retrieve/package_file/1" }
it 'returns api url based on node uri' do
expect(new_node.geo_retrieve_url(replicable_name: :package_file, replicable_id: 1)).to eq(retrieve_url)
end
end
describe '#geo_transfers_url' do
let(:transfers_url) { "https://localhost:3000/gitlab/api/#{api_version}/geo/transfers/lfs/1" }
......
......@@ -38,7 +38,7 @@ describe API::Geo do
end
end
describe 'GET /geo/retrieve/:replicable/:id' do
describe 'GET /geo/retrieve/:replicable_name/:replicable_id' do
before do
stub_current_geo_node(secondary_node)
end
......@@ -84,7 +84,7 @@ describe API::Geo do
end
it 'responds with 401 with mismatched params in auth headers' do
wrong_headers = Gitlab::Geo::TransferRequest.new({ replicable_name: 'wrong', id: 1234 }).headers
wrong_headers = Gitlab::Geo::TransferRequest.new({ replicable_name: 'wrong', replicable_id: 1234 }).headers
get api("/geo/retrieve/#{replicator.replicable_name}/#{resource.id}"), headers: wrong_headers
......@@ -92,7 +92,7 @@ describe API::Geo do
end
it 'responds with 404 when resource is not found' do
model_not_found_header = Gitlab::Geo::TransferRequest.new({ replicable_name: replicator.replicable_name, id: 1234 }).headers
model_not_found_header = Gitlab::Geo::TransferRequest.new({ replicable_name: replicator.replicable_name, replicable_id: 1234 }).headers
get api("/geo/retrieve/#{replicator.replicable_name}/1234"), headers: model_not_found_header
......
......@@ -5,7 +5,7 @@ require 'spec_helper'
describe Geo::BlobUploadService do
let(:package_file) { create(:package_file, :npm) }
subject { described_class.new(replicable_name: 'package_file', blob_id: package_file.id, decoded_params: {}) }
subject { described_class.new(replicable_name: 'package_file', replicable_id: package_file.id, decoded_params: {}) }
describe '#initialize' do
it 'initializes with valid attributes' do
......@@ -19,7 +19,7 @@ describe Geo::BlobUploadService do
end
it 'errors with an invalid attributes' do
service = described_class.new(replicable_name: 'package_file', blob_id: 1234567890, decoded_params: {})
service = described_class.new(replicable_name: 'package_file', replicable_id: non_existing_record_id, decoded_params: {})
response = service.execute
......@@ -27,7 +27,7 @@ describe Geo::BlobUploadService do
end
it 'returns a file with valid attributes' do
service = described_class.new(replicable_name: 'package_file', blob_id: package_file.id,
service = described_class.new(replicable_name: 'package_file', replicable_id: package_file.id,
decoded_params: { checksum: package_file.verification_checksum })
response = service.execute
......
......@@ -13,7 +13,7 @@ describe Geo::EventService do
describe '#execute' do
before do
resource_url = primary.geo_retrieve_url('package_file', model_record.id.to_s)
resource_url = primary.geo_retrieve_url(replicable_name: 'package_file', replicable_id: model_record.id.to_s)
content = model_record.file.open
File.unlink(model_record.file.path)
stub_request(:get, resource_url).to_return(status: 200, body: content)
......
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