Bearing formatting fix by korshaknn · Pull Request #1121 · mapbox/mapbox-java

korshaknn · 2020-02-20T08:22:48Z

removed bearing validation logic from MapboxDirections
now both RouteOptions and MapboxDirections use FormatUtils.formatBearings() to build a valid string from a list of bearings

closes #1120

LukasPaczos · 2020-02-20T12:08:31Z

-  protected void writePoint(JsonWriter out, Point value) throws  IOException {
-    writePointList(out, value.coordinates());
+  protected void writePoint(JsonWriter out, Point point) throws  IOException {
+    if (point == null) {


We are now choosing to ignore nulls, while we should actually be able to support them when serializing and deserializing Lists in the BaseCoordinatesTypeAdapter. I think we'll need to refactor both read and write methods.

@LukasPaczos what logic should writePoint has in case of null Point?
also readPoint always returns non null Point or throws an exception.

also readPoint always returns non null Point or throws an exception

That's what we'd need to refactor. Could we use JsonToken.NULL or a similar construct?

Is this because before null wasn't an option when reach serialization / deserialization and now it is? I believe the problem is that before we were always dealing with Strings and not anymore 🤔 How serialization / deserialization was working before?

@Guardiola31337 @LukasPaczos I think it's only because of waypointTargetsList() in RouteOptions.
Previously waypointTargets were stored as a String, so PointAsCoordinatesTypeAdapter was not used. But now they are a List<Point>, so PointAsCoordinatesTypeAdapter calls BaseCoordinatesTypeAdapter.writePoint().
It is possible to store nulls in waypointTargetsList, so we need to check for a null inside writePoint() method

@korshaknn could we add some tests in RouteOptions verifying that the serialization / deserialization works 👌 (toJson / fromJson)?

That's right @korshaknn, but we still need to serialize and deserialize this null, not skip it. I think the inconsistency will be caught with a test that @Guardiola31337 suggested.

@LukasPaczos how that null Point should be serialized?

how that null Point should be serialized?

Probably "" (empty)? Could we run a quick test of toJson / fromJson before and after #1118 to check how was working, compare and do the same?

Empty string sounds correct.

Guardiola31337

Apart from the comments below, when debugging this PR I also noticed that in DirectionsResponseFactory#generateRouteOptions we're using the deprecated annotations API instead of annotationsList 👀

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

Line 62 in 123e7cd

.annotations(mapboxDirections.annotation())

Guardiola31337 · 2020-02-21T00:03:45Z

-  protected void writePoint(JsonWriter out, Point value) throws  IOException {
-    writePointList(out, value.coordinates());
+  protected void writePoint(JsonWriter out, Point point) throws  IOException {
+    if (point == null) {


Is this because before null wasn't an option when reach serialization / deserialization and now it is? I believe the problem is that before we were always dealing with Strings and not anymore 🤔 How serialization / deserialization was working before?

Guardiola31337 · 2020-02-21T00:22:18Z

+        .bearings(bearings)
+        .accessToken(ACCESS_TOKEN)
+        .build();
+    assertEquals(";;45,90;2,90;",


Debugging this PR I found that ParseUtils#parseToListOfListOfDoubles in this concrete example is not working as expected

@Test public void checksParseToListOfListOfDoublesEmptyTrailing() { List<List<Double>> bearings = ParseUtils.parseToListOfListOfDoubles(";;45,90;2,90;"); assertEquals(5, bearings.size()); }

It should return a list of 5 elements and it's returning a list of 4 because of not splitting with trailing empty strings

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

Line 141 in 123e7cd

String[] pairs = original.split(SEMICOLON);

After changing to original.split(SEMICOLON, -1); other tests start failing. We should revisit the empty trailing scenarios for other fields too.

tested it with @kmadsen , @Guardiola31337 you are right, bearing string should HAVE trailing ; if we pass null bearings.
I'll fix logic and tests

@korshaknn is ";;45,90;2,90;" and ";;45,90;2,90" going to be equal in this case? I think it shouldn't be. We should take the strategy that Directions API uses:

what happens if we pass "45,90;" as the request parameter? Is it considered as 2 values (1 empty)?

what happens if we pass "45,90;;" as the request parameter? Is it considered as 3 values (2 empty)?
If yes, we shouldn't trim the trailing empty string.

/cc @danpaz

Oh, looks like we commented at the same time @korshaknn :) Thanks for verifying!

@LukasPaczos
";;45,90;2,90;" will be valid for 5 coordinates
";;45,90;2,90" will be valid for 4 coordinates

korshaknn · 2020-02-24T11:35:02Z

@LukasPaczos @Guardiola31337
I tested RouteOptions.waypointTargets logic before refactoring and got the next results:

waypointTargets("11,22;33,44;55,66") -> "waypoint_targets":"11,22;33,44;55,66"

waypointTargets("11,22;33,44;") -> "waypoint_targets":"11,22;33,44;"

waypointTargets("11,22;;55,66") -> "waypoint_targets":"11,22;;55,66"

waypointTargets(";33,44;55,66") -> "waypoint_targets":";33,44;55,66"

I decided to return old logic and use String to store waypointTargets, in that case there is no need to change serialization/deserialization logic, it won't break anything.
so there will be 2 methods in builder:
abstract waypointTargets(String)
nonAbstract waypointTargetsList(List) (it uses FormatUtils.formatPointsList to store as s string)

to get waypointTargets from RouteOptions we can call
String waypointTargets
or List<Point> waypointTargetsList (it uses ParseUtils.parseToPoints to get a List from a string)

commit

LukasPaczos · 2020-02-24T13:59:10Z

The only concern I have is that if the end-users (and our SDKs) are going to rely on the Lists implementation (because it's more convenient and that was the goal), with every call to the non-abstract List method we will have to parse the string and create new objects, again and again.

Did you run into any issues when trying to serialize/deserialize nulls?

korshaknn · 2020-02-24T14:02:45Z

@LukasPaczos no, I haven't had any issues with serialization/deserialization.
BTW with old implementation we got a string and converted it to a new List. So now we just have the same logic inside RouteOptions

LukasPaczos · 2020-02-24T14:06:30Z

BTW with old implementation we got a string and converted it to a new List. So now we just have the same logic inside RouteOptions

The question is whether we can hopefully skip one of those steps and optimize the process. If you had no issues with serialization, could we try it?

korshaknn · 2020-02-24T14:19:02Z

@LukasPaczos
if we store waypointTargets as List<Point>, RouteOptions will have 2 List<Point> :
List<Point> coordinates
and
List<Point> waypointTargets
so, they both will use the same serialization/deserialization adapter. But they have to be serialized in a different way.
"coordinates":[[1.0,2.0],[3.0,4.0]] - a list
"waypoint_targets":"33,44;55,66" - a string with semicolons

so I think it's better to store waypointTargets as a String
BTW, MapboxDirections also stores waypointTargets as a String. When you pass a List, it converts it with FormatUtils

LukasPaczos

Alright, let's just try it out in this case. We can continue iterating if needed.

Guardiola31337 · 2020-02-24T15:09:31Z

@korshaknn

But they have to be serialized in a different way.

Is this needed? Can't those be serialized / deserialized equally? Would that help solving @LukasPaczos's concern?

korshaknn · 2020-02-24T17:10:14Z

@Guardiola31337 it was serialized in a different way before refactoring.
If we change serialization logic It may work incorrect in some cases. That's why I'm a little afraid of those changes.
Where do we send a json? Can it break any external (API) logic?
Where do we receive json from? Currently we receive a json which stores coordinates as a list and waypointTragets as a String. What if we change logic to handle waypointTragets as List, but someone sends us a String?

or maybe serialization/deserialization of RouteOptions is just an internal logic and doesn't affect other parts of the SDK and we can change it without any problems?

Guardiola31337 · 2020-02-24T18:13:33Z

@korshaknn

it was serialized in a different way before refactoring.
If we change serialization logic It may work incorrect in some cases. That's why I'm a little afraid of those changes.
Where do we send a json? Can it break any external (API) logic?
Where do we receive json from? Currently we receive a json which stores coordinates as a list and waypointTragets as a String. What if we change logic to handle waypointTragets as List, but someone sends us a String?

These are valid questions 👍

or maybe serialization/deserialization of RouteOptions is just an internal logic and doesn't affect other parts of the SDK and we can change it without any problems?

That's a great question 🤔 Events maybe? It'd be great to find out testing downstream in the Navigation SDK and 👀 if everything is working 👌 or if there are any 💥 We can do that in a separate PR, so feel free to merge here 🚀

bearing formatting fix

a71b33f

korshaknn added the Bug label Feb 20, 2020

korshaknn added this to the v5.0.0 milestone Feb 20, 2020

korshaknn requested review from Guardiola31337 and LukasPaczos February 20, 2020 08:22

korshaknn self-assigned this Feb 20, 2020

LukasPaczos reviewed Feb 20, 2020

View reviewed changes

Guardiola31337 reviewed Feb 21, 2020

View reviewed changes

korshaknn added 2 commits February 21, 2020 12:45

fix bearings parsing

377949a

use string to store RouteOptions.waypointTargets

b95befc

LukasPaczos approved these changes Feb 24, 2020

View reviewed changes

korshaknn merged commit 9ffe942 into master Feb 24, 2020

This was referenced Feb 27, 2020

RouteOptions parsing improvements #1117

Closed

MapboxDirections builder throws Exception when approaches is empty. #1081

Closed

RouteOptions IllegalStateException - RouteOptions backwards incompatible #1122

Closed

langsmith mentioned this pull request Mar 23, 2020

Release 5.1.0-beta.2 #1131

Closed

2 tasks

Conversation

korshaknn commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Guardiola31337 left a comment

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!

korshaknn commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LukasPaczos commented Feb 24, 2020

Uh oh!

korshaknn commented Feb 24, 2020

Uh oh!

LukasPaczos commented Feb 24, 2020

Uh oh!

korshaknn commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LukasPaczos left a comment

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 commented Feb 24, 2020

Uh oh!

korshaknn commented Feb 24, 2020

Uh oh!

Guardiola31337 commented Feb 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

korshaknn commented Feb 20, 2020 •

edited

Loading

korshaknn commented Feb 24, 2020 •

edited

Loading

korshaknn commented Feb 24, 2020 •

edited

Loading