Opened 8 years ago

Closed 8 years ago

#666 closed defect (fixed)

ST_DumpPoints is not null safe

Reported by: robe Owned by: pramsey
Priority: medium Milestone: PostGIS 1.5.3
Component: postgis Version: 1.5.X
Keywords: history Cc:

Description (last modified by robe)

SELECT ST_DumpPoints(foo1.the_geom)
	FROM ((SELECT CAST(Null As geometry) As the_geom)) As foo1;

Gives error -

ERROR:  Unexpected error while dumping geometry <NULL>
CONTEXT:  SQL function "st_dumppoints" statement 1

Change History (5)

comment:1 Changed 8 years ago by yabo

Here's a patch. I'm not sure of what should be in the regress test though. Empty lines ? NULL ? I can't run make check from where I am now.

Index: postgis/
--- postgis/    (revision 6203)
+++ postgis/    (working copy)
@@ -1950,6 +1950,10 @@
   -- Special case collections : iterate and return the DumpPoints of the geometries
+  IF (the_geom IS NULL OR ST_IsEmpty(the_geom)) THEN
+    RETURN;
+  END IF;
   IF (ST_IsCollection(the_geom)) THEN
     i = 1;
Index: regress/dumppoints.sql
--- regress/dumppoints.sql      (revision 6203)
+++ regress/dumppoints.sql      (working copy)
@@ -157,3 +157,19 @@
         )'::geometry AS geom
     ) AS g
   ) j;
+SELECT path, ST_AsText(geom)
+  SELECT (ST_DumpPoints(g.geom)).*
+    (SELECT NULL::geometry AS geom
+    ) AS g
+  ) j;
+SELECT path, ST_AsText(geom)
+  SELECT (ST_DumpPoints(g.geom)).*
+    ) AS g
+  ) j;
Index: regress/dumppoints_expected
--- regress/dumppoints_expected (revision 6203)
+++ regress/dumppoints_expected (working copy)
@@ -82,3 +82,5 @@
 {5,1,2,2}|POINT(5 6)
 {5,1,2,3}|POINT(6 6)
 {5,1,2,4}|POINT(5 5)

comment:2 Changed 8 years ago by robe

I was wondering if it wouldn't be sufficient to just mark this function as STRICT like the way ST_Dump is marked. Do people think its necessary to even put logic about NULL handling in the function if the function is marked as strict.

Even though this is an SQL function, I don't see any benefit of keeping it non-strict since it could never utilize spatial indexes anyway.

comment:3 Changed 8 years ago by robe

I think this one is easy to knock off too -- opinions?

comment:4 Changed 8 years ago by pramsey

Don't ask, do.

comment:5 Changed 8 years ago by robe

Description: modified (diff)
Keywords: history added
Resolution: fixed
Status: newclosed

Fixed at r7360 for 1.5 and r7361 for PostGIS 2.0.0. Note the _ST_DumpPoints still has an issue thought no one would be calling that directly. Can't make that strict since it should have nulls passed in. For a more permanent solution - we might want to just go with yabo's suggestion.

With my fix, dumping a NULL will return no records which is hmm okay with me.

Note: See TracTickets for help on using tickets.