Geometry array being sent to server

[mstenta]

Noticed a bug today: it appears that when "Add my GPS location to the log" is clicked multiple times, it creates an array of geometries that are sent to the log. It should instead be sending a single geometry collection that contains all the points.

I feel like something must have changed recently that caused this, because I'm pretty sure I was testing multiple points in the past and they went through as a single geometry. But I could be wrong. Anything come to mind @jgaehring ?

[jgaehring]

Probably related to the fix to #272, specifically https://github.com/farmOS/farmOS-client/pull/280#issuecomment-569848850 and 3f8b77d. Although looking at the diffs briefly, it appears it was already storing them in an array prior to those changes.

It should instead be sending a single geometry collection that contains all the points.

This should be possible, in fact, I believe we already have the capability available in the mergeGeometries utility:

https://github.com/farmOS/farmOS-client/blob/425bae074da511268dd6684e4839d1b3ca2306f7/src/utils/geometry.js#L15-L63

However, I'm a little curious: why "should" it be a single string of of WKT instead of an array (if I'm understanding you correctly), and why have you labelled this a bug? The two formats seem more or less equivalent, and at least in Field Kit, the array format does not produce any unexpected behavior that I can see. Does this produce a bug in farmOS?

[mstenta]

Great! Let's do that.

Maybe labeling as a Field Kit bug isn't accurate. It is a bug that farmOS accepts multiple geometries like that. We do not allow multiple geometries to be created in the farmOS UI although technically it is possible to have multiple. Right now there is no need for that, nor any understanding of what that would mean from a data standpoint. So for consistency with the way the server works we need to "fix" this in Field Kit.

[mstenta]

Adding this to the v1.0.0 milestone as well.

[jgaehring]

I'm looking at the schema the server sends, which is in fact an array of geometries:

"geofield": {
  "label": "Geometry",
  "type": "geofield",
  "required": 0,
  "default_value": [],
  "data_schema": [
    {
      "geom": ""
    }
  ]
},

[jgaehring]

@mstenta, can you specify exactly what format the server needs? When you say,

It should instead be sending a single geometry collection that contains all the points

Do you mean the value of geofield should be a single string? Or an object { "geom": "POINT(...)" }, or an single object in an array? It's very unclear from the schema.

[mstenta]

Sorry, you're right, and I agree, it is unclear. This is a case where the server is not properly representing "the model" in my mind. It's a legacy thing, and will be fixed in 2.x... but basically:

What the server needs right now is an array with a single object in it.

The fact that it needs "an array with a single value" is where the server is wrong, IMO. Multiple objects are technically allowed, but they shouldn't be. Ideally it would not need an array, and would need a single object. The plan is to change that in 2.x, FWIW.

So... back to the original description of this issue: if the "Add my GPS location to the log" button is clicked multiple times, it should combine all those POINT()s into a GEOMETRYCOLLECTION(), and send it to the server like:

"geofield": [{ "geom": "GEOMETRYCOLLECTION(...)" }]

Hope that clarifies!

[jgaehring]

Awesome, thanks @mstenta, that definitely clarifies.

The fact that it needs "an array with a single value" is where the server is wrong, IMO.

Yea, I'm with you there, definitely seems flawed. But glad it will be resolved in 2.x!

[jgaehring]

Ugh, actually, can we just make everything GeoJSON in 2.x?

Just realizing what an added PITA this is now that I have to parse WKT to display and remove the individual GPS points now. :frowning_face:

[mstenta]

Yes I agree GeoJSON support will be a good thing!

For now, if this is the only case... you don't really need a whole parser to do what's necessary here. Assembling a GEOMETRYCOLLECTION() of POINT()s can be done with some good old fashioned string concatenation. :-)

[mstenta]

Here's what one looks like with 3 points in it:

GEOMETRYCOLLECTION(POINT(-95.61715246018137 42.79775953977622),POINT(-95.60723901566233 42.79760209252652),POINT(-95.59887052353587 42.79753911351443))

Note that it's POINT(lon lat), not POINT(lat lon). I've made that mistake before. :-)

[jgaehring]

I think the biggest issue is allowing one point to be deleted w/o deleting all of them. That requires them to be selected individually, which will require parsing.

Of course we really should have a full map for that, but as of now we allow individual points to be deleted and I don't want to remove that feature if I can help it.

[mstenta]

Ah yea that makes sense.

One thing I'd love to talk about sometime is the idea of making some "shims" ahead of farmOS 2.x. So as things come together in 2.x, and we know what it's going to look like, maybe we can start adapting Field Kit and farmOS.js ahead of time. So eg: we assemble a log object using GeoJSON, but then convert it to WKT before we send it... or something like that. Not sure if that would be helpful, but maybe worth thinking through. It would be great if we could somehow encourage people to start using the farmOS 2.x API in farmOS 1.x (and perform that backwards compatibility translation for them).

[jgaehring]

Yea! I think in general a conversation about migration paths would be great, with "shims" being a part of that discussion.