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

[Gert Burger]

(initially posted to https://forum.djangoproject.com/t/fk-field-caching-behavior-change-between-1-11-x-and-2-x/3151 on 26 June but this mailing list might be more appropriate)

Hi

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:

1. Is the behavior in >=2.0 correct? It seems quite unexpected.
2. 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):

---------------------code--------------------------

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)

---------------------code--------------------------

and executed that via:

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

Regards

Gert Burger

[charettes]

Hello Gert, that seems a bit surprising to me and was likely not a desired change.

Could you bisect the exact commit that caused the regression[0]? That would certainly help determining the action to take here.

Thanks,

Simon

[0] https://docs.djangoproject.com/en/3.1/internals/contributing/triaging-tickets/#bisecting-a-regression

[Gert Burger]

Hi Simon,

I think the commit is bfb746f983aa741afa3709794e70f1e0ab6040b5 "Refs #16043 -- Refactored internal fields value cache".

Cheers

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/c284e35e-ae02-44e5-b44d-4994eff9b248n%40googlegroups.com.

[charettes]

It was likely overlooked by the patch.

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

Using copy.deepcopy will circumvent the issue but I feel like copy.copy is common enough pattern that we should maintain compatibility for it. After all, the fact the _state_ of the model is now stored in _state.fields_cache is no more than an implementation detail and it would be normal to expect shallow copying to work in this case. On the other hand the fact this has been happening since Django 2.0 and has just been reported and that shallow copies won't work correctly for prefetched relationships are both non-negligible argument for a wontfix since this pattern was never explicitly documented.

There is a precedent for a similar undocumented pattern though [0].

Thoughts?

Simon

[0] https://github.com/django/django/commit/dee5896b55385d2c66d8c1b8386604868f7fc6b4

[Alexander Hill]

I reckon stick with your first instinct SImon.

I don't think using copy.copy needs to be an explicitly documented pattern. It's a heavily-used part of the standard library, and the objects Django provides should work with it as well as they can. The behaviour is surprising and buggy at face value: changing the value of an attribute on one object should not change the value of that attribute on another!

Alex

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/0b5d48d0-5594-40d6-9dce-80995f8635ecn%40googlegroups.com.

[charettes]

Unless someone objects to adjusting Model.__copy__(self) to deal with self._state.fields_cache I'd say we should create a ticket about it.

Would you be interested in creating the ticket and possibly submitting a patch Gert?

Simon

[Gert Burger]

I have created https://github.com/django/django/pull/13280 and I will need a day or so to test it against our code bases.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/8717dbd1-37a9-4249-bb27-5b3d688a37f3n%40googlegroups.com.

[Gert Burger]

Correction, it is https://github.com/django/django/pull/13281 now.

[Gert Burger]

The change has been merged into master, is there any chance it can be backported to 2.2? It does afterall correct a regression that could lead to data loss in a hard to detect manner ;P

If not then our choices are limited to:

1) Stay on 1.11 until 3.2. Not really feasible as our deps have started dropping support for it.

2) Migrate to master. Also not really feasible as our deps have not started supporting it yet. They tend to only support the current LTS.

3) Fork 2.2 and backport it ourselves

The last option is somewhat practical compared to the first two.

Regards

[Mariusz Felisiak]

Agreed, this qualifies for a backport as a data loss bug. I will do it.

Best,
Mariusz

[Mariusz Felisiak]

Backported.