Opened 4 years ago

Closed 3 years ago

#16409 closed Bug (fixed)

`defer()` and `only()` don't play nice with `annotate()`

Reported by: mrmachine Owned by: nobody
Component: GIS Version: master
Severity: Normal Keywords: annotate defer only count
Cc: petr.gorodechnyj@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When adding defer() or only() to an existing query that used annotate() to cut out some fields, I started getting IndexError exceptions.

At first, I thought it was due to aggregate_start not being reduced by the number of deferred fields in QuerySet.iterator(), but when I looked at the SQL being generated I saw that ALL fields were being dropped from the query, except for the annotation. I suspect that even if this is fixed, aggregate_start will also need to be reduced by the number of deferred fields.

Here's the traceback:

>>> from django.db.models import Count
>>> from django.contrib.auth.models import *
>>> from django.contrib.admin.models import *
>>> User.objects.annotate(Count('logentry'))
[<User: manager>, <User: trak:1:10>, <User: admin>, <User: trak:3:5>, <User: trak:1:24>, <User: trak:3:9>, <User: trak:1:3>, <User: trak:4:11>, <User: trak:5:21>, <User: trak:1:14>, <User: trak:1:15>, <User: trak:1:2>, <User: trak:4:18>, <User: trak:4:19>, <User: trak:5:17>, <User: trak:3:6>, <User: trak:1:23>, <User: trak:3:16>, <User: trak:5:22>, <User: trak:4:12>, '...(remaining elements truncated)...']
>>> User.objects.annotate(Count('logentry')).defer('first_name')
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Users/mrmachine/myproject/django/db/models/query.py", line 69, in __repr__
    data = list(self[:REPR_OUTPUT_SIZE + 1])
  File "/Users/mrmachine/myproject/django/db/models/query.py", line 84, in __len__
    self._result_cache.extend(self._iter)
  File "/Users/mrmachine/myproject/django/db/models/query.py", line 300, in iterator
    setattr(obj, aggregate, row[i+aggregate_start])
IndexError: tuple index out of range

Here's the generated SQL:

>>> str(User.objects.annotate(Count('logentry')).query)
'SELECT "auth_user"."id", "auth_user"."username", "auth_user"."first_name", "auth_user"."last_name", "auth_user"."email", "auth_user"."password", "auth_user"."is_staff", "auth_user"."is_active", "auth_user"."is_superuser", "auth_user"."last_login", "auth_user"."date_joined", COUNT("django_admin_log"."id") AS "logentry__count" FROM "auth_user" LEFT OUTER JOIN "django_admin_log" ON ("auth_user"."id" = "django_admin_log"."user_id") GROUP BY "auth_user"."id", "auth_user"."username", "auth_user"."first_name", "auth_user"."last_name", "auth_user"."email", "auth_user"."password", "auth_user"."is_staff", "auth_user"."is_active", "auth_user"."is_superuser", "auth_user"."last_login", "auth_user"."date_joined"'
>>> str(User.objects.annotate(Count('logentry')).defer('first_name').query)
'SELECT COUNT("django_admin_log"."id") AS "logentry__count" FROM "auth_user" LEFT OUTER JOIN "django_admin_log" ON ("auth_user"."id" = "django_admin_log"."user_id") GROUP BY "auth_user"."id", "auth_user"."username", "auth_user"."first_name", "auth_user"."last_name", "auth_user"."email", "auth_user"."password", "auth_user"."is_staff", "auth_user"."is_active", "auth_user"."is_superuser", "auth_user"."last_login", "auth_user"."date_joined"'

Attachments (3)

16409-defer-only-annotate-r16510.diff (2.7 KB) - added by mrmachine 4 years ago.
Failing test case and fix.
16409.diff (990 bytes) - added by aaugustin 3 years ago.
16409-geosqlcompiler.diff (728 bytes) - added by Petr Gorodechnyj <petr.gorodechnyj@…> 3 years ago.
Fixes this problem for GeoQuery

Download all attachments as: .zip

Change History (12)

comment:1 Changed 4 years ago by mrmachine

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set

Added a patch with a failing test case.

Creating test database for alias 'default'...
Creating test database for alias 'other'...
E
======================================================================
ERROR: test_defer (modeltests.defer.tests.DeferTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mrmachine/Subversion/django/tests/modeltests/defer/tests.py", line 141, in test_defer
    self.assertEqual(len(Secondary.objects.annotate(Count('primary')).defer('first')), 1)
  File "/Users/mrmachine/Subversion/django/django/db/models/query.py", line 82, in __len__
    self._result_cache = list(self.iterator())
  File "/Users/mrmachine/Subversion/django/django/db/models/query.py", line 300, in iterator
    setattr(obj, aggregate, row[i+aggregate_start])
IndexError: tuple index out of range

----------------------------------------------------------------------
Ran 1 test in 0.120s

FAILED (errors=1)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

comment:2 Changed 4 years ago by mrmachine

  • Patch needs improvement unset

Updated patch to include a fix. Hopefully good to go.

Changed 4 years ago by mrmachine

Failing test case and fix.

comment:3 Changed 4 years ago by mrmachine

Updated tests to use assertIsInstance. The test checks that the queryset can be successfully iterated through without raising an exception by converting it to a list.

comment:4 Changed 4 years ago by ramiro

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

In [16522]:

Fixed #16409 -- Fixed an error condition when using QuerySet only()/defer() on the result of an annotate() call. Thanks jaklaassen AT gmail DOT com and Tai Lee for the reports and Tai for the patch.

comment:5 Changed 4 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:6 Changed 3 years ago by Petr Gorodechnyj <petr.gorodechnyj@…>

  • Cc petr.gorodechnyj@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

OK, you did that all right, but now do the same for the GeoQuerySet please. If I call GeoManager's defer(), only() or values() I get the same problem that was fixed in the patch above.

Changed 3 years ago by aaugustin

comment:7 Changed 3 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

Indeed — I just attached a failing test case.

comment:8 Changed 3 years ago by aaugustin

  • Component changed from Database layer (models, ORM) to GIS

Changed 3 years ago by Petr Gorodechnyj <petr.gorodechnyj@…>

Fixes this problem for GeoQuery

comment:9 Changed 3 years ago by ramiro

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

In [17506]:

Fixed #16409 (again, this time for GeoDjango).

Thanks Aymeric Augustin for the regression test and Petr Gorodechnyj for
the patch.

Refs r16522.

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