go-geom icon indicating copy to clipboard operation
go-geom copied to clipboard

geojsonFeature does not support number ids

Open wichert opened this issue 3 years ago • 2 comments

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

wichert avatar Oct 19 '21 12:10 wichert

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?

twpayne avatar Oct 20 '21 20:10 twpayne

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.

wichert avatar Oct 20 '21 21:10 wichert

@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.

DmitriyVTitov avatar Jan 24 '23 10:01 DmitriyVTitov

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 avatar Jan 24 '23 10:01 twpayne

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

twpayne avatar Jan 24 '23 10:01 twpayne

@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?

DmitriyVTitov avatar Jan 24 '23 11:01 DmitriyVTitov

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

twpayne avatar Jan 24 '23 13:01 twpayne

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

DmitriyVTitov avatar Jan 24 '23 15:01 DmitriyVTitov

Fixed by @DmitriyVTitov in #225.

twpayne avatar Jan 24 '23 22:01 twpayne