Migrate BaseTransfer to use HTTP.rb instead of HTTParty

Specs now use stub_request instead of the multiple mocks it was using
before, so we can actually test the correct behavior

ee/lib/gitlab/geo/replication/base_transfer.rb

@@ -43,6 +43,11 @@ module Gitlab
           true
         end
 
+        # @return [String] URL to download the resource from
+        def resource_url
+          Gitlab::Geo.primary_node.geo_transfers_url(file_type, file_id.to_s)
+        end
+
         # Returns Result object with success boolean and number of bytes downloaded.
         def download_from_primary
           return skipped_result unless can_transfer?
@@ -53,10 +58,9 @@ module Gitlab
             return skipped_result
           end
 
-          url = Gitlab::Geo.primary_node.geo_transfers_url(file_type, file_id.to_s)
           req_headers = TransferRequest.new(request_data).headers
 
-          download_file(url, req_headers)
+          download_file(resource_url, req_headers)
         end
 
         class Result
@@ -100,8 +104,7 @@ module Gitlab
           true
         end
 
-        # Use Gitlab::HTTP for now but switch to curb if performance becomes
-        # an issue
+        # Download file from informed URL using HTTP.rb
         #
         # @return [Result] Object with transfer status and information
         def download_file(url, req_headers)
@@ -111,37 +114,42 @@ module Gitlab
           return failure_result unless temp_file
 
           begin
-            response = Gitlab::HTTP.get(url, allow_local_requests: true, headers: req_headers, stream_body: true) do |fragment|
-              temp_file.write(fragment)
-            end
+            # Make the request
+            response = ::HTTP.get(url, headers: req_headers)
 
-            temp_file.flush
-
-            unless response.success?
-              log_error("Unsuccessful download", filename: filename, response_code: response.code, response_msg: response.try(:msg), url: url)
+            # Check for failures
+            unless response.status.success?
+              log_error("Unsuccessful download", filename: filename, status_code: response.status.code, reason: response.status.reason, url: url)
 
-              return failure_result(primary_missing_file: primary_missing_file?(response, temp_file))
+              return failure_result(primary_missing_file: primary_missing_file?(response))
             end
 
-            if File.directory?(filename)
-              log_error("Destination file is a directory", filename: filename)
-
-              return failure_result
+            # Stream to temporary file on disk
+            response.body.each do |chunk|
+              temp_file.write(chunk)
             end
 
+            # Make sure file is written to the disk
+            # This is required to get correct file size.
+            temp_file.flush
+
             file_size = File.stat(temp_file.path).size
 
+            # Check for checksum mismatch
             if checksum_mismatch?(temp_file.path)
               log_error("Downloaded file checksum mismatch", expected_checksum: expected_checksum, actual_checksum: @actual_checksum, file_size_bytes: file_size)
 
               return failure_result(bytes_downloaded: file_size)
             end
 
+            # Move transfered file to the target location
             FileUtils.mv(temp_file.path, filename)
 
             log_info("Successful downloaded", filename: filename, file_size_bytes: file_size)
-          rescue StandardError, Gitlab::HTTP::Error => e
+          rescue StandardError, ::HTTP::Error => e
             log_error("Error downloading file", error: e, filename: filename, url: url)
+
+            return failure_result
           ensure
             temp_file.close
             temp_file.unlink
@@ -150,12 +158,11 @@ module Gitlab
           Result.new(success: file_size > -1, bytes_downloaded: [file_size, 0].max)
         end
 
-        def primary_missing_file?(response, temp_file)
-          body = File.read(temp_file.path) if File.exist?(temp_file.path)
-
-          if response.code == 404 && body.present?
+        def primary_missing_file?(response)
+          if response.status.not_found?
             begin
-              json_response = JSON.parse(body)
+              json_response = response.parse
+
               return code_file_not_found?(json_response['geo_code'])
             rescue JSON::ParserError
             end
@@ -180,7 +187,7 @@ module Gitlab
           temp.binmode
           temp
         rescue StandardError => e
-          log_error("Error creating temporary file", error: e)
+          log_error("Error creating temporary file", error: e, filename: target_filename)
           nil
         end
 

ee/spec/lib/gitlab/geo/replication/file_transfer_spec.rb

@@ -46,15 +46,14 @@ describe Gitlab::Geo::Replication::FileTransfer do
       it 'returns a successful result' do
         content = upload.build_uploader.file.read
         size = content.bytesize
-
-        expect(FileUtils).to receive(:mv).with(anything, upload.absolute_path).and_call_original
-        response = double(:response, success?: true)
-        expect(Gitlab::HTTP).to receive(:get).and_yield(content.to_s).and_return(response)
+        stub_request(:get, subject.resource_url).to_return(status: 200, body: content)
 
         result = subject.download_from_primary
 
         expect_result(result, success: true, bytes_downloaded: size, primary_missing_file: false)
+
         stat = File.stat(upload.absolute_path)
+
         expect(stat.size).to eq(size)
         expect(stat.mode & 0777).to eq(0666 - File.umask)
         expect(File.binread(upload.absolute_path)).to eq(content)
@@ -64,11 +63,10 @@ describe Gitlab::Geo::Replication::FileTransfer do
     context 'when the HTTP response is unsuccessful' do
       context 'when the HTTP response indicates a missing file on the primary' do
         it 'returns a failed result indicating primary_missing_file' do
-          expect(FileUtils).not_to receive(:mv).with(anything, upload.absolute_path).and_call_original
-          response = double(:response, success?: false, code: 404, msg: "No such file")
-
-          expect(File).to receive(:read).and_return("{\"geo_code\":\"#{Gitlab::Geo::Replication::FILE_NOT_FOUND_GEO_CODE}\"}")
-          expect(Gitlab::HTTP).to receive(:get).and_return(response)
+          stub_request(:get, subject.resource_url)
+            .to_return(status: 404,
+                       headers: { content_type: 'application/json' },
+                       body: { geo_code: Gitlab::Geo::Replication::FILE_NOT_FOUND_GEO_CODE }.to_json)
 
           result = subject.download_from_primary
 
@@ -78,10 +76,7 @@ describe Gitlab::Geo::Replication::FileTransfer do
 
       context 'when the HTTP response does not indicate a missing file on the primary' do
         it 'returns a failed result' do
-          expect(FileUtils).not_to receive(:mv).with(anything, upload.absolute_path).and_call_original
-          response = double(:response, success?: false, code: 404, msg: 'No such file')
-
-          expect(Gitlab::HTTP).to receive(:get).and_return(response)
+          stub_request(:get, subject.resource_url).to_return(status: 404, body: 'Not found')
 
           result = subject.download_from_primary
 
@@ -121,8 +116,8 @@ describe Gitlab::Geo::Replication::FileTransfer do
     context 'when the checksum of the downloaded file does not match' do
       it 'returns a failed result' do
         bad_content = 'corrupted!!!'
-        response = double(:response, success?: true)
-        expect(Gitlab::HTTP).to receive(:get).and_yield(bad_content).and_return(response)
+        stub_request(:get, subject.resource_url)
+          .to_return(status: 200, body: bad_content)
 
         result = subject.download_from_primary
 
@@ -134,8 +129,8 @@ describe Gitlab::Geo::Replication::FileTransfer do
       it 'returns a successful result' do
         upload.update_column(:checksum, nil)
         content = 'foo'
-        response = double(:response, success?: true)
-        expect(Gitlab::HTTP).to receive(:get).and_yield(content).and_return(response)
+        stub_request(:get, subject.resource_url)
+          .to_return(status: 200, body: content)
 
         result = subject.download_from_primary
 

ee/spec/lib/gitlab/geo/replication/job_artifact_transfer_spec.rb

@@ -54,14 +54,14 @@ describe Gitlab::Geo::Replication::JobArtifactTransfer, :geo do
       it 'returns a successful result' do
         content = job_artifact.file.read
         size = content.bytesize
-        expect(FileUtils).to receive(:mv).with(anything, job_artifact.file.path).and_call_original
-        response = double(:response, success?: true)
-        expect(Gitlab::HTTP).to receive(:get).and_yield(content.to_s).and_return(response)
+        stub_request(:get, subject.resource_url).to_return(status: 200, body: content)
 
         result = subject.download_from_primary
 
         expect_result(result, success: true, bytes_downloaded: size, primary_missing_file: false)
+
         stat = File.stat(job_artifact.file.path)
+
         expect(stat.size).to eq(size)
         expect(stat.mode & 0777).to eq(0666 - File.umask)
         expect(File.binread(job_artifact.file.path)).to eq(content)
@@ -71,10 +71,10 @@ describe Gitlab::Geo::Replication::JobArtifactTransfer, :geo do
     context 'when the HTTP response is unsuccessful' do
       context 'when the HTTP response indicates a missing file on the primary' do
         it 'returns a failed result indicating primary_missing_file' do
-          expect(FileUtils).not_to receive(:mv).with(anything, job_artifact.file.path).and_call_original
-          response = double(:response, success?: false, code: 404, msg: "No such file")
-          expect(File).to receive(:read).and_return("{\"geo_code\":\"#{Gitlab::Geo::Replication::FILE_NOT_FOUND_GEO_CODE}\"}")
-          expect(Gitlab::HTTP).to receive(:get).and_return(response)
+          stub_request(:get, subject.resource_url)
+            .to_return(status: 404,
+                       headers: { content_type: 'application/json' },
+                       body: { geo_code: Gitlab::Geo::Replication::FILE_NOT_FOUND_GEO_CODE }.to_json)
 
           result = subject.download_from_primary
 
@@ -84,9 +84,7 @@ describe Gitlab::Geo::Replication::JobArtifactTransfer, :geo do
 
       context 'when the HTTP response does not indicate a missing file on the primary' do
         it 'returns a failed result' do
-          expect(FileUtils).not_to receive(:mv).with(anything, job_artifact.file.path).and_call_original
-          response = double(:response, success?: false, code: 404, msg: 'No such file')
-          expect(Gitlab::HTTP).to receive(:get).and_return(response)
+          stub_request(:get, subject.resource_url).to_return(status: 404, body: 'Not found')
 
           result = subject.download_from_primary
 
@@ -124,8 +122,8 @@ describe Gitlab::Geo::Replication::JobArtifactTransfer, :geo do
     context 'when the checksum of the downloaded file does not match' do
       it 'returns a failed result' do
         bad_content = 'corrupted!!!'
-        response = double(:response, success?: true)
-        expect(Gitlab::HTTP).to receive(:get).and_yield(bad_content).and_return(response)
+        stub_request(:get, subject.resource_url)
+          .to_return(status: 200, body: bad_content)
 
         result = subject.download_from_primary
 
@@ -137,10 +135,11 @@ describe Gitlab::Geo::Replication::JobArtifactTransfer, :geo do
       it 'returns a successful result' do
         artifact = create(:ci_job_artifact, :archive)
         content = 'foo'
-        response = double(:response, success?: true)
-        expect(Gitlab::HTTP).to receive(:get).and_yield(content).and_return(response)
         transfer = described_class.new(artifact)
 
+        stub_request(:get, transfer.resource_url)
+          .to_return(status: 200, body: content)
+
         result = transfer.download_from_primary
 
         expect_result(result, success: true, bytes_downloaded: content.bytesize, primary_missing_file: false)

ee/spec/lib/gitlab/geo/replication/lfs_transfer_spec.rb

@@ -32,14 +32,14 @@ describe Gitlab::Geo::Replication::LfsTransfer do
       it 'returns a successful result' do
         content = lfs_object.file.read
         size = content.bytesize
-        expect(FileUtils).to receive(:mv).with(anything, lfs_object.file.path).and_call_original
-        response = double(:response, success?: true)
-        expect(Gitlab::HTTP).to receive(:get).and_yield(content.to_s).and_return(response)
+        stub_request(:get, subject.resource_url).to_return(status: 200, body: content)
 
         result = subject.download_from_primary
 
         expect_result(result, success: true, bytes_downloaded: size, primary_missing_file: false)
+
         stat = File.stat(lfs_object.file.path)
+
         expect(stat.size).to eq(size)
         expect(stat.mode & 0777).to eq(0666 - File.umask)
         expect(File.binread(lfs_object.file.path)).to eq(content)
@@ -49,10 +49,10 @@ describe Gitlab::Geo::Replication::LfsTransfer do
     context 'when the HTTP response is unsuccessful' do
       context 'when the HTTP response indicates a missing file on the primary' do
         it 'returns a failed result indicating primary_missing_file' do
-          expect(FileUtils).not_to receive(:mv).with(anything, lfs_object.file.path).and_call_original
-          response = double(:response, success?: false, code: 404, msg: "No such file")
-          expect(File).to receive(:read).and_return("{\"geo_code\":\"#{Gitlab::Geo::Replication::FILE_NOT_FOUND_GEO_CODE}\"}")
-          expect(Gitlab::HTTP).to receive(:get).and_return(response)
+          stub_request(:get, subject.resource_url)
+            .to_return(status: 404,
+                       headers: { content_type: 'application/json' },
+                       body: { geo_code: Gitlab::Geo::Replication::FILE_NOT_FOUND_GEO_CODE }.to_json)
 
           result = subject.download_from_primary
 
@@ -62,9 +62,7 @@ describe Gitlab::Geo::Replication::LfsTransfer do
 
       context 'when the HTTP response does not indicate a missing file on the primary' do
         it 'returns a failed result' do
-          expect(FileUtils).not_to receive(:mv).with(anything, lfs_object.file.path).and_call_original
-          response = double(:response, success?: false, code: 404, msg: 'No such file')
-          expect(Gitlab::HTTP).to receive(:get).and_return(response)
+          stub_request(:get, subject.resource_url).to_return(status: 404, body: 'Not found')
 
           result = subject.download_from_primary
 
@@ -102,8 +100,8 @@ describe Gitlab::Geo::Replication::LfsTransfer do
     context 'when the checksum of the downloaded file does not match' do
       it 'returns a failed result' do
         bad_content = 'corrupted!!!'
-        response = double(:response, success?: true)
-        expect(Gitlab::HTTP).to receive(:get).and_yield(bad_content).and_return(response)
+        stub_request(:get, subject.resource_url)
+          .to_return(status: 200, body: bad_content)
 
         result = subject.download_from_primary