Support for LineStringZM

[kanatohodets]

This PR adds support for LineStringZM, a Line String Z with a 4th value for
arbitrary metadata, like timestamps of measurements.

If this looks good to you, I'm happy to send more PRs adding 'M' variants of
other geo types (individually or batched, whatever your preference).

Kudos for an easy codebase to work with! I found it super straightforward to
implement this, even as a newcomer to GIS.

[kanatohodets]

Hey @bryanjos - I realized I hadn't updated the README, so that's been fixed.

Does it make sense to you for me to go ahead and add support for the other "ZM" variants which are not currently supported by geo?

No sweat if you don't have time to review or merge anything for a while, just wanted to make sure I'm not digging too much of a hole.

[bryanjos]

@kanatohodets I think what you have here is good. But there is a conflict here with the PR I just merged in. I also think that PR tightened up the JSON parsing to be more GeoJSON compliant and maybe the ZM variants don't work for GeoJSON. Other than that, everything else looks good!

[kanatohodets]

Well! Things got a bit interesting as I dug into the merge conflict.

TL;DR: I think I found an old bug with 'Z' things and GeoJSON decoding. I'm
happy to work on solutions, but it likely means some breaking changes.

1. The conflict was in the tests, and was really just a positional conflict (I shuffled around a neighboring test for the new one; the spec compliance PR renamed it).

2. Interestingly, when I untangled the conflict and left my test in place, both tests passed! This was weird, because the PR should have exactly broken my test of LineStringZM GeoJSON round-tripping.

3. It turns out that my test was passing because only for LineStringZ
   list_to_tuple/1 (with the "drop 3rd position or beyond" behavior) is not
   called on GeoJSON decode. Instead, it uses List.to_tuple. I copied this when
   I made LineStringZM. This dates from f49c6f1.

As far as I understand the GeoJSON spec, it does not allow LineStringZ (or any
other 'Z' types) in the "type" field. So, I think f49c6f was in error. The same
goes for MultiPolygonZ, introduced in 0b49b0b.

PointZ, however, is correct - it does not parse "PointZ".

I think this means that geo has been accidentally supporting "type" names of
LineStringZ and MultiPolygonZ when decoding GeoJSON. However, it was
a misleading situation, as it has discarded the Z bit since 2019
(9ebef60).

I would suggest the following:

1. Remove support for any 'Z' (or M) type names from the GeoJSON decoder. Error
   if such a type is given in the GeoJSON, as PostGIS does. This is a breaking
   change, but support for these types was already not working (as the 3rd
   position was stripped).

2. When GeoJSON types such as LineString are given with the 3rd position
   present in coordinates, assume it is Z, as the spec indicates. Parse it into
   the correct 'Z' type. This means that a LineString GeoJSON could be decoded
   into either Geo.LineString or Geo.LineStringZ, depending on the data.

3. Continue to strip out 4th or greater positions on GeoJSON decode, such as
   'M'. Decode 4+ member coordinates into 3 member 'Z' types.

4. (more of a discussion topic, as I'm really new to this space) - maybe
   there's appetite out there for "relaxed GeoJSON" which supports a wider range
   of types? could imagine this as an alternative function, with a config setting
   for the default behavior (defaulting that setting to "strict").

Do any of these suggestions resonate with you? I think doing 1-3 will bring
decoding into compliance with the spec, and also behave similarly to PostGIS.
4 is probably dubious, fomenting minor rebellion against the spec. That said,
it could provide a path for folks who have been relying on geo to parse
GeoJSON with the 'Z' type names.

I'm happy to pick up this work (or something else entirely!) in whatever order
you think is reasonable.

Examples from my reference parser, PostGIS 3.2:

ST_GeomFromGeoJSON throws an error when given 'LineStringZ' as type:

  yolo_dev=# select ST_AsText(ST_GeomFromGeoJSON('{"type": "LineStringZ",
  "coordinates":[[1, 2, 3], [4, 5, 6]]}'));
  ERROR:  invalid GeoJson representation

The same function decodes into a LineStringZ when the GeoJSON coords have
3 members:

yolo_dev=# select ST_AsText(ST_GeomFromGeoJSON('{"type": "LineString", "coordinates": [[1, 2, 3], [4, 5, 6]]}'));
         st_astext
----------------------------
 LINESTRING Z (1 2 3,4 5 6)

And when GeoJSON is provided with the 'M' member, it is ignored and decoded as
'LineStringZ':

yolo_dev=# select ST_AsText(ST_GeomFromGeoJSON('{"type": "LineString", "coordinates":[[1, 2, 3, 4], [5, 6, 7, 8]]}'));
         st_astext
----------------------------
 LINESTRING Z (1 2 3,5 6 7)

[bryanjos]

@kanatohodets thanks for the research you put in here. I am aligned in your thinking with points 1-3. And for point 4, I would just stick to the spec. There's been enough discussions and issues around it that I think it's best to stick to the spec.

[bryanjos]

@kanatohodets And I think geo will need a breaking change for some unrelated things so with what you have set forth, I think you can add some deprecations until that change is ready.

[s3cur3]

Thanks so much for your work on this, @kanatohodets! I learned a lot reading through this discussion, and the code and tests look great.

I'm gonna merge this with the tests fixed to match the existing behavior of dropping the M coordinate on GeoJSON encode, since it broadly aligns with the way the rest of the Z and M handling works already, but I've filed #188 to follow up on the GeoJSON compatibility stuff. I think I'm with you all of points 1-4—in particular, my preference would be to continue accepting "type": "LineStringZ" for backward compatibility. Even though it's out of spec, it seems easy enough to avoid breaking in case somebody's relying on that behavior, and I don't think anyone's using Geo's parser as a check for whether a file is spec-compliant.

[s3cur3]

I think this is a fine compromise for compatibility. I wonder, though, whether it'll be surprising to folks that the m value totally disappears. If people complain, we may want to do something like include the m in the GeoJSON feature's properties. That seems like a hack, though, so not something I'd do proactively.

[s3cur3]

The link is very much appreciated!