Opened 2 months ago

Closed 2 months ago

Last modified 2 months ago

#35604 closed Bug (fixed)

Unexpected behaviour of FileSystemStorage.exists() due to latest changes

Reported by: Stefan Hammer Owned by: Sarah Boyce
Component: File uploads/storage Version: 5.1
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Stefan Hammer)

With this ticket I would like to vote against the latest changes to FileSystemStorage.exists() with #35326.
The very basic file storage operation "exists" now returns False, if the option allow_overwrite is set to True, no matter if the file actually exists or not (see Github).
I know, the default behavior is unchanged (allow_overwrite is False by default), but nevertheless the change seems to me more like the easiest option, but not the cleanest for the API. Until now, the API was very self-explanatory.

The storages API is perfect due to its simplicity and extensibility, and we're using it extensively due to packages like django-storages. I think we should keep it that simple and rethink the above change of the basic "exists" operation, which in my opinion should return exactly that information.
Maybe we should also adapt the documentation for exists(), as it can be interpreted in multiple ways (see here and here).

I probably wouldn't have noticed this change until its occurrence in the next LTS, but (luckily) django-storages has synced the above changes into its latest version, which lead to a bug report with multiple affected people here.

Change History (6)

comment:1 by Stefan Hammer, 2 months ago

Description: modified (diff)

comment:2 by Sarah Boyce, 2 months ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Comment on the issue in django-storages

Given the engagement on the discussion around what .exists() should do in the overwrite case, and the recent security patch to Django, it makes sense for FileSystemStorage to also overwrite get_available_name() but leave the existing exists behaviour. This previously was not something we could endorse.
A clarification to the FileSystemStorage docs also make sense.

comment:3 by Sarah Boyce, 2 months ago

Has patch: set
Owner: set to Sarah Boyce
Status: newassigned

comment:4 by Natalia Bidart, 2 months ago

Triage Stage: AcceptedReady for checkin

comment:5 by Sarah Boyce <42296566+sarahboyce@…>, 2 months ago

Resolution: fixed
Status: assignedclosed

In 8d6a20b:

Fixed #35604, Refs #35326 -- Made FileSystemStorage.exists() behaviour independent from allow_overwrite.

Partially reverts 0b33a3abc2ca7d68a24f6d0772bc2b9fa603744e.

Storage.exists(name) was documented to "return False if
the name is available for a new file." but return True if
the file exists. This is ambiguous in the overwrite file
case. It will now always return whether the file exists.

Thank you to Natalia Bidart and Josh Schneier for the
review.

comment:6 by Sarah Boyce <42296566+sarahboyce@…>, 2 months ago

In e42defb:

[5.1.x] Fixed #35604, Refs #35326 -- Made FileSystemStorage.exists() behaviour independent from allow_overwrite.

Partially reverts 0b33a3abc2ca7d68a24f6d0772bc2b9fa603744e.

Storage.exists(name) was documented to "return False if
the name is available for a new file." but return True if
the file exists. This is ambiguous in the overwrite file
case. It will now always return whether the file exists.

Thank you to Natalia Bidart and Josh Schneier for the
review.

Backport of 8d6a20b656ff3fa18e36954668a44a831c2f6ddd from main.

Note: See TracTickets for help on using tickets.
Back to Top