From 1d39679e9cc8eec67fd10ce262e176ebd5b80a76 Mon Sep 17 00:00:00 2001 From: Peter Stace Date: Tue, 21 Mar 2023 06:00:25 +1100 Subject: [PATCH 1/5] Orient Polygon and MultiPolygon rings before GeoJSON marshalling This is to better follow RFC7946. See https://datatracker.ietf.org/doc/html/rfc7946 --- geom/geojson_marshal_test.go | 22 +++++++++++++++------- geom/type_multi_polygon.go | 1 + geom/type_polygon.go | 1 + 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/geom/geojson_marshal_test.go b/geom/geojson_marshal_test.go index 42c9cea9..0d2cfc4a 100644 --- a/geom/geojson_marshal_test.go +++ b/geom/geojson_marshal_test.go @@ -1,7 +1,9 @@ package geom_test import ( + "encoding/hex" "encoding/json" + "strings" "testing" ) @@ -181,13 +183,15 @@ func TestGeoJSONMarshal(t *testing.T) { wkt: `LINESTRING ZM (1 2 3 4,3 4 5 6,5 6 7 8)`, want: `{"type":"LineString","coordinates":[[1,2,3],[3,4,5],[5,6,7]]}`, }, + + // NOTE: Polygons oriented CCW in the output. { wkt: `POLYGON EMPTY`, want: `{"type":"Polygon","coordinates":[]}`, }, { wkt: `POLYGON((0 0,4 0,0 4,0 0),(1 1,2 1,1 2,1 1))`, - want: `{"type":"Polygon","coordinates":[[[0,0],[4,0],[0,4],[0,0]],[[1,1],[2,1],[1,2],[1,1]]]}`, + want: `{"type":"Polygon","coordinates":[[[0,0],[4,0],[0,4],[0,0]],[[1,1],[1,2],[2,1],[1,1]]]}`, }, { wkt: `POLYGON Z EMPTY`, @@ -195,7 +199,7 @@ func TestGeoJSONMarshal(t *testing.T) { }, { wkt: `POLYGON Z ((0 0 0,4 0 1,0 4 1,0 0 1),(1 1 2,2 1 3,1 2 4,1 1 5))`, - want: `{"type":"Polygon","coordinates":[[[0,0,0],[4,0,1],[0,4,1],[0,0,1]],[[1,1,2],[2,1,3],[1,2,4],[1,1,5]]]}`, + want: `{"type":"Polygon","coordinates":[[[0,0,0],[4,0,1],[0,4,1],[0,0,1]],[[1,1,5],[1,2,4],[2,1,3],[1,1,2]]]}`, }, { wkt: `POLYGON M EMPTY`, @@ -203,7 +207,7 @@ func TestGeoJSONMarshal(t *testing.T) { }, { wkt: `POLYGON M ((0 0 0,4 0 1,0 4 1,0 0 1),(1 1 2,2 1 3,1 2 4,1 1 5))`, - want: `{"type":"Polygon","coordinates":[[[0,0],[4,0],[0,4],[0,0]],[[1,1],[2,1],[1,2],[1,1]]]}`, + want: `{"type":"Polygon","coordinates":[[[0,0],[4,0],[0,4],[0,0]],[[1,1],[1,2],[2,1],[1,1]]]}`, }, { wkt: `POLYGON ZM EMPTY`, @@ -211,7 +215,7 @@ func TestGeoJSONMarshal(t *testing.T) { }, { wkt: `POLYGON ZM ((0 0 0 8,4 0 1 3,0 4 1 7,0 0 1 9),(1 1 2 3,2 1 3 7,1 2 4 8,1 1 5 4))`, - want: `{"type":"Polygon","coordinates":[[[0,0,0],[4,0,1],[0,4,1],[0,0,1]],[[1,1,2],[2,1,3],[1,2,4],[1,1,5]]]}`, + want: `{"type":"Polygon","coordinates":[[[0,0,0],[4,0,1],[0,4,1],[0,0,1]],[[1,1,5],[1,2,4],[2,1,3],[1,1,2]]]}`, }, { wkt: `MULTIPOINT EMPTY`, @@ -357,6 +361,8 @@ func TestGeoJSONMarshal(t *testing.T) { wkt: `MULTILINESTRING ZM ((0 1 1 2,2 3 4 8),EMPTY)`, want: `{"type":"MultiLineString","coordinates":[[[0,1,1],[2,3,4]],[]]}`, }, + + // NOTE: MultiPolygons oriented CCW in the output. { wkt: `MULTIPOLYGON EMPTY`, want: `{"type":"MultiPolygon","coordinates":[]}`, @@ -438,14 +444,16 @@ func TestGeoJSONMarshal(t *testing.T) { want: `{"type":"GeometryCollection","geometries":[{"type":"Point","coordinates":[1,2,3]},{"type":"Point","coordinates":[3,4,5]}]}`, }, } { - t.Run(tt.wkt, func(t *testing.T) { + desc := strings.ReplaceAll(tt.wkt, "(", "_") + desc = strings.ReplaceAll(desc, ")", "_") + t.Run(desc, func(t *testing.T) { geom := geomFromWKT(t, tt.wkt) gotJSON, err := json.Marshal(geom) expectNoErr(t, err) if string(gotJSON) != tt.want { t.Error("json doesn't match") - t.Logf("got: %v", string(gotJSON)) - t.Logf("want: %v", tt.want) + t.Logf("got:\n%v", hex.Dump(gotJSON)) + t.Logf("want:\n%v", hex.Dump([]byte(tt.want))) } }) } diff --git a/geom/type_multi_polygon.go b/geom/type_multi_polygon.go index 669556c7..2e865f30 100644 --- a/geom/type_multi_polygon.go +++ b/geom/type_multi_polygon.go @@ -307,6 +307,7 @@ func (m MultiPolygon) ConvexHull() Geometry { // MarshalJSON implements the encoding/json.Marshaler interface by encoding // this geometry as a GeoJSON geometry object. func (m MultiPolygon) MarshalJSON() ([]byte, error) { + m = m.ForceCCW() var dst []byte dst = append(dst, `{"type":"MultiPolygon","coordinates":`...) dst = appendGeoJSONSequenceMatrix(dst, m.Coordinates()) diff --git a/geom/type_polygon.go b/geom/type_polygon.go index d4fcf8a2..de2712c1 100644 --- a/geom/type_polygon.go +++ b/geom/type_polygon.go @@ -299,6 +299,7 @@ func (p Polygon) ConvexHull() Geometry { // MarshalJSON implements the encoding/json.Marshaler interface by encoding // this geometry as a GeoJSON geometry object. func (p Polygon) MarshalJSON() ([]byte, error) { + p = p.ForceCCW() var dst []byte dst = append(dst, `{"type":"Polygon","coordinates":`...) dst = appendGeoJSONSequences(dst, p.Coordinates()) From f0266ded73f6e28af85f8b5ee090fa6e491595a5 Mon Sep 17 00:00:00 2001 From: Peter Stace Date: Tue, 21 Mar 2023 06:03:07 +1100 Subject: [PATCH 2/5] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee48f3e9..1e27ff45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ - Add a `BoundingDiagonal` method to the `Envelope` type. +- Orient Polygon and MultiPolygon rings as per RFC7946 when serialising as + GeoJSON. + ## v0.41.0 2022-11-15 From ae47d78c50c12e6353854c6752aa81de183aa389 Mon Sep 17 00:00:00 2001 From: Peter Stace Date: Tue, 21 Mar 2023 09:05:08 +1100 Subject: [PATCH 3/5] Remove PostGIS GeoJSON reference implementation tests These are not worth the hassle that they're bringing. --- internal/cmprefimpl/cmppg/checks.go | 38 -------------------------- internal/cmprefimpl/cmppg/fuzz_test.go | 1 - 2 files changed, 39 deletions(-) diff --git a/internal/cmprefimpl/cmppg/checks.go b/internal/cmprefimpl/cmppg/checks.go index 8e65f616..913c140f 100644 --- a/internal/cmprefimpl/cmppg/checks.go +++ b/internal/cmprefimpl/cmppg/checks.go @@ -94,44 +94,6 @@ func hexStringToBytes(s string) ([]byte, error) { return buf, nil } -func checkGeoJSONParse(t *testing.T, pg PostGIS, candidates []string) { - var any bool - for i, geojson := range candidates { - if geojson == `{"type":"Point","coordinates":[]}` { - // From https://tools.ietf.org/html/rfc7946#section-3.1: - // - // > GeoJSON processors MAY interpret Geometry objects with - // > empty "coordinates" arrays as null objects. - // - // Simplefeatures chooses to accept this as an empty point, but - // Postgres rejects it. - continue - } - if geojson == `{"type":"MultiPolygon","coordinates":[[0,0]]}` { - // PostGIS erroneously accepts this as a valid geometry, but - // simplefeatures correctly rejects it. - continue - } - any = true - t.Run(fmt.Sprintf("CheckGeoJSONParse_%d", i), func(t *testing.T) { - _, sfErr := geom.UnmarshalGeoJSON([]byte(geojson)) - isValid, reason := pg.GeoJSONIsValidWithReason(t, geojson) - if (sfErr == nil) != isValid { - t.Logf("GeoJSON: %v", geojson) - t.Logf("SimpleFeatures err: %v", sfErr) - t.Logf("PostGIS IsValid: %v", isValid) - t.Logf("PostGIS Reason: %v", reason) - t.Errorf("mismatch") - } - }) - } - if !any { - // We know there are some some valid geojson strings, so if this happens - // then something is wrong with the extraction or conversion logic. - t.Errorf("could not extract any geojsons") - } -} - func checkWKB(t *testing.T, want UnaryResult, g geom.Geometry) { t.Run("CheckWKB", func(t *testing.T) { if g.IsEmpty() && ((g.IsGeometryCollection() && g.MustAsGeometryCollection().NumGeometries() > 0) || diff --git a/internal/cmprefimpl/cmppg/fuzz_test.go b/internal/cmprefimpl/cmppg/fuzz_test.go index 159b4920..e6ed71c1 100644 --- a/internal/cmprefimpl/cmppg/fuzz_test.go +++ b/internal/cmprefimpl/cmppg/fuzz_test.go @@ -23,7 +23,6 @@ func TestFuzz(t *testing.T) { checkWKTParse(t, pg, candidates) checkWKBParse(t, pg, candidates) - checkGeoJSONParse(t, pg, candidates) geoms := convertToGeometries(t, candidates) From 88ddc480c9e6ac6e4ba40c89f4c4f6ab3e8c8285 Mon Sep 17 00:00:00 2001 From: Peter Stace Date: Wed, 22 Mar 2023 05:41:51 +1100 Subject: [PATCH 4/5] Revert "Remove PostGIS GeoJSON reference implementation tests" This reverts commit ae47d78c50c12e6353854c6752aa81de183aa389. --- internal/cmprefimpl/cmppg/checks.go | 38 ++++++++++++++++++++++++++ internal/cmprefimpl/cmppg/fuzz_test.go | 1 + 2 files changed, 39 insertions(+) diff --git a/internal/cmprefimpl/cmppg/checks.go b/internal/cmprefimpl/cmppg/checks.go index 913c140f..8e65f616 100644 --- a/internal/cmprefimpl/cmppg/checks.go +++ b/internal/cmprefimpl/cmppg/checks.go @@ -94,6 +94,44 @@ func hexStringToBytes(s string) ([]byte, error) { return buf, nil } +func checkGeoJSONParse(t *testing.T, pg PostGIS, candidates []string) { + var any bool + for i, geojson := range candidates { + if geojson == `{"type":"Point","coordinates":[]}` { + // From https://tools.ietf.org/html/rfc7946#section-3.1: + // + // > GeoJSON processors MAY interpret Geometry objects with + // > empty "coordinates" arrays as null objects. + // + // Simplefeatures chooses to accept this as an empty point, but + // Postgres rejects it. + continue + } + if geojson == `{"type":"MultiPolygon","coordinates":[[0,0]]}` { + // PostGIS erroneously accepts this as a valid geometry, but + // simplefeatures correctly rejects it. + continue + } + any = true + t.Run(fmt.Sprintf("CheckGeoJSONParse_%d", i), func(t *testing.T) { + _, sfErr := geom.UnmarshalGeoJSON([]byte(geojson)) + isValid, reason := pg.GeoJSONIsValidWithReason(t, geojson) + if (sfErr == nil) != isValid { + t.Logf("GeoJSON: %v", geojson) + t.Logf("SimpleFeatures err: %v", sfErr) + t.Logf("PostGIS IsValid: %v", isValid) + t.Logf("PostGIS Reason: %v", reason) + t.Errorf("mismatch") + } + }) + } + if !any { + // We know there are some some valid geojson strings, so if this happens + // then something is wrong with the extraction or conversion logic. + t.Errorf("could not extract any geojsons") + } +} + func checkWKB(t *testing.T, want UnaryResult, g geom.Geometry) { t.Run("CheckWKB", func(t *testing.T) { if g.IsEmpty() && ((g.IsGeometryCollection() && g.MustAsGeometryCollection().NumGeometries() > 0) || diff --git a/internal/cmprefimpl/cmppg/fuzz_test.go b/internal/cmprefimpl/cmppg/fuzz_test.go index e6ed71c1..159b4920 100644 --- a/internal/cmprefimpl/cmppg/fuzz_test.go +++ b/internal/cmprefimpl/cmppg/fuzz_test.go @@ -23,6 +23,7 @@ func TestFuzz(t *testing.T) { checkWKTParse(t, pg, candidates) checkWKBParse(t, pg, candidates) + checkGeoJSONParse(t, pg, candidates) geoms := convertToGeometries(t, candidates) From 305a52b341418ab1b6b4701528b67e06f49a7996 Mon Sep 17 00:00:00 2001 From: Peter Stace Date: Wed, 22 Mar 2023 05:52:00 +1100 Subject: [PATCH 5/5] Use `aarch64` compatible PostGIS image --- .ci/docker-compose-cmppg.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.ci/docker-compose-cmppg.yml b/.ci/docker-compose-cmppg.yml index d22285e9..fadd7a3a 100644 --- a/.ci/docker-compose-cmppg.yml +++ b/.ci/docker-compose-cmppg.yml @@ -1,7 +1,9 @@ version: "2.1" services: postgis: - image: mdillon/postgis + image: ghcr.io/baosystems/postgis + environment: + POSTGRES_PASSWORD: password healthcheck: test: "pg_isready -U postgres" interval: '100ms'