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

Codable types should roundtrip unrecognized JSON properties #637

Closed
1ec5 opened this issue Dec 14, 2021 · 5 comments · Fixed by #669
Closed

Codable types should roundtrip unrecognized JSON properties #637

1ec5 opened this issue Dec 14, 2021 · 5 comments · Fixed by #669
Assignees
Labels
Core feature New feature request.

Comments

@1ec5
Copy link
Contributor

1ec5 commented Dec 14, 2021

Codable types defined by this package, such as RouteStep, should roundtrip from and to JSON in its entirety, including any unrecognized properties. This would ensure forward compatibility in the event that the Directions API includes content in a response that had not been publicly documented at the time of this package’s release.

When decoding from JSON, any unrecognized property in KeyedDecodingContainer.allKeys but not CodingKeys should be stored in a userInfo property of type JSONObject, which would get encoded back to JSON as the original properties. If the userInfo dictionary has a key that’s in CodingKeys, the Encodable implementation could ignore it in favor of the built-in support.

This would effectively be a lighterweight alternative to #60.

/ref mapbox/turf-swift#174
/cc @mapbox/navigation-ios @SiarheiFedartsou

@1ec5 1ec5 added Core feature New feature request. labels Dec 14, 2021
@1ec5 1ec5 changed the title Codable types should roundtrip foreign members Codable types should roundtrip unrecognized JSON properties Dec 14, 2021
@SiarheiFedartsou
Copy link

SiarheiFedartsou commented Dec 14, 2021

@mapbox/navigation-android while we discuss type-safe routes for NN public API, is it possible to do smth like this in https://github.com/mapbox/mapbox-java?

@LukasPaczos
Copy link

It might be possible but it's not a low-hanging fruit. By default, the library that we use to generate the types (https://github.com/google/auto/tree/master/value) does not support keeping unrecognized fields. We'd need to find a way to include this functionality in a custom serializer/deserialzier but it's not guaranteed that this would work either. It's something that would need more research. Tracking in mapbox/mapbox-java#1344.

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 14, 2021

any unrecognized property in KeyedDecodingContainer.allKeys but not CodingKeys

Unfortunately, KeyedDecodingContainer.allKeys only contains what’s in CodingKeys. The brute-force approach would be to create a redundant SingleValueDecodingContainer and decode the whole object as a JSONObject, then weed out the redundant keys. However, this blog post outlines a much more elegant and performant solution based on an extensible enumeration implementation of CodingKey.

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 14, 2021

mapbox/turf-swift#175 implements a generic approach to supporting foreign members in GeoJSON. In theory, Turf could expose this implementation to client code to use in arbitrary JSON round-tripping, but it’s rather out of scope for Turf, so maybe it’d be better for MapboxDirections to implement a redundant ForeignMemberContainer-like protocol for its own types.

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 20, 2021

Initially, we’ll want to only imbue response-related types with the arbitrary property round-tripping capability. Request-side types like RouteOptions are trickier because the output that really matters is the conversion to URL query items, but the exact format of URL query items isn’t defined by JSON or GeoJSON.

Since we haven’t eliminated abstract classes yet (#392), we need to carefully consider which class in the inheritance hierarchy is responsible for decoding and encoding the unrecognized properties. For example, if DirectionsResult were to implement this capability using the same approach as Turf’s ForeignMemberContainer, then any property defined by Route would get duplicated in the user info dictionary. Perhaps DirectionsResult should define an open property that defines the well-known coding keys, which subclasses can extend.

A couple of helper types engage in a kind of type punning, reinterpreting a container as a container of a smaller subset of coding keys, in order to modularize encoding and decoding. For example, RouteLeg.Attributes operates over the same concrete JSON object as RouteLeg. I think user info awareness can remain the responsibility of RouteLeg, but there will need to be a way to extend RouteLeg’s coding keys with those of RouteLeg.Attributes for the purpose of excluding well-known coding keys.

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

Successfully merging a pull request may close this issue.

4 participants