From cb0a68e94c00e45b2c6e9181828d9b579c688c10 Mon Sep 17 00:00:00 2001
From: Andrew Morton <andrewmorton@gocardless.com>
Date: Tue, 18 Apr 2023 13:45:35 +0100
Subject: [PATCH 01/17] Allow File uploads instead of only supporting Strings

To support uploading of large files this change allows an [IO](https://ruby-doc.org/core-2.7.1/IO.html) object to be provided instead of requiring a string. This allows large files to be streamed into the relevant backing store. The S3 / GCS SDKs already support this as a first class concept.

Prior to this change the individual adapters `upload!` method only supported the notion of uploading a string. With this change we that method think in terms of IO like objects with String uploads being a special case where we need to wrap the String in a `StringIO`.

The interface is kept backwards compatible through exposing a separate `upload_file!` method alongside the existing `upload!` method.
---
 lib/bucket_store/disk.rb              |  6 +--
 lib/bucket_store/gcs.rb               |  5 +--
 lib/bucket_store/in_memory.rb         |  4 +-
 lib/bucket_store/key_storage.rb       | 54 +++++++++++++++++++--------
 lib/bucket_store/s3.rb                |  4 +-
 spec/bucket_store/disk_spec.rb        | 27 ++++++++------
 spec/bucket_store/in_memory_spec.rb   | 33 ++++++++--------
 spec/bucket_store/key_storage_spec.rb | 27 ++++++++++++++
 8 files changed, 107 insertions(+), 53 deletions(-)

diff --git a/lib/bucket_store/disk.rb b/lib/bucket_store/disk.rb
index 28b183a..3379d4c 100644
--- a/lib/bucket_store/disk.rb
+++ b/lib/bucket_store/disk.rb
@@ -13,9 +13,9 @@ def initialize(base_dir)
       @base_dir = File.expand_path(base_dir)
     end
 
-    def upload!(bucket:, key:, content:)
-      File.open(key_path(bucket, key), "w") do |file|
-        file.write(content)
+    def upload!(bucket:, key:, file:)
+      File.open(key_path(bucket, key), "w") do |output_file|
+        output_file.write(file.read)
       end
       {
         bucket: bucket,
diff --git a/lib/bucket_store/gcs.rb b/lib/bucket_store/gcs.rb
index 802345e..30bad94 100644
--- a/lib/bucket_store/gcs.rb
+++ b/lib/bucket_store/gcs.rb
@@ -33,9 +33,8 @@ def initialize(timeout_seconds)
                  end
     end
 
-    def upload!(bucket:, key:, content:)
-      buffer = StringIO.new(content)
-      get_bucket(bucket).create_file(buffer, key)
+    def upload!(bucket:, key:, file:)
+      get_bucket(bucket).create_file(file, key)
 
       {
         bucket: bucket,
diff --git a/lib/bucket_store/in_memory.rb b/lib/bucket_store/in_memory.rb
index 6dfccc9..3a47611 100644
--- a/lib/bucket_store/in_memory.rb
+++ b/lib/bucket_store/in_memory.rb
@@ -24,8 +24,8 @@ def reset!
       @buckets = Hash.new { |hash, key| hash[key] = {} }
     end
 
-    def upload!(bucket:, key:, content:)
-      @buckets[bucket][key] = content
+    def upload!(bucket:, key:, file:)
+      @buckets[bucket][key] = file.read
 
       {
         bucket: bucket,
diff --git a/lib/bucket_store/key_storage.rb b/lib/bucket_store/key_storage.rb
index 4287d95..b912300 100644
--- a/lib/bucket_store/key_storage.rb
+++ b/lib/bucket_store/key_storage.rb
@@ -60,25 +60,27 @@ def download
     # @param [String] content The content to upload
     # @return [String] The final `key` where the content has been uploaded
     # @example Upload a file
-    #   BucketStore.for("inmemory://bucket/file.xml").upload("hello world")
+    #   BucketStore.for("inmemory://bucket/file.xml").upload!("hello world")
     def upload!(content)
-      raise ArgumentError, "Key cannot be empty" if key.empty?
-
-      BucketStore.logger.info(event: "key_storage.upload_started",
-                              **log_context)
-
-      start = BucketStore::Timing.monotonic_now
-      result = adapter.upload!(
-        bucket: bucket,
-        key: key,
-        content: content,
+      do_upload!(
+        StringIO.new(content),
+        "upload",
       )
+    end
 
-      BucketStore.logger.info(event: "key_storage.upload_finished",
-                              duration: BucketStore::Timing.monotonic_now - start,
-                              **log_context)
-
-      "#{adapter_type}://#{result[:bucket]}/#{result[:key]}"
+    # Uploads the given file like object to the reference key location.
+    #
+    # If the `key` already exists, its content will be replaced by the one in input.
+    #
+    # @param [IO / file like] file The content to upload
+    # @return [String] The final `key` where the content has been uploaded
+    # @example Upload a file
+    #   BucketStore.for("inmemory://bucket/file.xml").upload_file!(File.open("my_file.txt", "r"))
+    def upload_file!(file)
+      do_upload!(
+        file,
+        "upload_file",
+      )
     end
 
     # Lists all keys for the current adapter that have the reference key as prefix
@@ -167,5 +169,25 @@ def log_context
         adapter_type: adapter_type,
       }.compact
     end
+
+    def do_upload!(file, event_prefix)
+      raise ArgumentError, "Key cannot be empty" if key.empty?
+
+      BucketStore.logger.info(event: "key_storage.#{event_prefix}_started",
+                              **log_context)
+
+      start = BucketStore::Timing.monotonic_now
+      result = adapter.upload!(
+        bucket: bucket,
+        key: key,
+        file: file,
+      )
+
+      BucketStore.logger.info(event: "key_storage.#{event_prefix}_finished",
+                              duration: BucketStore::Timing.monotonic_now - start,
+                              **log_context)
+
+      "#{adapter_type}://#{result[:bucket]}/#{result[:key]}"
+    end
   end
 end
diff --git a/lib/bucket_store/s3.rb b/lib/bucket_store/s3.rb
index 393129a..37e223b 100644
--- a/lib/bucket_store/s3.rb
+++ b/lib/bucket_store/s3.rb
@@ -20,11 +20,11 @@ def initialize(open_timeout_seconds, read_timeout_seconds)
       )
     end
 
-    def upload!(bucket:, key:, content:)
+    def upload!(bucket:, key:, file:)
       storage.put_object(
         bucket: bucket,
         key: key,
-        body: content,
+        body: file,
       )
 
       {
diff --git a/spec/bucket_store/disk_spec.rb b/spec/bucket_store/disk_spec.rb
index e05a6e1..9aa650c 100644
--- a/spec/bucket_store/disk_spec.rb
+++ b/spec/bucket_store/disk_spec.rb
@@ -10,13 +10,16 @@
   let(:bucket) { "bucket" }
   let!(:base_dir) { Dir.mktmpdir("disk-adapter-test") }
 
+  let(:original_content) { "world" }
+  let(:file) { StringIO.new(original_content) }
+
   after do
     FileUtils.remove_entry(base_dir)
   end
 
   describe "#upload!" do
     it "uploads the given content" do
-      instance.upload!(bucket: bucket, key: "hello", content: "world")
+      instance.upload!(bucket: bucket, key: "hello", file: file)
 
       expect(instance.download(bucket: bucket, key: "hello")).to eq(
         bucket: bucket,
@@ -26,10 +29,10 @@
     end
 
     context "when uploading over a key that already exists" do
-      before { instance.upload!(bucket: bucket, key: "hello", content: "world") }
+      before { instance.upload!(bucket: bucket, key: "hello", file: file) }
 
       it "overrides the content" do
-        instance.upload!(bucket: bucket, key: "hello", content: "planet")
+        instance.upload!(bucket: bucket, key: "hello", file: StringIO.new("planet"))
 
         expect(instance.download(bucket: bucket, key: "hello")).to eq(
           bucket: bucket,
@@ -46,7 +49,7 @@
       end
 
       it "puts the content in the right directory" do
-        instance.upload!(bucket: bucket, key: "hello", content: "world")
+        instance.upload!(bucket: bucket, key: "hello", file: file)
 
         expect(instance.download(bucket: bucket, key: "hello")).to eq(
           bucket: bucket,
@@ -58,7 +61,7 @@
 
     context "when given a key with invalid chars" do
       it "sanitizes the filename" do
-        instance.upload!(bucket: bucket, key: "this is % invalid", content: "%%%%")
+        instance.upload!(bucket: bucket, key: "this is % invalid", file: StringIO.new("%%%%"))
 
         expect(instance.list(bucket: bucket, key: "", page_size: 1000).first).to match(
           bucket: bucket,
@@ -77,7 +80,7 @@
     end
 
     context "when the key has been uploaded" do
-      before { instance.upload!(bucket: bucket, key: "hello", content: "world") }
+      before { instance.upload!(bucket: bucket, key: "hello", file: file) }
 
       it "returns the uploaded content" do
         expect(instance.download(bucket: bucket, key: "hello")).to eq(
@@ -112,11 +115,11 @@
 
     context "when the bucket has some keys in it" do
       before do
-        instance.upload!(bucket: bucket, key: "2019-01/hello1", content: "world")
-        instance.upload!(bucket: bucket, key: "2019-01/hello2", content: "world")
-        instance.upload!(bucket: bucket, key: "2019-01/hello3", content: "world")
-        instance.upload!(bucket: bucket, key: "2019-02/hello", content: "world")
-        instance.upload!(bucket: bucket, key: "2019-03/hello", content: "world")
+        instance.upload!(bucket: bucket, key: "2019-01/hello1", file: file)
+        instance.upload!(bucket: bucket, key: "2019-01/hello2", file: file)
+        instance.upload!(bucket: bucket, key: "2019-01/hello3", file: file)
+        instance.upload!(bucket: bucket, key: "2019-02/hello", file: file)
+        instance.upload!(bucket: bucket, key: "2019-03/hello", file: file)
       end
 
       context "and we provide a matching prefix" do
@@ -159,7 +162,7 @@
   end
 
   describe "#delete!" do
-    before { instance.upload!(bucket: bucket, key: "hello", content: "world") }
+    before { instance.upload!(bucket: bucket, key: "hello", file: file) }
 
     it "deletes the given content" do
       expect(instance.delete!(bucket: bucket, key: "hello")).to eq(true)
diff --git a/spec/bucket_store/in_memory_spec.rb b/spec/bucket_store/in_memory_spec.rb
index de0551d..9160f8f 100644
--- a/spec/bucket_store/in_memory_spec.rb
+++ b/spec/bucket_store/in_memory_spec.rb
@@ -9,9 +9,12 @@
 
   let(:bucket) { "bucket" }
 
+  let(:original_content) { "world" }
+  let(:file) { StringIO.new(original_content) }
+
   describe "#upload!" do
     it "uploads the given content" do
-      instance.upload!(bucket: bucket, key: "hello", content: "world")
+      instance.upload!(bucket: bucket, key: "hello", file: file)
 
       expect(instance.download(bucket: bucket, key: "hello")).to eq(
         bucket: bucket,
@@ -21,10 +24,10 @@
     end
 
     context "when uploading over a key that already exists" do
-      before { instance.upload!(bucket: bucket, key: "hello", content: "world") }
+      before { instance.upload!(bucket: bucket, key: "hello", file: file) }
 
       it "overrides the content" do
-        instance.upload!(bucket: bucket, key: "hello", content: "planet")
+        instance.upload!(bucket: bucket, key: "hello", file: StringIO.new("planet"))
 
         expect(instance.download(bucket: bucket, key: "hello")).to eq(
           bucket: bucket,
@@ -44,7 +47,7 @@
     end
 
     context "when the key has been uploaded" do
-      before { instance.upload!(bucket: bucket, key: "hello", content: "world") }
+      before { instance.upload!(bucket: bucket, key: "hello", file: file) }
 
       it "returns the uploaded content" do
         expect(instance.download(bucket: bucket, key: "hello")).to eq(
@@ -71,11 +74,11 @@
 
     context "when the bucket has some keys in it" do
       before do
-        instance.upload!(bucket: bucket, key: "2019-01/hello1", content: "world")
-        instance.upload!(bucket: bucket, key: "2019-01/hello2", content: "world")
-        instance.upload!(bucket: bucket, key: "2019-01/hello3", content: "world")
-        instance.upload!(bucket: bucket, key: "2019-02/hello", content: "world")
-        instance.upload!(bucket: bucket, key: "2019-03/hello", content: "world")
+        instance.upload!(bucket: bucket, key: "2019-01/hello1", file: file)
+        instance.upload!(bucket: bucket, key: "2019-01/hello2", file: file)
+        instance.upload!(bucket: bucket, key: "2019-01/hello3", file: file)
+        instance.upload!(bucket: bucket, key: "2019-02/hello", file: file)
+        instance.upload!(bucket: bucket, key: "2019-03/hello", file: file)
       end
 
       context "and we provide a matching prefix" do
@@ -124,11 +127,11 @@
 
     context "when there's some content" do
       before do
-        instance.upload!(bucket: bucket, key: "2019-01/hello1", content: "world")
-        instance.upload!(bucket: bucket, key: "2019-01/hello2", content: "world")
-        instance.upload!(bucket: bucket, key: "2019-01/hello3", content: "world")
-        instance.upload!(bucket: bucket2, key: "2019-02/hello", content: "world")
-        instance.upload!(bucket: bucket2, key: "2019-03/hello", content: "world")
+        instance.upload!(bucket: bucket, key: "2019-01/hello1", file: file)
+        instance.upload!(bucket: bucket, key: "2019-01/hello2", file: file)
+        instance.upload!(bucket: bucket, key: "2019-01/hello3", file: file)
+        instance.upload!(bucket: bucket2, key: "2019-02/hello", file: file)
+        instance.upload!(bucket: bucket2, key: "2019-03/hello", file: file)
       end
 
       it "resets all the buckets" do
@@ -149,7 +152,7 @@
   end
 
   describe "#delete!" do
-    before { instance.upload!(bucket: bucket, key: "hello", content: "world") }
+    before { instance.upload!(bucket: bucket, key: "hello", file: file) }
 
     it "deletes the given content" do
       expect(instance.delete!(bucket: bucket, key: "hello")).to eq(true)
diff --git a/spec/bucket_store/key_storage_spec.rb b/spec/bucket_store/key_storage_spec.rb
index ef3c993..5e7ed09 100644
--- a/spec/bucket_store/key_storage_spec.rb
+++ b/spec/bucket_store/key_storage_spec.rb
@@ -117,6 +117,33 @@ def build_for(key)
     end
   end
 
+  describe "#upload_file!" do
+    let(:file) { StringIO.new("hello") }
+
+    it "uploads the given file" do
+      expect(build_for("inmemory://bucket/file1").upload_file!(file)).
+        to eq("inmemory://bucket/file1")
+    end
+
+    it "logs the operation" do
+      expect(BucketStore.logger).to receive(:info).with(
+        hash_including(event: "key_storage.upload_file_started"),
+      )
+      expect(BucketStore.logger).to receive(:info).with(
+        hash_including(event: "key_storage.upload_file_finished"),
+      )
+
+      build_for("inmemory://bucket/file1").upload_file!(file)
+    end
+
+    context "when we try to upload a bucket" do
+      it "raises an error" do
+        expect { build_for("inmemory://bucket").upload_file!(file) }.
+          to raise_error(ArgumentError, /key cannot be empty/i)
+      end
+    end
+  end
+
   describe "#delete!" do
     before do
       build_for("inmemory://bucket/file1").upload!("content1")

From 0e3bb1412856ba02ffd5f3bfc554b86647a4881b Mon Sep 17 00:00:00 2001
From: Andrew Morton <andrewmorton@gocardless.com>
Date: Tue, 18 Apr 2023 15:09:04 +0100
Subject: [PATCH 02/17] Unify `upload!` API

[Comment](https://github.com/gocardless/bucket-store/pull/64#discussion_r1170018685)

The approach taken here is to unify the `upload!` and `upload_file!` APIs into a single `upload!` API that will handle both Strings and file like objects implicitly for ease of us.
---
 lib/bucket_store/key_storage.rb       | 66 +++++++++++----------------
 spec/bucket_store/key_storage_spec.rb | 40 ++++++++--------
 2 files changed, 46 insertions(+), 60 deletions(-)

diff --git a/lib/bucket_store/key_storage.rb b/lib/bucket_store/key_storage.rb
index b912300..3ecc302 100644
--- a/lib/bucket_store/key_storage.rb
+++ b/lib/bucket_store/key_storage.rb
@@ -53,34 +53,40 @@ def download
       result
     end
 
-    # Uploads the given content to the reference key location.
+    # Uploads the given file to the reference key location.
+    # If the File is a file like object then upload as is.
+    # If the file variable is actually a string then treat is as the file
+    # contents and upload as is.
     #
     # If the `key` already exists, its content will be replaced by the one in input.
     #
-    # @param [String] content The content to upload
+    # @param [String or File like object] file The file like object to upload or a String
+    #         with the contents
     # @return [String] The final `key` where the content has been uploaded
     # @example Upload a file
     #   BucketStore.for("inmemory://bucket/file.xml").upload!("hello world")
-    def upload!(content)
-      do_upload!(
-        StringIO.new(content),
-        "upload",
-      )
-    end
+    def upload!(file)
+      raise ArgumentError, "Key cannot be empty" if key.empty?
 
-    # Uploads the given file like object to the reference key location.
-    #
-    # If the `key` already exists, its content will be replaced by the one in input.
-    #
-    # @param [IO / file like] file The content to upload
-    # @return [String] The final `key` where the content has been uploaded
-    # @example Upload a file
-    #   BucketStore.for("inmemory://bucket/file.xml").upload_file!(File.open("my_file.txt", "r"))
-    def upload_file!(file)
-      do_upload!(
-        file,
-        "upload_file",
+      if file.instance_of?(String)
+        file = StringIO.new(file)
+      end
+
+      BucketStore.logger.info(event: "key_storage.upload_started",
+                              **log_context)
+
+      start = BucketStore::Timing.monotonic_now
+      result = adapter.upload!(
+        bucket: bucket,
+        key: key,
+        file: file,
       )
+
+      BucketStore.logger.info(event: "key_storage.upload_finished",
+                              duration: BucketStore::Timing.monotonic_now - start,
+                              **log_context)
+
+      "#{adapter_type}://#{result[:bucket]}/#{result[:key]}"
     end
 
     # Lists all keys for the current adapter that have the reference key as prefix
@@ -169,25 +175,5 @@ def log_context
         adapter_type: adapter_type,
       }.compact
     end
-
-    def do_upload!(file, event_prefix)
-      raise ArgumentError, "Key cannot be empty" if key.empty?
-
-      BucketStore.logger.info(event: "key_storage.#{event_prefix}_started",
-                              **log_context)
-
-      start = BucketStore::Timing.monotonic_now
-      result = adapter.upload!(
-        bucket: bucket,
-        key: key,
-        file: file,
-      )
-
-      BucketStore.logger.info(event: "key_storage.#{event_prefix}_finished",
-                              duration: BucketStore::Timing.monotonic_now - start,
-                              **log_context)
-
-      "#{adapter_type}://#{result[:bucket]}/#{result[:key]}"
-    end
   end
 end
diff --git a/spec/bucket_store/key_storage_spec.rb b/spec/bucket_store/key_storage_spec.rb
index 5e7ed09..2fc313c 100644
--- a/spec/bucket_store/key_storage_spec.rb
+++ b/spec/bucket_store/key_storage_spec.rb
@@ -115,31 +115,31 @@ def build_for(key)
           to raise_error(ArgumentError, /key cannot be empty/i)
       end
     end
-  end
 
-  describe "#upload_file!" do
-    let(:file) { StringIO.new("hello") }
+    context "when given a file like object" do
+      let(:file) { StringIO.new("hello") }
 
-    it "uploads the given file" do
-      expect(build_for("inmemory://bucket/file1").upload_file!(file)).
-        to eq("inmemory://bucket/file1")
-    end
+      it "uploads the given file" do
+        expect(build_for("inmemory://bucket/file1").upload!(file)).
+          to eq("inmemory://bucket/file1")
+      end
 
-    it "logs the operation" do
-      expect(BucketStore.logger).to receive(:info).with(
-        hash_including(event: "key_storage.upload_file_started"),
-      )
-      expect(BucketStore.logger).to receive(:info).with(
-        hash_including(event: "key_storage.upload_file_finished"),
-      )
+      it "logs the operation" do
+        expect(BucketStore.logger).to receive(:info).with(
+          hash_including(event: "key_storage.upload_started"),
+        )
+        expect(BucketStore.logger).to receive(:info).with(
+          hash_including(event: "key_storage.upload_finished"),
+        )
 
-      build_for("inmemory://bucket/file1").upload_file!(file)
-    end
+        build_for("inmemory://bucket/file1").upload!(file)
+      end
 
-    context "when we try to upload a bucket" do
-      it "raises an error" do
-        expect { build_for("inmemory://bucket").upload_file!(file) }.
-          to raise_error(ArgumentError, /key cannot be empty/i)
+      context "when we try to upload a bucket" do
+        it "raises an error" do
+          expect { build_for("inmemory://bucket").upload!(file) }.
+            to raise_error(ArgumentError, /key cannot be empty/i)
+        end
       end
     end
   end

From ffdb92877b67ff0509f1ac132713db33491bd61f Mon Sep 17 00:00:00 2001
From: Ivan Giuliani <ivan@gocardless.com>
Date: Tue, 11 Apr 2023 16:39:37 +0100
Subject: [PATCH 03/17] Expose minio admin console

---
 docker-compose.yml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/docker-compose.yml b/docker-compose.yml
index eed352c..dbb3d0d 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -12,6 +12,7 @@ services:
       MINIO_REGION_NAME: us-east-1
     ports:
       - "9000:9000"
+      - "9001:9001"
   gcp-simulator:
     image: fsouza/fake-gcs-server
     hostname: gcp

From 7cd59867cfbc74e13b8ef1f30bc034f5d6154613 Mon Sep 17 00:00:00 2001
From: Ivan Giuliani <ivan@gocardless.com>
Date: Tue, 11 Apr 2023 17:29:03 +0100
Subject: [PATCH 04/17] Delete all files in the bucket before starting

Make sure integration tests always start with a clean slate, regardless
of how the previous test run has ended. Note that this can also fail,
but it's a good test in itself...
---
 spec/bucket_store_integration_spec.rb | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/spec/bucket_store_integration_spec.rb b/spec/bucket_store_integration_spec.rb
index c808615..c0c206c 100644
--- a/spec/bucket_store_integration_spec.rb
+++ b/spec/bucket_store_integration_spec.rb
@@ -24,8 +24,11 @@
   end
 
   shared_examples "adapter integration" do |base_bucket_uri|
-    # This is presented as a single idempotent test as otherwise resetting state between execution
-    # makes things very complicated with no huge benefits.
+    before do
+      described_class.for(base_bucket_uri).list.each do |path|
+        described_class.for(path).delete!
+      end
+    end
 
     it "has a consistent interface" do
       # Write 201 files

From 8c93b601112b6ba989d2ffb8b48b74ae2066da09 Mon Sep 17 00:00:00 2001
From: Ivan Giuliani <ivan@gocardless.com>
Date: Wed, 12 Apr 2023 10:17:42 +0100
Subject: [PATCH 05/17] Include the base bucket uri in the context name

---
 spec/bucket_store_integration_spec.rb | 92 ++++++++++++++-------------
 1 file changed, 47 insertions(+), 45 deletions(-)

diff --git a/spec/bucket_store_integration_spec.rb b/spec/bucket_store_integration_spec.rb
index c0c206c..0adfbaf 100644
--- a/spec/bucket_store_integration_spec.rb
+++ b/spec/bucket_store_integration_spec.rb
@@ -24,54 +24,56 @@
   end
 
   shared_examples "adapter integration" do |base_bucket_uri|
-    before do
-      described_class.for(base_bucket_uri).list.each do |path|
-        described_class.for(path).delete!
-      end
-    end
-
-    it "has a consistent interface" do
-      # Write 201 files
-      file_list = []
-      201.times do |i|
-        filename = "file#{(i + 1).to_s.rjust(3, '0')}.txt"
-        file_list << filename
-
-        # the body of the file is the filename itself
-        described_class.for("#{base_bucket_uri}/prefix/#{filename}").upload!(filename)
-      end
-
-      # Add some files with spaces
-      described_class.for("#{base_bucket_uri}/prefix/i have a space.txt").
-        upload!("i have a space.txt")
-      described_class.for("#{base_bucket_uri}/prefix/another space.txt").
-        upload!("another space.txt")
-
-      file_list << "i have a space.txt"
-      file_list << "another space.txt"
-
-      # List with prefix should only return the matching files
-      expect(described_class.for("#{base_bucket_uri}/prefix/file1").list.to_a.size).to eq(100)
-      expect(described_class.for("#{base_bucket_uri}/prefix/file2").list.to_a.size).to eq(2)
-      expect(described_class.for("#{base_bucket_uri}/prefix/").list.to_a.size).to eq(203)
-
-      # List (without prefixes) should return everything
-      expect(described_class.for(base_bucket_uri.to_s).list.to_a).
-        to match_array(file_list.map { |filename| "#{base_bucket_uri}/prefix/#{filename}" })
-
-      # We know the content of the file, we can check `.download` returns it as expected
-      all_files = file_list.map do |filename|
-        [filename, "#{base_bucket_uri}/prefix/#{filename}"]
-      end
-      all_files.each do |content, key|
-        expect(described_class.for(key).download[:content]).to eq(content)
+    context "using #{base_bucket_uri}" do
+      before do
+        described_class.for(base_bucket_uri).list.each do |path|
+          described_class.for(path).delete!
+        end
       end
 
-      # Delete all the files, the bucket should be empty afterwards
-      file_list.map { |filename| "#{base_bucket_uri}/prefix/#{filename}" }.each do |key|
-        described_class.for(key).delete!
+      it "has a consistent interface" do
+        # Write 201 files
+        file_list = []
+        201.times do |i|
+          filename = "file#{(i + 1).to_s.rjust(3, '0')}.txt"
+          file_list << filename
+
+          # the body of the file is the filename itself
+          described_class.for("#{base_bucket_uri}/prefix/#{filename}").upload!(filename)
+        end
+
+        # Add some files with spaces
+        described_class.for("#{base_bucket_uri}/prefix/i have a space.txt").
+          upload!("i have a space.txt")
+        described_class.for("#{base_bucket_uri}/prefix/another space.txt").
+          upload!("another space.txt")
+
+        file_list << "i have a space.txt"
+        file_list << "another space.txt"
+
+        # List with prefix should only return the matching files
+        expect(described_class.for("#{base_bucket_uri}/prefix/file1").list.to_a.size).to eq(100)
+        expect(described_class.for("#{base_bucket_uri}/prefix/file2").list.to_a.size).to eq(2)
+        expect(described_class.for("#{base_bucket_uri}/prefix/").list.to_a.size).to eq(203)
+
+        # List (without prefixes) should return everything
+        expect(described_class.for(base_bucket_uri.to_s).list.to_a).
+          to match_array(file_list.map { |filename| "#{base_bucket_uri}/prefix/#{filename}" })
+
+        # We know the content of the file, we can check `.download` returns it as expected
+        all_files = file_list.map do |filename|
+          [filename, "#{base_bucket_uri}/prefix/#{filename}"]
+        end
+        all_files.each do |content, key|
+          expect(described_class.for(key).download[:content]).to eq(content)
+        end
+
+        # Delete all the files, the bucket should be empty afterwards
+        described_class.for(base_bucket_uri.to_s).list.each do |key|
+          described_class.for(key).delete!
+        end
+        expect(described_class.for(base_bucket_uri.to_s).list.to_a.size).to eq(0)
       end
-      expect(described_class.for(base_bucket_uri.to_s).list.to_a.size).to eq(0)
     end
   end
 

From 90691a233299a3815944cd8aeb56f5e08f6b9a42 Mon Sep 17 00:00:00 2001
From: Andrew Morton <andrewmorton@gocardless.com>
Date: Tue, 18 Apr 2023 17:09:55 +0100
Subject: [PATCH 06/17] Change download method on adapters to be based on
 files.

This changes the adapters such that they think entirely in terms of
files for both uploads and downloads.

Both S3 and GCS support this notion / will stream internally.
---
 lib/bucket_store/disk.rb            | 10 ++-----
 lib/bucket_store/gcs.rb             | 15 +++-------
 lib/bucket_store/in_memory.rb       |  8 ++----
 lib/bucket_store/key_storage.rb     |  9 ++++--
 lib/bucket_store/s3.rb              | 11 ++------
 spec/bucket_store/disk_spec.rb      | 44 +++++++++++++----------------
 spec/bucket_store/in_memory_spec.rb | 41 +++++++++++++--------------
 7 files changed, 58 insertions(+), 80 deletions(-)

diff --git a/lib/bucket_store/disk.rb b/lib/bucket_store/disk.rb
index 3379d4c..6758e31 100644
--- a/lib/bucket_store/disk.rb
+++ b/lib/bucket_store/disk.rb
@@ -23,13 +23,9 @@ def upload!(bucket:, key:, file:)
       }
     end
 
-    def download(bucket:, key:)
-      File.open(key_path(bucket, key), "r") do |file|
-        {
-          bucket: bucket,
-          key: key,
-          content: file.read,
-        }
+    def download(bucket:, key:, file:)
+      File.open(key_path(bucket, key), "r") do |saved_file|
+        file.write(saved_file.read)
       end
     end
 
diff --git a/lib/bucket_store/gcs.rb b/lib/bucket_store/gcs.rb
index 30bad94..8a3a829 100644
--- a/lib/bucket_store/gcs.rb
+++ b/lib/bucket_store/gcs.rb
@@ -42,17 +42,10 @@ def upload!(bucket:, key:, file:)
       }
     end
 
-    def download(bucket:, key:)
-      file = get_bucket(bucket).file(key)
-
-      buffer = StringIO.new
-      file.download(buffer)
-
-      {
-        bucket: bucket,
-        key: key,
-        content: buffer.string,
-      }
+    def download(bucket:, key:, file:)
+      get_bucket(bucket).
+        file(key).
+        download(file)
     end
 
     def list(bucket:, key:, page_size:)
diff --git a/lib/bucket_store/in_memory.rb b/lib/bucket_store/in_memory.rb
index 3a47611..5698ccf 100644
--- a/lib/bucket_store/in_memory.rb
+++ b/lib/bucket_store/in_memory.rb
@@ -33,12 +33,8 @@ def upload!(bucket:, key:, file:)
       }
     end
 
-    def download(bucket:, key:)
-      {
-        bucket: bucket,
-        key: key,
-        content: @buckets[bucket].fetch(key),
-      }
+    def download(bucket:, key:, file:)
+      file.write(@buckets[bucket].fetch(key))
     end
 
     def list(bucket:, key:, page_size:)
diff --git a/lib/bucket_store/key_storage.rb b/lib/bucket_store/key_storage.rb
index 3ecc302..439f076 100644
--- a/lib/bucket_store/key_storage.rb
+++ b/lib/bucket_store/key_storage.rb
@@ -45,12 +45,17 @@ def download
       BucketStore.logger.info(event: "key_storage.download_started")
 
       start = BucketStore::Timing.monotonic_now
-      result = adapter.download(bucket: bucket, key: key)
+      buffer = StringIO.new
+      adapter.download(bucket: bucket, key: key, file: buffer)
 
       BucketStore.logger.info(event: "key_storage.download_finished",
                               duration: BucketStore::Timing.monotonic_now - start)
 
-      result
+      {
+        bucket: bucket,
+        key: key,
+        content: buffer.string,
+      }
     end
 
     # Uploads the given file to the reference key location.
diff --git a/lib/bucket_store/s3.rb b/lib/bucket_store/s3.rb
index 37e223b..89b5ef3 100644
--- a/lib/bucket_store/s3.rb
+++ b/lib/bucket_store/s3.rb
@@ -33,17 +33,12 @@ def upload!(bucket:, key:, file:)
       }
     end
 
-    def download(bucket:, key:)
-      file = storage.get_object(
+    def download(bucket:, key:, file:)
+      storage.get_object(
+        response_target: file,
         bucket: bucket,
         key: key,
       )
-
-      {
-        bucket: bucket,
-        key: key,
-        content: file.body.read,
-      }
     end
 
     def list(bucket:, key:, page_size:)
diff --git a/spec/bucket_store/disk_spec.rb b/spec/bucket_store/disk_spec.rb
index 9aa650c..a90e6bb 100644
--- a/spec/bucket_store/disk_spec.rb
+++ b/spec/bucket_store/disk_spec.rb
@@ -12,6 +12,8 @@
 
   let(:original_content) { "world" }
   let(:file) { StringIO.new(original_content) }
+  let(:output_file) { StringIO.new }
+  let(:downloaded_content) { output_file.string }
 
   after do
     FileUtils.remove_entry(base_dir)
@@ -21,11 +23,9 @@
     it "uploads the given content" do
       instance.upload!(bucket: bucket, key: "hello", file: file)
 
-      expect(instance.download(bucket: bucket, key: "hello")).to eq(
-        bucket: bucket,
-        key: "hello",
-        content: "world",
-      )
+      instance.download(bucket: bucket, key: "hello", file: output_file)
+
+      expect(downloaded_content).to eq("world")
     end
 
     context "when uploading over a key that already exists" do
@@ -34,11 +34,9 @@
       it "overrides the content" do
         instance.upload!(bucket: bucket, key: "hello", file: StringIO.new("planet"))
 
-        expect(instance.download(bucket: bucket, key: "hello")).to eq(
-          bucket: bucket,
-          key: "hello",
-          content: "planet",
-        )
+        instance.download(bucket: bucket, key: "hello", file: output_file)
+
+        expect(downloaded_content).to eq("planet")
       end
     end
 
@@ -51,11 +49,9 @@
       it "puts the content in the right directory" do
         instance.upload!(bucket: bucket, key: "hello", file: file)
 
-        expect(instance.download(bucket: bucket, key: "hello")).to eq(
-          bucket: bucket,
-          key: "hello",
-          content: "world",
-        )
+        instance.download(bucket: bucket, key: "hello", file: output_file)
+
+        expect(downloaded_content).to eq("world")
       end
     end
 
@@ -74,7 +70,7 @@
   describe "#download" do
     context "when the key does not exist" do
       it "raises an error" do
-        expect { instance.download(bucket: bucket, key: "unknown") }.
+        expect { instance.download(bucket: bucket, key: "unknown", file: output_file) }.
           to raise_error(Errno::ENOENT, /No such file or directory/)
       end
     end
@@ -83,16 +79,14 @@
       before { instance.upload!(bucket: bucket, key: "hello", file: file) }
 
       it "returns the uploaded content" do
-        expect(instance.download(bucket: bucket, key: "hello")).to eq(
-          bucket: bucket,
-          key: "hello",
-          content: "world",
-        )
+        instance.download(bucket: bucket, key: "hello", file: output_file)
+
+        expect(downloaded_content).to eq("world")
       end
 
       context "but we try to fetch it from a different bucket" do
         it "raises an error" do
-          expect { instance.download(bucket: "anotherbucket", key: "hello") }.
+          expect { instance.download(bucket: "anotherbucket", key: "hello", file: output_file) }.
             to raise_error(Errno::ENOENT, /No such file or directory/)
         end
       end
@@ -100,7 +94,9 @@
 
     context "when attempting to traverse outside of the bucket boundaries" do
       it "raises an error" do
-        expect { instance.download(bucket: bucket, key: "../../../../../../etc/passwd") }.
+        expect do
+          instance.download(bucket: bucket, key: "../../../../../../etc/passwd", file: output_file)
+        end.
           to raise_error(ArgumentError, /Directory traversal out of bucket boundaries/)
       end
     end
@@ -167,7 +163,7 @@
     it "deletes the given content" do
       expect(instance.delete!(bucket: bucket, key: "hello")).to eq(true)
 
-      expect { instance.download(bucket: bucket, key: "hello").download }.
+      expect { instance.download(bucket: bucket, key: "hello", file: output_file) }.
         to raise_error(Errno::ENOENT, /No such file or directory/)
     end
   end
diff --git a/spec/bucket_store/in_memory_spec.rb b/spec/bucket_store/in_memory_spec.rb
index 9160f8f..1914991 100644
--- a/spec/bucket_store/in_memory_spec.rb
+++ b/spec/bucket_store/in_memory_spec.rb
@@ -11,16 +11,16 @@
 
   let(:original_content) { "world" }
   let(:file) { StringIO.new(original_content) }
+  let(:output_file) { StringIO.new }
+  let(:downloaded_content) { output_file.string }
 
   describe "#upload!" do
     it "uploads the given content" do
       instance.upload!(bucket: bucket, key: "hello", file: file)
 
-      expect(instance.download(bucket: bucket, key: "hello")).to eq(
-        bucket: bucket,
-        key: "hello",
-        content: "world",
-      )
+      instance.download(bucket: bucket, key: "hello", file: output_file)
+
+      expect(downloaded_content).to eq("world")
     end
 
     context "when uploading over a key that already exists" do
@@ -29,11 +29,9 @@
       it "overrides the content" do
         instance.upload!(bucket: bucket, key: "hello", file: StringIO.new("planet"))
 
-        expect(instance.download(bucket: bucket, key: "hello")).to eq(
-          bucket: bucket,
-          key: "hello",
-          content: "planet",
-        )
+        instance.download(bucket: bucket, key: "hello", file: output_file)
+
+        expect(downloaded_content).to eq("planet")
       end
     end
   end
@@ -41,7 +39,7 @@
   describe "#download" do
     context "when the key does not exist" do
       it "raises an error" do
-        expect { instance.download(bucket: bucket, key: "unknown") }.
+        expect { instance.download(bucket: bucket, key: "unknown", file: output_file) }.
           to raise_error(KeyError, /key not found/)
       end
     end
@@ -50,11 +48,8 @@
       before { instance.upload!(bucket: bucket, key: "hello", file: file) }
 
       it "returns the uploaded content" do
-        expect(instance.download(bucket: bucket, key: "hello")).to eq(
-          bucket: bucket,
-          key: "hello",
-          content: "world",
-        )
+        instance.download(bucket: bucket, key: "hello", file: output_file)
+        expect(downloaded_content).to eq("world")
       end
 
       context "but we try to fetch it from a different bucket" do
@@ -137,15 +132,15 @@
       it "resets all the buckets" do
         instance.reset!
 
-        expect { instance.download(bucket: bucket, key: "2019-01/hello1") }.
+        expect { instance.download(bucket: bucket, key: "2019-01/hello1", file: output_file) }.
           to raise_error(KeyError, /key not found/)
-        expect { instance.download(bucket: bucket, key: "2019-01/hello2") }.
+        expect { instance.download(bucket: bucket, key: "2019-01/hello2", file: output_file) }.
           to raise_error(KeyError, /key not found/)
-        expect { instance.download(bucket: bucket, key: "2019-01/hello3") }.
+        expect { instance.download(bucket: bucket, key: "2019-01/hello3", file: output_file) }.
           to raise_error(KeyError, /key not found/)
-        expect { instance.download(bucket: bucket2, key: "2019-02/hello") }.
+        expect { instance.download(bucket: bucket2, key: "2019-02/hello", file: output_file) }.
           to raise_error(KeyError, /key not found/)
-        expect { instance.download(bucket: bucket2, key: "2019-03/hello") }.
+        expect { instance.download(bucket: bucket2, key: "2019-03/hello", file: output_file) }.
           to raise_error(KeyError, /key not found/)
       end
     end
@@ -157,7 +152,9 @@
     it "deletes the given content" do
       expect(instance.delete!(bucket: bucket, key: "hello")).to eq(true)
 
-      expect { instance.download(bucket: bucket, key: "hello").download }.to raise_error(KeyError)
+      expect do
+        instance.download(bucket: bucket, key: "hello", file: output_file)
+      end.to raise_error(KeyError)
     end
   end
 end

From 9ab5e333e84882cd16e44d1bf92baf3cc4c03a71 Mon Sep 17 00:00:00 2001
From: Andrew Morton <andrewmorton@gocardless.com>
Date: Tue, 18 Apr 2023 17:13:36 +0100
Subject: [PATCH 07/17] Change upload to no longer silently convert Strings to
 StringIO

The interface for this will change to add a `stream` version, similar to https://github.com/gocardless/bucket-store/pull/63.

To prepare for that we simplify the `upload` method
---
 lib/bucket_store/key_storage.rb       | 14 +++-----------
 spec/bucket_store/key_storage_spec.rb | 27 ---------------------------
 2 files changed, 3 insertions(+), 38 deletions(-)

diff --git a/lib/bucket_store/key_storage.rb b/lib/bucket_store/key_storage.rb
index 439f076..17709a7 100644
--- a/lib/bucket_store/key_storage.rb
+++ b/lib/bucket_store/key_storage.rb
@@ -59,24 +59,16 @@ def download
     end
 
     # Uploads the given file to the reference key location.
-    # If the File is a file like object then upload as is.
-    # If the file variable is actually a string then treat is as the file
-    # contents and upload as is.
     #
     # If the `key` already exists, its content will be replaced by the one in input.
     #
-    # @param [String or File like object] file The file like object to upload or a String
-    #         with the contents
+    # @param [String] content Contents of the file
     # @return [String] The final `key` where the content has been uploaded
     # @example Upload a file
     #   BucketStore.for("inmemory://bucket/file.xml").upload!("hello world")
-    def upload!(file)
+    def upload!(content)
       raise ArgumentError, "Key cannot be empty" if key.empty?
 
-      if file.instance_of?(String)
-        file = StringIO.new(file)
-      end
-
       BucketStore.logger.info(event: "key_storage.upload_started",
                               **log_context)
 
@@ -84,7 +76,7 @@ def upload!(file)
       result = adapter.upload!(
         bucket: bucket,
         key: key,
-        file: file,
+        file: StringIO.new(content),
       )
 
       BucketStore.logger.info(event: "key_storage.upload_finished",
diff --git a/spec/bucket_store/key_storage_spec.rb b/spec/bucket_store/key_storage_spec.rb
index 2fc313c..ef3c993 100644
--- a/spec/bucket_store/key_storage_spec.rb
+++ b/spec/bucket_store/key_storage_spec.rb
@@ -115,33 +115,6 @@ def build_for(key)
           to raise_error(ArgumentError, /key cannot be empty/i)
       end
     end
-
-    context "when given a file like object" do
-      let(:file) { StringIO.new("hello") }
-
-      it "uploads the given file" do
-        expect(build_for("inmemory://bucket/file1").upload!(file)).
-          to eq("inmemory://bucket/file1")
-      end
-
-      it "logs the operation" do
-        expect(BucketStore.logger).to receive(:info).with(
-          hash_including(event: "key_storage.upload_started"),
-        )
-        expect(BucketStore.logger).to receive(:info).with(
-          hash_including(event: "key_storage.upload_finished"),
-        )
-
-        build_for("inmemory://bucket/file1").upload!(file)
-      end
-
-      context "when we try to upload a bucket" do
-        it "raises an error" do
-          expect { build_for("inmemory://bucket").upload!(file) }.
-            to raise_error(ArgumentError, /key cannot be empty/i)
-        end
-      end
-    end
   end
 
   describe "#delete!" do

From f10ae95710b632b523fc213936a199608dfcf3dd Mon Sep 17 00:00:00 2001
From: Andrew Morton <andrewmorton@gocardless.com>
Date: Tue, 18 Apr 2023 17:51:40 +0100
Subject: [PATCH 08/17] Add KeyStreamer interface for uploads and downloads.

Completely stolen from https://github.com/gocardless/bucket-store/pull/63.

The KeyStreamer expects uploads / downloads to be expressed in terms of IO operations.

For example a download is actually `download into this IO` and upload is `upload the content from this file`.

Uploads / downloads of Strings are considered a special case of this. However is distinct enough that we have streaming be it's own API.
---
 lib/bucket_store/key_storage.rb       | 132 ++++++++++++++++++--------
 spec/bucket_store/key_storage_spec.rb |  92 +++++++++++++++++-
 2 files changed, 182 insertions(+), 42 deletions(-)

diff --git a/lib/bucket_store/key_storage.rb b/lib/bucket_store/key_storage.rb
index 17709a7..1141822 100644
--- a/lib/bucket_store/key_storage.rb
+++ b/lib/bucket_store/key_storage.rb
@@ -15,6 +15,88 @@ class KeyStorage
       disk: Disk,
     }.freeze
 
+    # Defines a streaming interface for download and upload operations.
+    #
+    # Note that individual adapters may require additional configuration for the correct
+    # behavior of the streaming interface.
+    class KeyStreamer
+      attr_reader :bucket, :key, :adapter, :adapter_type
+
+      def initialize(adapter:, adapter_type:, bucket:, key:)
+        @adapter = adapter
+        @adapter_type = adapter_type
+        @bucket = bucket
+        @key = key
+      end
+
+      # Streams the content of the reference key into a File like object
+      #
+      # @return hash containing the bucket, the key and file like object passed in as input
+      #
+      # @see KeyStorage#download
+      # @example Download a key
+      #   buffer = StringIO.new
+      #   BucketStore.for("inmemory://bucket/file.xml").stream.download(file: buffer)
+      #   buffer.string == "Imagine I'm a 2GB file"
+      def download(file:)
+        BucketStore.logger.info(event: "key_storage.stream.download_started")
+
+        start = BucketStore::Timing.monotonic_now
+        adapter.download(
+          bucket: bucket,
+          key: key,
+          file: file,
+        )
+
+        BucketStore.logger.info(event: "key_storage.stream.download_finished",
+                                duration: BucketStore::Timing.monotonic_now - start)
+
+        {
+          bucket: bucket,
+          key: key,
+          file: file,
+        }
+      end
+
+      # Performs a streaming upload to the backing object store
+      #
+      # @return the generated key for the new object
+      #
+      # @see KeyStorage#upload!
+      # @example Upload a key
+      #   buffer = StringIO.new("Imagine I'm a 2GB file")
+      #   BucketStore.for("inmemory://bucket/file.xml").stream.upload!(file: buffer)
+      def upload!(file:)
+        raise ArgumentError, "Key cannot be empty" if key.empty?
+
+        BucketStore.logger.info(event: "key_storage.stream.upload_started",
+                                **log_context)
+
+        start = BucketStore::Timing.monotonic_now
+        adapter.upload!(
+          bucket: bucket,
+          key: key,
+          file: file,
+        )
+
+        BucketStore.logger.info(event: "key_storage.stream.upload_finished",
+                                duration: BucketStore::Timing.monotonic_now - start,
+                                **log_context)
+
+        "#{adapter_type}://#{bucket}/#{key}"
+      end
+
+      private
+
+      def log_context
+        {
+          bucket: bucket,
+          key: key,
+          adapter_type: adapter_type,
+        }.compact
+      end
+    end
+
     attr_reader :bucket, :key, :adapter_type
 
     def initialize(adapter:, bucket:, key:)
@@ -40,22 +122,11 @@ def filename
     # @example Download a key
     #   BucketStore.for("inmemory://bucket/file.xml").download
     def download
-      raise ArgumentError, "Key cannot be empty" if key.empty?
-
-      BucketStore.logger.info(event: "key_storage.download_started")
-
-      start = BucketStore::Timing.monotonic_now
       buffer = StringIO.new
-      adapter.download(bucket: bucket, key: key, file: buffer)
-
-      BucketStore.logger.info(event: "key_storage.download_finished",
-                              duration: BucketStore::Timing.monotonic_now - start)
-
-      {
-        bucket: bucket,
-        key: key,
-        content: buffer.string,
-      }
+      stream.download(file: buffer).tap do |result|
+        result.delete(:file)
+        result[:content] = buffer.string
+      end
     end
 
     # Uploads the given file to the reference key location.
@@ -67,23 +138,16 @@ def download
     # @example Upload a file
     #   BucketStore.for("inmemory://bucket/file.xml").upload!("hello world")
     def upload!(content)
-      raise ArgumentError, "Key cannot be empty" if key.empty?
-
-      BucketStore.logger.info(event: "key_storage.upload_started",
-                              **log_context)
-
-      start = BucketStore::Timing.monotonic_now
-      result = adapter.upload!(
-        bucket: bucket,
-        key: key,
-        file: StringIO.new(content),
-      )
+      stream.upload!(file: StringIO.new(content))
+    end
 
-      BucketStore.logger.info(event: "key_storage.upload_finished",
-                              duration: BucketStore::Timing.monotonic_now - start,
-                              **log_context)
+    # Returns an interface for streaming operations
+    #
+    # @return [KeyStreamer] An interface for streaming operations
+    def stream
+      raise ArgumentError, "Key cannot be empty" if key.empty?
 
-      "#{adapter_type}://#{result[:bucket]}/#{result[:key]}"
+      KeyStreamer.new(adapter: adapter, adapter_type: adapter_type, bucket: bucket, key: key)
     end
 
     # Lists all keys for the current adapter that have the reference key as prefix
@@ -164,13 +228,5 @@ def exists?
     private
 
     attr_reader :adapter
-
-    def log_context
-      {
-        bucket: bucket,
-        key: key,
-        adapter_type: adapter_type,
-      }.compact
-    end
   end
 end
diff --git a/spec/bucket_store/key_storage_spec.rb b/spec/bucket_store/key_storage_spec.rb
index ef3c993..8fd8814 100644
--- a/spec/bucket_store/key_storage_spec.rb
+++ b/spec/bucket_store/key_storage_spec.rb
@@ -75,10 +75,10 @@ def build_for(key)
 
     it "logs the operation" do
       expect(BucketStore.logger).to receive(:info).with(
-        hash_including(event: "key_storage.download_started"),
+        hash_including(event: "key_storage.stream.download_started"),
       )
       expect(BucketStore.logger).to receive(:info).with(
-        hash_including(event: "key_storage.download_finished"),
+        hash_including(event: "key_storage.stream.download_finished"),
       )
 
       build_for("inmemory://bucket/file1").download
@@ -100,10 +100,10 @@ def build_for(key)
 
     it "logs the operation" do
       expect(BucketStore.logger).to receive(:info).with(
-        hash_including(event: "key_storage.upload_started"),
+        hash_including(event: "key_storage.stream.upload_started"),
       )
       expect(BucketStore.logger).to receive(:info).with(
-        hash_including(event: "key_storage.upload_finished"),
+        hash_including(event: "key_storage.stream.upload_finished"),
       )
 
       build_for("inmemory://bucket/file1").upload!("hello")
@@ -117,6 +117,90 @@ def build_for(key)
     end
   end
 
+  describe "#stream" do
+    let(:stream) { build_for("inmemory://bucket/file1").stream }
+
+    it "will return an object" do
+      expect { stream }.to_not raise_error
+      expect(stream).to_not be_nil
+    end
+
+    context "when we try to upload a bucket" do
+      it "raises an error" do
+        expect { build_for("inmemory://bucket").stream }.
+          to raise_error(ArgumentError, /key cannot be empty/i)
+      end
+    end
+
+    describe "#download" do
+      let(:input_file_1) { StringIO.new("content1") }
+      let(:input_file_2) { StringIO.new("content") }
+      let(:output_file) { StringIO.new }
+
+      before do
+        build_for("inmemory://bucket/file1").
+          stream.
+          upload!(file: StringIO.new("content1"))
+        build_for("inmemory://bucket/file2").
+          stream.
+          upload!(file: StringIO.new("content2"))
+      end
+
+      it "downloads the given file" do
+        expect(
+          build_for("inmemory://bucket/file1").
+          stream.
+          download(file: output_file),
+        ).
+          to match(hash_including(file: output_file))
+        expect(output_file.string).to eq("content1")
+      end
+
+      it "logs the operation" do
+        expect(BucketStore.logger).to receive(:info).with(
+          hash_including(event: "key_storage.stream.download_started"),
+        )
+        expect(BucketStore.logger).to receive(:info).with(
+          hash_including(event: "key_storage.stream.download_finished"),
+        )
+
+        build_for("inmemory://bucket/file1").stream.download(file: output_file)
+      end
+
+      context "when we try to download a bucket" do
+        it "raises an error" do
+          expect { build_for("inmemory://bucket").download }.
+            to raise_error(ArgumentError, /key cannot be empty/i)
+        end
+      end
+    end
+
+    describe "#upload!" do
+      it "will upload from a file" do
+        expect(stream.upload!(file: StringIO.new("hello"))).
+          to eq("inmemory://bucket/file1")
+      end
+
+      it "logs the operation" do
+        expect(BucketStore.logger).to receive(:info).with(
+          hash_including(event: "key_storage.stream.upload_started"),
+        )
+        expect(BucketStore.logger).to receive(:info).with(
+          hash_including(event: "key_storage.stream.upload_finished"),
+        )
+
+        stream.upload!(file: StringIO.new("hello"))
+      end
+
+      context "when we try to upload a bucket" do
+        it "raises an error" do
+          expect { build_for("inmemory://bucket").upload!("content") }.
+            to raise_error(ArgumentError, /key cannot be empty/i)
+        end
+      end
+    end
+  end
+
   describe "#delete!" do
     before do
       build_for("inmemory://bucket/file1").upload!("content1")

From 4946ae9701bc6710d65a58b6de207334ba24a2c1 Mon Sep 17 00:00:00 2001
From: Andrew Morton <andrewmorton@gocardless.com>
Date: Wed, 19 Apr 2023 10:03:58 +0100
Subject: [PATCH 09/17] Add streaming interface to the integration spec

Add exercising of the streaming interface to the integration spec.

For this we primarily focus on verifying the streaming API provides the same
output for a given input.
---
 spec/bucket_store_integration_spec.rb | 108 +++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 2 deletions(-)

diff --git a/spec/bucket_store_integration_spec.rb b/spec/bucket_store_integration_spec.rb
index 0adfbaf..b3caa05 100644
--- a/spec/bucket_store_integration_spec.rb
+++ b/spec/bucket_store_integration_spec.rb
@@ -4,6 +4,8 @@
 
 require "aws-sdk-s3"
 
+require "tempfile"
+
 RSpec.describe BucketStore, :integration do
   before do
     # Setup AWS connectivity to minio
@@ -31,7 +33,110 @@
         end
       end
 
-      it "has a consistent interface" do
+      context "when handling streaming uploads / downloads" do
+        let(:temp_filename) { "temp-file" }
+        let(:buffer_filename) { "in-memory-buffer" }
+        let(:input_temp_file) do
+          Tempfile.new.tap do |file|
+            file.write(Random.base64(1024))
+            file.rewind
+          end
+        end
+        let(:output_temp_file) do
+          Tempfile.new
+        end
+
+        let(:input_in_memory_buffer) do
+          StringIO.new(Random.base64(1024))
+        end
+        let(:output_in_memory_buffer) do
+          StringIO.new
+        end
+
+        before do
+          described_class.for("#{base_bucket_uri}/#{temp_filename}").
+            stream.
+            upload!(file: input_temp_file)
+          input_temp_file.rewind
+
+          described_class.for("#{base_bucket_uri}/#{buffer_filename}").
+            stream.
+            upload!(file: input_in_memory_buffer)
+          input_in_memory_buffer.rewind
+        end
+
+        after do
+          input_temp_file.close
+          output_temp_file.close
+        end
+
+        it "all file are listable on the store" do
+          expect(described_class.for(base_bucket_uri.to_s).list.to_a.size).to eq(2)
+        end
+
+        it "files are deletable on the store" do
+          described_class.for(base_bucket_uri.to_s).list.each do |key|
+            described_class.for(key).delete!
+          end
+          expect(described_class.for(base_bucket_uri.to_s).list.to_a.size).to eq(0)
+        end
+
+        it "files are downloadable from the store" do
+          described_class.for("#{base_bucket_uri}/#{temp_filename}").
+            stream.
+            download(file: output_temp_file)
+          output_temp_file.rewind
+
+          expect(output_temp_file.read).to eq(input_temp_file.read)
+
+          described_class.for("#{base_bucket_uri}/#{buffer_filename}").
+            stream.
+            download(file: output_in_memory_buffer)
+          output_in_memory_buffer.rewind
+
+          expect(output_in_memory_buffer.read).to eq(input_in_memory_buffer.read)
+        end
+
+        context "when the files are binary" do
+          let(:input_temp_file) do
+            Tempfile.new.binmode.tap do |file|
+              file.write(Random.bytes(1024))
+              file.rewind
+            end
+          end
+          let(:output_temp_file) do
+            Tempfile.new.binmode
+          end
+
+          let(:input_in_memory_buffer) do
+            StringIO.new.binmode.tap do |buffer|
+              buffer.write(Random.bytes(1024))
+              buffer.rewind
+            end
+          end
+          let(:output_in_memory_buffer) do
+            StringIO.new.binmode
+          end
+
+          it "files are downloadable from the store" do
+            described_class.for("#{base_bucket_uri}/#{temp_filename}").
+              stream.
+              download(file: output_temp_file)
+            output_temp_file.rewind
+
+            expect(output_temp_file.read).to eq(input_temp_file.read)
+
+            described_class.for("#{base_bucket_uri}/#{buffer_filename}").
+              stream.
+              download(file: output_in_memory_buffer)
+            output_in_memory_buffer.rewind
+
+            expect(output_in_memory_buffer.read).to eq(input_in_memory_buffer.read)
+          end
+        end
+      end
+
+      it "has a consistent interface for string uploads / downloads" do
         # Write 201 files
         file_list = []
         201.times do |i|
@@ -77,7 +182,6 @@
     end
   end
 
-  # We don't test GCS as there's no sensible way of running a local simulator
   include_examples "adapter integration", "inmemory://bucket"
   include_examples "adapter integration", "disk://bucket"
   include_examples "adapter integration", "s3://bucket"

From e8cc20360e4db37a0da2cd75117d799eba056542 Mon Sep 17 00:00:00 2001
From: Andrew Morton <ahjmorton@gmail.com>
Date: Wed, 19 Apr 2023 10:28:01 +0100
Subject: [PATCH 10/17] Readability / comment improvements

Stop adapter being leaked on the keystreamer public interface

Update lib/bucket_store/key_storage.rb

Co-authored-by: Ivan Giuliani <ivan@gocardless.com>

Update lib/bucket_store/key_storage.rb

Co-authored-by: Ivan Giuliani <ivan@gocardless.com>

Update spec/bucket_store_integration_spec.rb

Co-authored-by: Ivan Giuliani <ivan@gocardless.com>

Add param documentation

Drop "stream" prefix from logs

Rather than trying to maintain "stream" and regular log prefixes we just default back to upload started / finished

Use file instead of strings
---
 lib/bucket_store/key_storage.rb       | 18 ++++++++++--------
 spec/bucket_store/disk_spec.rb        | 10 +++++-----
 spec/bucket_store/in_memory_spec.rb   | 10 +++++-----
 spec/bucket_store/key_storage_spec.rb | 16 ++++++++--------
 4 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/lib/bucket_store/key_storage.rb b/lib/bucket_store/key_storage.rb
index 1141822..2fd9965 100644
--- a/lib/bucket_store/key_storage.rb
+++ b/lib/bucket_store/key_storage.rb
@@ -20,7 +20,7 @@ class KeyStorage
     # Note that individual adapters may require additional configuration for the correct
     # behavior of the streaming interface.
     class KeyStreamer
-      attr_reader :bucket, :key, :adapter, :adapter_type
+      attr_reader :bucket, :key, :adapter_type
 
       def initialize(adapter:, adapter_type:, bucket:, key:)
         @adapter = adapter
@@ -29,8 +29,8 @@ def initialize(adapter:, adapter_type:, bucket:, key:)
         @key = key
       end
 
-      # Streams the content of the reference key into a File like object
-      #
+      # Streams the content of the reference key into a file-like object
+      # @param [IO] file a writeable IO instance, or a file-like object such as `StringIO`
       # @return hash containing the bucket, the key and file like object passed in as input
       #
       # @see KeyStorage#download
@@ -39,7 +39,7 @@ def initialize(adapter:, adapter_type:, bucket:, key:)
       #   BucketStore.for("inmemory://bucket/file.xml").stream.download(file: buffer)
       #   buffer.string == "Imagine I'm a 2GB file"
       def download(file:)
-        BucketStore.logger.info(event: "key_storage.stream.download_started")
+        BucketStore.logger.info(event: "key_storage.download_started")
 
         start = BucketStore::Timing.monotonic_now
         adapter.download(
@@ -48,7 +48,7 @@ def download(file:)
           file: file,
         )
 
-        BucketStore.logger.info(event: "key_storage.stream.download_finished",
+        BucketStore.logger.info(event: "key_storage.download_finished",
                                 duration: BucketStore::Timing.monotonic_now - start)
 
         {
@@ -59,7 +59,7 @@ def download(file:)
       end
 
       # Performs a streaming upload to the backing object store
-      #
+      # @param [IO] file a readable IO instance, or a file-like object such as `StringIO`
       # @return the generated key for the new object
       #
       # @see KeyStorage#upload!
@@ -69,7 +69,7 @@ def download(file:)
       def upload!(file:)
         raise ArgumentError, "Key cannot be empty" if key.empty?
 
-        BucketStore.logger.info(event: "key_storage.stream.upload_started",
+        BucketStore.logger.info(event: "key_storage.upload_started",
                                 **log_context)
 
         start = BucketStore::Timing.monotonic_now
@@ -79,7 +79,7 @@ def upload!(file:)
           file: file,
         )
 
-        BucketStore.logger.info(event: "key_storage.stream.upload_finished",
+        BucketStore.logger.info(event: "key_storage.upload_finished",
                                 duration: BucketStore::Timing.monotonic_now - start,
                                 **log_context)
 
@@ -88,6 +88,8 @@ def upload!(file:)
 
       private
 
+      attr_reader :adapter
+
       def log_context
         {
           bucket: bucket,
diff --git a/spec/bucket_store/disk_spec.rb b/spec/bucket_store/disk_spec.rb
index a90e6bb..784b51d 100644
--- a/spec/bucket_store/disk_spec.rb
+++ b/spec/bucket_store/disk_spec.rb
@@ -111,11 +111,11 @@
 
     context "when the bucket has some keys in it" do
       before do
-        instance.upload!(bucket: bucket, key: "2019-01/hello1", file: file)
-        instance.upload!(bucket: bucket, key: "2019-01/hello2", file: file)
-        instance.upload!(bucket: bucket, key: "2019-01/hello3", file: file)
-        instance.upload!(bucket: bucket, key: "2019-02/hello", file: file)
-        instance.upload!(bucket: bucket, key: "2019-03/hello", file: file)
+        instance.upload!(bucket: bucket, key: "2019-01/hello1", file: StringIO.new("world"))
+        instance.upload!(bucket: bucket, key: "2019-01/hello2", file: StringIO.new("world"))
+        instance.upload!(bucket: bucket, key: "2019-01/hello3", file: StringIO.new("world"))
+        instance.upload!(bucket: bucket, key: "2019-02/hello", file: StringIO.new("world"))
+        instance.upload!(bucket: bucket, key: "2019-03/hello", file: StringIO.new("world"))
       end
 
       context "and we provide a matching prefix" do
diff --git a/spec/bucket_store/in_memory_spec.rb b/spec/bucket_store/in_memory_spec.rb
index 1914991..5a56848 100644
--- a/spec/bucket_store/in_memory_spec.rb
+++ b/spec/bucket_store/in_memory_spec.rb
@@ -122,11 +122,11 @@
 
     context "when there's some content" do
       before do
-        instance.upload!(bucket: bucket, key: "2019-01/hello1", file: file)
-        instance.upload!(bucket: bucket, key: "2019-01/hello2", file: file)
-        instance.upload!(bucket: bucket, key: "2019-01/hello3", file: file)
-        instance.upload!(bucket: bucket2, key: "2019-02/hello", file: file)
-        instance.upload!(bucket: bucket2, key: "2019-03/hello", file: file)
+        instance.upload!(bucket: bucket, key: "2019-01/hello1", file: StringIO.new("world"))
+        instance.upload!(bucket: bucket, key: "2019-01/hello2", file: StringIO.new("world"))
+        instance.upload!(bucket: bucket, key: "2019-01/hello3", file: StringIO.new("world"))
+        instance.upload!(bucket: bucket2, key: "2019-02/hello", file: StringIO.new("world"))
+        instance.upload!(bucket: bucket2, key: "2019-03/hello", file: StringIO.new("world"))
       end
 
       it "resets all the buckets" do
diff --git a/spec/bucket_store/key_storage_spec.rb b/spec/bucket_store/key_storage_spec.rb
index 8fd8814..13865c2 100644
--- a/spec/bucket_store/key_storage_spec.rb
+++ b/spec/bucket_store/key_storage_spec.rb
@@ -75,10 +75,10 @@ def build_for(key)
 
     it "logs the operation" do
       expect(BucketStore.logger).to receive(:info).with(
-        hash_including(event: "key_storage.stream.download_started"),
+        hash_including(event: "key_storage.download_started"),
       )
       expect(BucketStore.logger).to receive(:info).with(
-        hash_including(event: "key_storage.stream.download_finished"),
+        hash_including(event: "key_storage.download_finished"),
       )
 
       build_for("inmemory://bucket/file1").download
@@ -100,10 +100,10 @@ def build_for(key)
 
     it "logs the operation" do
       expect(BucketStore.logger).to receive(:info).with(
-        hash_including(event: "key_storage.stream.upload_started"),
+        hash_including(event: "key_storage.upload_started"),
       )
       expect(BucketStore.logger).to receive(:info).with(
-        hash_including(event: "key_storage.stream.upload_finished"),
+        hash_including(event: "key_storage.upload_finished"),
       )
 
       build_for("inmemory://bucket/file1").upload!("hello")
@@ -158,10 +158,10 @@ def build_for(key)
 
       it "logs the operation" do
         expect(BucketStore.logger).to receive(:info).with(
-          hash_including(event: "key_storage.stream.download_started"),
+          hash_including(event: "key_storage.download_started"),
         )
         expect(BucketStore.logger).to receive(:info).with(
-          hash_including(event: "key_storage.stream.download_finished"),
+          hash_including(event: "key_storage.download_finished"),
         )
 
         build_for("inmemory://bucket/file1").stream.download(file: output_file)
@@ -183,10 +183,10 @@ def build_for(key)
 
       it "logs the operation" do
         expect(BucketStore.logger).to receive(:info).with(
-          hash_including(event: "key_storage.stream.upload_started"),
+          hash_including(event: "key_storage.upload_started"),
         )
         expect(BucketStore.logger).to receive(:info).with(
-          hash_including(event: "key_storage.stream.upload_finished"),
+          hash_including(event: "key_storage.upload_finished"),
         )
 
         stream.upload!(file: StringIO.new("hello"))

From 78b5a07bbca4c0b11d5b0f9cd1c44cfd758dcd02 Mon Sep 17 00:00:00 2001
From: Andrew Morton <andrewmorton@gocardless.com>
Date: Wed, 19 Apr 2023 10:26:19 +0100
Subject: [PATCH 11/17] Update change log, version number and README

Preparing for a release
---
 CHANGELOG.md                |  5 +++++
 README.md                   | 20 ++++++++++++++++++--
 lib/bucket_store/version.rb |  2 +-
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 337b458..461dd98 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,8 @@
+v0.6.0
+------
+- [BREAKING] Remove support for Ruby 2.6
+- Add support for streaming upoads / downloads
+
 v0.5.0
 ------
 - Support escaping of URI keys. Fixes #46.
diff --git a/README.md b/README.md
index 274af45..748a334 100644
--- a/README.md
+++ b/README.md
@@ -135,18 +135,34 @@ in mind:
 
 ## Examples
 
-### Uploading a file to a bucket
+### Uploading a string to a bucket
 ```ruby
 BucketStore.for("inmemory://bucket/path/file.xml").upload!("hello world")
 => "inmemory://bucket/path/file.xml"
 ```
 
-### Accessing a file in a bucket
+### Accessing a string in a bucket
 ```ruby
 BucketStore.for("inmemory://bucket/path/file.xml").download
 => {:bucket=>"bucket", :key=>"path/file.xml", :content=>"hello world"}
 ```
 
+### Uploading a file-like object to a bucket
+```ruby
+buffer = StringIO.new("This could also be an actual file")
+BucketStore.for("inmemory://bucket/path/file.xml").stream.upload!(file: buffer)
+=> "inmemory://bucket/path/file.xml"
+```
+
+### Downloading to a file-like object from a bucket
+```ruby
+buffer = StringIO.new
+BucketStore.for("inmemory://bucket/path/file.xml").stream.download(file: buffer)
+=> {:bucket=>"bucket", :key=>"path/file.xml", :file=>buffer}
+buffer.string 
+=> "This could also be an actual file"
+```
+
 ### Listing all keys under a prefix
 ```ruby
 BucketStore.for("inmemory://bucket/path/").list
diff --git a/lib/bucket_store/version.rb b/lib/bucket_store/version.rb
index ac71f96..5195582 100644
--- a/lib/bucket_store/version.rb
+++ b/lib/bucket_store/version.rb
@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module BucketStore
-  VERSION = "0.5.0"
+  VERSION = "0.6.0"
 end

From d80f5abc9835672cadec10b4d8385950601519cb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Art=C5=ABrs=20Pirogovs?= <apirogovs@gocardless.com>
Date: Thu, 7 Sep 2023 13:44:52 +0300
Subject: [PATCH 12/17] Add .rewind

---
 lib/bucket_store/in_memory.rb | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/bucket_store/in_memory.rb b/lib/bucket_store/in_memory.rb
index 5698ccf..4aa682c 100644
--- a/lib/bucket_store/in_memory.rb
+++ b/lib/bucket_store/in_memory.rb
@@ -34,7 +34,10 @@ def upload!(bucket:, key:, file:)
     end
 
     def download(bucket:, key:, file:)
-      file.write(@buckets[bucket].fetch(key))
+      file.tap do |f|
+        f.write(@buckets[bucket].fetch(key))
+        f.rewind
+      end
     end
 
     def list(bucket:, key:, page_size:)

From eaf9356edb10ad7d7e3fe00c897f6ca79dcfd9a9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Art=C5=ABrs=20Pirogovs?= <apirogovs@gocardless.com>
Date: Mon, 11 Sep 2023 13:48:09 +0300
Subject: [PATCH 13/17] Add .rewind to lib files, remove .rewind from spec
 files

---
 CHANGELOG.md                          |  2 +-
 lib/bucket_store/disk.rb              |  6 ++++++
 lib/bucket_store/gcs.rb               | 12 +++++++++---
 lib/bucket_store/in_memory.rb         |  5 ++++-
 lib/bucket_store/s3.rb                |  2 ++
 spec/bucket_store_integration_spec.rb |  6 ------
 6 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 461dd98..738d1c8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,7 +1,7 @@
 v0.6.0
 ------
 - [BREAKING] Remove support for Ruby 2.6
-- Add support for streaming upoads / downloads
+- Add support for streaming uploads / downloads
 
 v0.5.0
 ------
diff --git a/lib/bucket_store/disk.rb b/lib/bucket_store/disk.rb
index 6758e31..651c025 100644
--- a/lib/bucket_store/disk.rb
+++ b/lib/bucket_store/disk.rb
@@ -16,7 +16,11 @@ def initialize(base_dir)
     def upload!(bucket:, key:, file:)
       File.open(key_path(bucket, key), "w") do |output_file|
         output_file.write(file.read)
+        output_file.rewind
       end
+
+      file.rewind
+
       {
         bucket: bucket,
         key: key,
@@ -26,6 +30,8 @@ def upload!(bucket:, key:, file:)
     def download(bucket:, key:, file:)
       File.open(key_path(bucket, key), "r") do |saved_file|
         file.write(saved_file.read)
+        saved_file.rewind
+        file.rewind
       end
     end
 
diff --git a/lib/bucket_store/gcs.rb b/lib/bucket_store/gcs.rb
index 8a3a829..ea3ddfa 100644
--- a/lib/bucket_store/gcs.rb
+++ b/lib/bucket_store/gcs.rb
@@ -36,6 +36,8 @@ def initialize(timeout_seconds)
     def upload!(bucket:, key:, file:)
       get_bucket(bucket).create_file(file, key)
 
+      file.rewind
+
       {
         bucket: bucket,
         key: key,
@@ -43,9 +45,13 @@ def upload!(bucket:, key:, file:)
     end
 
     def download(bucket:, key:, file:)
-      get_bucket(bucket).
-        file(key).
-        download(file)
+      file.tap do |f|
+        get_bucket(bucket).
+          file(key).
+          download(f)
+
+        f.rewind
+      end
     end
 
     def list(bucket:, key:, page_size:)
diff --git a/lib/bucket_store/in_memory.rb b/lib/bucket_store/in_memory.rb
index 4aa682c..4a59029 100644
--- a/lib/bucket_store/in_memory.rb
+++ b/lib/bucket_store/in_memory.rb
@@ -25,7 +25,10 @@ def reset!
     end
 
     def upload!(bucket:, key:, file:)
-      @buckets[bucket][key] = file.read
+      file.tap do |f|
+        @buckets[bucket][key] = f.read
+        f.rewind
+      end
 
       {
         bucket: bucket,
diff --git a/lib/bucket_store/s3.rb b/lib/bucket_store/s3.rb
index 89b5ef3..43d44c1 100644
--- a/lib/bucket_store/s3.rb
+++ b/lib/bucket_store/s3.rb
@@ -27,6 +27,8 @@ def upload!(bucket:, key:, file:)
         body: file,
       )
 
+      file.rewind
+
       {
         bucket: bucket,
         key: key,
diff --git a/spec/bucket_store_integration_spec.rb b/spec/bucket_store_integration_spec.rb
index b3caa05..d83cad4 100644
--- a/spec/bucket_store_integration_spec.rb
+++ b/spec/bucket_store_integration_spec.rb
@@ -57,12 +57,10 @@
           described_class.for("#{base_bucket_uri}/#{temp_filename}").
             stream.
             upload!(file: input_temp_file)
-          input_temp_file.rewind
 
           described_class.for("#{base_bucket_uri}/#{buffer_filename}").
             stream.
             upload!(file: input_in_memory_buffer)
-          input_in_memory_buffer.rewind
         end
 
         after do
@@ -85,14 +83,12 @@
           described_class.for("#{base_bucket_uri}/#{temp_filename}").
             stream.
             download(file: output_temp_file)
-          output_temp_file.rewind
 
           expect(output_temp_file.read).to eq(input_temp_file.read)
 
           described_class.for("#{base_bucket_uri}/#{buffer_filename}").
             stream.
             download(file: output_in_memory_buffer)
-          output_in_memory_buffer.rewind
 
           expect(output_in_memory_buffer.read).to eq(input_in_memory_buffer.read)
         end
@@ -122,14 +118,12 @@
             described_class.for("#{base_bucket_uri}/#{temp_filename}").
               stream.
               download(file: output_temp_file)
-            output_temp_file.rewind
 
             expect(output_temp_file.read).to eq(input_temp_file.read)
 
             described_class.for("#{base_bucket_uri}/#{buffer_filename}").
               stream.
               download(file: output_in_memory_buffer)
-            output_in_memory_buffer.rewind
 
             expect(output_in_memory_buffer.read).to eq(input_in_memory_buffer.read)
           end

From 128324f9e5b3308d4b530c16cb2194a2c2a0b7cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Art=C5=ABrs=20Pirogovs?= <apirogovs@gocardless.com>
Date: Mon, 11 Sep 2023 15:16:44 +0300
Subject: [PATCH 14/17] remove dead spec code

---
 spec/bucket_store/key_storage_spec.rb | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/spec/bucket_store/key_storage_spec.rb b/spec/bucket_store/key_storage_spec.rb
index 13865c2..65a4a37 100644
--- a/spec/bucket_store/key_storage_spec.rb
+++ b/spec/bucket_store/key_storage_spec.rb
@@ -133,8 +133,6 @@ def build_for(key)
     end
 
     describe "#download" do
-      let(:input_file_1) { StringIO.new("content1") }
-      let(:input_file_2) { StringIO.new("content") }
       let(:output_file) { StringIO.new }
 
       before do
@@ -151,8 +149,8 @@ def build_for(key)
           build_for("inmemory://bucket/file1").
           stream.
           download(file: output_file),
-        ).
-          to match(hash_including(file: output_file))
+        ).to match(hash_including(file: output_file))
+
         expect(output_file.string).to eq("content1")
       end
 

From 0cdbd73a2d7975bfe841605bb294090fab1726bb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Art=C5=ABrs=20Pirogovs?= <apirogovs@gocardless.com>
Date: Wed, 13 Sep 2023 14:52:52 +0300
Subject: [PATCH 15/17] remove .rewind from upload methods

---
 lib/bucket_store/disk.rb              | 3 ---
 lib/bucket_store/gcs.rb               | 2 --
 lib/bucket_store/in_memory.rb         | 1 -
 lib/bucket_store/s3.rb                | 2 --
 spec/bucket_store_integration_spec.rb | 2 ++
 5 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/lib/bucket_store/disk.rb b/lib/bucket_store/disk.rb
index 651c025..dbf4a19 100644
--- a/lib/bucket_store/disk.rb
+++ b/lib/bucket_store/disk.rb
@@ -19,8 +19,6 @@ def upload!(bucket:, key:, file:)
         output_file.rewind
       end
 
-      file.rewind
-
       {
         bucket: bucket,
         key: key,
@@ -30,7 +28,6 @@ def upload!(bucket:, key:, file:)
     def download(bucket:, key:, file:)
       File.open(key_path(bucket, key), "r") do |saved_file|
         file.write(saved_file.read)
-        saved_file.rewind
         file.rewind
       end
     end
diff --git a/lib/bucket_store/gcs.rb b/lib/bucket_store/gcs.rb
index ea3ddfa..e4b97fe 100644
--- a/lib/bucket_store/gcs.rb
+++ b/lib/bucket_store/gcs.rb
@@ -36,8 +36,6 @@ def initialize(timeout_seconds)
     def upload!(bucket:, key:, file:)
       get_bucket(bucket).create_file(file, key)
 
-      file.rewind
-
       {
         bucket: bucket,
         key: key,
diff --git a/lib/bucket_store/in_memory.rb b/lib/bucket_store/in_memory.rb
index 4a59029..8fd88fd 100644
--- a/lib/bucket_store/in_memory.rb
+++ b/lib/bucket_store/in_memory.rb
@@ -27,7 +27,6 @@ def reset!
     def upload!(bucket:, key:, file:)
       file.tap do |f|
         @buckets[bucket][key] = f.read
-        f.rewind
       end
 
       {
diff --git a/lib/bucket_store/s3.rb b/lib/bucket_store/s3.rb
index 43d44c1..89b5ef3 100644
--- a/lib/bucket_store/s3.rb
+++ b/lib/bucket_store/s3.rb
@@ -27,8 +27,6 @@ def upload!(bucket:, key:, file:)
         body: file,
       )
 
-      file.rewind
-
       {
         bucket: bucket,
         key: key,
diff --git a/spec/bucket_store_integration_spec.rb b/spec/bucket_store_integration_spec.rb
index d83cad4..68798ad 100644
--- a/spec/bucket_store_integration_spec.rb
+++ b/spec/bucket_store_integration_spec.rb
@@ -57,10 +57,12 @@
           described_class.for("#{base_bucket_uri}/#{temp_filename}").
             stream.
             upload!(file: input_temp_file)
+          input_temp_file.rewind
 
           described_class.for("#{base_bucket_uri}/#{buffer_filename}").
             stream.
             upload!(file: input_in_memory_buffer)
+          input_in_memory_buffer.rewind
         end
 
         after do

From a3b411c98813af38eca87002bc49a00a70f8f969 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Art=C5=ABrs=20Pirogovs?= <apirogovs@gocardless.com>
Date: Wed, 13 Sep 2023 15:40:56 +0300
Subject: [PATCH 16/17] add .unlink for Tempfiles

---
 spec/bucket_store_integration_spec.rb | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/spec/bucket_store_integration_spec.rb b/spec/bucket_store_integration_spec.rb
index 68798ad..bf68638 100644
--- a/spec/bucket_store_integration_spec.rb
+++ b/spec/bucket_store_integration_spec.rb
@@ -6,6 +6,8 @@
 
 require "tempfile"
 
+require "pry-byebug"
+
 RSpec.describe BucketStore, :integration do
   before do
     # Setup AWS connectivity to minio
@@ -66,8 +68,17 @@
         end
 
         after do
-          input_temp_file.close
-          output_temp_file.close
+          if input_temp_file.is_a?(File)
+            input_temp_file.close
+          else
+            input_temp_file.unlink
+          end
+
+          if output_temp_file.is_a?(File)
+            output_temp_file.close
+          else
+            output_temp_file.unlink
+          end
         end
 
         it "all file are listable on the store" do

From a5589b13de4ec14d03546ad44f947c9345cc4690 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Art=C5=ABrs=20Pirogovs?= <apirogovs@gocardless.com>
Date: Wed, 13 Sep 2023 16:54:45 +0300
Subject: [PATCH 17/17] fix tempfile with binmode

---
 spec/bucket_store_integration_spec.rb | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/spec/bucket_store_integration_spec.rb b/spec/bucket_store_integration_spec.rb
index bf68638..22eef94 100644
--- a/spec/bucket_store_integration_spec.rb
+++ b/spec/bucket_store_integration_spec.rb
@@ -6,8 +6,6 @@
 
 require "tempfile"
 
-require "pry-byebug"
-
 RSpec.describe BucketStore, :integration do
   before do
     # Setup AWS connectivity to minio
@@ -68,17 +66,8 @@
         end
 
         after do
-          if input_temp_file.is_a?(File)
-            input_temp_file.close
-          else
-            input_temp_file.unlink
-          end
-
-          if output_temp_file.is_a?(File)
-            output_temp_file.close
-          else
-            output_temp_file.unlink
-          end
+          input_temp_file.close!
+          output_temp_file.close!
         end
 
         it "all file are listable on the store" do
@@ -108,13 +97,13 @@
 
         context "when the files are binary" do
           let(:input_temp_file) do
-            Tempfile.new.binmode.tap do |file|
+            Tempfile.new(binmode: true).tap do |file|
               file.write(Random.bytes(1024))
               file.rewind
             end
           end
           let(:output_temp_file) do
-            Tempfile.new.binmode
+            Tempfile.new(binmode: true)
           end
 
           let(:input_in_memory_buffer) do