Ticket #13788: 13788_geos_transform.diff

docs/internals/deprecation.txt

@@ -100 +100 @@
         * The ``no`` language code has been deprecated in favor of the ``nb``
           language code.
 
+    * 1.5
+        * :meth:`~django.contrib.gis.geos.GEOSGeometry.transform` will raise 
+          an exception when called and the geometry has no SRID set.
+
     * 2.0
         * ``django.views.defaults.shortcut()``. This function has been moved
           to ``django.contrib.contenttypes.views.shortcut()`` as part of the

docs/releases/1.3-alpha-1.txt

@@ +1 @@
+.. _releases-1.3-alpha-1:
+
+================================
+Django 1.3 alpha 1 release notes
+================================
+
+Backwards-incompatible changes in 1.3
+=====================================
+
+Wherever possible the new features above have been introduced in a
+backwards-compatible manner per :ref:`our API stability policy
+<misc-api-stability>` policy. This means that practically all existing
+code which worked with Django 1.2 will continue to work with Django
+1.3; such code will, however, begin issuing warnings (see below for
+details).
+
+However, a handful of features *have* changed in ways that, for some
+users, will be immediately backwards-incompatible. Those changes are
+detailed below.
+
+GeoDjango
+---------------
+
+* Calling :meth:`~django.contrib.gis.geos.GEOSGeometry.transform` when
+the GDAL library is not available used to silently do nothing. Except
+when the source & destination SRIDs match or are ``None``, this is a 
+potentially serious bug in application code. A 
+:class:`~django.contrib.gis.geos.GEOSException` is now raised. 

docs/ref/contrib/gis/install.txt

@@ -98 +98 @@
 .. 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.
+    Some features of GeoDjango (including the :ref:`ref-layermapping`, 
+    geometry reprojection, and the geographic admin) depend on its 
+    functionality.
 
 .. note::
 

docs/ref/contrib/gis/geos.txt

@@ -529 +529 @@
 
 .. note::
 
-    Requires GDAL.
+    Requires GDAL. Before 1.3, this method would silently no-op if GDAL wasn't 
+    available, but a GEOSException is now raised, since this is in all cases a bug.
+
+.. note::
+    
+    From 1.3, calling ``transform()`` when the SRID of the geometry is ``None`` or 
+    < 0 generates a warning, but continues. From 1.5 this behavior will change and 
+    a GEOSException will be raised. 
 
 ``Point``
 ---------

django/contrib/gis/geos/geometry.py

@@ -4 +4 @@
 """
 # 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
@@ -489 +490 @@
         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 transform!", stacklevel=2)
+            warnings.warn("Calling transform() with no SRID will raise a GEOSException from 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

@@ -853 +853 @@
             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
+        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)
+        
+        # test warning is raised
+        try:
+            warnings.simplefilter('error', FutureWarning)
+            
+            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', category=FutureWarning)
+
+        print "\END - expecting Warnings; safe to ignore.\n"
+
+    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.