Opened 6 years ago

Closed 6 years ago

Last modified 2 years ago

#27188 closed New feature (fixed)

Allow using unique=True with FileField

Reported by: Jon Ribbens Owned by: Michael Scott
Component: File uploads/storage Version: 1.10
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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.)

Attachments (1)

27188.diff (3.4 KB) - added by Tim Graham 6 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 6 years ago by Tim Graham

Summary: FileField cannot be used with unique=TrueAllow using unique=True with FileField
Triage Stage: UnreviewedAccepted
Type: BugNew feature

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.

Changed 6 years ago by Tim Graham

Attachment: 27188.diff added

comment:2 Changed 6 years ago by Jon Ribbens

That's great! If this works then presumably there's no reason the primary_key prevention check should be there either...

comment:3 Changed 6 years ago by Michael Scott

Owner: changed from nobody to Michael Scott
Status: newassigned

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.

comment:4 Changed 6 years ago 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?

comment:5 Changed 6 years ago 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.

comment:6 Changed 6 years ago 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

comment:7 Changed 6 years ago by Tim Graham

Has patch: set

comment:8 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In ec9ed07:

Fixed #27188 -- Allowed using unique=True with FileField.

Thanks Tim Graham for the initial patch.

comment:9 Changed 2 years ago 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.

It might be a good idea to reopen the discussion about this if you're interested.

Last edited 2 years ago by Sultan (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top