Opened 14 months ago

Closed 14 months ago

Last modified 14 months ago

#34433 closed New feature (duplicate)

OneToOneField can only be saved one way

Reported by: Alexis Lesieur Owned by: nobody
Component: Database layer (models, ORM) Version: 4.1
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Alexis Lesieur)

Hi!
I encountered this unexpected (to me) behavior for work, and I have been able to replicate on a bare django app (albeit with slightly different symptoms).

The TLDR is that is model A has a OneToOneField to model B. The field had to be saved from the instance of model A, and that's not only not documented anywhere I could find, but counter-intuitive, and contradicts how other fields like ForeignKeys work.

Setup:

❯ python --version
Python 3.11.2

❯ pip freeze | grep -i django
Django==4.1.7

❯ django-admin startproject mysite

❯ cd mysyte/
❯ django-admin startapp myapp
❯ vim myapp/models.py
# partially re-using your example from https://docs.djangoproject.com/en/4.1/topics/db/examples/one_to_one/
```
from django.db import models

class Place(models.Model):
    name = models.CharField(max_length=50)
    address = models.CharField(max_length=80)

    def __str__(self):
        return "%s the place" % self.name

class Restaurant(models.Model):
    place = models.OneToOneField(
        Place,
        on_delete=models.CASCADE,
    )
    serves_hot_dogs = models.BooleanField(default=False)
    serves_pizza = models.BooleanField(default=False)

    def __str__(self):
        return "%s the restaurant" % self.place.name
```

❯ vim mysite/settings.py

[...]
INSTALLED_APPS = [
    'myapp.apps.MyappConfig',
[...]

❯ python manage.py makemigrations
❯ python manage.py migrate

Creating the initial objects:

❯ python manage.py shell
❯ from myapp.models import Place
❯ from myapp.models import Restaurant

❯ p1 = Place(name="1st place", address="1st address")
❯ p2 = Place(name="2nd place", address="2nd address")
❯ r1 = Restaurant(place=p1)
❯ r2 = Restaurant(place=p2)
❯ p1.save()
❯ p2.save()
❯ r1.save()
❯ r2.save()
❯ p3 = Place(name="3rd place", address="3rd address")
❯ p3.save()

This should give us a two restaurants with their respective places, and an additional place we can use to play.

First, what works:

❯ r1.place = p3
❯ r1.save()

❯ Restaurant.objects.get(id=1).place
<Place: 3rd place the place>

❯ p3.restaurant
<Restaurant: 3rd place the restaurant>

❯ Place.objects.get(id=1).restaurant
[...]
RelatedObjectDoesNotExist: Place has no restaurant.

This is all expected. r1 now uses p3, which means that p1 has no restaurant assigned.

Now I would expect, to be able to do the other way. Assign a new restaurant to a place, save, and be all good.
However that doesn't work.
First using plain .save() which fails silently:

❯ p1 = Place.objects.get(id=1)
❯ p1.restaurant = r1
❯ p1.save()

❯ Restaurant.objects.get(id=1).place
<Place: 3rd place the place>  # this should be p1

And when explicitly asking to save the field:

❯ p1.save(update_fields=["restaurant"])
❯ ValueError: The following fields do not exist in this model, are m2m fields, or are non-concrete fields: restaurant

NB: on my use case for work (django 3.2.18) I was also getting the following error:

UniqueViolation: duplicate key value violates unique constraint "response_timelineevent_pkey"
DETAIL:  Key (id)=(91) already exists.

I'm not sure why it's different, but it doesn't work either way.

This is problematic for a few reasons IMO:

  • Unless I missed it, the docs really don't advertise this limitation.
  • .save() "fails" silently, there is no way to know that the change didn't take, especially when this happens:
    ❯ p1 = Place(name="1st place", address="1st address")
    ❯ p2 = Place(name="2nd place", address="2nd address")
    ❯ p3 = Place(name="3rd place", address="3rd address")
    ❯ p1.save()
    ❯ p2.save()
    ❯ p3.save()
    ❯ r1 = Restaurant(place=p1)
    ❯ r1.save()
    ❯ r2 = Restaurant(place=p2)
    ❯ r2.save()
    
    ❯ r1.place
    <Place: 1st place the place>
    ❯ p3.restaurant = r1
    ❯ r1.place
    <Place: 3rd place the place>
    ❯ p3.save()
    ❯ Restaurant.objects.get(id=1).place
    <Place: 1st place the place>
    

which leads to thinking the change is working and affecting both objects, when it's not.

It's also problematic as foreigh keys work this way: (from my work example)

❯ me = ExternalUser.objects.get(id=1)
❯ other = ExternalUser.objects.get(id=2)
❯ p = PinnedMessage.objects.get(id=11)

❯ p.author
<ExternalUser: first.last (slack)>  # i.e. `me`

❯ [p.id for p in me.authored_pinnedmessage.all()]
[1, 3, 5, 11]

❯ p.author = other
❯ p.save()

❯ [p.id for p in ExternalUser.objects.get(id=1).authored_pinnedmessage.all()]
[1, 3, 5]

❯ me.authored_pinnedmessage.add(p)
❯ me.save()

❯ PinnedMessage.objects.get(id=11).author
<ExternalUser: first.last (slack)>

Hopefully this is all enough explanation / details.
Let me know if you need anything else from me!
Thank you for your help.

[EDIT] This is also counterintuitive because the documentation for OneToOneField explicitely states:

A one-to-one relationship. Conceptually, this is similar to a ForeignKey with unique=True, but the “reverse” side of the relation will directly return a single object.

Change History (4)

comment:1 by Alexis Lesieur, 14 months ago

Description: modified (diff)

comment:2 by Mariusz Felisiak, 14 months ago

Resolution: duplicate
Status: newclosed
Type: BugNew feature

Duplicate of #18638. You can comment in the original ticket. If you don't agree with the decision made in #18638 please ​follow triaging guidelines with regards to wontfix tickets.

comment:3 by Mariusz Felisiak, 14 months ago

It's also problematic as foreigh keys work this way: (from my work example)

That's not true, on foreign keys you also cannot assign to a reverse side.

.save() "fails" silently, there is no way to know that the change didn't take, especially when this happens:

save() doesn't fail silently. By assigning an object to the .restaurant you override a lazy reverse side of OneToOneField to a "static" instance so it's not recognize as a relationship anymore.

in reply to:  3 comment:4 by Alexis Lesieur, 14 months ago

First off apologies for missing the older ticket. I searched for similar problems but...

Replying to Mariusz Felisiak:

It's also problematic as foreigh keys work this way: (from my work example)

That's not true, on foreign keys you also cannot assign to a reverse side.

You cannot "assign" as I may not just be able to do me.authored_pinnedmessage = ... (to reference my earlier example) but I can definitely do:

❯ PinnedMessage.objects.get(id=1).author
<ExternalUser: other.user (slack)>

❯ me = ExternalUser.objects.get(id=1)
❯ me.authored_pinnedmessages.add(p)
❯ me.save()

❯ PinnedMessage.objects.get(id=1).author
<ExternalUser: first.last (slack)>

And this works. So I can definitely use the reverse relationship.

Replying to Mariusz Felisiak:

.save() "fails" silently, there is no way to know that the change didn't take, especially when this happens:

save() doesn't fail silently. By assigning an object to the .restaurant you override a lazy reverse side of OneToOneField to a "static" instance so it's not recognize as a relationship anymore.

I wholeheartedly disagree. Not only does it recognize the relationship still as this proves:

❯ p1 = Place(name="1st place", address="1st address")
❯ p1.save()
❯ r1 = Restaurant(place=p1)
❯ r1.save()
❯ r2 = Restaurant(place=p2)
❯ r2.save()

❯ p1.restaurant = r2
❯ r2.place
<Place: 1st place the place>

It is clearly reflected. The relationship is definitely recognized.
It is just being ignored on save.

When as we showed it's being honored for ForeignKeys. Which the documentation claims behaves the same.

I understand that from a pure implementation pov you're telling me that doing model.related_set.add(...) is different than doing place.restaurant = new_place here.
But that's really besides the point. From a user POV, the behavior is highly unexpected, and really the main problem IMO (assuming you do keep the current behavior) the documentation is really not addressing this issue in any way.
In addition to the "OneToOneFields are just ForeignKeys with unique=True" the documentation on how to use them clearly says:

Set the place using assignment notation. Because place is the primary key on Restaurant, the save will create a new restaurant:

>>> r.place = p2
>>> r.save()
>>> p2.restaurant
<Restaurant: Ace Hardware the restaurant>
>>> r.place
<Place: Ace Hardware the place>

Set the place back again, using assignment in the reverse direction:

>>> p1.restaurant = r
>>> p1.restaurant
<Restaurant: Demon Dogs the restaurant>

Without addressing anywhere that you can't do p1.save() here. Which is very counterintuitive. It is extremely expected that if I do p1.restaurant = r, to apply/save that change, I should be doing p1.save().
If we're saying this is not applicable, please do call it out in the documentation :-)

Note: See TracTickets for help on using tickets.
Back to Top