[Django] #35604: Unexpected behaviour of FileSystemStorage.exists() due to latest changes

[Django]

#35604: Unexpected behaviour of FileSystemStorage.exists() due to latest changes
-------------------------------------+-------------------------------------
Reporter: Stefan Hammer | Type:
| Uncategorized
Status: new | Component: File
| uploads/storage
Version: 5.1 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
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` (see
[https://github.com/django/django/pull/18020/files#diff-
63fca472a0ca8c04505cb4ec9c132663ac71fc964110afc8ae9218416f37821cR196-R202
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
[https://github.com/jschneier/django-storages 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
[https://docs.djangoproject.com/en/dev/ref/files/storage/#django.core.files.storage.Storage.exists
documentation] for `exists()`, as it can be interpreted in multiple ways
(see [https://github.com/django/django/pull/18020#issuecomment-2107608379
here] and [https://github.com/jschneier/django-
storages/issues/1430#issuecomment-2221213695 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 [https://github.com/jschneier/django-storages/issues/1430 here].
--
Ticket URL: <https://code.djangoproject.com/ticket/35604>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35604: Unexpected behaviour of FileSystemStorage.exists() due to latest changes
-------------------------------------+-------------------------------------

Reporter: Stefan Hammer | Owner: (none)
Type: Uncategorized | Status: new
Component: File | Version: 5.1
uploads/storage |
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Stefan Hammer:

Old description:

New description:

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 [https://github.com/django/django/pull/18020/files

#diff-
63fca472a0ca8c04505cb4ec9c132663ac71fc964110afc8ae9218416f37821cR196-R202
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
[https://github.com/jschneier/django-storages 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
[https://docs.djangoproject.com/en/dev/ref/files/storage/#django.core.files.storage.Storage.exists
documentation] for `exists()`, as it can be interpreted in multiple ways
(see [https://github.com/django/django/pull/18020#issuecomment-2107608379
here] and [https://github.com/jschneier/django-
storages/issues/1430#issuecomment-2221213695 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 [https://github.com/jschneier/django-storages/issues/1430 here].

--

--
Ticket URL: <https://code.djangoproject.com/ticket/35604#comment:1>

[Django]

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

--------------------------------------+------------------------------------

Reporter: Stefan Hammer | Owner: (none)

Type: Bug | Status: new

Component: File uploads/storage | Version: 5.1

Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Sarah Boyce):

* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
* type: Uncategorized => Bug

Comment:

[https://github.com/jschneier/django-
storages/issues/1430#issuecomment-2230281809 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35604#comment:2>

[Django]

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

-------------------------------------+-------------------------------------
Reporter: Stefan Hammer | Owner: Sarah
| Boyce
Type: Bug | Status: assigned

Component: File | Version: 5.1
uploads/storage |

Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* has_patch: 0 => 1
* owner: (none) => Sarah Boyce
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35604#comment:3>

[Django]

#35604: Unexpected behaviour of FileSystemStorage.exists() due to latest changes
-------------------------------------+-------------------------------------
Reporter: Stefan Hammer | Owner: Sarah
| Boyce
Type: Bug | Status: assigned
Component: File | Version: 5.1
uploads/storage |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35604#comment:4>

[Django]

#35604: Unexpected behaviour of FileSystemStorage.exists() due to latest changes
-------------------------------------+-------------------------------------
Reporter: Stefan Hammer | Owner: Sarah
| Boyce

Type: Bug | Status: closed

Component: File | Version: 5.1
uploads/storage |

Severity: Release blocker | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"8d6a20b656ff3fa18e36954668a44a831c2f6ddd" 8d6a20b]:
{{{#!CommitTicketReference repository=""
revision="8d6a20b656ff3fa18e36954668a44a831c2f6ddd"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35604#comment:5>

[Django]

#35604: Unexpected behaviour of FileSystemStorage.exists() due to latest changes
-------------------------------------+-------------------------------------
Reporter: Stefan Hammer | Owner: Sarah
| Boyce
Type: Bug | Status: closed
Component: File | Version: 5.1
uploads/storage |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"e42defb63b765b589fb8ef54e2d6773999d41939" e42defb]:
{{{#!CommitTicketReference repository=""
revision="e42defb63b765b589fb8ef54e2d6773999d41939"
[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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35604#comment:6>