Opened 12 months ago

Last modified 2 months ago

#29085 new Cleanup/optimization

Possible data loss on .save() with unsaved related model

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

Description

  • Create unsaved model C
  • Assign unsaved related model P
  • Save related model P and C
  • Relationship is lost

This is similar to #25160 I guess?

I reproduced this with all versions of Django 1.6 up to master.

from django.db import models


class Parent(models.Model):
    pass


class Child(models.Model):
    parent = models.ForeignKey(Parent, null=True, blank=True, on_delete=models.CASCADE)
from .models import Parent, Child
from django.test import TestCase

class MyTest(TestCase):
    def test_save(self):
        child = Child()
        child.parent = Parent()
        child.parent.save()
        # This makes the problem go away:
        # child.parent = child.parent
        child.save()
        child.refresh_from_db()
        self.assertIsNotNone(child.parent)

Attachments (1)

patch.diff (2.4 KB) - added by Jonas Haag 12 months ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 12 months ago by Jonas Haag

Type: UncategorizedCleanup/optimization

comment:2 Changed 12 months ago by Tim Graham

Triage Stage: UnreviewedAccepted

Changed 12 months ago by Jonas Haag

Attachment: patch.diff added

comment:3 Changed 12 months ago by Jonas Haag

Is this the correct approach? Or should we rather consider the behaviour a bug and not use the "prohibited error" in the first place?

comment:4 Changed 12 months ago by Tim Graham

Without knowing what an implementation might look like, I'd expect the save to work since the parent is saved before the child.

comment:5 Changed 7 months ago by Tim Graham

#29497 is a duplicate.

comment:6 Changed 7 months ago by Matthew Schinckel

I believe I hit the same issue today.

I have a set of related objects that I want to construct all of the forms for, validate all of them, and then save.

However, the reference to the primary object (which is still "connected" to the child instances) is removed when saving.

I _think_ it's got to do with Field.pre_save(self, model_instance, add) being called before saving: which uses field.attname, which is necessarily empty initially (because we don't have a primary key on the reference object, yet).

I'm going to play around, but I think it could be that a ForeignKey can have slightly different behaviour, where it falls back to the related (in-memory) object, if one exists, and field.attname is None.

comment:7 Changed 2 months ago by Ian Foote

I've been looking into this a bit and I don't see how we can do the right thing here. The problem I see is that the Child model has two sources of truth about the Parent instance and we don't know which we can trust. These sources are Child.parent_id and Child.parent.id. When we save the Parent instance, Child.parent.id changes, but Child.parent_id does not and I don't see a way to push the new pk to Child.parent_id behind the scenes. One can reassign Child.parent explicitly (as noted above), which ensures Child.parent_id is correct.

I looked into adjusting ForeignKey.pre_save to read from Child.parent and Model.save to avoid clearing Child._state.fields_cache, but this either didn't work or broke many other Django tests.

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