FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

#6567 add support for writing GeoJSON with Z values by xfischer · Pull Request #6602 · npgsql/npgsql · GitHub

/ npgsql Public

#6567 add support for writing GeoJSON with Z values - #6602

Open
xfischer wants to merge 1 commit into
npgsql:mainfrom
xfischer:feature/6567-geojson-writez
Open

#6567 add support for writing GeoJSON with Z values#6602
xfischer wants to merge 1 commit into
npgsql:mainfrom
xfischer:feature/6567-geojson-writez

Conversation

xfischer commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

As discussed in #6567
Original typed Write methods wrote the Z double when a Position had an altitude, but never set the HasZ flag on the EWKB type header.
So 3D geometries were serialized with a 2D type tag. This change sets type |= EwkbGeometryType.HasZ.
It's a "first coordinate decides" assumption, to avoid going over the entire geojson tree for exhaustive check.

xfischer requested review from roji and vonzshik as code owners June 9, 2026 16:35

hiSandog commented Jul 4, 2026

Copy link
Copy Markdown

For GeoJSON Z support, the tests should include mixed 2D and 3D coordinates in the same geometry collection if PostgreSQL/PostGIS allows it. That catches serializers that infer dimensionality from the first coordinate and then drop or reject later Z values. It would also be useful to assert round-tripping of Position arrays with Z but no M, so Z support does not accidentally imply M handling.

xfischer commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@hiSandog thanks for the review! Both scenarios are now covered, and digging into them turned up a related bug beyond just missing test coverage:

  • Mixed 2D/3D within a MultiPoint/MultiLineString/MultiPolygon: FirstCoordinateHasZ decides the outer EWKB HasZ flag from the first member only, but nothing checked that the other members agreed. If they didn't, the writer emitted variably-sized member data while the reader kept assuming uniform sizing from the outer flag — silently corrupting the stream on read, exactly the failure mode you called out. Fixed by extending the existing "all-or-none" consistency check (already used for positions within one LineString/Polygon) up to these composite types, so a mismatch now throws a clear ArgumentException instead of producing bad EWKB.
  • GeometryCollection: I initially assumed heterogeneous 2D/3D children already round-tripped fine, since the reader re-parses each child's own header. Turns out PostGIS itself rejects it server-side (Dimensions mismatch in lwcollection), confirmed directly against a live instance with plain WKT — independent of anything in this library. So GeometryCollection now gets the same client-side guard, and the new test asserts it throws rather than round-trips.
  • Position with Z but no M: added Roundtrip_Z_without_M, confirming the altitude survives round-tripping on its own without implying/attaching M.

Pushed as a new commit on top of the branch.

Copy link
Copy Markdown
Contributor Author

Rebased to match main new file organisation

Typed Write methods wrote the Z double when a Position had an
altitude, but never set the HasZ flag on the EWKB type header, so 3D
geometries were serialized with a 2D type tag. Set type |=
EwkbGeometryType.HasZ using a "first coordinate decides" assumption to
avoid walking the entire geojson tree for an exhaustive check.

Extend the existing all-or-none Z consistency validation to
MultiPoint/MultiLineString/MultiPolygon/GeometryCollection members,
matching what PostGIS enforces server-side (it rejects mixed 2D/3D
collections with "Dimensions mismatch in lwcollection"). Point,
LineString, and Polygon already guarded intra-geometry consistency;
composite types had no equivalent check across members, which could
corrupt the EWKB since the reader assumes uniform dimensionality based
on the outer flag.

Adds tests covering the new guards and a Z-without-M round-trip case.
xfischer force-pushed the feature/6567-geojson-writez branch from 6b1dcd3 to 42a4f93 Compare July 20, 2026 12:21
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL