Roundtrip of unrecognised JSON properties by vadzim-vys · Pull Request #1394 · mapbox/mapbox-java

vadzim-vys · 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.

vadzim-vys · 2022-04-04T09:27:20Z

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

vadzim-vys · 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

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

    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

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

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

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

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

vadzim-vys · 2022-04-22T15:11:38Z

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

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

+        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

vadzim-vys · 2022-04-25T16:55:53Z

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

LukasPaczos

LGTM, nice work!

vadzim-vys requested review from a team, LukasPaczos and RingerJK April 4, 2022 09:21

vadzim-vys commented Apr 4, 2022

View reviewed changes

vadzim-vys mentioned this pull request Apr 4, 2022

roundtrip unrecognized JSON properties #1344

Closed

vadzim-vys commented Apr 4, 2022

View reviewed changes

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

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

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

vadzim-vys 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

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

vadzim-vys commented Apr 22, 2022

View reviewed changes

LukasPaczos reviewed Apr 25, 2022

View reviewed changes

vadzim-vys 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

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

vadzim-vys added 3 commits April 26, 2022 13:24

code cleanup

20b35c2

code cleanup

997c5fd

code cleanup

e1b863c

vadzim-vys added 4 commits April 26, 2022 13:40

code cleanup

f6be7b9

code cleanup

b326916

code cleanup

987ba05

reverted dependencies

2f0c24e

vadzim-vys marked this pull request as ready for review April 26, 2022 11:50

vadzim-vys requested review from LukasPaczos and RingerJK April 26, 2022 11:50

LukasPaczos approved these changes Apr 26, 2022

View reviewed changes

vadzim-vys changed the title ~~Round trip of unrecognised JSON properties~~ Roundtrip of unrecognised JSON properties Apr 26, 2022

added changelog entry

edb8750

vadzim-vys merged commit 977adad into main Apr 26, 2022

vadzim-vys 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

Conversation

vadzim-vys commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Goal

example

Implementation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vadzim-vys commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test 1

Test 2

Test 3

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Do breaking change

Workaround

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadzim-vys Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadzim-vys commented Apr 25, 2022

Uh oh!

LukasPaczos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

vadzim-vys commented Apr 4, 2022 •

edited

Loading

vadzim-vys commented Apr 7, 2022 •

edited

Loading

vadzim-vys Apr 14, 2022 •

edited

Loading

codecov Bot commented Apr 22, 2022 •

edited

Loading