search

twpayne / go-geom

Package geom implements efficient geometry types for geospatial applications.
BSD 2-Clause "Simplified" License
839 stars 104 forks source link

geojsonFeature does not support number ids #205

Closed wichert closed 1 year ago

wichert commented 2 years ago

According to RFC 7946 a feature can have a numeric identifier:

If a Feature has a commonly used identifier, that identifier SHOULD be included as a member of the Feature object with the name "id", and the value of this member is either a JSON string or number.

When I try to load a file that uses numeric identifiers this currently results in an error, because geojsonFeature only accepts a string:

https://github.com/twpayne/go-geom/blob/25827fa27637655126807e5b5466b28bbeff7fc9/encoding/geojson/geojson.go#L58-L64

twpayne commented 2 years ago

Thank you very much for reporting this. I'd missed this in the GeoJSON spec.

I think that the only way to fix this without breaking backwards compatibility is to create a second go-geom/encoding/geojsonint package that uses ints for IDs instead of strings. Another option is to wait for Go generics and make the ID field a parameterized type, which would necessitate a major version bump in the library.

What are your thoughts on how to fix this?

wichert commented 2 years ago

I'm not sure a geojsonint package is the solution here: that would require knowing in advance if a file will have int or string ids, which may not be true. A single file could also theoretically use both ints and strings, making life even more difficult.

For our case (users uploading arbitrary GeoJSON files) I ended up implementing a streaming decoder that does not need to read the entire file in memory, and using interface{} for the id field.

DmitriyVTitov commented 1 year ago

@twpayne Any updates on this issue? I've also encountered this bug and it blocks my work. I think ID as interface{} would be the best solution.

twpayne commented 1 year ago

Making ID an interface{} would break backwards compatibility.

I'm not personally affected by this bug, but would accept a high quality PR that fixes the issue without breaking backwards compatibility for existing users.

twpayne commented 1 year ago

Note also that since numbers in JSON are all float64s, ids can also befloat64s.

DmitriyVTitov commented 1 year ago

@twpayne I've made PR but cannot push it: access denied. But I didn't manage to save type information with backwards compatibility. Since if we keep Feature.ID as string, we would loose original type info after Unmasrhal/Marshal. Anyways can you give me permissions to pull request?

twpayne commented 1 year ago

You need to create the pull request from your own fork. See how to create a pull request in the GitHub documentation.

DmitriyVTitov commented 1 year ago

https://github.com/twpayne/go-geom/pull/225

twpayne commented 1 year ago

Fixed by @DmitriyVTitov in #225.