Fixed incomplete merge of geographic aggregates; added support for `Extent` aggregate to Oracle spatial backend. Refs #3566.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@9748 bcc190cf-cafb-0310-a4f2-bffc1f526a37

7 files changed, 115 insertions, 51 deletions

diff --git a/django/contrib/gis/db/models/__init__.py b/django/contrib/gis/db/models/__init__.py
index 02a2c5318e..270c73b283 100644
--- a/django/contrib/gis/db/models/__init__.py
+++ b/django/contrib/gis/db/models/__init__.py
@@ -1,6 +1,9 @@
 # Want to get everything from the 'normal' models package.
 from django.db.models import *
 
+# Geographic aggregate functions
+from django.contrib.gis.db.models.aggregates import *
+
 # The GeoManager
 from django.contrib.gis.db.models.manager import GeoManager
 
diff --git a/django/contrib/gis/db/models/aggregates.py b/django/contrib/gis/db/models/aggregates.py
index 111601171b..ab90d4d678 100644
--- a/django/contrib/gis/db/models/aggregates.py
+++ b/django/contrib/gis/db/models/aggregates.py
@@ -1,10 +1,28 @@
 from django.db.models import Aggregate
+from django.contrib.gis.db.backend import SpatialBackend
+from django.contrib.gis.db.models.sql import GeomField
 
-class Extent(Aggregate):
+class GeoAggregate(Aggregate):
+
+    def add_to_query(self, query, alias, col, source, is_summary):
+        if hasattr(source, '_geom'):
+            # Doing additional setup on the Query object for spatial aggregates.
+            aggregate = getattr(query.aggregates_module, self.name)
+            
+            # Adding a conversion class instance and any selection wrapping
+            # SQL (e.g., needed by Oracle).
+            if aggregate.conversion_class is GeomField:
+                query.extra_select_fields[alias] = GeomField()
+                if SpatialBackend.select:
+                    query.custom_select[alias] = SpatialBackend.select
+     
+        super(GeoAggregate, self).add_to_query(query, alias, col, source, is_summary)
+
+class Extent(GeoAggregate):
     name = 'Extent'
 
-class MakeLine(Aggregate):
+class MakeLine(GeoAggregate):
     name = 'MakeLine'
 
-class Union(Aggregate):
+class Union(GeoAggregate):
     name = 'Union'
diff --git a/django/contrib/gis/db/models/query.py b/django/contrib/gis/db/models/query.py
index 8eb435de93..fa127b5aaf 100644
--- a/django/contrib/gis/db/models/query.py
+++ b/django/contrib/gis/db/models/query.py
@@ -8,7 +8,6 @@ from django.contrib.gis.db.models.fields import GeometryField, PointField
 from django.contrib.gis.db.models.sql import AreaField, DistanceField, GeomField, GeoQuery, GeoWhereNode
 from django.contrib.gis.measure import Area, Distance
 from django.contrib.gis.models import get_srid_info
-qn = connection.ops.quote_name
 
 # For backwards-compatibility; Q object should work just fine
 # after queryset-refactor.
@@ -331,7 +330,7 @@ class GeoQuerySet(QuerySet):
         if SpatialBackend.oracle: agg_kwargs['tolerance'] = tolerance
 
         # Calling the QuerySet.aggregate, and returning only the value of the aggregate.
-        return self.aggregate(_geoagg=aggregate(agg_col, **agg_kwargs))['_geoagg']
+        return self.aggregate(geoagg=aggregate(agg_col, **agg_kwargs))['geoagg']
 
     def _spatial_attribute(self, att, settings, field_name=None, model_att=None):
         """
diff --git a/django/contrib/gis/db/models/sql/__init__.py b/django/contrib/gis/db/models/sql/__init__.py
index 4a66b41664..38d950706e 100644
--- a/django/contrib/gis/db/models/sql/__init__.py
+++ b/django/contrib/gis/db/models/sql/__init__.py
@@ -1,2 +1,3 @@
-from django.contrib.gis.db.models.sql.query import AreaField, DistanceField, GeomField, GeoQuery
+from django.contrib.gis.db.models.sql.conversion import AreaField, DistanceField, GeomField
+from django.contrib.gis.db.models.sql.query import GeoQuery
 from django.contrib.gis.db.models.sql.where import GeoWhereNode
diff --git a/django/contrib/gis/db/models/sql/aggregates.py b/django/contrib/gis/db/models/sql/aggregates.py
index ff76334249..d7f4acfcf1 100644
--- a/django/contrib/gis/db/models/sql/aggregates.py
+++ b/django/contrib/gis/db/models/sql/aggregates.py
@@ -1,24 +1,70 @@
 from django.db.models.sql.aggregates import *
-
 from django.contrib.gis.db.models.fields import GeometryField
+from django.contrib.gis.db.models.sql.conversion import GeomField
 from django.contrib.gis.db.backend import SpatialBackend
 
-if SpatialBackend.oracle:
+# Default SQL template for spatial aggregates.
+geo_template = '%(function)s(%(field)s)'
+
+# Default conversion functions for aggregates; will be overridden if implemented
+# for the spatial backend.
+def convert_extent(box):
+    raise NotImplementedError('Aggregate extent not implemented for this spatial backend.')
+
+def convert_geom(wkt, geo_field):
+    raise NotImplementedError('Aggregate method not implemented for this spatial backend.')
+
+if SpatialBackend.postgis:
+    def convert_extent(box):
+        # Box text will be something like "BOX(-90.0 30.0, -85.0 40.0)"; 
+        # parsing out and returning as a 4-tuple.
+        ll, ur = box[4:-1].split(',')
+        xmin, ymin = map(float, ll.split())
+        xmax, ymax = map(float, ur.split())
+        return (xmin, ymin, xmax, ymax)
+
+    def convert_geom(hex, geo_field):
+        if hex: return SpatialBackend.Geometry(hex)
+        else: return None
+elif SpatialBackend.oracle:
+    # Oracle spatial aggregates need a tolerance.
     geo_template = '%(function)s(SDOAGGRTYPE(%(field)s,%(tolerance)s))'
-else:
-    geo_template = '%(function)s(%(field)s)'
+
+    def convert_extent(clob):
+        if clob:
+            # Oracle returns a polygon for the extent, we construct
+            # the 4-tuple from the coordinates in the polygon.
+            poly = SpatialBackend.Geometry(clob.read())
+            shell = poly.shell
+            ll, ur = shell[0], shell[2]
+            xmin, ymin = ll
+            xmax, ymax = ur
+            return (xmin, ymin, xmax, ymax)
+        else:
+            return None
+    
+    def convert_geom(clob, geo_field):
+        if clob: return SpatialBackend.Geometry(clob.read(), geo_field._srid)
+        else: return None
 
 class GeoAggregate(Aggregate):
     # Overriding the SQL template with the geographic one.
     sql_template = geo_template
 
+    # Conversion class, if necessary.
+    conversion_class = None
+
+    # Flags for indicating the type of the aggregate.
     is_extent = False
 
     def __init__(self, col, source=None, is_summary=False, **extra):
         super(GeoAggregate, self).__init__(col, source, is_summary, **extra)
 
+        if not self.is_extent and SpatialBackend.oracle:
+            self.extra.setdefault('tolerance', 0.05)
+
         # Can't use geographic aggregates on non-geometry fields.
-        if not isinstance(self.source, GeometryField):
+        if not isinstance(self.source, GeometryField): 
             raise ValueError('Geospatial aggregates only allowed on geometry fields.')
 
         # Making sure the SQL function is available for this spatial backend.
@@ -28,9 +74,16 @@ class GeoAggregate(Aggregate):
 class Extent(GeoAggregate):
     is_extent = True
     sql_function = SpatialBackend.extent
+        
+if SpatialBackend.oracle:
+    # Have to change Extent's attributes here for Oracle.
+    Extent.conversion_class = GeomField
+    Extent.sql_template = '%(function)s(%(field)s)'
 
 class MakeLine(GeoAggregate):
+    conversion_class = GeomField
     sql_function = SpatialBackend.make_line
 
 class Union(GeoAggregate):
+    conversion_class = GeomField
     sql_function = SpatialBackend.unionagg
diff --git a/django/contrib/gis/db/models/sql/conversion.py b/django/contrib/gis/db/models/sql/conversion.py
new file mode 100644
index 0000000000..e7dd2fb62f
--- /dev/null
+++ b/django/contrib/gis/db/models/sql/conversion.py
@@ -0,0 +1,25 @@
+"""
+This module holds simple classes used by GeoQuery.convert_values
+to convert geospatial values from the database.
+"""
+class BaseField(object):
+    def get_internal_type(self):
+        "Overloaded method so OracleQuery.convert_values doesn't balk."
+        return None
+
+class AreaField(BaseField):
+    "Wrapper for Area values."
+    def __init__(self, area_att):
+        self.area_att = area_att
+
+class DistanceField(BaseField):
+    "Wrapper for Distance values."
+    def __init__(self, distance_att):
+        self.distance_att = distance_att
+
+class GeomField(BaseField):
+    """
+    Wrapper for Geometry values.  It is a lightweight alternative to 
+    using GeometryField (which requires a SQL query upon instantiation).
+    """
+    pass
diff --git a/django/contrib/gis/db/models/sql/query.py b/django/contrib/gis/db/models/sql/query.py
index 246ea0300f..928f209329 100644
--- a/django/contrib/gis/db/models/sql/query.py
+++ b/django/contrib/gis/db/models/sql/query.py
@@ -6,6 +6,7 @@ from django.db.models.fields.related import ForeignKey
 from django.contrib.gis.db.backend import SpatialBackend
 from django.contrib.gis.db.models.fields import GeometryField
 from django.contrib.gis.db.models.sql import aggregates as gis_aggregates_module
+from django.contrib.gis.db.models.sql.conversion import AreaField, DistanceField, GeomField
 from django.contrib.gis.db.models.sql.where import GeoWhereNode
 from django.contrib.gis.measure import Area, Distance
 
@@ -13,28 +14,6 @@ from django.contrib.gis.measure import Area, Distance
 ALL_TERMS = sql.constants.QUERY_TERMS.copy()
 ALL_TERMS.update(SpatialBackend.gis_terms)
 
-# Conversion functions used in normalizing geographic aggregates.
-if SpatialBackend.postgis:
-    def convert_extent(box):
-        # TODO: Parsing of BOX3D, Oracle support (patches welcome!)
-        # Box text will be something like "BOX(-90.0 30.0, -85.0 40.0)";
-        # parsing out and returning as a 4-tuple.
-        ll, ur = box[4:-1].split(',')
-        xmin, ymin = map(float, ll.split())
-        xmax, ymax = map(float, ur.split())
-        return (xmin, ymin, xmax, ymax)
-
-    def convert_geom(hex, geo_field):
-        if hex: return SpatialBackend.Geometry(hex)
-        else: return None
-else:
-    def convert_extent(box):
-        raise NotImplementedError('Aggregate extent not implemented for this spatial backend.')
-
-    def convert_geom(clob, geo_field):
-        if clob: return SpatialBackend.Geometry(clob.read(), geo_field._srid)
-        else: return None
-
 class GeoQuery(sql.Query):
     """
     A single spatial SQL query.
@@ -118,7 +97,7 @@ class GeoQuery(sql.Query):
 
         result.extend([
                 '%s%s' % (
-                    aggregate.as_sql(quote_func=qn),
+                    self.get_extra_select_format(alias) % aggregate.as_sql(quote_func=qn),
                     alias is not None and ' AS %s' % alias or ''
                     )
                 for alias, aggregate in self.aggregate_select.items()
@@ -228,7 +207,7 @@ class GeoQuery(sql.Query):
         """
         if SpatialBackend.oracle:
             # Running through Oracle's first.
-            value = super(GeoQuery, self).convert_values(value, field)
+            value = super(GeoQuery, self).convert_values(value, field or GeomField())
         if isinstance(field, DistanceField):
             # Using the field's distance attribute, can instantiate
             # `Distance` with the right context.
@@ -246,9 +225,9 @@ class GeoQuery(sql.Query):
         """
         if isinstance(aggregate, self.aggregates_module.GeoAggregate):
             if aggregate.is_extent:
-                return convert_extent(value)
+                return self.aggregates_module.convert_extent(value)
             else:
-                return convert_geom(value, aggregate.source)
+                return self.aggregates_module.convert_geom(value, aggregate.source)
         else:
             return super(GeoQuery, self).resolve_aggregate(value, aggregate)
 
@@ -361,17 +340,3 @@ class GeoQuery(sql.Query):
             # Otherwise, check by the given field name -- which may be
             # a lookup to a _related_ geographic field.
             return self._check_geo_field(self.model, field_name)
-
-### Field Classes for `convert_values` ####
-class AreaField(object):
-    def __init__(self, area_att):
-        self.area_att = area_att
-
-class DistanceField(object):
-    def __init__(self, distance_att):
-        self.distance_att = distance_att
-
-# Rather than use GeometryField (which requires a SQL query
-# upon instantiation), use this lighter weight class.
-class GeomField(object):
-    pass