Fix s3 download ranges for chunks (#2860)

Author: mullermp

gems/aws-sdk-s3/CHANGELOG.md

@@ -1,6 +1,8 @@
 Unreleased Changes
 ------------------
 
+* Issue - Fix multipart `download_file` so that it does not download bytes out of range (#2859).
+
 1.123.0 (2023-05-31)
 ------------------
 

gems/aws-sdk-s3/lib/aws-sdk-s3/file_downloader.rb

@@ -58,15 +58,19 @@ def multipart_download
         resp = @client.head_object(@params.merge(part_number: 1))
         count = resp.parts_count
         if count.nil? || count <= 1
-          resp.content_length < MIN_CHUNK_SIZE ?
-            single_request :
+          if resp.content_length <= MIN_CHUNK_SIZE
+            single_request
+          else
             multithreaded_get_by_ranges(construct_chunks(resp.content_length))
+          end
         else
           # partNumber is an option
           resp = @client.head_object(@params)
-          resp.content_length < MIN_CHUNK_SIZE ?
-            single_request :
+          if resp.content_length <= MIN_CHUNK_SIZE
+            single_request
+          else
             compute_mode(resp.content_length, count)
+          end
         end
       end
 
@@ -84,10 +88,11 @@ def construct_chunks(file_size)
         offset = 0
         default_chunk_size = compute_chunk(file_size)
         chunks = []
-        while offset <= file_size
+        while offset < file_size
           progress = offset + default_chunk_size
-          chunks << "bytes=#{offset}-#{progress < file_size ? progress : file_size}"
-          offset = progress + 1
+          progress = file_size if progress > file_size
+          chunks << "bytes=#{offset}-#{progress - 1}"
+          offset = progress
         end
         chunks
       end
@@ -96,12 +101,9 @@ def compute_chunk(file_size)
         if @chunk_size && @chunk_size > file_size
           raise ArgumentError, ":chunk_size shouldn't exceed total file size."
         else
-          chunk_size = @chunk_size || [
-            (file_size.to_f / MAX_PARTS).ceil,
-            MIN_CHUNK_SIZE
+          @chunk_size || [
+            (file_size.to_f / MAX_PARTS).ceil, MIN_CHUNK_SIZE
           ].max.to_i
-          chunk_size -= 1 if file_size % chunk_size == 1
-          chunk_size
         end
       end
 

gems/aws-sdk-s3/spec/object/download_file_spec.rb

@@ -36,19 +36,10 @@ module S3
           )
         end
 
-        let(:boundary_obj) do
-          S3::Object.new(
-            bucket_name: 'bucket',
-            key: 'boundary',
-            client: client
-          )
-        end
-
         let(:one_meg) { 1024 * 1024 }
         let(:small_file) { Tempfile.new('small-file') }
         let(:large_file) { Tempfile.new('large-file') }
         let(:single_part_file) { Tempfile.new('single-part-file') }
-        let(:boundary_file) { Tempfile.new('range-boundary-file') }
         let(:version_id) { 'a-fake-version-id' }
 
         before(:each) do
@@ -180,51 +171,14 @@ module S3
             )
         end
 
-        context 'file size is chunk size boundary' do
-          let(:file_size) { 5 * one_meg + 1 }
-          let(:chunk_size) { 5 * one_meg }
-
-          before :each do
-            allow(client).to receive(:head_object).with({
-              bucket: 'bucket',
-              key: 'boundary',
-              part_number: 1
-            }).and_return(
-              client.stub_data(
-                :head_object,
-                content_length: file_size
-              )
-            )
+        it 'downloads the file in range chunks' do
+          client.stub_responses(:get_object, ->(context) {
+            ranges = context.params[:range].split('=').last.split('-')
+            expect(ranges[1].to_i - ranges[0].to_i + 1).to eq(one_meg)
+            { content_range: "bytes #{ranges[0]}-#{ranges[1]}/#{20 * one_meg}" }
+          })
 
-            client.stub_responses(:get_object, ->(context) {
-              # S3 does not allow 1 byte ranges. This shouldn't be raised.
-              if context.params[:range] == "bytes=#{file_size}-#{file_size}"
-                'InvalidRange'
-              else
-                { content_range: "bytes 0-#{chunk_size - 1}/#{file_size}" }
-              end
-            })
-          end
-
-          it 'downloads the file with default chunk size' do
-            expect(client).to receive(:head_object).with({
-              bucket: 'bucket',
-              key: 'boundary',
-              part_number: 1
-            }).exactly(1).times
-
-            boundary_obj.download_file(path)
-          end
-
-          it 'downloads the file with provided chunk size' do
-            expect(client).to receive(:head_object).with({
-              bucket: 'bucket',
-              key: 'boundary',
-              part_number: 1
-            }).exactly(1).times
-
-            boundary_obj.download_file(path, chunk_size: chunk_size)
-          end
+          large_obj.download_file(path, chunk_size: one_meg)
         end
       end
     end