Skip to content
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

support geojson (de-)serialization #17

Merged
merged 2 commits into from
Jul 9, 2023

Conversation

Plunts
Copy link
Contributor

@Plunts Plunts commented Jun 26, 2023

See issue #16

Open points:

  • Overall a code review should be done, as this is my first time using rust "in real life".
  • Shall this be a new feature (as proposed) or shall I replace the built-in serde (de-)serialization entirly?
  • A lot of the code is only necessary, because all types are available "stand-alone" and inside a GeometryContainer. Would it be possible to make this more compact?

@Plunts Plunts force-pushed the 16-use-geojson branch 2 times, most recently from 61588b9 to 26d89f1 Compare June 27, 2023 08:36
@vitaly-m
Copy link
Owner

Thank you for pull request. I will be able to analyze it not earlier than the beginning of July. My first thought to implement JSON serialization was to do GeoJSON support, but it required a lot of code, GeoJSON has some limitation and I decided to do custom serialization, because in my practice you ether work with custom data structure by some reason or with something like tiles. But more capabilities usually better than less :)

@vitaly-m
Copy link
Owner

vitaly-m commented Jul 9, 2023

If understand correctly the GeoJSON is applicable only in case the coordinate system is WGS 84 https://datatracker.ietf.org/doc/html/rfc7946#section-4, https://epsg.io/4326. I think that it is better to add that restriction for serialization and create geometry objects with srid=4326 in deserialization process. Can you add this change?

@Plunts
Copy link
Contributor Author

Plunts commented Jul 9, 2023

The current implementation already works only with Point and PointZ. The latter is my interpretation of "An OPTIONAL third-position element SHALL be the height in meters above or below the WGS 84 reference ellipsoid." PointM and PointZM are therefore not (de-)serializable. Or do you want to add an srid check to the serialization and return an error?

@vitaly-m
Copy link
Owner

vitaly-m commented Jul 9, 2023

Yes, I want to add SRID check. I see two options:

  1. Add transformation from the given SRID to 4326 and do serialization after it. I don't think that this is preferred solution for that library.
  2. Return an error in case SRID is not equal to 4326, that at least will prevent from an incorrect data in GeoJSON, because according to RFC-7946 all coordinates should be treated as WSG 84 coordinates. That is preferred solution currently.

And do the same in deserialization process, so that objects will have correct SRID, because GeoJSON explicitly defines it.

@Plunts
Copy link
Contributor Author

Plunts commented Jul 9, 2023

Yes, adding transformations to the library sounds like a bad idea. I already added it to the deserialization, but to avoid another review-round: how do you want to handle SIRD = None? I'd again say to ignore it and still serialize, so only when there is an explicit different SIRD it'll return an error.

@vitaly-m
Copy link
Owner

vitaly-m commented Jul 9, 2023

Looks like none can be treated as 4326 https://postgis.net/docs/using_postgis_dbmanagement.html, and should lead to a serialization without errors.

the SRID modifier restricts the spatial reference system SRID to a particular number. If omitted, the SRID defaults to 4326 (WGS84 geodetic), and all calculations are performed using WGS84.

I think it should work fine:

  1. Serialization:
    1.1. SRID=None -> OK
    1.2. SRID=4326 -> OK
    1.3. SRID!=4326 -> Error
  2. Deserialization:
    2.1. SRID become equal to 4326, my understanding is that even it is not set in PostGIS it should save data without any error

@Plunts
Copy link
Contributor Author

Plunts commented Jul 9, 2023

That makes sense and explains why I never had to bother about the srid :) Pushed a new version, also containing two tests for the newly defined behaivour.

@vitaly-m vitaly-m merged commit a970a45 into vitaly-m:master Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants