Opened 12 months ago

Closed 11 months ago

Last modified 11 months ago

#22508 closed Bug (fixed)

select_related on a foreign related object fails to load fields on original object

Reported by: boxm Owned by: aaugustin
Component: Database layer (models, ORM) Version: 1.6
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

Consider these models:

class Show(models.Model):
    pass


class Poll(models.Model):
    event = models.ForeignKey('Event')


class Event(models.Model)
    show = models.ForeignKey(Show)

The following code fails to load the poll.event.show attribute:

In [3]: event = Event.objects.all()[2]

In [4]: poll = event.poll_set.select_related('event', 'event__show').all()[0]

In [5]: hasattr(poll.event, '_show_cache')
Out[5]: False

For comparison, in 1.4 the same code produces:

In [12]: event = Event.objects.all()[2]

In [13]: poll = event.poll_set.select_related('event', 'event__show').all()[0]

In [14]: hasattr(poll.event, '_show_cache')
Out[14]: True

The bug appears to be in the known_objects code around line 247 in db/models/query.py which looks like it will overwrite the poll.event attribute that was just retrieved from the DB (via select_related) and replaces it with the parent model, which in this case doesn't have the show attribute loaded.

Since the ORM was explicitly instructed to load the Event model in the call, it shouldn't then replace the poll.event attribute with the previously-known parent model.

Change History (12)

comment:1 Changed 12 months ago by boxm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to Bug

comment:3 Changed 12 months ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from new to assigned

I knew I shouldn't have touched this code...

comment:4 Changed 12 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 11 months ago by aaugustin

  • Has patch set

I've sent a pull request: https://github.com/django/django/pull/2646. The fix is straightforward.

Since this can cause silent performance regressions, I would suggest backporting f574220f0988f3aa1aca4f133887fbde0e5a6f10 to 1.7, 1.6 and 1.5.

QuerySet.iterator is growing a bit complex and might benefit from a refactoring.

comment:6 Changed 11 months ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

Looks good, also for backporting, with a slight reserve about 1.5 being in security/data-loss bug fix only.

comment:7 Changed 11 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In f574220f0988f3aa1aca4f133887fbde0e5a6f10:

Fixed #22508 -- Avoided overwriting select_related.

Previously, known related objects overwrote related objects loaded
though select_related. This could cancel the effect of select_related
when it was used over more than one level.

Thanks boxm for the bug report and timo for bisecting the regression.

comment:8 Changed 11 months ago by Aymeric Augustin <aymeric.augustin@…>

In e9d0ef19bc87aafd71d513cd927db2655267809b:

[1.7.x] Fixed #22508 -- Avoided overwriting select_related.

Previously, known related objects overwrote related objects loaded
though select_related. This could cancel the effect of select_related
when it was used over more than one level.

Thanks boxm for the bug report and timo for bisecting the regression.

Backport of f574220f from master

comment:9 Changed 11 months ago by Aymeric Augustin <aymeric.augustin@…>

In b6d3212190b114f1ef90d7ee9edcdae046432779:

[1.6.x] Fixed #22508 -- Avoided overwriting select_related.

Previously, known related objects overwrote related objects loaded
though select_related. This could cancel the effect of select_related
when it was used over more than one level.

Thanks boxm for the bug report and timo for bisecting the regression.

Conflicts:

tests/select_related_regress/tests.py

Backport of f574220f from master

comment:10 Changed 11 months ago by Tim Graham <timograham@…>

In 13087020a92afe778970f8445b8add6b167ba084:

Added 1.6.5 release note for refs #22508.

comment:11 Changed 11 months ago by Tim Graham <timograham@…>

In b718e292ac7d2e6ea9764aa618f913ead8b8bbb1:

[1.6.x] Added 1.6.5 release note for refs #22508.

Backport of 13087020a9 from master

comment:12 Changed 11 months ago by Tim Graham <timograham@…>

In e1f0efc501ad4e8999af51824806c091587fb21c:

[1.7.x] Added 1.6.5 release note for refs #22508.

Backport of 13087020a9 from master

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