From 979e3f584e136eb1be7015ae26ece89f469b7e0b Mon Sep 17 00:00:00 2001
From: MikeD <5084545+devmonkey22@users.noreply.github.com>
Date: Wed, 20 Nov 2024 13:04:08 -0500
Subject: [PATCH] [s3] Fix to allow reopening closed file with different mode.
 (jschneier#1474)

---
 storages/backends/s3.py | 15 +++++++++++++-
 tests/test_s3.py        | 43 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/storages/backends/s3.py b/storages/backends/s3.py
index 2686ccf1..2db27ad8 100644
--- a/storages/backends/s3.py
+++ b/storages/backends/s3.py
@@ -146,10 +146,23 @@ def _reset_file_properties(self):
         self._is_dirty = False
 
     def open(self, mode=None):
+        """
+        Open the file.
+
+        - If the file is already open, it will seek to the start.
+        - If the file is closed, it will re-open with the last mode or new ``mode``.
+
+        :param mode: File mode to open with.  Defaults to last opened mode.
+        :returns: ``S3File`` after opening.
+        """
         if self._file is not None and not self.closed:
             self.seek(0)  # Mirror Django's behavior
         elif mode and mode != self._mode:
-            raise ValueError("Cannot reopen file with a new mode.")
+            if not self.closed:
+                raise ValueError("Cannot change mode with an open file.")
+
+            # Allow reopening file with a diff mode
+            self._mode = mode
 
         # Accessing the file will functionally re-open it
         self.file  # noqa: B018
diff --git a/tests/test_s3.py b/tests/test_s3.py
index c6d87a06..39f4216c 100644
--- a/tests/test_s3.py
+++ b/tests/test_s3.py
@@ -1057,6 +1057,11 @@ def test_closed(self):
     def test_reopening(self):
         f = s3.S3File("test", "wb", self.storage)
 
+        with self.assertRaisesMessage(ValueError, "Cannot change mode with an open file."):
+            # Test raising mode error while file not init, but we're marked not closed (can happen after init before file access)
+            # However, if the file was properly closed, we're allowed to reopen with a diff mode.
+            _ = f.open("w")
+
         with f.open() as fp:
             fp.write(b"xyz")
 
@@ -1069,6 +1074,14 @@ def test_reopening(self):
         self.assertFalse(f._is_dirty)
         self.assertIsNone(f._multipart)
 
+        # Allowed to reopen with diff mode while closed
+        self.assertTrue(f.closed)
+        self.assertEqual(f._mode, 'wb')
+        with f.open('rb') as fp:
+            self.assertIsNotNone(fp.file)
+            self.assertFalse(fp.closed)
+            self.assertEqual(fp._mode, 'rb')
+
 
 @mock_s3
 class S3StorageTestsWithMoto(TestCase):
@@ -1191,6 +1204,36 @@ def test_storage_open_reading_with_newlines(self):
             file.close()
         self.assertEqual(content_lines, [b"line1\n", b"line2\r\n", b"more\r", b"text"])
 
+    def test_reopening_change_modes(self):
+        """
+        Test reopening the file with different modes.
+
+        This exercises scenarios like a model instance with a ``FileField`` tied to this storage,
+        where the same file needs to be read as text and later binary (ie. to compute hash) from the
+        same model instance. Django ``FileField``s (really ``FieldFile``s) keep a reference to the
+        underlying ``File`` (aka ``S3File``) which makes it require being able to reopen with a
+        different mode to support these scenarios.
+        """
+        name = "test_reopening_change_modes.txt"
+        with io.BytesIO() as temp_file:
+            temp_file.write(b"line1\nline2\n")
+            self.storage.save(name, temp_file)
+
+        file = self.storage.open(name, mode="r")
+
+        # Open with original mode, then closes file
+        with file.open():
+            self.assertEqual(file._mode, 'r')
+            self.assertEqual(file.readline(), "line1\n")
+            self.assertEqual(file.readline(), "line2\n")
+        
+        # Open with different mode (should work as long as file was prev closed)
+        self.assertTrue(file.closed)
+        with file.open("rb") as fp:
+            self.assertEqual(file._mode, 'rb')
+            self.assertEqual(fp.read(), b"line1\nline2\n")
+
+
 
 class TestBackwardsNames(TestCase):
     def test_importing(self):