Fix WKTWriter invalid dimensional EMPTY tokens for nested empty geometries - #1225
Fix WKTWriter invalid dimensional EMPTY tokens for nested empty geometries#1225arimu1 wants to merge 1 commit into
Conversation
…ries When writing empty geometries with Z, M, or ZM ordinates, ensure the dimension marker and EMPTY are separate tokens so WKTReader can round-trip the output. Add regression tests for nested empty Z, M, and ZM cases. Fixes locationtech#1223 Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
grootstebozewolf
left a comment
grootstebozewolf
left a comment
There was a problem hiding this comment.
LGTM — approve with nits.
The EMPTY-only space is the right shape for #1223. appendOrdinateText has no trailing space, so a nested empty used to emit MULTILINESTRING ZEMPTY and WKTReader failed with Expected EMPTY or ( but found 'ZEMPTY'. Do not “fix” this by adding a trailing space on the ordinate marker; existing goldens pin non-empty WKT as POINT Z(1 1 1).
All six instance EMPTY sites go through appendEmptyText. XY empty stays POINT EMPTY. The new test is the exact #1223 repro for Z, M, and ZM.
Nits (non-blocking):
- Coverage is only nested
MULTILINESTRING. One top-levelPOINT Z EMPTYwould pin the other call sites. containsinstead of exact WKT is fine next to theZEMPTYreject.
| private void appendEmptyText(EnumSet<Ordinate> outputOrdinates, Writer writer) throws IOException { | ||
| if (outputOrdinates.contains(Ordinate.Z) || outputOrdinates.contains(Ordinate.M)) { | ||
| writer.write(" "); | ||
| } | ||
| writer.write(WKTConstants.EMPTY); | ||
| } |
There was a problem hiding this comment.
Right cut. appendOrdinateText writes Z/M/ZM with no trailing space, and existing goldens pin non-empty WKT as POINT Z(1 1 1), not Z (. Do not “fix” this by adding a trailing space on the ordinate marker — that would change every dimensional token.
Spacing only before EMPTY when Z or M is present also keeps XY as POINT EMPTY: the tagged writer already wrote the space after the type, so this helper must not add a second one.
| { | ||
| if (geometryCollection.getNumGeometries() == 0) { | ||
| writer.write(WKTConstants.EMPTY); | ||
| appendEmptyText(outputOrdinates, writer); |
There was a problem hiding this comment.
All six instance writer.write(EMPTY) sites now go through the helper (sequence / polygon / multi-point / multi-linestring / multi-polygon / geometry collection). Static toLineString already used " " + EMPTY and is correctly left alone.
| Geometry geometryZ = reader.read( | ||
| "GEOMETRYCOLLECTION Z (" | ||
| + "LINESTRING Z (0 0 1, 1 1 2), " | ||
| + "MULTILINESTRING Z EMPTY)"); | ||
| String writtenZ = writer4.write(geometryZ); | ||
| assertTrue(writtenZ.contains("MULTILINESTRING Z EMPTY")); | ||
| assertFalse(writtenZ.contains("ZEMPTY")); | ||
| reader.read(writtenZ); |
There was a problem hiding this comment.
Exact #1223 repro for Z (and the M / ZM siblings below), plus a WKTReader round-trip. Enough to lock the reported bug.
contains + assertFalse(...ZEMPTY) is fine. An exact-string assert on the collection would also lock that the non-empty sibling still writes LINESTRING Z(...) with no space before (.
Optional, non-blocking: one top-level POINT Z EMPTY (and maybe POLYGON Z EMPTY) would pin appendSequenceText / appendPolygonText without changing the design. Nested empty LINESTRING / POLYGON take the same helper.
There was a problem hiding this comment.
Dimensional EMPTY belongs here, not in WKTReadWriteTest (that file already has XY empty round-trips). Leaving Z/M/ZM EMPTY in WKTWriterTest is the right place.
3 participants
Summary
WKTWriterso dimensional empty geometries emit separateZ/M/ZMandEMPTYtokens (e.g.MULTILINESTRING Z EMPTY) instead of invalid concatenated tokens likeZEMPTY,MEMPTY, orZMEMPTY.Fixes #1223
Test plan
mvn -pl modules/core -Dtest=WKTWriterTest test— 15 tests, 0 failuresmvn -pl modules/core -Dtest=WKTReadWriteTest,WKTWriterTest,WKTWriterStaticFnTest test— 32 tests, 0 failuresLabels
Contributor notes
19286898+arimu1@users.noreply.github.com-sMade with Cursor