#14368 closed Bug (fixed)
Reverse relation attribute for OneToOneField fails when set to None
Reported by: | George Sakkis | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.2 |
Severity: | Normal | Keywords: | OneToOneField, bug |
Cc: | George Sakkis, Yeago, Filip Gruszczyński | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Initially reported by reopening #12420 (http://code.djangoproject.com/ticket/12420#comment:10), moved to a new ticket after the assignee's suggestion.
Test case with the sample models used at #14043:
In [6]: bob = Person.objects.create(age=34) In [7]: bobs_soul = Soul.objects.create(person=bob) In [8]: bob.soul == bobs_soul Out[8]: True In [9]: bob.soul = None --------------------------------------------------------------------------- AttributeError Traceback (most recent call last) --> 258 setattr(value, self.related.field.attname, getattr(instance, self.related.field.rel.get_related_field().attname)) 259 260 # Since we already know what the related object is, seed the related AttributeError: 'NoneType' object has no attribute 'person_id'
The offending lines are http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py#L258 and http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py#L264. It's clearly a bug because the comment at http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py#L238 explicitly mentions "If null=True, we can assign null here".
Attachments (2)
Change History (23)
comment:1 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 14 years ago
comment:3 by , 14 years ago
Cc: | added |
---|
comment:4 by , 14 years ago
At the very least, I expect a well-thought out exception message that explains what happened and why, like those checked for a few lines above:
if value is None and self.related.field.null == False: raise ValueError('Cannot assign None: "%s.%s" does not allow null values.' % (instance._meta.object_name, self.related.get_accessor_name())) elif value is not None and not isinstance(value, self.related.model): raise ValueError('Cannot assign "%r": "%s.%s" must be a "%s" instance.' % (value, instance._meta.object_name, self.related.get_accessor_name(), self.related.opts.object_name))
The fact that there is no such message for this case means that it was missed, it's not a deliberate choice.
As for what it should happen, I think it should do the equivalent of what the ForeignRelatedObjectsDescriptor does for foreign keys: make A.link_to_B = None
, B.link_to_A = None
and B.save()
, thus breaking the relation between A and B (we're always talking about a null=True
OneToOneField).
follow-up: 6 comment:5 by , 14 years ago
I disagree. Do you expect a message when you do: instance.queryset_set = SomeQueryset.objects.all()?
You're explicitly defining an accessor and expecting the ORM to know what you mean. In your example, soul isn't a field on Bob so stop treating it like it is one. person is a field on Soul.
comment:6 by , 14 years ago
I'm baffled, your comment doesn't make any sense.
I disagree. Do you expect a message when you do: instance.queryset_set = SomeQueryset.objects.all()?
Even if setting to None is not allowed, are you seriously against an error message that explains why, when ten lines above it does exactly that for non-nullable fields? And I have no idea what the instance.queryset_set = SomeQueryset.objects.all()
analogy is supposed to mean; it's perfectly valid code that does work, so I don't expect any message.
You're explicitly defining an accessor and expecting the ORM to know what you mean. In your example, soul isn't a field on Bob so stop treating it like it is one. person is a field on Soul.
So bob.soul = None
is "treating it like a field" but bob.soul = Soul.objects.create()
doesn't ?
comment:7 by , 14 years ago
Wow. I just tried bob.soul = Soul.objects.create() and I must admit you're right. I've never seen that syntax and I'm quite shocked it exists. Unfortunate really.
comment:8 by , 14 years ago
Cc: | added |
---|
I have created a unit test for this and a patch, but I don't know, if it is any good. I have several concerns. The test looks like this:
first = First.objects.create() second = Second.objects.create() first.second = second first.second = None self.assertRaises(Second.DoesNotExist, getattr, first, 'second') self.assert_(second.first is None)
and I believe this is an expected behaviour.
However, if we take the snippet from this ticket:
In [6]: bob = Person.objects.create(age=34) In [7]: bobs_soul = Soul.objects.create(person=bob) In [8]: bob.soul == bobs_soul Out[8]: True In [9]: bob.soul = None
then setting bob.soul to None doesn't have any effect, because bob.soul is created by a Query, so without changing data in the database you can't really affect it. I will post my patch and if someone could point me, how it should work, I will be glad to change it the way it should work.
by , 14 years ago
Attachment: | 14368.patch added |
---|
comment:9 by , 14 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
According to the discussion on django-dev [1]
, updating the database is the right thing to do and is consistent with the behaviour of ForeignKey for the analogous operation.
Also the second assertion (self.assert_(second.first is None)
) is impossible to enforce in general; at best you can do it for the existing cached related instance of first
. For example the second_copy
below will become stale, there is no way to magically update it:
first = First.objects.create() second = Second.objects.create() first.second = second second.save() second_copy = Second.objects.get(pk=second.pk) self.assert_(second_copy.first is not None) first.second = None # this can be made to succeed self.assert_(second.first is None) # but this cannot self.assert_(second_copy.first is None)
I'm working on a patch too, will hopefully post it later today.
by , 14 years ago
Attachment: | ticket-14368.patch added |
---|
Sets NULL the related_field of the existing child object (if any)
comment:10 by , 14 years ago
Patch needs improvement: | unset |
---|
comment:11 by , 14 years ago
Thanks for the comment. I'll download your patch and take a look at how it should have been done, so next time I can provide a better one.
comment:12 by , 14 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:13 by , 13 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:13 by , 13 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
The patch does no longer apply, and thus this isn't ready for commit.
I don't like the idea of doing an implicit save when assigning to the o2o field. Explicit saves are better. I also wrote to django-developers about removing the implicit save on foreign key assignments: http://groups.google.com/group/django-developers/browse_thread/thread/4558af28adef5271
comment:14 by , 9 years ago
Patch needs improvement: | unset |
---|
This still crashes as reported in the description.
I made an updated PR which doesn't have the implicit save of the original patch.
comment:15 by , 9 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
As noted on the ticket the changes and tests provided by Tim LGTM!
I'm not really sure this is a bug. What are you hoping to achieve by setting the accessor to be None?
If you have A.attribute which points to B, the information is still stored on B and so changes need to be made in the context of B.