#11811 closed Bug (fixed)
No error raised on update(foreignkey=unsavedobject) on nullable fk
Reported by: | Afief | Owned by: | Aymeric Augustin |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.1 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Found this while coding at 2AM:
When there is a field that can be NULL, and you do something like this:
obj1=Obj1(...)
obj2=Obj2.objects.all().update(foreignobj=obj1)
django will assign the fields to NULL.
I'm not sure if this behaviour is made to be so by design, but it seems to me that if someone passes an unsaved object instead of None they made a mistake and django should raise an exception.
Attachments (2)
Change History (19)
comment:1 by , 15 years ago
milestone: | → 1.2 |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
by , 15 years ago
Attachment: | 11811-patch-rev-3.diff added |
---|
comment:3 by , 15 years ago
The same surprising behavior occurs whether you assign to the ForeignKey field by attribute assignment, a model initializer, model.objects.create(), or queryset.update(). In any of the cases, passing an unsaved model acts the same as passing None. (That's assuming no manual pk assignment.) The first three use a different code path than the last.
I'm attaching a strawman patch that will raise a ValueError with a helpful message in all four cases. The docs had left the behavior undefined.
However, the patch needs further work:
- Raising an exception at assignment time may be a bad idea, even though the inconsistency between my_fk_field (an object) and my_fk_field_id (None) starts then. Everything would have worked OK while the same object remained in memory; only the copy saved in the DB will have no link at all to the other instance. Checking at save time may be better.
- To match the scope of the ticket, I'm only handling assignment for non-null fields. Non-nullable fields already raise an exception, though it occurs later (on save), and the error message is less specific as a result. But the queryset.update() code path now always throws the new exception regardless of whether the field is nullable, because field information isn't available to it.
- Two tests of multi-DB support now fail, because they make the same mistake the new exception is designed to guard against. Either putting explicit IDs in the tests or changing the timing of the exceptions as per point 1 may help. The rest of the test suite has no additional failures.
The patch includes tests but not docs. I'll attach a docs patch separately once the implementation settles down.
comment:4 by , 15 years ago
I shouldn't have said "passing an unsaved model acts the same as passing None". That's true only for the ForeignKeyField as saved in the DB. (It doesn't matter whether the unsaved foreign model itself is saved after the assignment.) In memory, the attribute still points to the unsaved model instance.
by , 15 years ago
Attachment: | 11811-patch-rev-5.diff added |
---|
Patch (with error on save instead of on assignment)
comment:5 by , 15 years ago
Has patch: | set |
---|
The updated patch (rev-5) doesn't raise exceptions for model instances that are never saved anyway, which fixes the multi-DB test failures.
The one remaining design issue is a backward compatibility vs. consistency tradeoff. The patch errs on the side of backward compatibility: if the foreign key field is declared non-null, assignment works as before, and lets the database raise its own integrity error instead of the new ValueError exception with a detailed error message. The queryset.update() patch can't tell how the field is declared, though, so it always raises ValueError. Raising ValueError in all cases would be more consistent and probably more helpful, but it could break compatibility for code that's already expecting to catch a particular integrity error.
Also, the patch could attempt to be smart. When it detects that the foreign key value is missing but there is a cached instance, it could fill the key in from that instance, in case the instance was saved after the assignment. But that's starting to get magical, so it doesn't do that.
comment:6 by , 15 years ago
Here are some examples to make this more concrete.
# Nullable foreign key field, without patch, previous surprising behavior: >>> s = Site(name='My new site') >>> f = Flatpage(id=1, text='I am being published on that site') >>> f.site = s # set a site for the Flatpage >>> s.save() # (site is saved now, but doesn't change outcome) >>> f.site # site value is still there, looks good <Site: My new site>
Old behavior from that point:
>>> f.save() # save it to DB >>> f = Flatpage.objects.get(id=1) # read it back >>> print f.site # ...and the Flatpage's site value disappears! None
Patched behavior from that point:
>>> f.save() # will now raise an exception about the anomaly Traceback (most recent call last): ... ValueError: Cannot save a foreign key referring to the instance "<Site: My new site>" into the field "Flatpage.site", because the key value for the instance was not available at the time of assignment to the field. (For autonumbered keys, the related instance may need to be saved prior to the assignment.)
The behavior for assignment to non-null fields with the rev-5 patch is the same as before:
# Test a non-null field. Behavior is unchanged. >>> d = Department(name='An Unsaved Department') >>> w = Worker(id=1, name='Milton Waddams', department=d) >>> w.save() Traceback (most recent call last): ... IntegrityError: model_regress_worker.department_id may not be NULL or Warning: Column 'department_id' cannot be null or other DB-specific error
However, the batch queryset.update() method raises a new ValueError exception regardless of nullability:
>>> d1 = Department(id=1, name='A Real Department') >>> d1.save() >>> d2 = Department(name='An Unsaved Department') >>> Worker.objects.create(id=2, name='Milton Waddams', department=d1) <Worker: Milton Waddams> >>> Worker.objects.filter(id=2).update(department=d2) Traceback (most recent call last): ... ValueError: Cannot assign the instance "<Department: An Unsaved Department>" to a foreign key field, because its primary key is not available. (For autonumbered keys, the instance would need to be saved first.)
comment:7 by , 15 years ago
Needs documentation: | set |
---|
comment:9 by , 13 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:14 by , 12 years ago
Owner: | changed from | to
---|
comment:15 by , 11 years ago
comment:16 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Patch