Merge branch 'master' into do_not_panic_when_no_rings_to_extract

.ci/docker-compose-cmppg.yml

@@ -1,7 +1,9 @@
 version: "2.1"
 services:
   postgis:
-    image: mdillon/postgis
+    image: ghcr.io/baosystems/postgis:15
+    environment:
+      POSTGRES_PASSWORD: password
     healthcheck:
       test: "pg_isready -U postgres"
       interval: '100ms'

.ci/docker-compose-pgscan.yml

@@ -1,7 +1,9 @@
 version: "2.1"
 services:
   postgis:
-    image: mdillon/postgis
+    image: ghcr.io/baosystems/postgis:15
+    environment:
+      POSTGRES_PASSWORD: password
     healthcheck:
       test: "pg_isready -U postgres"
       interval: '100ms'

.ci/run_ci.sh

@@ -5,11 +5,11 @@ set -eo pipefail
 start="$(date +%s.%N)"
 here="$(dirname "$(readlink -f "$0")")"
 
-docker-compose -f "$here/docker-compose-lint.yml"    up --abort-on-container-exit
-docker-compose -f "$here/docker-compose-unit.yml"    up --abort-on-container-exit
-docker-compose -f "$here/docker-compose-geos.yml"    up --abort-on-container-exit
-docker-compose -f "$here/docker-compose-pgscan.yml"  up --abort-on-container-exit
-docker-compose -f "$here/docker-compose-cmppg.yml"   up --abort-on-container-exit
-docker-compose -f "$here/docker-compose-cmpgeos.yml" up --abort-on-container-exit
+docker-compose -p sf-lint    -f "$here/docker-compose-lint.yml"    up --abort-on-container-exit
+docker-compose -p sf-unit    -f "$here/docker-compose-unit.yml"    up --abort-on-container-exit
+docker-compose -p sf-geos    -f "$here/docker-compose-geos.yml"    up --abort-on-container-exit
+docker-compose -p sf-pgscan  -f "$here/docker-compose-pgscan.yml"  up --abort-on-container-exit
+docker-compose -p sf-cmppg   -f "$here/docker-compose-cmppg.yml"   up --abort-on-container-exit
+docker-compose -p sf-cmpgeos -f "$here/docker-compose-cmpgeos.yml" up --abort-on-container-exit
 
 printf "\nduration: %.1f seconds\n" "$(echo "$(date +%s.%N) - $start" | bc)"

geos/entrypoints_test.go

@@ -732,7 +732,14 @@ func TestBuffer(t *testing.T) {
 			t.Logf("WKT: %v", g.AsText())
 			got, err := Buffer(g, tt.radius, tt.opts...)
 			expectNoErr(t, err)
-			expectGeomEq(t, got, geomFromWKT(t, tt.want), geom.IgnoreOrder)
+			expectGeomEq(t, got, geomFromWKT(t, tt.want),
+				// Different versions of GEOS can output geometry parts in
+				// different orders, but there is no semantic difference.
+				geom.IgnoreOrder,
+				// Account for some slight differences in GEOS behaviour
+				// between `x86_64` and `aarch64`:
+				geom.ToleranceXY(1e-15),
+			)
 		})
 	}
 }

internal/cmprefimpl/cmppg/checks.go

@@ -104,14 +104,23 @@ func checkGeoJSONParse(t *testing.T, pg PostGIS, candidates []string) {
 			// > empty "coordinates" arrays as null objects.
 			//
 			// Simplefeatures chooses to accept this as an empty point, but
-			// Postgres rejects it.
+			// PostGIS rejects it.
 			continue
 		}
 		if geojson == `{"type":"MultiPolygon","coordinates":[[0,0]]}` {
 			// PostGIS erroneously accepts this as a valid geometry, but
 			// simplefeatures correctly rejects it.
 			continue
 		}
+		if strings.Contains(geojson, "MultiPoint") && strings.Contains(geojson, "[]") {
+			// From https://www.rfc-editor.org/rfc/rfc7946#section-3.1.1:
+			//
+			// > A position is an array of numbers. There MUST be
+			// > two or more elements.
+			//
+			// PostGIS erroneously accepts MultiPoints with empty positions.
+			continue
+		}
 		any = true
 		t.Run(fmt.Sprintf("CheckGeoJSONParse_%d", i), func(t *testing.T) {
 			_, sfErr := geom.UnmarshalGeoJSON([]byte(geojson))
@@ -170,6 +179,13 @@ func checkGeoJSON(t *testing.T, want UnaryResult, g geom.Geometry) {
 			// even valid JSON, let alone valid GeoJSON).
 			return
 		}
+		if isNonZeroCollectionWithOnlyEmptyChildren(g) {
+			// The spec [1] gives some leeway to implementers as to how these
+			// should be marshalled. The behaviour of PostGIS and
+			// simplefeatures differs slightly.
+			// [1]: https://www.rfc-editor.org/rfc/rfc7946
+			return
+		}
 		got, err := g.MarshalJSON()
 		if err != nil {
 			t.Fatalf("could not convert to geojson: %v", err)
@@ -187,6 +203,30 @@ func checkGeoJSON(t *testing.T, want UnaryResult, g geom.Geometry) {
 	})
 }
 
+func isNonZeroCollectionWithOnlyEmptyChildren(g geom.Geometry) bool {
+	if !g.IsEmpty() {
+		return false
+	}
+	switch g.Type() {
+	case geom.TypePoint, geom.TypeLineString, geom.TypePolygon:
+		return false
+	case geom.TypeMultiPoint:
+		mp := g.MustAsMultiPoint()
+		return mp.NumPoints() > 0
+	case geom.TypeMultiLineString:
+		mls := g.MustAsMultiLineString()
+		return mls.NumLineStrings() > 0
+	case geom.TypeMultiPolygon:
+		mp := g.MustAsMultiPolygon()
+		return mp.NumPolygons() > 0
+	case geom.TypeGeometryCollection:
+		gc := g.MustAsGeometryCollection()
+		return gc.NumGeometries() > 0
+	default:
+		panic("unhandled")
+	}
+}
+
 func geojsonEqual(gj1, gj2 string) error {
 	ts1 := tokenize(gj1)
 	ts2 := tokenize(gj2)

internal/cmprefimpl/cmppg/fuzz_test.go

@@ -39,6 +39,9 @@ func TestFuzz(t *testing.T) {
 				t.Skip("Causes unmarshalling to fail for derivative geometry")
 			}
 
+			if isMultiPointWithEmptyPoint(g) {
+				t.Skip("PostGIS cannot handle MultiPoints that contain empty Points")
+			}
 			want, err := BatchPostGIS(pg).Unary(g)
 			if err != nil {
 				t.Fatalf("could not get result from postgis: %v", err)
@@ -158,3 +161,16 @@ func convertToGeometries(t *testing.T, candidates []string) []geom.Geometry {
 
 	return geoms
 }
+
+func isMultiPointWithEmptyPoint(g geom.Geometry) bool {
+	mp, ok := g.AsMultiPoint()
+	if !ok {
+		return false
+	}
+	for i := 0; i < mp.NumPoints(); i++ {
+		if mp.PointN(i).IsEmpty() {
+			return true
+		}
+	}
+	return false
+}

internal/cmprefimpl/cmppg/pg.go

@@ -2,7 +2,9 @@ package main
 
 import (
 	"database/sql"
+	"math"
 	"strings"
+	"time"
 
 	"github.com/lib/pq"
 	"github.com/peterstace/simplefeatures/geom"
@@ -46,6 +48,28 @@ const (
 
 // Unary runs a batch of unary operations on a geometry.
 func (p BatchPostGIS) Unary(g geom.Geometry) (UnaryResult, error) {
+	// Retry several times because requests to PostGIS can intermittently fail.
+	// Requests to PostGIS could fail for many reasons. For example, a previous
+	// test case could cause PostGIS to segfault, causing it to restart (so it
+	// may not yet be ready to accept connections by the time this test
+	// executes).
+	var i int
+	for {
+		u, err := p.tryUnary(g)
+		if err == nil {
+			return u, nil
+		}
+
+		i++
+		if i >= 10 {
+			return UnaryResult{}, err
+		}
+
+		time.Sleep(time.Millisecond * time.Duration(math.Pow(2, float64(i))))
+	}
+}
+
+func (p BatchPostGIS) tryUnary(g geom.Geometry) (UnaryResult, error) {
 	// WKB and WKB forms returned from PostGIS don't _always_ give the same
 	// result (usually differences around empty geometries). In the case of
 	// boundary, convex hull, and reverse, they are different enough that it's