Fixed #26789 -- Fixed handling of empty geometries in BaseSpatialField.get_db_prep_save #7428
Conversation
|
Feature wise implementation looks alright. I'm just not sure if passing responsibility to figure out geometry to Adapter best option here. Also to me it looks like whole |
What do you mean by 'figure out geometry'?
In this PR it's used to skip related test. Also it could be used to raise an exception if empty geometry is used on backend that doesn't support empty geometries (replacing empty geometry with NULL in query will lead to unexpected in some cases). But if we decide to do it, it should be done within another ticket. |
|
Sorry bad wording. I meant that if Adapter is correct place to make decision about empty geometry handling. Also doesn't Geometry class has .empty property you could test instead of WKT string? But beyond that, otherwise I think these changes are good to go. |
| self._adapter.prepare(conn) | ||
|
|
||
| def getquoted(self): | ||
| """ | ||
| Return a properly quoted string for use in PostgreSQL/PostGIS. | ||
| """ | ||
| if not self.ewkb: | ||
| return "ST_GeomFromText('POINT EMPTY', %s)" % self.srid |
There was a problem hiding this comment.
There's other types of empty geometry, shouldn't this be type-specific? (ala GEOMETRYCOLLECTION EMPTY, POLYGON EMPTY, etc)
There was a problem hiding this comment.
Only POINT EMPTY is not representable as WKB.
| @@ -19,7 +19,9 @@ def __init__(self, obj, geography=False): | |||
|
|
|||
| # Getting the WKB (in string form, to allow easy pickling of | |||
| # the adaptor) and the SRID from the geometry or raster. | |||
| if self.is_geometry: | |||
| if isinstance(obj, PostGISAdapter) and not obj.ewkb or isinstance(obj, Geometry) and obj.wkt == 'POINT EMPTY': | |||
There was a problem hiding this comment.
Check obj.empty instead of serialising to WKT and checking the string?
sir-sigurd
force-pushed
the
gis-db-empty-2
branch
2 times, most recently
from
146fb76
to
bb87061
Compare
| @@ -19,7 +19,12 @@ def __init__(self, obj, geography=False): | |||
|
|
|||
| # Getting the WKB (in string form, to allow easy pickling of | |||
| # the adaptor) and the SRID from the geometry or raster. | |||
| if self.is_geometry: | |||
| if (isinstance(obj, PostGISAdapter) and not obj.ewkb or | |||
| isinstance(obj, Geometry) and obj.empty and obj.geom_type == 'Point'): | |||
There was a problem hiding this comment.
In a brief spot check, I noticed that no tests are failing if obj.geom_type == 'Point' is removed. Not sure if it's something that should be tested or not.
sir-sigurd
force-pushed
the
gis-db-empty-2
branch
4 times, most recently
from
454004b
to
b261db5
Compare
timgraham
force-pushed
the
gis-db-empty-2
branch
from
b261db5
to
2e844a5
Compare
timgraham
force-pushed
the
gis-db-empty-2
branch
from
2e844a5
to
183f501
Compare
https://code.djangoproject.com/ticket/26789