Roundtrip of unrecognised JSON properties #1394

VysotskiVadim · 2022-04-04T09:21:22Z

Goal

Implementation of #1344
Models from mapbox-java should not erase unrecognised json properties during fromJson -> toJson

example

If we have a model with a single property a

public abstract Model {
    public abstract String a();
}

and a JSON with properties a and b

{"a":"value", "b":"value"}

the model should let unrecognised fields to do a roundtrip

String json = {"a":"value", "b":"value"}
assertEquals(json, Model.fromJson(json).toJson());

Implementation

I forked the library which generates gson type adapters for us. Now if there are some unrecognised fields a type adapter put them into unrecognised fields map.
I will share a link when fork will be publicly available.

VysotskiVadim · 2022-04-04T09:27:20Z

...ections-models/src/test/java/com/mapbox/api/directions/v5/models/DirectionsResponseTest.java

-    DirectionsResponse responseFromJson1 = DirectionsResponse.fromJson(json);
+  public void testToFromJsonWithRealResponse() throws Exception {
+    Gson gson = new GsonBuilder().create();
+    String originalJson = loadJsonFixture(DIRECTIONS_V5_PRECISION6_FIXTURE_ARTIFICIAL_FIELDS);


Added DIRECTIONS_V5_PRECISION6_FIXTURE_ARTIFICIAL_FIELDS just because DirectionsResponse always adds routeIndex and requestUuid.

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/Bearing.java

VysotskiVadim · 2022-04-07T15:55:25Z

Did rough testing of performance. No microbenchmarks was involved, but I guess we can less or more understand performance impact.

Test 1

Deserialised and serialised directions repose 1000 times:

   for (int i = 0; i < 1000; i++) {
      DirectionsResponse response = DirectionsResponse.fromJson(regularJsonString);
      String json = response.toJson();
    }

2210 ms

Test 2

Deserialised and serialised mutated directions repose 1000 times:

   for (int i = 0; i < 1000; i++) {
      DirectionsResponse response = DirectionsResponse.fromJson(mutatedJsonString);
      String json = response.toJson();
    }

3075 ms
Mutated json is a directions response where to each property was added a few unrecognised fields. See https://github.com/mapbox/mapbox-java/pull/1394/files#diff-2c855af9309578033b9a51b9306ce977b4241a7b7197984bb16b83f26a49fdfbR12

Test 3

Tested mutated json serialisation and deserialisation 1000 times on main branch:

   for (int i = 0; i < 1000; i++) {
      DirectionsResponse response = DirectionsResponse.fromJson(mutatedJson);
      String back = response.toJson();
    }

2333 ms

...es-directions-models/src/main/java/com/mapbox/api/directions/v5/models/BannerComponents.java

RingerJK · 2022-04-12T09:57:58Z

...es-directions-models/src/main/java/com/mapbox/api/directions/v5/models/BannerComponents.java

@@ -501,5 +502,8 @@ public abstract static class Builder {
     * @since 3.0.0
     */
    public abstract BannerComponents build();
+
+    @Nullable
+    abstract Builder unrecognised(Map<String, Object> value);


we know that unrecognized fields are Strings, this way, let's make Map<String, String>

it's not only strings. It can also be JsonObject, JsonArray, etc.

why do you want to operate with these fields as JsonObject or JsonArrays? Looks like we only need to serialize/deserialize them. So no need to keep Objects

I was actually thinking whether we could consider making this map public (no in builder but in the resulting type), so that if someone would like to access the unrecognized fields, they don't need to serialize the type first. I believe this will be needed soon (for example for experimental EV properties).

@LukasPaczos Experimental EV and other options are allocated in RouteOptions. We don't extend RouteOptions with Unrecognized fields I think 🤔 Suppose, it's separate initiative, not really connected with this PR

Yea, as discussed internally - I think we should follow the same logic in both places.

RouteOptions extends DirectionsJsonObject so it already has unrecognised fields

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/Bearing.java

LukasPaczos · 2022-04-12T11:10:28Z

...irections-models/src/main/java/com/mapbox/api/directions/v5/models/DirectionsJsonObject.java


 /**
 * Provides a base class for Directions model classes.
 *
 * @since 3.4.0
 */
-public class DirectionsJsonObject implements Serializable {
+public abstract class DirectionsJsonObject implements Serializable {


This is a breaking change (not sure how impactful it would actually be in practice). Could we make the methods return empty and implement downstream, how would it look like?

nice catch 👍

AutoValue won't generate fields for not abstract methods

I see a few possible solutions right now.

Do breaking change

Maybe we can make ourself believe that it's not really a breaking change using following arguments: it more like a fix of a bug, nobody should have created an instance of DirectionsJsonObject, it doesn't make any sense to create an instance of DirectionsJsonObject. But abstract do really changes API.

Workaround

I can create an abstract class BaseDirectionsJsonObject which is going to be a parent of DirectionsJsonObject and move unrecognised filed there. I find this solution ugly but it won't break API.

I think that we need to decide if somebody could be affected by not being able to instantiate DirectionsJsonObject. If we're sure that we will affect nobody, and even if we will we're ready to reply in their issues on github, then I let's break the rule.
If we don't feel confident, let's workaround

I feel confident. I can't imaging a case when somebody instantiate DirectionsJsonObject but if somebody does it, I'd like to learn their case when they open an issue on our github 🙂
@LukasPaczos , @RingerJK , what do you think about it?

@VysotskiVadim I agree. There's no practical reason to extend the DirectionsJsonObject for developers, I think we can break here. Let's clearly state this in the changelog and go from there.

LukasPaczos · 2022-04-12T11:18:08Z

...ections-models/src/test/java/com/mapbox/api/directions/v5/models/DirectionsResponseTest.java

+    JsonObject originalJsonObject = gson.fromJson(originalJson, JsonObject.class);
+
+    DirectionsResponse model = DirectionsResponse.fromJson(originalJson);
+    JsonObject jsonFromObject = gson.fromJson(model.toJson(), JsonObject.class);
+
+    assertEquals(originalJsonObject, jsonFromObject);


Suggested change

JsonObject originalJsonObject = gson.fromJson(originalJson, JsonObject.class);

DirectionsResponse model = DirectionsResponse.fromJson(originalJson);

JsonObject jsonFromObject = gson.fromJson(model.toJson(), JsonObject.class);

assertEquals(originalJsonObject, jsonFromObject);

DirectionsResponse model = DirectionsResponse.fromJson(originalJson);

String jsonFromModel = model.toJson();

assertEquals(originalJson, jsonFromModel);

We want to ensure the resulting strings are correct, the object wrapping step seems unnecessary.

Strings won't be the same. An order of fields changes when json gets serialised to java model and then gets deserialised back. And I think it's acceptable behaviour. JsonObject compares that 2 JSONs has the same set of fields with the same value and it doesn't care about order.

That makes sense 👍 I guess the equality of a serialized form is just a side effect, not a guarantee.

services-directions-models/src/test/java/com/mapbox/api/directions/v5/utils/MutateJsonUtil.java

codecov · 2022-04-22T15:10:04Z

Codecov Report

Merging #1394 (edb8750) into main (d9d5af3) will increase coverage by 0.22%.
The diff coverage is 95.74%.

@@             Coverage Diff              @@
##               main    #1394      +/-   ##
============================================
+ Coverage     75.23%   75.45%   +0.22%     
- Complexity      920      926       +6     
============================================
  Files           125      125              
  Lines          3957     3960       +3     
  Branches        645      646       +1     
============================================
+ Hits           2977     2988      +11     
+ Misses          706      698       -8     
  Partials        274      274

Impacted Files	Coverage Δ
...ox/api/directions/v5/DirectionsAdapterFactory.java	`100.00% <ø> (ø)`
...ctions/v5/models/ExperimentalWaypointMetadata.java	`0.00% <0.00%> (ø)`
.../com/mapbox/api/directions/v5/models/Metadata.java	`0.00% <0.00%> (ø)`
...onsrefresh/v1/DirectionsRefreshAdapterFactory.java	`100.00% <ø> (ø)`
...i/geocoding/v5/models/GeocodingAdapterFactory.java	`100.00% <ø> (ø)`
.../matching/v5/models/MapMatchingAdapterFactory.java	`100.00% <ø> (ø)`
...com/mapbox/api/matrix/v1/MatrixAdapterFactory.java	`100.00% <ø> (ø)`
...mization/v1/models/OptimizationAdapterFactory.java	`100.00% <ø> (ø)`
...rsions/models/RouteTileVersionsAdapterFactory.java	`100.00% <ø> (ø)`
...ava/com/mapbox/api/directions/v5/models/Admin.java	`57.14% <100.00%> (ø)`
... and 38 more

VysotskiVadim · 2022-04-22T15:11:38Z

...irections-models/src/main/java/com/mapbox/api/directions/v5/models/DirectionsJsonObject.java

+
+  @Nullable
+  @UnrecognizedJsonProperties
+  abstract Map<String, SerializableJsonElement> unrecognised();


@LukasPaczos , let me open it or provide alternative API in separate PR so that we can discuss it separately.

...irections-models/src/main/java/com/mapbox/api/directions/v5/models/DirectionsJsonObject.java

services-directions/build.gradle

services-matrix/build.gradle

LukasPaczos · 2022-04-25T15:59:07Z

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java

+        if (property.equals(ACCESS_TOKEN_URL_PARAM_NAME)) {
+          continue;
+        }


Do we have a test that covers this already?

yep, I added this line because one of tests failed

VysotskiVadim · 2022-04-25T16:55:53Z

@RingerJK , @LukasPaczos , please review companion PR mapbox/auto-value-gson#2

LukasPaczos

LGTM, nice work!

VysotskiVadim requested review from RingerJK, LukasPaczos and a team April 4, 2022 09:21

VysotskiVadim commented Apr 4, 2022

View reviewed changes

VysotskiVadim mentioned this pull request Apr 4, 2022

roundtrip unrecognized JSON properties #1344

Closed

VysotskiVadim commented Apr 4, 2022

View reviewed changes

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/Bearing.java Show resolved Hide resolved

RingerJK reviewed Apr 12, 2022

View reviewed changes

LukasPaczos reviewed Apr 12, 2022

View reviewed changes

This was referenced Apr 14, 2022

support unrecognised fields rharter/auto-value-gson#267

Closed

Support unrecognised fields mapbox/auto-value-gson#1

Merged

VysotskiVadim force-pushed the vv-unrecognized-fileds branch from 5c1ca3a to 91ded5c Compare April 22, 2022 14:57

VysotskiVadim added 4 commits April 22, 2022 17:05

keep unrecognized fields

f447f0f

adopted pre release snapshot

fecee7b

trying to consume artifacts from sdk registry

fc59c2a

make unrecognised field notpublic

047da92

VysotskiVadim force-pushed the vv-unrecognized-fileds branch from 91ded5c to 047da92 Compare April 22, 2022 15:05

VysotskiVadim commented Apr 22, 2022

View reviewed changes

LukasPaczos reviewed Apr 25, 2022

View reviewed changes

VysotskiVadim added 3 commits April 25, 2022 18:35

renamed to american english

f2c7e28

fixed formatting

132b796

reverted removed dependency

fd2228b

switched to released version

a8d931f

VysotskiVadim force-pushed the vv-unrecognized-fileds branch from 06a3461 to a8d931f Compare April 26, 2022 11:23

VysotskiVadim added 3 commits April 26, 2022 13:24

code cleanup

20b35c2

code cleanup

997c5fd

code cleanup

e1b863c

VysotskiVadim added 4 commits April 26, 2022 13:40

code cleanup

f6be7b9

code cleanup

b326916

code cleanup

987ba05

reverted dependencies

2f0c24e

VysotskiVadim marked this pull request as ready for review April 26, 2022 11:50

VysotskiVadim requested review from RingerJK and LukasPaczos April 26, 2022 11:50

LukasPaczos approved these changes Apr 26, 2022

View reviewed changes

VysotskiVadim changed the title ~~Round trip of unrecognised JSON properties~~ Roundtrip of unrecognised JSON properties Apr 26, 2022

added changelog entry

edb8750

VysotskiVadim merged commit 977adad into main Apr 26, 2022

VysotskiVadim deleted the vv-unrecognized-fileds branch April 26, 2022 12:32

Guardiola31337 mentioned this pull request Apr 27, 2022

Bump mapboxSdkServices dependency version to 6.5.0-beta.3 mapbox/mapbox-navigation-android#5751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roundtrip of unrecognised JSON properties #1394

Roundtrip of unrecognised JSON properties #1394

VysotskiVadim commented Apr 4, 2022 •

edited

Loading

VysotskiVadim Apr 4, 2022

VysotskiVadim commented Apr 7, 2022 •

edited

Loading

RingerJK Apr 12, 2022

VysotskiVadim Apr 12, 2022

RingerJK Apr 12, 2022

LukasPaczos Apr 12, 2022

RingerJK Apr 12, 2022

LukasPaczos Apr 12, 2022

VysotskiVadim Apr 14, 2022

LukasPaczos Apr 12, 2022

VysotskiVadim Apr 14, 2022

VysotskiVadim Apr 14, 2022

VysotskiVadim Apr 14, 2022

VysotskiVadim Apr 14, 2022

VysotskiVadim Apr 14, 2022

LukasPaczos Apr 15, 2022

LukasPaczos Apr 12, 2022

VysotskiVadim Apr 14, 2022 •

edited

Loading

LukasPaczos Apr 15, 2022

codecov bot commented Apr 22, 2022 •

edited

Loading

VysotskiVadim Apr 22, 2022

VysotskiVadim Apr 25, 2022

LukasPaczos Apr 25, 2022

VysotskiVadim Apr 25, 2022

VysotskiVadim commented Apr 25, 2022

LukasPaczos left a comment

Roundtrip of unrecognised JSON properties #1394

Roundtrip of unrecognised JSON properties #1394

Conversation

VysotskiVadim commented Apr 4, 2022 • edited Loading

Goal

example

Implementation

Choose a reason for hiding this comment

VysotskiVadim commented Apr 7, 2022 • edited Loading

Test 1

Test 2

Test 3

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Do breaking change

Workaround

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VysotskiVadim Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 22, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VysotskiVadim commented Apr 25, 2022

LukasPaczos left a comment

Choose a reason for hiding this comment

VysotskiVadim commented Apr 4, 2022 •

edited

Loading

VysotskiVadim commented Apr 7, 2022 •

edited

Loading

VysotskiVadim Apr 14, 2022 •

edited

Loading

codecov bot commented Apr 22, 2022 •

edited

Loading