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

roundtrip unrecognized JSON properties #1344

Closed
LukasPaczos opened this issue Dec 14, 2021 · 9 comments
Closed

roundtrip unrecognized JSON properties #1344

LukasPaczos opened this issue Dec 14, 2021 · 9 comments
Assignees

Comments

@LukasPaczos
Copy link
Contributor

Refs mapbox/mapbox-directions-swift#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
Copy link
Contributor

1ec5 commented Dec 21, 2021

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
Copy link
Contributor Author

Tagging @tobrun who is also looking into this.

@VysotskiVadim
Copy link
Contributor

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: rharter/auto-value-gson#266. Spoiler: they didn't like the idea.

@VysotskiVadim
Copy link
Contributor

PR: #1394

@VysotskiVadim
Copy link
Contributor

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

@1ec5
Copy link
Contributor

1ec5 commented Apr 6, 2022

The performance impact mentioned in rharter/auto-value-gson#266 (comment) 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: mapbox/mapbox-directions-swift#669 (comment). 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
Copy link
Contributor

I will try to measure performance impact of new changes then

@VysotskiVadim
Copy link
Contributor

Performance measurements: #1394 (comment)

@VysotskiVadim
Copy link
Contributor

Landed in #1394

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants