Allow promoting column as an id in ST_AsGeoJsonRow

[jtojnar]

As per GeoJSON RFC, the id should go directly to the feature object rather than to properties:

If a Feature has a commonly used identifier, that identifier SHOULD be included as a member of the Feature object with the name "id"

Let’s add an argument that will allow designating a column as an identifier so that people do not have to tediously build the JSON object manually.

References https://trac.osgeo.org/postgis/ticket/5596

[jtojnar]

I have not opened trac issue since I have not received mantra for osgeo account yet.

Tests pass (PGUSER=postgres make check) for me locally.

[strk]

I have not opened trac issue since I have not received mantra for osgeo account yet.

You should have received the mantra by now, could you file that ticket and reference it here in comments and in commit log please ?

The mail was sent to you on Sat, 28 Oct 2023 18:49:23 -0400 by Vicky Vergara. Check also in the spam folder.

[jtojnar]

I have opened https://trac.osgeo.org/postgis/ticket/5596

[robe2]

@pramsey have any issues with this pull request?

[robe2]

@strk at a glance, most of this is in order, @jtojnar did a good job of updating the documentation and adding tests. My only concern is I don't think our plumbing is set up to handle a change in args of function correct? So adding to drop the old function and create a new is required. I'm going to shove this thru one of our upgrade testers to see if it breaks. I suspect it will.

[robe2]

As I feared it fails the upgrade test - https://woodie.osgeo.org/repos/30/pipeline/1540/20

@jtojnar this will cause some upgrade burdens. There are two approaches to go with this

1. Don't remove the old signature, just add a new signature marking it as Availability: 3.5.0

approach 2) a) overwrite the old signature as you are doing now
b) Add a drop line to https://github.com/postgis/postgis/blob/master/postgis/postgis_before_upgrade.sql

The benefit of option one is I think it will cause less upgrade pain for folks who are using ST_AsGeoJSON row variant in their views and materialized views, but does cause some extra baggage for us to carry around

My vote is for option 2 even though it will cause more pain for folks.

@strk, @pramsey what's your thought on the matter

[robe2]

Needs to add a drop statement here - https://github.com/postgis/postgis/blob/master/postgis/postgis_before_upgrade.sql to drop the old signature or define a whole new function leaving the old signature intact.

[jtojnar]

but does cause some extra baggage for us to carry around

Would not the drop be extra baggage to carry around just as well?

[pramsey]

Either approach to the new SQL signature is fine with me

[jtojnar]

Don't remove the old signature, just add a new signature marking it as Availability: 3.5.0

Actually, it looks like just adding a new signature is what is currently happening. I tried the following:

CREATE OR REPLACE FUNCTION dup(int) RETURNS TABLE(f1 int, f2 text)
    AS $$ SELECT $1, CAST($1 AS text) || ' is text' $$
    LANGUAGE SQL;
CREATE OR REPLACE FUNCTION dup(int, int=4) RETURNS TABLE(f1 int, f2 text)
    AS $$ SELECT $1 + $2, CAST($1 AS text) || ' is text' $$
    LANGUAGE SQL;
select dup(1);

And got the same error as in the upgrade check:

ERROR: function dup(integer) is not unique

---

Pushed the suggested removal.

[robe2]

Don't remove the old signature, just add a new signature marking it as Availability: 3.5.0

Actually, it looks like just adding a new signature is what is currently happening. I tried the following:

CREATE OR REPLACE FUNCTION dup(int) RETURNS TABLE(f1 int, f2 text)
    AS $$ SELECT $1, CAST($1 AS text) || ' is text' $$
    LANGUAGE SQL;
CREATE OR REPLACE FUNCTION dup(int, int=4) RETURNS TABLE(f1 int, f2 text)
    AS $$ SELECT $1 + $2, CAST($1 AS text) || ' is text' $$
    LANGUAGE SQL;
select dup(1);

And got the same error as in the upgrade check:

ERROR: function dup(integer) is not unique

Pushed the suggested removal.

I forgot to mention for the new signature, you can't have it have a default. That default needs to be removed otherwise you do get a duplicate.

[robe2]

but does cause some extra baggage for us to carry around

Would not the drop be extra baggage to carry around just as well?

A little, but not as much because it doesn't bloat the function base. We still end up with the same number of functions in the end, which as a user, is all I care about. From a developer standpoint I prefer it more, cause I'm always staring at the function definitions sql.in files, and I'm not staring so much at the drop sql file. :)

[robe2]

I see you went with the drop approach. At a glance all looks in order to me. I'll do one final upgrade check and accept unless @strk has any opinions

[robe2]

still failing with not unique during upgrade https://woodie.osgeo.org/repos/30/pipeline/1540/20

The postgis_before_drop should have taken care of this. I'm investigating now.

regress/core/out_geojson .. failed (diff expected obtained: /tmp/pgis_reg/test_114_diff)
-----------------------------------------------------------------------------
--- /woodpecker/src/git.osgeo.org/gitea/postgis/postgis/regress/core/out_geojson_expected	2023-11-27 18:07:13.029181299 +0000
+++ /tmp/pgis_reg/test_114_out	2023-11-27 19:02:38.189018095 +0000
@@ -6,13 +6,7 @@
 gj01|5|{"type":"Point","coordinates":[]}
 gj01|6|
 gj01|7|{"type":"GeometryCollection","geometries":[{"type":"Point","coordinates":[]},{"type":"Point","coordinates":[1,2]}]}
-gj02|1|{"type": "Feature", "geometry": {"type":"Point","coordinates":[42,42]}, "properties": {"i": 1, "f": 1.1, "t": "one", "d": "2001-01-01"}}
-gj02|2|{"type": "Feature", "geometry": {"type":"LineString","coordinates":[[42,42],[45,45]]}, "properties": {"i": 2, "f": 2.2, "t": "two", "d": "2002-02-02"}}
-gj02|3|{"type": "Feature", "geometry": {"type":"Polygon","coordinates":[[[42,42],[45,45],[45,42],[42,42]]]}, "properties": {"i": 3, "f": 3.3, "t": "three", "d": "2003-03-03"}}
-gj02|4|{"type": "Feature", "geometry": {"type":"GeometryCollection","geometries":[{"type":"Point","coordinates":[42,42]}]}, "properties": {"i": 4, "f": 4.4, "t": "four", "d": "2004-04-04"}}
-gj02|5|{"type": "Feature", "geometry": {"type":"Point","coordinates":[]}, "properties": {"i": 5, "f": 5.5, "t": "five", "d": "2005-05-05"}}
-gj02|6|{"type": "Feature", "geometry": {"type": null}, "properties": {"i": 6, "f": 6.6, "t": "six", "d": "2006-06-06"}}
-gj02|7|{"type": "Feature", "geometry": {"type":"GeometryCollection","geometries":[{"type":"Point","coordinates":[]},{"type":"Point","coordinates":[1,2]}]}, "properties": {"i": 7, "f": 7.7, "t": "seven", "d": "2007-07-07"}}
+ERROR:  function st_asgeojson(g) is not unique at character 19
 gj03|1|{"g":{"type":"Point","coordinates":[42,42]},"i":1,"f":1.1,"t":"one","d":"2001-01-01"}
 gj03|2|{"g":{"type":"LineString","coordinates":[[42,42],[45,45]]},"i":2,"f":2.2,"t":"two","d":"2002-02-02"}
 gj03|3|{"g":{"type":"Polygon","coordinates":[[[42,42],[45,45],[45,42],[42,42]]]},"i":3,"f":3.3,"t":"three","d":"2003-03-03"}
@@ -34,4 +28,4 @@
 gj05|5|{"type": "Feature", "geometry": {"type":"Point","coordinates":[]}, "id": 5, "properties": {"f": 5.5, "t": "five", "d": "2005-05-05"}}
 gj05|6|{"type": "Feature", "geometry": {"type": null}, "id": 6, "properties": {"f": 6.6, "t": "six", "d": "2006-06-06"}}
 gj05|7|{"type": "Feature", "geometry": {"type":"GeometryCollection","geometries":[{"type":"Point","coordinates":[]},{"type":"Point","coordinates":[1,2]}]}, "id": 7, "properties": {"f": 7.7, "t": "seven", "d": "2007-07-07"}}
-4695|{"type": "Feature", "geometry": {"type":"Point","crs":{"type":"name","properties":{"name":"EPSG:2227"}},"coordinates":[0,1]}, "properties": {"v": 1}}
+ERROR:  function st_asgeojson(record) is not unique at character 16
-----------------------------------------------------------------------------

[robe2]

There is still an issue with our dump restore tests on 3.0, but I'm let this go in since I think the issue might be on our end.

I also made some minor adjustments, changing bool to boolean in the postgis.sql.in (I know it was like that when you saw it)., but figure we should make it match with the drop. I also need to do the same for the other functions we have in postgis.sql.in that use the short-hand bool.

Added the NEWS item.

[strk]

For the record: by adding a new parameter we actually created a NEW SIGNATURE (rather than changing the identity of an existing signature). So we'd have needed a different function (drop by signature) BUT the simplest way to deal with upgrades is the use a Replaces comment, like it was done to fix the bug which resulted by this misues of _postgis_drop_function_by_identity, see

https://trac.osgeo.org/postgis/ticket/5623#comment:5

https://git.osgeo.org/gitea/postgis/postgis/commit/a6f4fa8282d3fef7e298f66cb7db96674486f028