Opened 15 years ago

Last modified 7 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 Raffaele Salmaso)

Saving FileFields with a none value sets the field to a empty string in the db and not NULL as it should.

Attachments (5)

filefield_null.diff (793 bytes ) - added by oyvind 15 years ago.
Patch that solves this issue, FieldFile's name can now be None also "is" and "==" is not the same
null-filefield.diff (2.0 KB ) - added by Alex Gaynor 15 years ago.
null-filefield-2.diff (2.4 KB ) - added by oyvind 15 years ago.
Meade FieldFile set name to u on delete, made test show filefields and charfields act the same
filefield_null.patch (1.4 KB ) - added by siroky@… 10 years ago.
None/NULL for FileField (v1.5.1)
nullable_file_field.diff (1.1 KB ) - added by Ondrej Pudiš 18 months ago.

Download all attachments as: .zip

Change History (25)

comment:1 by oyvind, 15 years ago

Patch does not seem to work quite like I expected. So the problem persists.

comment:2 by oyvind, 15 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 oyvind, 15 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 Alex Gaynor, 15 years ago

Attachment: null-filefield.diff added

comment:3 by Alex Gaynor, 15 years ago

Triage Stage: UnreviewedDesign decision needed

needs to be decided whether the CharField behavior should be used throughout, or NULL where explicitly requested

by oyvind, 15 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 Karen Tracey, 15 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 Thejaswi Puthraya, 15 years ago

Component: UncategorizedDatabase layer (models, ORM)

comment:6 by Chris Beaven, 13 years ago

Severity: Normal
Type: Bug

comment:7 by Aymeric Augustin, 12 years ago

Easy pickings: unset
Triage Stage: Design decision neededAccepted
UI/UX: unset

I see two possible solutions:

  • 1. document that FileField ignores the null keyword argument; this is the most backwards compatible solution;
  • 2. make FileField behave exactly like CharField 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 siroky@…, 10 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:9 by Aymeric Augustin, 10 years ago

It's waiting for someone to write a patch.

by siroky@…, 10 years ago

Attachment: filefield_null.patch added

None/NULL for FileField (v1.5.1)

comment:10 by siroky@…, 10 years ago

How about the filefield_null.patch? It follows solution 2.

comment:11 by Claude Paroz, 10 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 FileFieldTestsin tests/model_fields/tests.py.

comment:12 by anonymous, 10 years ago

How can I run the FileFieldTests without the whole suite?

comment:14 by siroky@…, 10 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:16 by Claude Paroz, 8 years ago

#25528 was a duplicate.

comment:17 by Raffaele Salmaso, 3 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 Raffaele Salmaso, 3 years ago

Description: modified (diff)

comment:19 by dennisvang, 22 months 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 even my_filefield__in=['', None] (example)
  • FileField(null=True, unique=True) is broken, because the database entry is '' instead of null, so we get a unique-constraint violation if we try to create another instance with None

The latter is basically what is described in the model field reference:

... One exception is when a CharField has both unique=True and blank=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?

Last edited 22 months ago by dennisvang (previous) (diff)

comment:20 by Ondrej Pudiš, 18 months ago

Has patch: unset
Needs tests: unset
Owner: changed from nobody to Ondrej Pudiš
Status: newassigned

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 Ondrej Pudiš, 18 months ago

Attachment: nullable_file_field.diff added

comment:21 by Ondrej Pudiš, 18 months 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.

Note: See TracTickets for help on using tickets.
Back to Top