Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#24615 closed Bug (fixed)

ordering by expression not part of SELECT is broken

Reported by: Mattia Procopio Owned by: nobody
Component: GIS Version: 1.8
Severity: Release blocker Keywords:
Cc: miroslav@… Triage Stage: Ready for checkin
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I was ordering my queryset by distance after calling .distance() on it, it was working well with django 1.7 but after upgrading to django 1.8 I am now getting this error:

File "/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/query.py", line 162, in __iter__
    self._fetch_all()
  File "/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/query.py", line 965, in _fetch_all
    self._result_cache = list(self.iterator())
  File "/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/query.py", line 1217, in iterator
    for row in compiler.results_iter():
  File "/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 783, in results_iter
    results = self.execute_sql(MULTI)
  File "/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 818, in execute_sql
    sql, params = self.as_sql()
  File "/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 367, in as_sql
    extra_select, order_by, group_by = self.pre_sql_setup()
  File "/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 49, in pre_sql_setup
    order_by = self.get_order_by()
  File "/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 276, in get_order_by
    field, self.query.get_meta(), default_order=asc))
  File "/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 552, in find_ordering_name
    if field.rel and path and opts.ordering and name != field.attname:
AttributeError: 'DistanceField' object has no attribute 'rel'

Is there anything I missed upgrading to 1.8?

Attachments (1)

ticket_24615.diff (1.7 KB) - added by Miroslav Shubernetskiy 5 years ago.
reproduction test case

Download all attachments as: .zip

Change History (14)

comment:1 Changed 5 years ago by Tim Graham

It seems likely to be a bug. Could you write a regression test for Django's test suite or at least tell us how we can reproduce the error?

comment:2 Changed 5 years ago by Mattia Procopio

After digging a little bit more into this I discovered this is not strictly related to ordering, in fact normal ordering still works:

In [1]: from django.contrib.gis.geos import Point

In [2]: qs = GeoData.objects.all()

In [3]: nqs = qs.distance(Point(3, 3)).order_by('distance')

In [4]: nqs[0].distance
Out[5]: Distance(m=157177.768212)

but when iterating the queryset ordered by distance I get the error:

In [1]: from django.contrib.gis.geos import Point

In [2]: qs = GeoData.objects.all()

In [3]: nqs = qs.distance(Point(3, 3)).order_by('distance')

In [4]: vlqs = nqs.values_list('wisp', flat=True)

In [5]: id_list = [id for id in vlqs]
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-5-7fa41993bc21> in <module>()
----> 1 id_list = [id for id in vlqs]

/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/query.pyc in __iter__(self)
    160                - Responsible for turning the rows into model objects.
    161         """
--> 162         self._fetch_all()
    163         return iter(self._result_cache)
    164 

/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/query.pyc in _fetch_all(self)
    963     def _fetch_all(self):
    964         if self._result_cache is None:
--> 965             self._result_cache = list(self.iterator())
    966         if self._prefetch_related_lookups and not self._prefetch_done:
    967             self._prefetch_related_objects()

/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/query.pyc in iterator(self)
   1215         compiler = self.query.get_compiler(self.db)
   1216         if self.flat and len(self._fields) == 1:
-> 1217             for row in compiler.results_iter():
   1218                 yield row[0]
   1219         elif not self.query.extra_select and not self.query.annotation_select:

/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/sql/compiler.pyc in results_iter(self, results)
    781         converters = None
    782         if results is None:
--> 783             results = self.execute_sql(MULTI)
    784         fields = [s[0] for s in self.select[0:self.col_count]]
    785         converters = self.get_converters(fields)

/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/sql/compiler.pyc in execute_sql(self, result_type)
    816             result_type = NO_RESULTS
    817         try:
--> 818             sql, params = self.as_sql()
    819             if not sql:
    820                 raise EmptyResultSet

/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/sql/compiler.pyc in as_sql(self, with_limits, with_col_aliases, subquery)
    365         refcounts_before = self.query.alias_refcount.copy()
    366         try:
--> 367             extra_select, order_by, group_by = self.pre_sql_setup()
    368             if with_limits and self.query.low_mark == self.query.high_mark:
    369                 return '', ()

/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/sql/compiler.pyc in pre_sql_setup(self)
     47         """
     48         self.setup_query()
---> 49         order_by = self.get_order_by()
     50         extra_select = self.get_extra_select(order_by, self.select)
     51         group_by = self.get_group_by(self.select + extra_select, order_by)

/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/sql/compiler.pyc in get_order_by(self)
    274                 # '-field1__field2__field', etc.
    275                 order_by.extend(self.find_ordering_name(
--> 276                     field, self.query.get_meta(), default_order=asc))
    277             else:
    278                 if col not in self.query.extra_select:

/home/matt/repos/uwncom/lib/python2.7/site-packages/django/db/models/sql/compiler.pyc in find_ordering_name(self, name, opts, alias, default_order, already_seen)
    550         # append the default ordering for that model unless the attribute name
    551         # of the field is specified.
--> 552         if field.rel and path and opts.ordering and name != field.attname:
    553             # Firstly, avoid infinite loops.
    554             if not already_seen:

AttributeError: 'DistanceField' object has no attribute 'rel'

the model I use is very simple:

import uuid

from django.contrib.gis.db import models


class GeoData(models.Model):
    id = UUIDField(primary_key=True, editable=False, default=uuid.uuid4)
    coverage = models.MultiPolygonField(null=True, blank=True)

    objects = models.GeoManager()

    def __unicode__(self):
        return "%s" % self.coverage  # pragma: no cover

Creating a multipolygon:

g = GeoData(coverage=MultiPolygon(Polygon( ((1, 1), (1, 2), (2, 2), (1, 1)) )) )
Last edited 5 years ago by Mattia Procopio (previous) (diff)

comment:3 Changed 5 years ago by Miroslav Shubernetskiy

Cc: miroslav@… added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Was able to reproduce the same traceback in Django. For Django1.7 this passes. Also Django1.9 (master branch) traceback is different. Attached is the patch with the test which reproduces the issue.

Last edited 5 years ago by Miroslav Shubernetskiy (previous) (diff)

Changed 5 years ago by Miroslav Shubernetskiy

Attachment: ticket_24615.diff added

reproduction test case

comment:4 Changed 5 years ago by Claude Paroz

Bisected the issue to 0c7633178fa9410f102e4708cef979b873bccb76
Anssi, does the traceback give you a hint about a possible cause/resolution?

comment:5 Changed 5 years ago by Claude Paroz

Also note if we simply add a rel = None attribute to geometry BaseField, the same find_ordering_name method fails a bit later with:

  File "/home/claude/virtualenvs/djangogit/local/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 261, in get_order_by
    default_order=asc))
  File "/home/claude/virtualenvs/djangogit/local/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 540, in find_ordering_name
    return [(t.get_col(alias), order, False) for t in targets]
AttributeError: 'RawSQL' object has no attribute 'get_col'

comment:6 Changed 5 years ago by Anssi Kääriäinen

I'm a bit busy right now. I'll take a look by Thursday.

comment:7 Changed 5 years ago by Anssi Kääriäinen

Triage Stage: AcceptedReady for checkin

The problem turns out to be that order_by references to expressions which are masked out of the SELECT clause (by .values() for example) weren't resolved correctly.

comment:8 Changed 5 years ago by Anssi Kääriäinen

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

comment:9 Changed 5 years ago by Mattia Procopio

Summary: ordering queryset by distance is brokenordering by expression not part of SELECT is broken

comment:10 Changed 5 years ago by Anssi Kääriäinen

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:11 Changed 5 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In fb5c7748:

Fixed #24615 -- ordering by expression not part of SELECT

Fixed queries where an expression was used in order_by() but the
expression wasn't in the query's select clause (for example the
expression could be masked by .values() call)

Thanks to Trac alias MattBlack85 for the report.

comment:12 Changed 5 years ago by Claude Paroz <claude@…>

In 70ff455:

[1.8.x] Fixed #24615 -- ordering by expression not part of SELECT

Fixed queries where an expression was used in order_by() but the
expression wasn't in the query's select clause (for example the
expression could be masked by .values() call)

Thanks to Trac alias MattBlack85 for the report.
Backport of fb5c7748da from master.

comment:13 Changed 5 years ago by Mattia Procopio

Awesome, thanks guys!

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