Allow using unique=True with FileField

[Jon Ribbens]

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

[Tim Graham]

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.

[Jon Ribbens]

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

[Michael Scott]

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.

[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?

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

[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

[Tim Graham <timograham@…>]

In ec9ed07:

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

Thanks Tim Graham for the initial patch.

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