Opened 15 years ago

Last modified 3 years ago

#10227 new New feature

Support a related_default on OneToOne fields

Reported by: rvdrijst Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: onetoone related expection null
Cc: rvdrijst, eallik@…, olau@…, django@…, Sebastian Goll, Aymeric Augustin, cvrebert, Francis Devereux, mike@…, Ryan Hiebert, Peter Thomassen Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Aymeric Augustin)

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 Shops 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.

Attachments (1)

patch.diff (2.9 KB ) - added by rvdrijst 15 years ago.
Patch with fix and tests

Download all attachments as: .zip

Change History (32)

by rvdrijst, 15 years ago

Attachment: patch.diff added

Patch with fix and tests

comment:1 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:2 by Jacob, 15 years ago

Triage Stage: UnreviewedDesign decision needed

I'm not entirely sure what the correct behavior here ought to be: the fact that Shop.place may be null doesn't necessarily imply that Place.shop also may be null. That is, it may be correct for some situations that a Shop may not have a Place but that Place always has an associated Shop.

comment:3 by Malcolm Tredinnick, 15 years ago

This has come up before (#3106). I've closed that as dupe, simply because the description here is more useful.

comment:4 by Erik Allik, 15 years ago

I just ran into the same issue. I think this is very unintuitive... The way I see it is that a OneToOneField should behave just like a foreign key from both sides.

comment:5 by Erik Allik, 15 years ago

Assuming related_name='foobar' is desired, the workaround would be to use related_name='_foobar' and then write a property in the related model that looks like this:

def foobar(self):
    try:
        return self._foobar
    except Foobar.DoesNotExist:
        return None

comment:6 by Erik Allik, 15 years ago

Cc: eallik@… added

comment:7 by Ole Laursen, 14 years ago

Cc: olau@… added

I've run into the case where every Shop has a Place, but some places don't have a shop. So I think we need to model it with a separate parameter, e.g. related_null=True. That would also be backwards-compatible. Aren't there four cases? So we need two bits to describe them:

one           to  one
zero or one   to  one
one           to  zero or one
zero or one   to  zero or one

in reply to:  7 comment:8 by Jonas von Poser, 14 years ago

Cc: django@… added

Replying to olau:

So I think we need to model it with a separate parameter, e.g. related_null=True. That would also be backwards-compatible.

This seems an elegant solution. I would really like to have this functionality, as it makes extending the User model much easier. For instance, we have a Member model with a OneToOneField to User. Since I can't add a property to User (except by monkeypatching it), I have to wrap every access to request.user.member with a try/except block.

comment:9 by George Sakkis, 13 years ago

Cc: George Sakkis added

comment:10 by George Sakkis, 13 years ago

I ran into the same problem although in my case a hypothetical related_null=True would not help; a new default object would have be created. So instead of related_null I wrote a OneToOneField that uses related_default, a callable lambda instance: value. I posted the snippet at http://gist.github.com/601977.

To me this seems more elegant, not only because it is a generalization of the related_null (just set related_default=lambda instance: None) but because null (and by association related_null) is a DB-level parameter, it determines whether a DB column is created as NULL or NOT NULL. In the case of a OneToOneField, the related model does not have a column linking to the model where the OneToOneField was defined (no shop_id column in the place table), so it doesn't make sense to say it's null.

comment:11 by anonymous, 13 years ago

milestone: 1.3
Version: 1.0SVN

comment:12 by Chris Beaven, 13 years ago

milestone: 1.3
Severity: Normal
Type: Bug

comment:13 by Luke Plant, 13 years ago

Easy pickings: unset
Has patch: unset
Triage Stage: Design decision neededAccepted
Type: BugNew feature

It looks to me that the consensus is that raising the DoesNotExist exception does make sense (at least in some cases, and we have to take the general case). The argument gsakkis makes seems sensible, so his/her solution is preferable. It would also make it very easy to solve the original problem i.e. just provide related_default=lambda: None.

But this is definitely a feature, not a bug, so changing accordingly.

I'm removing the 'has patch' flag since we don't have a patch that implements the chosen solution.

comment:14 by anonymous, 13 years ago

UI/UX: unset

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()
Last edited 9 years ago by Tim Graham (previous) (diff)

comment:15 by Ole Laursen, 13 years ago

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

comment:16 by George Sakkis, 13 years ago

Cc: George Sakkis removed

comment:17 by Sebastian Goll, 12 years ago

Cc: Sebastian Goll added

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.

comment:18 by Sebastian Goll, 12 years ago

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

in reply to:  18 comment:19 by Łukasz Rekucki, 12 years ago

Replying to 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.

comment:20 by Aymeric Augustin, 12 years ago

Cc: Aymeric Augustin added

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.

comment:21 by cvrebert, 12 years ago

Cc: cvrebert added

comment:22 by Francis Devereux, 11 years ago

Cc: Francis Devereux added

comment:23 by Aymeric Augustin, 11 years ago

Summary: OneToOne fields with null=True raise DoesNotExist exception on related modelSupport a related_default on OneToOne fields

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.

comment:24 by Anssi Kääriäinen, 11 years ago

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.

comment:25 by Ole Laursen, 11 years ago

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?

comment:26 by Anssi Kääriäinen, 11 years ago

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.

comment:27 by Ole Laursen, 11 years ago

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. :)

comment:28 by Mike Fogel, 11 years ago

Cc: mike@… added

in reply to:  26 comment:29 by Aymeric Augustin, 10 years ago

Description: modified (diff)

Replying to 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.

comment:30 by Ryan Hiebert, 5 years ago

Cc: Ryan Hiebert added

comment:31 by Peter Thomassen, 3 years ago

Cc: Peter Thomassen added
Note: See TracTickets for help on using tickets.
Back to Top