Fixed #35326 -- added allow_overwrite parameter to FileSystemStorage.

[bcail]

Trac ticket number

ticket-35326

Branch description

This patch handled temporary uploaded files when the filesystem storage allows overwriting files.

Checklist

• This PR targets the main branch.
• The commit message is written in past tense, mentions the ticket number, and ends with a period.
• I have checked the "Has patch" ticket flag in the Trac system.
• I have added or updated relevant tests.
• I have added or updated relevant docs, including release notes if applicable.
• For UI changes, I have attached screenshots in both light and dark modes.

[jschneier]

What about using the OPTIONS dict to implement this feature more generally?

[bcail]

OK, so then I would add an allow_overwrite parameter to both the file & memory storage __init__ methods? That seems doable.

[sarahboyce]

Thank you for this @bcail! I have initial comments 👍

[sarahboyce]

Can you explain why this change is required? I also believe it is untested.

[bcail]

This change is needed so that the name is allowed even if there's already a file by that name. If I remove this change, the two file_storage.tests.OverwritingStorageTests tests fail.

[sarahboyce]

I think we need to add file_storage.test_generate_filename.GenerateFilenameStorageTests.test_storage_dangerous_paths_dir_name or create a very similar test for when to FileSystemStorage has allow_overwrite=True

[bcail]

I updated the tests.

[sarahboyce]

I think instead of overwriting get_available_name here, we can overwrite exists to something like:

    def exists(self, name):
        exists = os.path.lexists(self.path(name))
        if self._allow_overwrite:
            return False
        return exists

It requires some test updates (these need a think and can probably be better) but something like:

@@ -99,14 +99,14 @@ class FileStorageTests(SimpleTestCase):
         f = self.storage.open("storage_test", "w")
         f.write("storage contents")
         f.close()
-        self.assertTrue(self.storage.exists("storage_test"))
+        self.assertTrue(os.path.exists(os.path.join(self.temp_dir, "storage_test")))

         f = self.storage.open("storage_test", "r")
         self.assertEqual(f.read(), "storage contents")
         f.close()

         self.storage.delete("storage_test")
-        self.assertFalse(self.storage.exists("storage_test"))
+        self.assertFalse(os.path.exists(os.path.join(self.temp_dir, "storage_test")))

     def _test_file_time_getter(self, getter):
         # Check for correct behavior under both USE_TZ=True and USE_TZ=False.
@@ -275,10 +275,10 @@ class FileStorageTests(SimpleTestCase):
         """
         Saving a pathname should create intermediate directories as necessary.
         """
-        self.assertFalse(self.storage.exists("path/to"))
+        self.assertFalse(os.path.exists(os.path.join(self.temp_dir, "path/to")))
         self.storage.save("path/to/test.file", ContentFile("file saved with path"))

-        self.assertTrue(self.storage.exists("path/to"))
+        self.assertTrue(os.path.exists(os.path.join(self.temp_dir, "path/to")))
         with self.storage.open("path/to/test.file") as f:
             self.assertEqual(f.read(), b"file saved with path")

@@ -694,13 +694,11 @@ class OverwritingStorageTests(FileStorageTests):
         stored_name_1 = self.storage.save(name, f_1)
         try:
             self.assertEqual(stored_name_1, name)
-            self.assertTrue(self.storage.exists(name))
             self.assertTrue(os.path.exists(os.path.join(self.temp_dir, name)))
             with self.storage.open(name) as fp:
                 self.assertEqual(fp.read(), content_1)
             stored_name_2 = self.storage.save(name, f_2)
             self.assertEqual(stored_name_2, name)
-            self.assertTrue(self.storage.exists(name))
             self.assertTrue(os.path.exists(os.path.join(self.temp_dir, name)))
             with self.storage.open(name) as fp:
                 self.assertEqual(fp.read(), content_2)
@@ -722,13 +720,11 @@ class OverwritingStorageTests(FileStorageTests):
         stored_name_1 = self.storage.save(name, f_1)
         try:
             self.assertEqual(stored_name_1, name)
-            self.assertTrue(self.storage.exists(name))
             self.assertTrue(os.path.exists(os.path.join(self.temp_dir, name)))
             with self.storage.open(name) as fp:
                 self.assertEqual(fp.read(), content_1)
             stored_name_2 = self.storage.save(name, f_2)
             self.assertEqual(stored_name_2, name)
-            self.assertTrue(self.storage.exists(name))
             self.assertTrue(os.path.exists(os.path.join(self.temp_dir, name)))
             with self.storage.open(name) as fp:

[bcail]

@sarahboyce Thanks for your review. I've added OS_OPEN_FLAGS back in, for deprecation, and I added some documentation. Do we need to add a deprecation warning in the code if OS_OPEN_FLAGS is used? If so, I'm not sure how to do that since we're always setting OS_OPEN_FLAGS here in the django code. Should I check for whether OS_OPEN_FLAGS has been changed from the default and raise a warning if so?

I added a commit with a warning if the default OS_OPEN_FLAGS is overridden.

[sarahboyce]

I added a commit with a warning if the default OS_OPEN_FLAGS is overridden.

Thank you for doing this @bcail, I think this approach looks good to me.

I have a few minor comments, this is getting really close 👍

[jschneier]

Kind of derailed the original ticket so would rename the title

[sarahboyce]

Thank you for the updates @bcail, have you seen this suggestion?
As it would change how exists works (a method we should overwrite/define) rather than get_available_name (that in theory we shouldn't overwrite/define), I think this might be a better way to go 🤔

[bcail]

@sarahboyce I wonder if there would be any downstream code that would want to call exists(), to see if a file is in storage or not, and would get confused by us changing exists() to always return False for overwriting storage?

But I'll do it whichever way you think is best.

[sarahboyce]

To clarify my thoughts, if we look at the docs for exists

Returns True if a file referenced by the given name already exists in the storage system, or False if the name is available for a new file.

I believe it is available for a new file when we are in overwrite mode regardless of whether a file already exists (though we might want to slightly tweak the wording of the true case).

I also believe this would better suit the documented approach for writing a custom file storage system (i.e. defining exists but generally leaving get_available_name alone).

@sarahboyce I wonder if there would be any downstream code that would want to call exists(), to see if a file is in storage or not, and would get confused by us changing exists() to always return False for overwriting storage?

Thank you for raising this. 👍
There should be no impact when allow_overwrite is False (which is the default).
Any other impact can be handled when someone chooses to add support for this newly added allow_overwrite parameter (including overwriting exists to be as before).
Hence, I currently believe that leaving get_available_name alone and instead handling this with exists would be the better approach.

@jschneier do you have an opinion here?

[sarahboyce]

Thank you @bcail 🥳 this looks great, I pushed up small edits and will seek a second opinion before merging 👍

[bcail]

Thanks, @sarahboyce.