Re: [Django] #10244: FileFields can't be set to NULL in the db
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: filefield NULL | Needs documentation: 0
empty | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* ui_ux: => 0
* easy: => 0
* stage: Design decision needed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:7>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: filefield NULL | Needs documentation: 0
empty | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by siroky@…):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: filefield NULL | Needs documentation: 0
empty | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
It's waiting for someone to write a patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: filefield NULL | Needs documentation: 0
empty | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by siroky@…):
How about the filefield_null.patch? It follows solution 2.
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
empty | Patch needs improvement: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by claudep):
* needs_docs: 0 => 1
* has_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: filefield NULL | Needs documentation: 1
empty | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
How can I run the FileFieldTests without the whole suite?
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: filefield NULL | Needs documentation: 1
empty | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
See https://docs.djangoproject.com/en/dev/internals/contributing/writing-
code/unit-tests/#running-only-some-of-the-tests
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: filefield NULL | Needs documentation: 1
empty | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by siroky@…):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: filefield NULL | Needs documentation: 1
empty | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
So? :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) |
Keywords: filefield NULL | Triage Stage: Accepted
empty |
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by claudep):
#25528 was a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: filefield NULL | Triage Stage: Accepted
empty |
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> Saving FileFields with a none value sets the field to a empty string in
> the db and not NULL as it should.
New description:
Hi,
--
Comment (by Raffaele Salmaso):
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: filefield NULL | Triage Stage: Accepted
empty |
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> Hi,
New description:
Saving FileFields with a none value sets the field to a empty string in
the db and not NULL as it should.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: filefield NULL | Triage Stage: Accepted
empty |
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by dvg):
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
[https://docs.djangoproject.com/en/4.0/ref/models/fields/#filefield
FileField].
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` on the python side, and
it is stored as an empty string `''` in the database.
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` ([https://stackoverflow.com/q/8850415 example])
- we cannot use the `__isnull` field lookup, but have to check for
equality with the empty string `''` ([https://stackoverflow.com/q/4771464
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
[https://docs.djangoproject.com/en/4.0/ref/models/fields/#django.db.models.Field.null
model field reference] (my emphasis):
> ... 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.
Why not just bite the bullet and make a backward incompatible change, for
the sake of consistency?
[1]:
https://docs.djangoproject.com/en/4.0/ref/models/fields/#django.db.models.FileField.storage
[2]: https://docs.djangoproject.com/en/4.0/ref/models/fields/#filefield
[3]: https://stackoverflow.com/q/8850415
[4]:
https://docs.djangoproject.com/en/4.0/ref/models/fields/#django.db.models.Field.null
[5]: https://stackoverflow.com/q/4771464
[7]:
https://docs.djangoproject.com/en/4.0/ref/models/fields/#django.db.models.fields.files.FieldFile
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:19>
Django
-------------------------------------+-------------------------------------
| Pudiš
Type: Bug | Status: assigned
Component: Database layer | Version: 1.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: filefield NULL | Triage Stage: Accepted
empty |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Ondrej Pudiš
* status: new => assigned
* has_patch: 1 => 0
* needs_tests: 1 => 0
Comment:
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: Ondrej
| Pudiš
Type: Bug | Status: assigned
Component: Database layer | Version: 1.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: filefield NULL | Triage Stage: Accepted
empty |
Has patch: 0 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Ondrej Pudiš):
* Attachment "nullable_file_field.diff" added.
--
Ticket URL: <https://code.djangoproject.com/ticket/10244>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: Ondrej
| Pudiš
Type: Bug | Status: assigned
Component: Database layer | Version: 1.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: filefield NULL | Triage Stage: Accepted
empty |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Ondrej Pudiš):
* has_patch: 0 => 1
Comment:
[https://code.djangoproject.com/attachment/ticket/10244/nullable_file_field.diff
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: Ondrej
| Pudiš
Type: Bug | Status: assigned
Component: Database layer | Version: 1.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: filefield NULL | Triage Stage: Accepted
empty |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by daway0):
the one who didn't path this after 15 years...
[[Image(https://images.genius.com/84fe15e61ad5563e8fa1e315a1385f79.892x926x1.png)]]
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: Ondrej
| Pudiš
Type: Bug | Status: assigned
Component: Database layer | Version: 1.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: filefield NULL | Triage Stage: Accepted
empty |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Adam Zapletal (added)
Django
-------------------------------------+-------------------------------------
Reporter: oyvind | Owner: Ondrej
| Pudiš
Type: Bug | Status: assigned
Component: Database layer | Version: 1.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: filefield NULL | Triage Stage: Accepted
empty |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
I started a forum thread about this:
https://forum.djangoproject.com/t/ticket-10244-filefield-cant-be-set-to-
null-in-the-database/37137
Ondrej Pudiš, are you still interested in working on this ticket?
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:23>
Django
-------------------------------------+-------------------------------------
| Zapletal
Component: Database layer | Version: 1.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: filefield NULL | Triage Stage: Accepted
empty |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Ondrej Pudiš => Adam Zapletal
--
Ticket URL: <https://code.djangoproject.com/ticket/10244#comment:24>