Opened 16 years ago
Last modified 14 months ago
#10244 assigned Bug
FileFields can't be set to NULL in the db
Reported by: | oyvind | Owned by: | Ondrej Pudiš |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.0 |
Severity: | Normal | Keywords: | filefield NULL empty |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Saving FileFields with a none value sets the field to a empty string in the db and not NULL as it should.
Attachments (5)
Change History (25)
comment:1 by , 16 years ago
comment:2 by , 16 years ago
Example to test this issue:
class File(models.Model): file = models.FileField(null=True, upload_to='files')
from models import File f = File() f.file = None f.save() from django.db import connection print connection.queries
by , 16 years ago
Attachment: | filefield_null.diff added |
---|
Patch that solves this issue, FieldFile's name can now be None also "is" and "==" is not the same
by , 16 years ago
Attachment: | null-filefield.diff added |
---|
comment:3 by , 16 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
needs to be decided whether the CharField behavior should be used throughout, or NULL where explicitly requested
by , 16 years ago
Attachment: | null-filefield-2.diff added |
---|
Meade FieldFile set name to u on delete, made test show filefields and charfields act the same
comment:4 by , 16 years ago
Dev list discussion: http://groups.google.com/group/django-developers/browse_thread/thread/cf37abc95e49f0e/
Also, #10749 reported surprise at searching for None not working when looking for empty ImageFields. I still think changing this now breaks backwards-compatibility, though.
comment:5 by , 16 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|
comment:6 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:7 by , 13 years ago
Easy pickings: | unset |
---|---|
Triage Stage: | Design decision needed → Accepted |
UI/UX: | unset |
I see two possible solutions:
- 1. document that
FileField
ignores thenull
keyword argument; this is the most backwards compatible solution; - 2. make
FileField
behave exactly likeCharField
wrt.null
; this is the most consistent solution.
Option 2. will be backwards incompatible only for people who set null=True
on a FileField
. The docs discourage that heavily, and it doesn't behave as expected, actually it doesn't even do anything, so there isn't much reason to have such code. That's why I think the change would be acceptable.
I have a slight preference for option 2, which is better in the long term.
comment:8 by , 11 years ago
Hi! What is the status of this ticket? I see it is 5 years old ticket and the problem is still present at least in 1.5.1.
comment:11 by , 11 years ago
Has patch: | set |
---|---|
Needs documentation: | set |
Needs tests: | set |
Thanks, it's a first step :-) However, tests are crucial with such a change. That will be the next step. See FileFieldTests
in tests/model_fields/tests.py
.
comment:13 by , 11 years ago
comment:14 by , 11 years ago
I bumped into a problem. If instance.null_file_field
is None then methods like save()
will not work (AttributeError: 'NoneType' object has no attribute 'save'). I'm thinking about 2 solutions:
1) Make the descriptor return value comparable to None (via __eq__
). This is not very clean because the best practise is to use operator is
(is_none = x is None
).
2) Keep the empty string ("") on the Python side as a representation of file's database NULL. Not very consistent but it has better backward compatibility if someone already uses the string comparison (has_no_file = filefield.name == ""
).
Any suggestions?
comment:17 by , 4 years ago
Description: | modified (diff) |
---|
Hi all, what is the status of this ticket?
FileField
behaviour is not really clear right now and flake8-django
just made a change to allow FileField(null=True)
(see https://github.com/rocioar/flake8-django/issues/82), which seems wrong to me.
I'm for option 2 make FileField behave exactly like CharField wrt. null
.
comment:18 by , 4 years ago
Description: | modified (diff) |
---|
comment:19 by , 2 years ago
From ten years ago:
... only for people who set null=True on a FileField. The docs discourage that heavily ...
As far as I can see, there is no mention of null
, at all, in the current documentation for FileField, nor is FileField
mentioned in the documentation for field option null.
Yet Django's FileField
behavior is still plainly inconsistent, compared to other fields, when it comes to null=True
.
A short summary:
We can specify null=True
, and we can assign None
to the field, but this is stored as a FieldFile
with name=None
(initially) on the python side, and it is stored as an empty string ''
in the database. After e.g. refresh_from_db
, the FieldFile.name
will also be an empty string, instead of None
.
Basically, the following snippet passes, where my_filefield
is a FileField
with null=True
:
obj = MyModel.objects.create(my_filefield=None) assert not MyModel.objects.filter(my_filefield__isnull=True).exists() assert obj.my_filefield is not None assert isinstance(obj.my_filefield, FieldFile) assert obj.my_filefield.name is None obj.refresh_from_db() assert obj.my_filefield.name is not None assert obj.my_filefield.name == ''
Some resulting inconveniences:
MyModel.objects.create(my_filefield=None).my_filefield is not None
my_model_instance.my_filefield
is always "truthy", so we need to check e.g.my_filefield.name
(example)- we cannot use the
__isnull
field lookup, but have to check for equality with the empty string (my_filefield=''
), or evenmy_filefield__in=['', None]
(example) FileField(null=True, unique=True)
is broken, because the database entry is''
instead ofnull
, so we get a unique-constraint violation if we try to create another instance withNone
The latter is basically what is described in the model field reference:
... One exception is when a
CharField
has bothunique=True
andblank=True
set. In this situation,null=True
is required to avoid unique constraint violations when saving multiple objects with blank values.
So, currently, we cannot make a FileField
unique and optional at the same time, in the database.
This also breaks e.g. get_or_create
for FileField
.
Why not make a backward incompatible change for the next major release, for the sake of consistency?
comment:20 by , 2 years ago
Has patch: | unset |
---|---|
Needs tests: | unset |
Owner: | changed from | to
Status: | new → assigned |
As I understand the issue, if the file field is specified as nullable, it should accept None
and in that case, store NULL
on the database level instead of an empty string. In fact, it shouldn't even create an instance of FieldFile
when creating a new NullableDocument
object.
The tests listed below are broken now but the patch should fix all of them giving a solution to all the issues enumerated in the comment above.
def test_create_empty(self): # when I create a new document with no file provided d = NullableDocument.objects.create(myfile=None) # I expect that the attribute itself is None self.assertIs(d.myfile, None) # I expect that I can filter the documents and find the one query = NullableDocument.objects.filter(myfile__isnull=True).exists() self.assertTrue(query) # I expect that the object remains None even after refreshing from the DB d.refresh_from_db() self.assertIs(d.myfile, None) def test_create_empty_multiple(self): # when the files are expected to be unique but nullable, I expect that I'm # allowed to create multiple records NullableDocument.objects.create(myfile=None) NullableDocument.objects.create(myfile=None) # and both are created query = NullableDocument.objects.filter(myfile__isnull=True).count() self.assertEqual(query, 2) def test_create_empty_on_not_null(self): # when I try to store an empty file on non-nullable model, I expect to get an # integrity error with self.assertRaises(IntegrityError): Document.objects.create(myfile=None)
by , 2 years ago
Attachment: | nullable_file_field.diff added |
---|
comment:21 by , 2 years ago
Has patch: | set |
---|
My proposed patch is to not create an instance of FieldFile
when the file is None
. However, this is a significant breaking change as people might be accessing the FileField
and checking for name
or path
being an empty string.
Many tests would need to be adjusted to this new logic.
As suggested earlier, another possibility would be to store None
on the DB level while interpreting it as an empty FieldFile
on the application level. This cloud introduce more confusion to the whole mechanics of the files storing.
I believe that the first approach with not creating the FieldFile
object when the file doesn't actually exists is a cleaner solution which is consistent with other types of database fields.
Patch does not seem to work quite like I expected. So the problem persists.