Re: [Django] #10227: OneToOne fields with null=True raise DoesNotExist exception on related model
Django
model
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New | Status: new
feature | Component: Database layer
Milestone: | (models, ORM)
Version: SVN | Severity: Normal
Resolution: | Keywords: onetoone related
Triage Stage: Accepted | expection null
Needs documentation: 0 | Has patch: 0
Patch needs improvement: 0 | Needs tests: 0
UI/UX: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by anonymous):
* ui_ux: => 0
Comment:
I came up with this work around as well, to check that when a user is
saved it has an associated userprofile
@receiver(post_save, sender=User)
def user_saved(sender, instance, created, **kwargs):
if not hasattr(instance, 'userprofile'):
user_profile = UserProfile()
user_profile.user = instance
user_profile.save()
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:14>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
model
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New | Status: new
feature | Component: Database layer
Milestone: | (models, ORM)
Version: SVN | Severity: Normal
Resolution: | Keywords: onetoone related
Triage Stage: Accepted | expection null
Needs documentation: 0 | Has patch: 0
Patch needs improvement: 0 | Needs tests: 0
UI/UX: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Comment (by olau):
Rereading this, can I suggest that related_default does not have to be a
function? It would make things a bit clearer if one could say
related_default=None instead =lambda x: None.
I realize this means you have to apply slight cleverness when defining the
parameter since you can't use None, e.g.
{{{
class RelatedDefaultRaiseException: pass
def __init__(..., related_default=RelatedDefaultRaiseException):
if related_default == RelatedDefaultRaiseException:
# turn on exception raising behaviour
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:15>
Django
model
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New | Status: new
feature | Component: Database layer
Milestone: | (models, ORM)
Version: SVN | Severity: Normal
Resolution: | Keywords: onetoone related
Triage Stage: Accepted | expection null
Needs documentation: 0 | Has patch: 0
Patch needs improvement: 0 | Needs tests: 0
UI/UX: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
* cc: gsakkis (removed)
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:16>
Django
model
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Component: Database layer | Version: SVN
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by sebastian):
* cc: sebastian (added)
Comment:
I'd also like to see the functionality of the reverse side not throwing an
exception. This would also make both sides of the one-to-one relation
behave more symmetric. I agree that an additional parameter is the way to
go, since all four types of blank/non-blank make sense (comment:7).
Can gsakkis' snippet (comment:10) be adapted to the current trunk version?
I agree that passing `None` or constant values to `related_default` should
be possible in addition to passing in functions/lambdas, as it makes the
model definition much cleaner in the case of simple values.
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:17>
Django
model
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: SVN
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by sebastian):
Alright, here is another inconsistent behavior that falls in the lines of
raising or not raising an exception.
The behavior of the reverse lookup for one-to-one-fields changes when
`select_related` has been called on the reverse relation. In that case,
accessing the reverse relation does not raise an exception but returns
`None` in the case of a missing related object. This feels rather strange
to me, it should either always raise an exception or never.
I am not sure if gsakkis' snippet addresses this issue.
Here is an example:
{{{
class A(models.Model):
pass
class B(models.Model):
a = models.OneToOneField(A)
########
a = A.objects.create()
A.objects.get(pk=a.pk).b # raises B.DoesNotExist
A.objects.select_related('b').get(pk=a.pk).b # returns None
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:18>
Django
model
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: SVN
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by lrekucki):
Replying to [comment:18 sebastian]:
> Alright, here is another inconsistent behavior that falls in the lines
of raising or not raising an exception.
>
> The behavior of the reverse lookup for one-to-one-fields changes when
`select_related` has been called on the reverse relation. In that case,
accessing the reverse relation does not raise an exception but returns
`None` in the case of a missing related object. This feels rather strange
to me, it should either always raise an exception or never.
This is a bug described in #13839. In context of this ticket, it should
always raise an exception, unless {{{related_null=True}}} is given, in
which case it should always return {{{None}}}.
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:19>
Django
model
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: SVN
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: aaugustin (added)
Comment:
I just spent some time on this bug while investigating related problems.
I agree that the proper solution is to add a `related_default` parameter
to `OneToOneField`. Like `default`, this parameter could be a value or a
callable.
Independently, there are some bugs and inconsistencies in the caching of
the lack of a value (#13839 and #17439). Fixing this bug would provide a
way to hide some of these problems, but not address their root cause.
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:20>
Django
model
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: cvrebert (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:21>
Django
model
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: frankoid (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
This problem was discussed again in #18153. I still believe that Luke's
comment 13 nails it.
I'm updating the title to better reflect the design decision.
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
The problem is that changing reverse o2o field handling will break
existing code. The API might not be ideal, but that is what we got.
Adding a related_default argument doesn't seem like a nice solution to me.
Specifically, if the related_default can be anything else than None it
will lead to surprising situations. Also, having the API of obj.o2ofield
change based on field setting could be somewhat annoying...
I would love to have a more generic approach to this, something like a
"nget()" method which returns None if no objects found. We could hook this
into the fields by:
{{{
# works like obj.o2ofield except no DoesNotExist...
obj.nget('o2ofield')
# Could also be used as
Code.objects.nget(type='imei', value='bar')
}}}
This is nicer to write than:
{{{
try:
res = obj.o2ofield
except obj.DoesNotExist:
res = None
}}}
To me this seems like an explicit and easy to implement approach. Does
this approach seem workable to others? If not, IMO we should just wontfix
this issue, as I don't see any nice way to change the API without breaking
existing code.
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by olau):
As much as I would love to see a A.objects.none_get() or perhaps a more
Pythonic A.objects.getdefault(), IMHO that is orthogonal to this issue. I
don't think your obj.nget('o2ofield') is intuitive.
Regarding related_default: I just need None (that's why I proposed
related_null), but you can conceivably do a lambda that makes sense, e.g.
{{{
class UserProfile(models.Model):
user = models.OneToOneField(User, related_default=lambda u:
UserProfile(user=u))
...
if not request.user.profile.name:
request.user.profile.name = ...
request.user.profile.save()
}}}
And it is arguably relatively in line with the existing API, except the
lambda must receive the instance which I don't believe it does for the
ordinary default (without the instance, you can't set the one-to-one key
to the original object and then it's really confusing :).
I don't understand why you think it would be a problem to change the API
based on a field setting. That's already happening with null and default
today?
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
The example of related_default usage is exactly what I am afraid. That
code contains a serious pitfall: the user.profile isn't saved, and it will
not be associated to the user if you don't save it. And, you do get that
default value even for saved user instances.
There is a big difference to how foreign key default argument works:
{{{
class A(models.Model):
pass
def get_a():
return A.objects.get_or_create(pk=1)[0]
class B(models.Model):
a = models.ForeignKey(A, null=True, default=get_a)
b = B()
print b.a
b = B(a=None)
print b.a
b = B(a=None)
b.save()
b = B.objects.get()
print b.a
}}}
The output is:
{{{
A object
None
None
}}}
My understanding is that the "related_default" would return "A object" for
each case. This is broken - what you get from b.a doesn't reflect the
database state.
The related_none could work. Here I am mainly worried of not knowing what
happens when I write:
{{{
p = request.user.profile
}}}
does it raise `DoesNotExist` if the profile doesn't exists, or will p just
be None? However, I do not feel too strongly about this case.
The good thing about b.nget('a') is that when you see the code, you
immediately know what the code does. You are explicitly asking for a value
that can be None value, so you will be prepared to handle the None case.
This is relatively simple to do in user code, so there is no strong reason
to have this convenience method in Django core.
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by olau):
Well, if the conceptual model is adding objects lazily, it's not really a
problem that you need to save the user profile to get your changes to
stick - they wouldn't stick otherwise. My example would never have worked
without the save if the intention was to change the name persistently.
But it's true that one would have to careful to make sure the user object
was saved so it had an id. You have the same problem if you write the code
manually, though.
I had some trouble understanding your example on the difference to the
foreign key default parameter - it seems what you are thinking about is a
zero-or-one to one mapping, not a zero-or-one to zero-to-one (that would
be related_default=None). Yes, it's less powerful in the related end in
that you can't avoid confounding null and default. But there's no way
around that. When I said relatively in line with the existing API, I just
meant that the conceptual idea of returning an in-memory object when
nothing exists yet was the same.
Thanks for looking at this, by the way. :)
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: mike@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: onetoone related | Needs documentation: 0
expection null | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Old description:
> Referencing a {{{OneToOneField}}} with {{{null=True}}} (i.e. it's
> optional) when there is no value set will return {{{None}}}, as expected.
> However, referencing the reverse side of the relation does not follow
> this behavior and raises a {{{DoesNotExist}}} exception.
>
> For example, in the following situation where {{{Shop}}}s have optionally
> a {{{Place}}} (e.g. webshops need not have a physical location):
> {{{
> class Place(models.Model)
> address = models.CharField(max_length=80)
> class Shop(models.Model)
> place = models.OneToOneField(Place, null=True)
> name = models.CharField(max_length=50)
> website = models.URLField()
> }}}
> This ''does'' work as expected:
> {{{
> >>> s1 = Shop.objects.create(name='Shop', website='shop.com')
> >>> print s1.place
> None
> }}}
> But this ''doesn't'' work as expected:
> {{{
> >>> p1 = Place.objects.create(address='123 somestr')
> >>> p1.shop
> ... [exception stack trace] ...
> DoesNotExist: Shop matching query does not exist.
> }}}
> I would expect this to be {{{None}}} when {{{null}}} is allowed on the
> {{{OneToOneField}}}.
>
> Please correct my if I'm wrong.
>
> I have attached a patch to fix this (checking if {{{null}}} is allowed
> and returning {{{None}}} or raising the exception appropriately),
> including tests.
>
> Unfortunately this is slightly backwards incompatible, since someone may
> currently rely on the exception being thrown.
New description:
EDIT: scroll to comment 13 to see what this ticket is about.
Referencing a {{{OneToOneField}}} with {{{null=True}}} (i.e. it's
optional) when there is no value set will return {{{None}}}, as expected.
However, referencing the reverse side of the relation does not follow this
behavior and raises a {{{DoesNotExist}}} exception.
For example, in the following situation where {{{Shop}}}s have optionally
a {{{Place}}} (e.g. webshops need not have a physical location):
{{{
class Place(models.Model)
address = models.CharField(max_length=80)
class Shop(models.Model)
place = models.OneToOneField(Place, null=True)
name = models.CharField(max_length=50)
website = models.URLField()
}}}
This ''does'' work as expected:
{{{
>>> s1 = Shop.objects.create(name='Shop', website='shop.com')
>>> print s1.place
None
}}}
But this ''doesn't'' work as expected:
{{{
>>> p1 = Place.objects.create(address='123 somestr')
>>> p1.shop
... [exception stack trace] ...
DoesNotExist: Shop matching query does not exist.
}}}
I would expect this to be {{{None}}} when {{{null}}} is allowed on the
{{{OneToOneField}}}.
Please correct my if I'm wrong.
I have attached a patch to fix this (checking if {{{null}}} is allowed and
returning {{{None}}} or raising the exception appropriately), including
tests.
Unfortunately this is slightly backwards incompatible, since someone may
currently rely on the exception being thrown.
--
Comment (by aaugustin):
Replying to [comment:26 akaariai]:
> The example of related_default usage is exactly what I am afraid. That
code contains a serious pitfall: the user.profile isn't saved, and it will
not be associated to the user if you don't save it. And, you do get that
default value even for saved user instances.
This was (probably) fixed by #10811.
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:29>
Django
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Keywords: onetoone related | Triage Stage: Accepted
expection null |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Ryan Hiebert):
* cc: Ryan Hiebert (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:30>
Django
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: onetoone related | Triage Stage: Accepted
expection null |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Peter Thomassen (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:31>
Django
-------------------------------------+-------------------------------------
Reporter: rvdrijst | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: onetoone related | Triage Stage: Accepted
expection null |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Roman Donchenko (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10227#comment:32>