search

mapbox / mapbox-java

The Mapbox Java SDK – Java wrappers around Mapbox APIs and other location data
https://docs.mapbox.com/android/java/overview/
MIT License
414 stars 121 forks source link

roundtrip unrecognized JSON properties #1344

Closed LukasPaczos closed 2 years ago

LukasPaczos commented 2 years ago

Refs https://github.com/mapbox/mapbox-directions-swift/issues/637.

We should find a way to keep and serialize back any, at the time of serializing, unrecognized properties to support forward compatibility.

cc @1ec5 @SiarheiFedartsou @Guardiola31337

1ec5 commented 2 years ago

In principle, persisting and roundtripping unrecognized properties would be beneficial to any models that correspond to server API responses. It’s particularly needed for the Directions and Map Matching APIs, which may return undocumented response properties that Navigation Native may want to consume. However, multiple APIs routinely introduce beta request options that result in beta response properties.

Similarly, GeoJSON has a provision for foreign members, acknowledging that GeoJSON authoring tools often extend GeoJSON objects with arbitrary properties. Sometimes foreign members are vendor-specific, while sometimes they’re defined by separate standards like GeoJSON-T and TopoJSON. mapbox/turf-swift#175 added Swift support for round-tripping foreign members (but not interpreting them); it would be nice to support them consistently across languages.

The Geocoding API represents most of a result feature’s metadata as foreign members in a GeoJSON-compliant response format, so foreign member support could allow us to remove custom Geocoding API response handling code.

LukasPaczos commented 2 years ago

Tagging @tobrun who is also looking into this.

VysotskiVadim commented 2 years ago

We can add support of unrecognised properties to the generated type adapters. The type adapters we use are generated by auto-value-gson library. What if we fork it and add support of unrecognised json properties. It shouldn't be too hard to generate handling of unrecognised properties in our type adapters.

Discussion with library's authors: https://github.com/rharter/auto-value-gson/issues/266. Spoiler: they didn't like the idea.

VysotskiVadim commented 2 years ago

PR: https://github.com/mapbox/mapbox-java/pull/1394

VysotskiVadim commented 2 years ago

Waiting for the official fork to be created + need to reconsider with team where to store binary artefacts from the fork.

1ec5 commented 2 years ago

The performance impact mentioned in https://github.com/rharter/auto-value-gson/issues/266#issuecomment-1044672157 is a valid concern. This is one reason why many GeoJSON libraries opt not to support foreign members. We’re seeing a performance hit adding support for foreign members in Directions API responses containing GeoJSON-formatted geometries: https://github.com/mapbox/mapbox-directions-swift/pull/669#discussion_r842877486. Since we don’t expect these GeoJSON geometries to contain any meaningful foreign members for the foreseeable future, we’re considering selectively processing foreign members in the overall Directions API response but not in the geometry properties that could be more complex: mapbox/turf-swift#186.

VysotskiVadim commented 2 years ago

I will try to measure performance impact of new changes then

VysotskiVadim commented 2 years ago

Performance measurements: https://github.com/mapbox/mapbox-java/pull/1394#issuecomment-1091908546

VysotskiVadim commented 2 years ago

Landed in https://github.com/mapbox/mapbox-java/pull/1394