Skip to content

Bearing formatting fix #1121

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.

Sign up for GitHub

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

Merged
korshaknn merged 3 commits into from
Feb 24, 2020
Merged

Bearing formatting fix #1121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a71b33f
bearing formatting fix
korshaknn Feb 20, 2020
377949a
fix bearings parsing
korshaknn Feb 21, 2020
b95befc
use string to store RouteOptions.waypointTargets
korshaknn Feb 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 10 additions & 17 deletions ...ces-directions-models/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,13 +481,10 @@ public String waypointNames() {
* Must be used with {@link RouteOptions#steps()} = true.
* @return a list of Points representing coordinate pairs for drop-off locations
* @since 4.3.0
* @deprecated use {@link #waypointTargetsList()}
*/
@SerializedName("waypoint_targets")
@Nullable
@Deprecated
public String waypointTargets() {
return FormatUtils.formatPointsList(waypointTargetsList());
}
public abstract String waypointTargets();

/**
* A list of points used to specify drop-off
Expand All @@ -499,9 +496,10 @@ public String waypointTargets() {
* Must be used with {@link RouteOptions#steps()} = true.
* @return a list of Points representing coordinate pairs for drop-off locations
*/
@SerializedName("waypoint_targets")
@Nullable
public abstract List<Point> waypointTargetsList();
public List<Point> waypointTargetsList() {
return ParseUtils.parseToPoints(waypointTargets());
}

/**
* To be used to specify settings for use with the walking profile.
Expand Down Expand Up @@ -1029,16 +1027,8 @@ public Builder waypointNames(@NonNull String waypointNames) {
* @param waypointTargets list of coordinate pairs for drop-off locations (;)
* @return this builder for chaining options together
* @since 4.3.0
* @deprecated use {@link #waypointTargetsList(List)}
*/
@Deprecated
public Builder waypointTargets(@NonNull String waypointTargets) {
List<Point> targets = ParseUtils.parseToPoints(waypointTargets);
if (targets != null) {
waypointTargetsList(targets);
}
return this;
}
public abstract Builder waypointTargets(@NonNull String waypointTargets);

/**
* A list of coordinate pairs used to specify drop-off
Expand All @@ -1054,7 +1044,10 @@ public Builder waypointTargets(@NonNull String waypointTargets) {
* @param waypointTargets list of Points for drop-off locations
* @return this builder for chaining options together
*/
public abstract Builder waypointTargetsList(@NonNull List<Point> waypointTargets);
public Builder waypointTargetsList(@NonNull List<Point> waypointTargets) {
waypointTargets(FormatUtils.formatPointsList(waypointTargets));
return this;
}

/**
* To be used to specify settings for use with the walking profile.
Expand Down
22 changes: 18 additions & 4 deletions services-directions-models/src/main/java/com/mapbox/api/directions/v5/utils/FormatUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,26 @@ public static String formatBearings(@Nullable List<List<Double>> bearings) {

List<String> bearingsToJoin = new ArrayList<>();
for (List<Double> bearing : bearings) {
if (bearing == null || bearing.size() == 0) {
if (bearing == null) {
bearingsToJoin.add(null);
} else {
bearingsToJoin.add(String.format(Locale.US, "%s,%s",
formatCoordinate(bearing.get(0)),
formatCoordinate(bearing.get(1))));
if (bearing.size() != 2) {
throw new RuntimeException("Bearing size should be 2.");
}

Double angle = bearing.get(0);
Double tolerance = bearing.get(1);
if (angle == null || tolerance == null) {
bearingsToJoin.add(null);
} else {
if (angle < 0 || angle > 360 || tolerance < 0 || tolerance > 360) {
throw new RuntimeException("Angle and tolerance have to be from 0 to 360.");
}

bearingsToJoin.add(String.format(Locale.US, "%s,%s",
formatCoordinate(angle),
formatCoordinate(tolerance)));
}
}
}
return join(";", bearingsToJoin);
Expand Down
2 changes: 1 addition & 1 deletion services-directions-models/src/main/java/com/mapbox/api/directions/v5/utils/ParseUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public static List<List<Double>> parseToListOfListOfDoubles(@Nullable String ori
}

List<List<Double>> result = new ArrayList<>();
String[] pairs = original.split(SEMICOLON);
String[] pairs = original.split(SEMICOLON, -1);
for (String pair : pairs) {
if (pair.isEmpty()) {
result.add(null);
Expand Down
54 changes: 51 additions & 3 deletions ...directions-models/src/test/java/com/mapbox/api/directions/v5/models/RouteOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

public class RouteOptionsTest {

private static final String ROUTE_OPTIONS_JSON = "{\"baseUrl\":\"base_url\",\"user\":\"user\",\"profile\":\"profile\",\"coordinates\":[[1.0,2.0],[3.0,4.0]],\"access_token\":\"token\",\"uuid\":\"requestUuid\",\"waypoint_targets\":\";33,44;55,66\"}";

@Test
public void toBuilder() {
RouteOptions routeOptions = routeOptions();
Expand Down Expand Up @@ -91,12 +93,14 @@ public void bearingsString() {
bearing.add(5.1);
bearing.add(7.4);

assertEquals(";5.1,7.4", routeOptions.bearings());
assertEquals(";5.1,7.4;;", routeOptions.bearings());

List<List<Double>> bearings = routeOptions.bearingsList();
assertEquals(2, bearings.size());
assertEquals(4, bearings.size());
assertEquals(null, bearings.get(0));
assertEquals(bearing, bearings.get(1));
assertEquals(null, bearings.get(2));
assertEquals(null, bearings.get(3));
}

@Test
Expand Down Expand Up @@ -255,7 +259,7 @@ public void waypointTargetsString() {
.waypointTargets(targetsStr)
.build();

assertEquals("1.2,3.4;;;5.65,7.123", routeOptions.waypointTargets());
assertEquals("1.2,3.4;;;5.65,7.123;;;", routeOptions.waypointTargets());

List<Point> targets = routeOptions.waypointTargetsList();
assertEquals(4, targets.size());
Expand Down Expand Up @@ -320,6 +324,50 @@ public void annotationsList() {
assertEquals("congestion;distance;maxspeed;speed", routeOptions.annotations());
}

@Test
public void waypointTargetsStringToJson() {
RouteOptions options = routeOptions().toBuilder()
.waypointTargets(";33,44;55,66")
.build();

String json = options.toJson();

assertEquals(ROUTE_OPTIONS_JSON, json);
}

@Test
public void waypointTargetsListToJson() {
List<Point> waypointTargets = new ArrayList<>();
waypointTargets.add(null);
waypointTargets.add(Point.fromLngLat(33, 44));
waypointTargets.add(Point.fromLngLat(55, 66));

RouteOptions options = routeOptions().toBuilder()
.waypointTargetsList(waypointTargets)
.build();

String json = options.toJson();

assertEquals(ROUTE_OPTIONS_JSON, json);
}

@Test
public void waypointTargetsStringFromJson() {
RouteOptions options = RouteOptions.fromJson(ROUTE_OPTIONS_JSON);

assertEquals(";33,44;55,66", options.waypointTargets());
}

@Test
public void waypointTargetsListFromJson() {
RouteOptions options = RouteOptions.fromJson(ROUTE_OPTIONS_JSON);

assertEquals(3, options.waypointTargetsList().size());
assertEquals(null, options.waypointTargetsList().get(0));
assertEquals(Point.fromLngLat(33, 44), options.waypointTargetsList().get(1));
assertEquals(Point.fromLngLat(55, 66), options.waypointTargetsList().get(2));
}

private RouteOptions routeOptions() {
List<Point> coordinates = new ArrayList<>();
coordinates.add(Point.fromLngLat(1.0, 2.0));
Expand Down
2 changes: 1 addition & 1 deletion ...ices-directions/src/main/java/com/mapbox/api/directions/v5/DirectionsResponseFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ private List<DirectionsRoute> generateRouteOptions(Response<DirectionsResponse>
.waypointNamesList(ParseUtils.parseToStrings(mapboxDirections.waypointNames()))
.waypointTargetsList(ParseUtils.parseToPoints(mapboxDirections.waypointTargets()))
.continueStraight(mapboxDirections.continueStraight())
.annotations(mapboxDirections.annotation())
.annotationsList(ParseUtils.parseToStrings(mapboxDirections.annotation()))
.approachesList(ParseUtils.parseToStrings(mapboxDirections.approaches()))
.bearingsList(ParseUtils.parseToListOfListOfDoubles(mapboxDirections.bearing()))
.alternatives(mapboxDirections.alternatives())
Expand Down
25 changes: 2 additions & 23 deletions services-directions/src/main/java/com/mapbox/api/directions/v5/MapboxDirections.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,11 +658,7 @@ public Builder addAnnotation(@AnnotationCriteria @NonNull String annotation) {
*/
public Builder addBearing(@Nullable @FloatRange(from = 0, to = 360) Double angle,
@Nullable @FloatRange(from = 0, to = 360) Double tolerance) {
if (angle == null || tolerance == null) {
bearings.add(new ArrayList<Double>());
} else {
bearings.add(Arrays.asList(angle, tolerance));
}
bearings.add(Arrays.asList(angle, tolerance));
return this;
}

Expand All @@ -679,24 +675,7 @@ public Builder addBearing(@Nullable @FloatRange(from = 0, to = 360) Double angle
* @return this builder for chaining options together
*/
public Builder bearings(@NonNull List<List<Double>> bearings) {
List<List<Double>> newBearings = new ArrayList<>();
for (List<Double> bearing : bearings) {
if (bearing.size() != 2) {
throw new ServicesException("Bearing size should be 2.");
}
Double angle = bearing.get(0);
Double tolerance = bearing.get(1);
if (angle == null || tolerance == null) {
newBearings.add(new ArrayList<Double>());
} else {
if (angle < 0 || angle > 360 || tolerance < 0 || tolerance > 360) {
throw new ServicesException("Angle and tolerance have to be from 0 to 360.");
}
newBearings.add(Arrays.asList(angle, tolerance));
}
}

this.bearings = newBearings;
this.bearings = bearings;
return this;
}

Expand Down
49 changes: 49 additions & 0 deletions services-directions/src/test/java/com/mapbox/api/directions/v5/MapboxDirectionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.mapbox.api.directions.v5.models.DirectionsRoute;
import com.mapbox.api.directions.v5.models.LegAnnotation;
import com.mapbox.api.directions.v5.models.RouteOptions;
import com.mapbox.api.directions.v5.utils.ParseUtils;
import com.mapbox.core.TestUtils;
import com.mapbox.core.exceptions.ServicesException;
import com.mapbox.geojson.Point;
Expand Down Expand Up @@ -470,6 +471,54 @@ public void addBearing_doesGetFormattedInUrlCorrectly() throws Exception {
directions.cloneCall().request().url().queryParameter("bearings"));
}

@Test
public void addNullBearings_doesGetFormattedInUrlCorrectly() throws Exception {
List<Double> bearing1 = new ArrayList<>();
bearing1.add(45d);
bearing1.add(90d);

List<Double> bearing2 = new ArrayList<>();
bearing2.add(2d);
bearing2.add(90d);

List<List<Double>> bearings = new ArrayList<>();
bearings.add(null);
bearings.add(null);
bearings.add(bearing1);
bearings.add(bearing2);
bearings.add(null);

MapboxDirections directions = MapboxDirections.builder()
.destination(Point.fromLngLat(13.4930, 9.958))
.origin(Point.fromLngLat(1.234, 2.345))
.bearings(bearings)
.accessToken(ACCESS_TOKEN)
.build();
assertEquals(";;45,90;2,90;",
Copy link
Contributor

@Guardiola31337 Guardiola31337 Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@korshaknn korshaknn Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@LukasPaczos LukasPaczos Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@LukasPaczos LukasPaczos Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@korshaknn korshaknn Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

directions.cloneCall().request().url().queryParameter("bearings"));
}

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

assertEquals(5, bearings.size());

List<Double> bearing1 = new ArrayList<>();
bearing1.add(45d);
bearing1.add(90d);

List<Double> bearing2 = new ArrayList<>();
bearing2.add(2d);
bearing2.add(90d);

assertEquals(null, bearings.get(0));
assertEquals(null, bearings.get(1));
assertEquals(bearing1, bearings.get(2));
assertEquals(bearing2, bearings.get(3));
assertEquals(null, bearings.get(4));
}

@Test
public void bearing_doesGetFormattedInUrlCorrectly() throws Exception {
List<List<Double>> bearings = new ArrayList<>();
Expand Down
7 changes: 5 additions & 2 deletions services-geojson/src/main/java/com/mapbox/geojson/BaseCoordinatesTypeAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,11 @@
abstract class BaseCoordinatesTypeAdapter<T> extends TypeAdapter<T> {


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) {
Copy link
Contributor

@LukasPaczos LukasPaczos Feb 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@korshaknn korshaknn Feb 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@LukasPaczos LukasPaczos Feb 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@Guardiola31337 Guardiola31337 Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@korshaknn korshaknn Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@Guardiola31337 Guardiola31337 Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@LukasPaczos LukasPaczos Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@korshaknn korshaknn Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LukasPaczos how that null Point should be serialized?

Copy link
Contributor

@Guardiola31337 Guardiola31337 Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@LukasPaczos LukasPaczos Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty string sounds correct.

return;
}
writePointList(out, point.coordinates());
}

protected Point readPoint(JsonReader in) throws IOException {
Expand Down