Opened 10 months ago

Closed 9 months ago

#23001 closed Bug (fixed)

Annotation breaks with deferred fields and select_related

Reported by: smeatonj Owned by: jarshwah
Component: Database layer (models, ORM) Version: master
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

I've discovered a bug that I think was introduced by: https://github.com/django/django/commit/0b6f05ede648ce62a5c91c7c38a0a362711f0656

The problem manifests as annotations being assigned an incorrect value when there are deferred fields and select_related is in use.

Passing Test:

def test_annotate_defer(self):
        qs = Book.objects.annotate(
            page_sum=Sum("pages")).defer('name').filter(pk=1)

        rows = [
            (1, "159059725", 447, "The Definitive Guide to Django: Web Development Done Right")
        ]
        self.assertQuerysetEqual(
            qs.order_by('pk'), rows,
            lambda r: (r.id, r.isbn, r.page_sum, r.name)
        )

Failing Test:

def test_annotate_defer_select_related(self):
        qs = Book.objects.select_related('contact').annotate(
            page_sum=Sum("pages")).defer('name').filter(pk=1)

        rows = [
            (1, "159059725", 447, "Adrian Holovaty",
            "The Definitive Guide to Django: Web Development Done Right")
        ]
        self.assertQuerysetEqual(
            qs.order_by('pk'), rows,
            lambda r: (r.id, r.isbn, r.page_sum, r.contact.name, r.name)
        )

The problem is in the iterator method of django.db.models.query, which results in the aggregate_start variable being off by len(deferred):

if load_fields and not fill_cache:
            # Some fields have been deferred, so we have to initialize
            # via keyword arguments.
            skip = set()
            init_list = []
            for field in fields:
                if field.name not in load_fields:
                    skip.add(field.attname)
                else:
                    init_list.append(field.attname)
            model_cls = deferred_class_factory(self.model, skip)
        else:
            model_cls = self.model
            init_list = [f.attname for f in fields]

fill_cache is set to true when select_related is in use, which skips to the else block. The else block doesn't take deferred fields into account like the if block does. I'll attempt to patch this by including similar logic in the else block (skipping deferred fields), but I'm not yet sure what else that might affect.

Change History (5)

comment:1 Changed 10 months ago by smeatonj

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Both tests pass on the 1.7 branch.

comment:3 Changed 10 months ago by timo

  • Triage Stage changed from Unreviewed to Ready for checkin
  • Type changed from Uncategorized to Bug

comment:4 Changed 9 months ago by jarshwah

  • Owner changed from smeatonj to jarshwah
  • Status changed from new to assigned

comment:5 Changed 9 months ago by Anssi Kääriäinen <akaariai@…>

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

In 5b0375ec3e7473fcc29c21003fef80c0d0be183f:

Fixed #23001 -- Fixed mixing defer and annotations

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