[Django] #27188: FileField cannot be used with unique=True
Django
--------------------------------------+--------------------
Reporter: jribbens | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+--------------------
In #4345 , code was added which explicitly prevents the use of
`unique=True` on `FileField` fields. This seems bizarre and wrong, since
making unique FileFields not only makes sense, it would be rare for
''not'' making them unique to make sense - after all, you can't have two
different files with the same filename! Not allowing `unique=True` leads
to impaired data integrity as the database can't therefore enforce the
necessary uniqueness of filenames. (Looking at the previous ticket, it
appears people were confused that it would be the file contents that was
supposed to be unique, not the filenames.)
--
Ticket URL: <https://code.djangoproject.com/ticket/27188>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: jribbens | Owner: nobody
Type: New feature | Status: new
Component: File uploads/storage | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
* type: Bug => New feature
* stage: Unreviewed => Accepted
Comment:
I did a quick check and didn't get the "can't adapt" error from #4345 when
uploading to a unique `ImageField` in the admin. I'm attaching a partial
patch, missing tests and release notes. Probably at least a test in
`tests/model_fields/test_filefield.py` that verifies unique validation
works correct is needed. It would be good to do some manual testing to try
to identify anything that doesn't work.
--
Ticket URL: <https://code.djangoproject.com/ticket/27188#comment:1>
Django
--------------------------------------+------------------------------------
Type: New feature | Status: new
Component: File uploads/storage | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* Attachment "27188.diff" added.
Django
--------------------------------------+------------------------------------
Type: New feature | Status: new
Component: File uploads/storage | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by jribbens):
That's great! If this works then presumably there's no reason the
`primary_key` prevention check should be there either...
--
Ticket URL: <https://code.djangoproject.com/ticket/27188#comment:2>
Django
Reporter: jribbens | Owner: mscott250
Type: New feature | Status: assigned
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by mscott250):
* status: new => assigned
* owner: nobody => mscott250
Comment:
From reading the ticket details I don't believe someone has picked this
up, so I'll assign it to myself, please let me know if this isn't the case
though.
--
Ticket URL: <https://code.djangoproject.com/ticket/27188#comment:3>
Django
-------------------------------------+-------------------------------------
| Scott
Type: New feature | Status: assigned
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Michael Scott):
I did a little testing over the weekend on the behaviour of FileField's
after the patch has been applied. In general things behaved as you would
expect them to, when unique is set to True then it doesn't allow you to
save more than one instance of the model when a duplicate file name is
used for the FileField field.
However there is a bit of unexpected behaviour when using unique
FileField's across separate models. If the same directory is used, which
by default is the MEDIA_ROOT setting, to store the files from the multiple
models, then the unique constraint may have no effect. This is because
when the (default) storage object saves the file, it checks if the
destination directory already contains a file with the specified name, and
if so it generates a unique file name instead (appends a unique suffix to
the end). In this case that generated file name is what's stored in the
database and therefore as the generated file name is unique the constraint
has no effect.
For example, with models ModelFoo and ModelBar, each having a FileField
with unique set to True. If saving an instance of ModelFoo with file name
"example.txt", the first time will succeed, then if trying to save another
instance of ModelFoo with the same file, the second time will fail as
expected because the table already has a entry in it with that file name.
However if then saving an instance of ModelBar with the same name, then
the storage object generates a unique file name such as
"example_abfgr.txt" as "example.txt" already exists in the directory, and
saves the ModelBar instance with that name. Then when saving another
instance of ModelBar with the same file, the same thing happens again,
meaning the unique constraint has no effect.
I hope this all makes sense. Is the above behaviour something we need to
worry about?
--
Ticket URL: <https://code.djangoproject.com/ticket/27188#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: Michael
| Scott
Type: New feature | Status: assigned
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Michael Scott):
Also, do we want to remove the primary_key restriction within this ticket
or should it be left to another ticket?
I've now got a branch with the Tim's patch applied, some additional unit
tests added and the documentation updated.
--
Ticket URL: <https://code.djangoproject.com/ticket/27188#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: Michael
| Scott
Type: New feature | Status: assigned
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Michael Scott):
I've made the suggested changes, under the assumption that we don't mind
about the above mentioned behaviour, and created a pull request for the
changes under https://github.com/django/django/pull/7424 . If the
mentioned issue is a blocker for merging these changes in, then please let
me know and I imagine we can look at ways to address it, I figured it'd be
better for me to create a pull request at this stage, to try and avoid the
ticket/branch going stale.
Please let me know if there are any other issues with the proposed changes
or any potential improvements as well.
Cheers,
Michael
--
Ticket URL: <https://code.djangoproject.com/ticket/27188#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: Michael
| Scott
Type: New feature | Status: assigned
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27188#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: Michael
| Scott
Component: File | Version: 1.10
uploads/storage |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ec9ed07488aa46fa9d9fd70ed99e82fdf184632a" ec9ed07]:
{{{
#!CommitTicketReference repository=""
revision="ec9ed07488aa46fa9d9fd70ed99e82fdf184632a"
Fixed #27188 -- Allowed using unique=True with FileField.
Thanks Tim Graham for the initial patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27188#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: Michael
| Scott
Type: New feature | Status: closed
Component: File | Version: 1.10
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Sultan):
Why not implement this with the `Model.validate_unique()` method? It was
created precisely for this. It is already using resources for connecting
to the database and checking that there is no duplication.
I think that before `Model.validate_unique()` is called, we need first to
prepare the final `file.name` value by calling the
`FileFIeld.generate_filename()` method. The most appropriate place to do
this is in the `FileDescriptor.__set__` method, rather than in the
`FieldFile.save()` method, which I prefer, so that it always only accepts
the final filename.
--
Ticket URL: <https://code.djangoproject.com/ticket/27188#comment:9>