Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#29896 closed Bug (fixed)

Regression when saving a ForeignKey with to_field attr. Cached model is lost.

Reported by: Maciej Gol Owned by: nobody
Component: Database layer (models, ORM) Version: 2.1
Severity: Release blocker 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 (last modified by Maciej Gol)

Hey guys, during recent patching run I've encountered regression between Django 2.0.9 and 2.1.2.

Given models:

class Author(models.Model):
    username = models.CharField(primary_key=True)
    numeric_id = models.AutoField()

class Book(models.Model):
    author = models.ForeignKey(Author, on_delete=models.CASCADE)
    author_numeric = models.ForeignKey(Author, on_delete=models.CASCADE, to_field="numeric_id")

The following test fails, although it shouldn't.

class SomeTest(TestCase):
    def test_foreign_key_with_to_field_should_properly_persist_through_save(self):
        author = Author.objects.create(username="yada")
        book = Book(author=author, author_numeric=author)
        book.save()

        with self.assertNumQueries(0):
            book.author_numeric  # No queries below Django 2.1. One query for Django 2.1.

The problem comes from the save() method here:

                # If the relationship's pk was changed, clear the cached
                # relationship.
                if obj and obj.pk != getattr(self, field.attname):
                    field.delete_cached_value(self)

We compare obj.pk with book.author_numeric_id, but pk is a string (username) and author_numeric_id is an integer. It should look for to_field value, instead:

                # If the relationship's pk was changed, clear the cached
                # relationship.
                if obj and getattr(obj, field.target_field.attname) != getattr(self, field.attname):
                    field.delete_cached_value(self)

Change History (5)

comment:1 by Maciej Gol, 6 years ago

Description: modified (diff)

comment:2 by Tim Graham, 6 years ago

Has patch: set
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Thanks for the great analysis! PR

comment:3 by Simon Charette, 6 years ago

Triage Stage: AcceptedReady for checkin

comment:4 by GitHub <noreply@…>, 6 years ago

Resolution: fixed
Status: newclosed

In f77fc56:

Fixed #29896 -- Fixed incorrect Model.save() cache relation clearing for foreign keys that use to_field.

Regression in ee49306176a2d2f1751cb890bd607d42c7c09196.

comment:5 by Tim Graham <timograham@…>, 6 years ago

In 0f02d719:

[2.1.x] Fixed #29896 -- Fixed incorrect Model.save() cache relation clearing for foreign keys that use to_field.

Regression in ee49306176a2d2f1751cb890bd607d42c7c09196.
Backport of f77fc56c9671a2e2498e3920d79e247e6de18c16 from master.

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