[s3] Fix to allow reopening closed file with different mode

CHANGELOG.rst

@@ -14,6 +14,13 @@ Azure
 
 .. _#1432: https://github.com/jschneier/django-storages/pull/1432
 
+S3
+--
+
+- Fix to allow reopening closed file with different mode (`#1474`)
+
+.. _#1475: https://github.com/jschneier/django-storages/pull/1475
+
 1.14.4 (2024-07-09)
 *******************
 

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

tests/test_s3.py

@@ -1057,6 +1057,14 @@ 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 +1077,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 +1207,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):