Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#31863 closed Bug (fixed)

FK field caching behavior change between 1.11.x and 2.x

Reported by: Gert Burger Owned by: Gert Burger
Component: Database layer (models, ORM) Version: 2.2
Severity: Normal Keywords:
Cc: 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

Whilst upgrading a codebase from 1.11.x to 2.0/2.2 I noticed a weird change in behavior of FK fields when copying model instances.

At the bottom of the post there is a testcase that succeeds on 1.11.x and fails on 2.x
I think the commit that changed the behavior is bfb746f983aa741afa3709794e70f1e0ab6040b5

So my question is two fold:

  • Is the behavior in >=2.0 correct? It seems quite unexpected.
  • What is the recommended way to clone a model instance? To date we have been using copy() in a similar fashion to the test without issue. deepcopy seems to work fine in >=2.0 but we haven’t done too much testing yet.

Test (placed in tests/model_fields/test_field_caching_change.py):

import copy
from django.test import TestCase

from .models import Bar, Foo


class ForeignKeyCachingBehaviorTest(TestCase):
    def test_copy(self):
        foo1 = Foo.objects.create(a='foo1', d=1)
        foo2 = Foo.objects.create(a='foo2', d=2)
        bar1 = Bar.objects.create(a=foo1, b='bar1')

        bar2 = copy.copy(bar1)

        bar2.pk = None
        bar2.a = foo2

        # bar2 points to foo2
        self.assertEqual(bar2.a, foo2)
        self.assertEqual(bar2.a.id, bar2.a_id)

        # bar1 is unchanged and must still point to foo1
        # These fail on Django >= 2.0
        self.assertEqual(bar1.a, foo1)
        self.assertEqual(bar1.a.id, bar1.a_id)

and executed that via:

python3.6 tests/runtests.py --parallel 1 model_fields

In https://groups.google.com/g/django-developers/c/QMhVPIqVVP4/m/mbezfaBEAwAJ Simon suggests:

..... Model.copy should make sure to make a deep-copy of self._state now that fields are cached in self._state.fields_cache.

which I will attempt to implement.

Change History (14)

comment:1 Changed 8 months ago by Mariusz Felisiak

Triage Stage: UnreviewedAccepted

comment:2 Changed 8 months ago by Gert Burger

Has patch: set

Can someone please double check the fields on the ticket and PR, not sure if I got them right.

https://github.com/django/django/pull/13281/

comment:3 Changed 8 months ago by Mariusz Felisiak

Patch needs improvement: set

comment:4 Changed 8 months ago by Gert Burger

Patch needs improvement: unset

comment:5 Changed 8 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:6 Changed 8 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 94ea79be:

Fixed #31863 -- Prevented mutating model state by copies of model instances.

Regression in bfb746f983aa741afa3709794e70f1e0ab6040b5.

comment:7 Changed 8 months ago by Mariusz Felisiak

Version: 3.12.2

This qualifies for a backport as a data loss bug.

comment:8 Changed 8 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 85c47b9a:

[3.1.x] Fixed #31863 -- Prevented mutating model state by copies of model instances.

Regression in bfb746f983aa741afa3709794e70f1e0ab6040b5.

Backport of 94ea79be137f3cb30949bf82198e96e094f2650d from master

comment:9 Changed 8 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In c33b6ce:

[3.0.x] Fixed #31863 -- Prevented mutating model state by copies of model instances.

Regression in bfb746f983aa741afa3709794e70f1e0ab6040b5.

Backport of 94ea79be137f3cb30949bf82198e96e094f2650d from master

comment:10 Changed 8 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 0a7d321b:

[2.2.x] Fixed #31863 -- Prevented mutating model state by copies of model instances.

Regression in bfb746f983aa741afa3709794e70f1e0ab6040b5.

Backport of 94ea79be137f3cb30949bf82198e96e094f2650d from master

comment:11 Changed 8 months ago by GitHub <noreply@…>

In 21768a99:

Refs #31863 -- Added release notes for 94ea79be137f3cb30949bf82198e96e094f2650d.

comment:12 Changed 8 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 18e87ac:

[3.1.x] Refs #31863 -- Added release notes for 94ea79be137f3cb30949bf82198e96e094f2650d.

Backport of 21768a99f47ee73a2f93405151550ef7c3d9c8a2 from master

comment:13 Changed 8 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In ab5491c:

[3.0.x] Refs #31863 -- Added release notes for 94ea79be137f3cb30949bf82198e96e094f2650d.

Backport of 21768a99f47ee73a2f93405151550ef7c3d9c8a2 from master

comment:14 Changed 8 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In dc39e62e:

[2.2.x] Refs #31863 -- Added release notes for 94ea79be137f3cb30949bf82198e96e094f2650d.

Backport of 21768a99f47ee73a2f93405151550ef7c3d9c8a2 from master

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