-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add PostGIS GeoJSON support #1564
Conversation
47dea30
to
2d58fb3
Compare
Resourcing embedding is working fine, the embedded resource gets included inside the "properties" key. Check this test to confirm: 237f4a0#diff-916d27c5920ad2514d327ee39a639b47R56-R71 |
imo the output response shape here is ugly, it might be an RFC for geo data types but the rest of the columns in a row are not properties of the geo object, it's the other way around.
st_asgeojson only to the geo columnthe url would look like /shops?select=id,shop_geom::GEOASJSON , no need for special headers, response shape stays the same, easy code change.In case, in the future postgrest start detecting column types (thus not needing the cast), it's easy to remove from frontend code (as opposed to handling the new response shape change) and also postgrest can silently ignore the special cast if need be. |
Maybe even my suggestion is not necessary since one can define CAST like this https://www.postgresql.org/docs/current/sql-createcast.html and have the geo/json conversion done transparently by the database |
@ruslantalpa The problem with the column casting approach is that it would only give you the Geometry object(the If we don't comply with geojson feature objects, then clients would not be able to easily use ready-made map libs like leaflet, mapbox gl, google maps. And that is really the main use case for geojson. For example, the |
Selecting the geometry column in case of multiple
Multiple geometry columns outputNot a common use case, I believe. Users can have a single geometry column with collection types. That being said, we could work this out in PostgREST. If a table has more than one geometry column, we can join them in a single feature by using a This can be a future enhancement though. |
Something I've noticed. In the latest postgis version, getting the geojson geometry object is done automatically for Though of course, we still need the geojson feature object. |
in regard to geo columns and json_agg, are you saying in the new postgis casting is done automatically (geo->json)? that is cool, does it work the other way around when inserting (json->geo)? (if that is the case, 2 test proving that in this pr seem ideal) if i am understanding this correctly, you can use the custom header and get a format compatible with geo libs or you can just omit the header and get a normal output and the geo columns are transparently casted to geojson (this would be the ideal/flexible feature set) |
Yeah, that works good! I've added a test that confirms it: 1fd8a0a
That does not work with the geojson object unfortunately. Though using the literal representation works, as in this test: c3d7ca6#diff-916d27c5920ad2514d327ee39a639b47R89. Haven't tried a CAST for this case.
Yes, that's pretty much it. |
Could you go more into detail why this doesn't work? This would be really nice to have 🙂. |
@LorenzHenk Sure. It's just that casting from json to geometry doesn't work now, I assume PostGIS doesn't have a CAST defined in this case: select '{"type":"Point","coordinates":[1,1]}'::geometry
-- error: invalid geometry.. So POSTing a json to a geometry column through PostgREST won't work too. However casting the literal representation does work: select 'SRID=4326;POINT(1 1)'::geometry
-- 0101000020E6100000000000000000F03F000000000000F03F And thus POSTing that will work: http POST localhost:3000/tbl << JSON
{"geocol": "SRID=4326;POINT(1 1)"}
JSON (related: PostgREST/postgrest-docs#273)
Really this PR was ready, I didn't merge it because I wanted to keep releases more focused at the time, but that ship has sailed(many features on the pipeline). So I'll rebase and merge this one after #1760. |
Does PostgREST automatically try to cast? Could we just create a cast on our own (e.g. by following this guide) from json to geometry and PostgREST would use that one automatically? |
That one doesn't seem to work unfortunately. Creating the cast does work: create or replace function json_to_geometry(json) returns geometry as $$
SELECT ST_AsText(ST_GeomFromGeoJSON($1))::geometry;
$$ language sql;
create cast (json as geometry) with function json_to_geometry(json) as implicit;
select ('{"type":"Point","coordinates":[1,1]}'::json)::geometry
-- 01010000009279E40F061E48C0F2B0506B9A1F3440 But then PostgREST uses create table entities(
id int,
geocol geometry
);
SELECT "id", "geocol" FROM json_populate_record (null:: "public"."entities" , '{"id": 12, "geocol": {"type":"Point","coordinates":[1,1]}}') _
-- parse error - invalid geometry I thought |
I just checked the docs, here are the notes:
In your case it'd hit the last point, meaning the JSON is taken as string and tried to cast to geometry. I don't know if it's possible to overwrite the input conversion function, or rather if this is something we should aim for. |
Isn't this just a cast text -> geometry that is needed instead of json -> geometry? Not 100% sure about "input conversion function" == "cast text -> ...", though. |
That won't work, because it's already used for the CAST I put above. Also: create cast (text as geometry) with function text_to_geometry(text) as implicit;
-- error: cast from type text to type geometry already exists |
From the discussion here, we can conclude that's not possible to use a CAST for converting the payload to geojson. Another option could be to do the transformation ourselves. When a I think we can continue the "writing" part on another issue, the "reading" part here should be good to merge. It just needs a rebase. |
If we were to implement #1582 (comment) and #1582 (comment), this should be fairly easy with
Assuming we implement a good |
Yeah, but the thing is we don't know how good the interface would be, and meanwhile this PR is already useful - geojson is a common use case. Besides that, if geojson output would be in core, I think we could later allow override it through the Edit: it'd be nice to show we're entering the postgis space in this release, there are some excting postgis-related features (postgis filters, mvt output, etc) I have in mind for later. |
Agreed!
Yes, I think so, too.
Cool! Let's do it! |
We cannot handle postgis and other postgresql dependencies with stack. Also delete test/with_tmp_db since it's no longer used.
Works for GET, POST, PATCH, PUT, DELETE, RPC.
First step towards solving the long wanted #223!
This feature won't be included in the upcoming release. But I wanted to do a draft and kickstart discussion.
Implementation
PostGIS 3.0 added the ST_AsGeoJSON(record .. function(not available in a prior version). Now
ST_AsGeoJSON
just works:It even includes all the non-geometry columns in
properties
. This means that we don't have to detect the geometry column, ST_AsGeoJSON does this internally.A table that doesn't have a
geometry
column will err accordingly:We take advantage of these for the implementation:
TODO
application/vnd.geo+json
media type? We'll see if there's demand for this.bbox
andid
- on the feature. In the case ofid
, I think it'll be fine to keep it inside theproperties
attribute and use the standard ST_AsGeoJson. Forbbox
, not sure. I think we can omit that for now.return=representation
).properties
key.References