Code

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#13788 closed (fixed)

GEOSGeometry.transform() silently no-ops when GDAL isn't available or srid is None

Reported by: rcoup Owned by: rcoup
Component: GIS Version: master
Severity: Keywords: transform geos gdal
Cc: jbronn Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

GEOSGeometry.transform() uses the GDAL library to re-project geometries. If GDAL is not available however, it silently does nothing.

Geodjango-Users discussion thread: http://groups.google.com/group/geodjango/browse_thread/thread/788cafdbd592d617

Current behaviour if GDAL is unavailable:

  • .transform(clone=False) (the default) does nothing
  • .transform(clone=True) returns None

Also related:

  • .transform(clone=False) (the default) called on a geometry where .srid is None does nothing
  • .transform(clone=True) called on a geometry where .srid is None returns None

Since transform(clone=False) (the default) mutates the geometry, it should raise an exception if it can't do what the caller asked.

The attached patch:

  • short-circuits re-projection if the source and destination SRIDs match
  • makes .transform() raise a GEOSGeometry.Error if GDAL isn't available
  • adds tests around the new behaviour
  • adds a FutureWarning if the srid of the geometry is None or <0, pointing out that in 1.5 the behaviour will change an error will be raised.
  • adds some docs, including notes to the backwards-incompatibilities for 1.3

The backwards incompatible behaviour change is being introduced because:

  • .transform() no-op'ing if source & destination SRIDs don't match is NOT what the caller wants - it is in all cases a bug.
  • It's unlikely the user was manually checking either the return value (clone=True) or the geometry's srid (clone=False) and raising their own exception, the more common case would be to assert on HAS_GDAL.

Attachments (2)

13788_geos_transform.diff (10.1 KB) - added by rcoup 4 years ago.
initial patch
13788_geos_transform.2.diff (10.0 KB) - added by jbronn 3 years ago.
Updated

Download all attachments as: .zip

Change History (6)

Changed 4 years ago by rcoup

initial patch

comment:1 Changed 4 years ago by rcoup

  • Cc jbronn added
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

My documentation/rest-fu isn't strong, so probably worth a look there, and i'm not sure whether the notes for the release are all in the right places :)

comment:2 Changed 4 years ago by rcoup

  • Owner changed from nobody to rcoup
  • Status changed from new to assigned

Changed 3 years ago by jbronn

Updated

comment:3 Changed 3 years ago by jbronn

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

(In [15025]) Fixed #13788 -- GEOSGeometry.transform no longer silently no-ops when GDAL isn't available. Thanks, Rob Coup.

comment:4 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.