Fixed #13788 -- `GEOSGeometry.transform` no longer silently no-ops when GDAL…

Fixed #13788 -- `GEOSGeometry.transform` no longer silently no-ops when GDAL isn't available.  Thanks, Rob Coup.

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

django/contrib/gis/geos/geometry.py

@@ -4,6 +4,7 @@
 """
 # Python, ctypes and types dependencies.
 import re
+import warnings
 from ctypes import addressof, byref, c_double, c_size_t
 
 # super-class for mutable list behavior
@@ -498,23 +499,40 @@ class GEOSGeometry(GEOSBase, ListMixin):
         instead.
         """
         srid = self.srid
-        if gdal.HAS_GDAL and srid:
-            # Creating an OGR Geometry, which is then transformed.
-            g = gdal.OGRGeometry(self.wkb, srid)
-            g.transform(ct)
-            # Getting a new GEOS pointer
-            ptr = wkb_r().read(g.wkb)
+
+        if ct == srid:
+            # short-circuit where source & dest SRIDs match
             if clone:
-                # User wants a cloned transformed geometry returned.
-                return GEOSGeometry(ptr, srid=g.srid)
-            if ptr:
-                # Reassigning pointer, and performing post-initialization setup
-                # again due to the reassignment.
-                capi.destroy_geom(self.ptr)
-                self.ptr = ptr
-                self._post_init(g.srid)
+                return self.clone()
             else:
-                raise GEOSException('Transformed WKB was invalid.')
+                return
+
+        if (srid is None) or (srid < 0):
+            warnings.warn("Calling transform() with no SRID set does no transformation!",
+                          stacklevel=2)
+            warnings.warn("Calling transform() with no SRID will raise GEOSException in v1.5",
+                          FutureWarning, stacklevel=2)
+            return
+
+        if not gdal.HAS_GDAL:
+            raise GEOSException("GDAL library is not available to transform() geometry.")
+
+        # Creating an OGR Geometry, which is then transformed.
+        g = gdal.OGRGeometry(self.wkb, srid)
+        g.transform(ct)
+        # Getting a new GEOS pointer
+        ptr = wkb_r().read(g.wkb)
+        if clone:
+            # User wants a cloned transformed geometry returned.
+            return GEOSGeometry(ptr, srid=g.srid)
+        if ptr:
+            # Reassigning pointer, and performing post-initialization setup
+            # again due to the reassignment.
+            capi.destroy_geom(self.ptr)
+            self.ptr = ptr
+            self._post_init(g.srid)
+        else:
+            raise GEOSException('Transformed WKB was invalid.')
 
     #### Topology Routines ####
     def _topology(self, gptr):

django/contrib/gis/geos/tests/test_geos.py

@@ -852,6 +852,114 @@ class GEOSTest(unittest.TestCase, TestDataMixin):
             self.assertAlmostEqual(trans.x, p.x, prec)
             self.assertAlmostEqual(trans.y, p.y, prec)
 
+    def test23_transform_noop(self):
+        """ Testing `transform` method (SRID match) """
+        # transform() should no-op if source & dest SRIDs match,
+        # regardless of whether GDAL is available.
+        if gdal.HAS_GDAL:
+            g = GEOSGeometry('POINT (-104.609 38.255)', 4326)
+            gt = g.tuple
+            g.transform(4326)
+            self.assertEqual(g.tuple, gt)
+            self.assertEqual(g.srid, 4326)
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', 4326)
+            g1 = g.transform(4326, clone=True)
+            self.assertEqual(g1.tuple, g.tuple)
+            self.assertEqual(g1.srid, 4326)
+            self.assert_(g1 is not g, "Clone didn't happen")
+
+        old_has_gdal = gdal.HAS_GDAL
+        try:
+            gdal.HAS_GDAL = False
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', 4326)
+            gt = g.tuple
+            g.transform(4326)
+            self.assertEqual(g.tuple, gt)
+            self.assertEqual(g.srid, 4326)
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', 4326)
+            g1 = g.transform(4326, clone=True)
+            self.assertEqual(g1.tuple, g.tuple)
+            self.assertEqual(g1.srid, 4326)
+            self.assert_(g1 is not g, "Clone didn't happen")
+        finally:
+            gdal.HAS_GDAL = old_has_gdal
+
+    def test23_transform_nosrid(self):
+        """ Testing `transform` method (no SRID) """
+        # raise a warning if SRID <0/None
+        import warnings
+        print "\nBEGIN - expecting Warnings; safe to ignore.\n"
+
+        # test for do-nothing behaviour.
+        try:
+            # Keeping line-noise down by only printing the relevant
+            # warnings once.
+            warnings.simplefilter('once', UserWarning)
+            warnings.simplefilter('once', FutureWarning)    
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', srid=None)
+            g.transform(2774)
+            self.assertEqual(g.tuple, (-104.609, 38.255))
+            self.assertEqual(g.srid, None)
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', srid=None)
+            g1 = g.transform(2774, clone=True)
+            self.assert_(g1 is None)
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', srid=-1)
+            g.transform(2774)
+            self.assertEqual(g.tuple, (-104.609, 38.255))
+            self.assertEqual(g.srid, -1)
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', srid=-1)
+            g1 = g.transform(2774, clone=True)
+            self.assert_(g1 is None)
+
+        finally:
+            warnings.simplefilter('default', UserWarning)
+            warnings.simplefilter('default', FutureWarning)
+
+        print "\nEND - expecting Warnings; safe to ignore.\n"
+
+
+        # test warning is raised
+        try:
+            warnings.simplefilter('error', FutureWarning)
+            warnings.simplefilter('ignore', UserWarning)
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', srid=None)
+            self.assertRaises(FutureWarning, g.transform, 2774)
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', srid=None)
+            self.assertRaises(FutureWarning, g.transform, 2774, clone=True)
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', srid=-1)
+            self.assertRaises(FutureWarning, g.transform, 2774)
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', srid=-1)
+            self.assertRaises(FutureWarning, g.transform, 2774, clone=True)
+        finally:
+            warnings.simplefilter('default', FutureWarning)
+            warnings.simplefilter('default', UserWarning)
+
+
+    def test23_transform_nogdal(self):
+        """ Testing `transform` method (GDAL not available) """
+        old_has_gdal = gdal.HAS_GDAL
+        try:
+            gdal.HAS_GDAL = False
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', 4326)
+            self.assertRaises(GEOSException, g.transform, 2774)
+
+            g = GEOSGeometry('POINT (-104.609 38.255)', 4326)
+            self.assertRaises(GEOSException, g.transform, 2774, clone=True)
+        finally:
+            gdal.HAS_GDAL = old_has_gdal
+
     def test24_extent(self):
         "Testing `extent` method."
         # The xmin, ymin, xmax, ymax of the MultiPoint should be returned.

docs/internals/deprecation.txt

@@ -147,6 +147,10 @@ their deprecation, as per the :ref:`Django deprecation policy
           The ``supports_inactive_user`` variable is not checked any
           longer and can be removed.
 
+        * :meth:`~django.contrib.gis.geos.GEOSGeometry.transform` will raise 
+          a :class:`~django.contrib.gis.geos.GEOSException` when called 
+          on a geometry with no SRID value.
+
     * 2.0
         * ``django.views.defaults.shortcut()``. This function has been moved
           to ``django.contrib.contenttypes.views.shortcut()`` as part of the

docs/ref/contrib/gis/geos.txt

@@ -523,7 +523,9 @@ corresponding to the SRID of the geometry or ``None``.
 
     Requires GDAL.
 
-.. method:: transform(ct, clone=False)
+.. method:: GEOSGeometry.transform(ct, clone=False)
+
+.. versionchanged:: 1.3
 
 Transforms the geometry according to the given coordinate transformation paramter
 (``ct``), which may be an integer SRID, spatial reference WKT string,
@@ -537,6 +539,16 @@ is returned instead.
 
     Requires GDAL.
 
+.. note::
+
+   Prior to 1.3, this method would silently no-op if GDAL was not available.
+   Now, a :class:`~django.contrib.gis.geos.GEOSException` is raised as
+   application code relying on this behavior is in error. In addition,
+   use of this method when the SRID is ``None`` or less than 0 now generates
+   a warning because a :class:`~django.contrib.gis.geos.GEOSException` will
+   be raised instead in version 1.5.
+
+
 ``Point``
 ---------
 

docs/ref/contrib/gis/install.txt

@@ -98,8 +98,9 @@ Program                   Description                           Required
 .. admonition::  Install GDAL
 
     While :ref:`gdalbuild` is technically not required, it is *recommended*.
-    Some features of GeoDjango (including the :ref:`ref-layermapping` and the geographic
-    admin) depend on its functionality.
+    Important features of GeoDjango (including the :ref:`ref-layermapping`, 
+    geometry reprojection, and the geographic admin) depend on its 
+    functionality.
 
 .. note::
 
@@ -273,9 +274,9 @@ supports :ref:`GDAL's vector data <ref-gdal>` capabilities [#]_.
 
 First download the latest GDAL release version and untar the archive::
 
-    $ wget http://download.osgeo.org/gdal/gdal-1.7.2.tar.gz
-    $ tar xzf gdal-1.7.2.tar.gz
-    $ cd gdal-1.7.2
+    $ wget http://download.osgeo.org/gdal/gdal-1.7.3.tar.gz
+    $ tar xzf gdal-1.7.3.tar.gz
+    $ cd gdal-1.7.3
 
 Configure, make and install::
 

docs/releases/1.3.txt

@@ -528,6 +528,14 @@ statements manually.
 GeoDjango
 ~~~~~~~~~
 
-The :setting:`TEST_RUNNER` previously used to execute the GeoDjango test suite,
-:func:`django.contrib.gis.tests.run_gis_tests`, is deprecated in favor of
-the :class:`django.contrib.gis.tests.GeoDjangoTestSuiteRunner` class.
+  * The function-based :setting:`TEST_RUNNER` previously used to execute
+    the GeoDjango test suite, :func:`django.contrib.gis.tests.run_gis_tests`,
+    was deprecated for the class-bassed runner,
+    :class:`django.contrib.gis.tests.GeoDjangoTestSuiteRunner`.
+
+  * Previously, calling :meth:`~django.contrib.gis.geos.GEOSGeometry.transform`
+    would silently do nothing when GDAL wasn't available.  Now,
+    a :class:`~django.contrib.gis.geos.GEOSException` is properly raised
+    to indicate possible faulty application code.  A warning is now raised
+    if :meth:`~django.contrib.gis.geos.GEOSGeometry.transform` is called when
+    the SRID of the geometry is less than 0 or ``None``.