Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ public interface GeocodingService {
* @param language The locale in which results should be returned.
* @param reverseMode Set the factors that are used to sort nearby results.
* @param fuzzyMatch Set whether to allow the geocoding API to attempt exact matching or not.
* @param routing Set whether to request additional metadata
* about the recommended navigation destination.
* @return A retrofit Call object
* @since 1.0.0
*/
Expand All @@ -52,7 +54,8 @@ Call<GeocodingResponse> getCall(
@Query("limit") String limit,
@Query("language") String language,
@Query("reverseMode") String reverseMode,
@Query("fuzzyMatch") Boolean fuzzyMatch);
@Query("fuzzyMatch") Boolean fuzzyMatch,
@Query("routing") Boolean routing);

/**
* Constructs the html call using the information passed in through the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ protected Call<GeocodingResponse> initializeCall() {
limit(),
languages(),
reverseMode(),
fuzzyMatch());
fuzzyMatch(),
routing());
}

private Call<List<GeocodingResponse>> getBatchCall() {
Expand Down Expand Up @@ -215,6 +216,9 @@ public Call<List<GeocodingResponse>> cloneBatchCall() {
@Nullable
abstract Boolean fuzzyMatch();

@Nullable
abstract Boolean routing();

@Nullable
abstract String clientAppName();

Expand Down Expand Up @@ -560,6 +564,24 @@ public abstract Builder reverseMode(
*/
public abstract Builder fuzzyMatch(Boolean fuzzyMatch);

/**
* Specify whether to request additional metadata about the recommended navigation destination
* corresponding to the feature (true) or not (false, default).
* Only applicable for address features.
*
* For example, if routing=true the response could include data about a point
* on the road the feature fronts.
* Response features may include an array containing one or more routable points.
* Routable points cannot always be determined. Consuming applications should fall back to
* using the feature's normal geometry for routing if a separate routable point
* is not returned.
*
* @param routing optionally set whether to request
* additional metadata about the recommended navigation destination
* @return this builder for chaining options together
*/
public abstract Builder routing(Boolean routing);

/**
* Required to call when this is being built. If no access token provided,
* {@link ServicesException} will be thrown.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

/**
* Array representing a hierarchy of parents. Each parent includes id, text keys along with
* (if avaliable) a wikidata, short_code, and Maki key.
* (if available) a wikidata, short_code, and Maki key.
*
* @see <a href="https://github.com/mapbox/carmen/blob/master/carmen-geojson.md">Carmen Geojson information</a>
* @see <a href="https://www.mapbox.com/api-documentation/search/#geocoding">Mapbox geocoder documentation</a>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,15 @@ public Point center() {
@SerializedName("matching_place_name")
public abstract String matchingPlaceName();

/**
* An object with the routable points for the {@link CarmenFeature}.
*
* @return an object with the routable points for the {@link CarmenFeature}
*/
@Nullable
@SerializedName("routable_points")
public abstract RoutablePoints routablePoints();

/**
* A string of the IETF language tag of the query's primary language.
*
Expand Down Expand Up @@ -482,6 +491,14 @@ public abstract static class Builder {
*/
public abstract Builder language(@Nullable String language);

/**
* An object with the routable points for the {@link CarmenFeature}.
*
* @param routablePoints an object with the routable points for the {@link CarmenFeature}
* @return this builder for chaining options together
*/
public abstract Builder routablePoints(@Nullable RoutablePoints routablePoints);

/**
* Build a new {@link CarmenFeature} object.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package com.mapbox.api.geocoding.v5.models;

import androidx.annotation.Nullable;

import com.google.auto.value.AutoValue;
import com.google.gson.Gson;
import com.google.gson.TypeAdapter;
import com.google.gson.annotations.SerializedName;
import com.mapbox.geojson.Point;

/**
* Object representing {@link CarmenFeature}'s routable point.
*/
@AutoValue
public abstract class RoutablePoint {

/**
* A string representing the routable point name.
*
* @return routable point name
*/
@Nullable
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this really nullable? I mean, what I'd expect is RoutablePoints to be nullable but If they're not, any RoutablePoint from the list to have valid values. Does that make sense?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@Guardiola31337 Do you mean by duplicating e.g. the rooftop location in RoutablePoints? If yes, I disagree.

I assume the correct distinction would be to leave RoutablePoints null if there are none and let the app logic decide how to behave in that case (e.g. fallback to original location). Otherwise it's becoming ambiguous whether the data is actually available for the specific search result.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Guardiola31337 in addition to what @kalbaxa said, I think it's better to leave it as nullable because all the service-geocoder relies on AutoValue/GSON serialisation/deserialisation. If we declare any field as non-nullable and for some reason backend don't send the value, the SDK will crash. I don't want this. I think it's better to check for null on customer's side and use alternative fallback. Also, nullable fields are consistent with the current approach.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kalbaxa What I meant was that routable points could be null (that's totally fine) but if the server emits a routable point, I would expect not to be null (having null fields), that's the contract I'd expect from the backend.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@DzmitryFomchyn IMO errors in the backend should fail-fast, this approach would hide this type of issues as they'd go unnoticed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A can agree with that, but we should also be consistent here. All the current code relies on nullability, it won't make any better if we make one field as non-null while everything else remain nullable. Given everything that's written above, I think we should leave as is now and discuss another approach in case of SDK refactoring, if it happen. More likely, service-geocoder will be deprecated soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we declare any field as non-nullable and for some reason backend don't send the value, the SDK will crash. I don't want this.

+++

Event if it's not an error on the backend side, it can be a logic change. For example, we accidentally made waypoints nullable but that played well because after some time they did become nullable because a case was introduced that they can be returned in a different response level.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it can be a logic change

A logic change like this should mean a major version upgrade of an endpoint. All customers (not only Nav SDK) would be impacted by such an intentional change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not necessarily. With waypoints they wouldn't (and weren't). Because it depends on a new request parameter which falls back to old behaviour.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We would have to provide backwards compatibility if that wasn't the case (either on backend or mapbox-java level). We were lucky and needed less work for that specific situation but that shouldn't be the reason to make contracts less strict going forward.

@SerializedName("name")
public abstract String name();

/**
* A {@link Point} object which represents the routable point location.
*
* @return a GeoJson {@link Point} which defines the routable point location
*/
@Nullable
Comment thread
DzmitryFomchyn marked this conversation as resolved.
public Point coordinate() {
final double[] coordinate = rawCoordinate();
if (coordinate != null && coordinate.length == 2) {
return Point.fromLngLat(coordinate[0], coordinate[1]);
}
return null;
}

// No public access thus, we lessen enforcement on mutability here.
@Nullable
Comment thread
DzmitryFomchyn marked this conversation as resolved.
@SerializedName("coordinates")
@SuppressWarnings("mutable")
abstract double[] rawCoordinate();

/**
* Convert current instance values into another Builder to quickly change one or more values.
*
* @return a new instance of {@link Builder}
*/
@SuppressWarnings("unused")
public abstract Builder toBuilder();

/**
* Gson type adapter for parsing Gson to this class.
*
* @param gson the built {@link Gson} object
* @return the type adapter for this class
*/
public static TypeAdapter<RoutablePoint> typeAdapter(Gson gson) {
return new AutoValue_RoutablePoint.GsonTypeAdapter(gson);
}

/**
* This builder can be used to set the values describing the {@link RoutablePoint}.
*/
@AutoValue.Builder
@SuppressWarnings("unused")
public abstract static class Builder {

/**
* A string representing the routable point name.
*
* @param name routable point name
* @return this builder for chaining options together
*/
public abstract Builder name(@Nullable String name);
Comment thread
DzmitryFomchyn marked this conversation as resolved.

/**
* Raw coordinates (longitude and latitude, order matters)
* that represent the routable point location.
*
* @param coordinate raw coordinates that represent the routable point location
* @return this builder for chaining options together
*/
public abstract Builder rawCoordinate(@Nullable double[] coordinate);
Comment thread
DzmitryFomchyn marked this conversation as resolved.

/**
* Build a new {@link RoutablePoint} object.
*
* @return a new {@link RoutablePoint} using the provided values in this builder
*/
public abstract RoutablePoint build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package com.mapbox.api.geocoding.v5.models;

import androidx.annotation.Nullable;

import com.google.auto.value.AutoValue;
import com.google.gson.Gson;
import com.google.gson.TypeAdapter;
import com.google.gson.annotations.SerializedName;

import java.util.List;

/**
* An object with the routable points for the {@link CarmenFeature}.
*/
@AutoValue
public abstract class RoutablePoints {

/**
* A list of routable points for the {@link CarmenFeature}, or null if no points were found.
*
* @return a list of routable points for the {@link CarmenFeature},
* or null if no points were found
*/
@Nullable
@SerializedName("points")
public abstract List<RoutablePoint> points();

/**
* Convert current instance values into another Builder to quickly change one or more values.
*
* @return a new instance of {@link Builder}
*/
@SuppressWarnings("unused")
public abstract Builder toBuilder();

/**
* Gson type adapter for parsing Gson to this class.
*
* @param gson the built {@link Gson} object
* @return the type adapter for this class
*/
public static TypeAdapter<RoutablePoints> typeAdapter(Gson gson) {
return new AutoValue_RoutablePoints.GsonTypeAdapter(gson);
}

/**
* This builder can be used to set the values describing the {@link RoutablePoints}.
*/
@AutoValue.Builder
@SuppressWarnings("unused")
public abstract static class Builder {

/**
* A list of routable points for the {@link CarmenFeature},
* or null if no points were found.
*
* @param points a list of routable points for the {@link CarmenFeature},
* or null if no points were found
* @return this builder for chaining options together
*/
public abstract Builder points(@Nullable List<RoutablePoint> points);

/**
* Build a new {@link RoutablePoints} object.
*
* @return a new {@link RoutablePoints} using the provided values in this builder
*/
public abstract RoutablePoints build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import static org.hamcrest.Matchers.startsWith;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;

Expand Down Expand Up @@ -312,6 +313,44 @@ public void fuzzyMatch_getsAddedToUrlCorrectly() {
.contains("fuzzyMatch=true"));
}

@Test
public void routingSanity() throws Exception {
MapboxGeocoding mapboxGeocoding = MapboxGeocoding.builder()
.accessToken(ACCESS_TOKEN)
.query("wahsington")
.routing(true)
.baseUrl(mockUrl.toString())
.build();
assertNotNull(mapboxGeocoding);
Response<GeocodingResponse> response = mapboxGeocoding.executeCall();
assertEquals(200, response.code());
}

@Test
public void routing_defaultIsNotSpecified() {
MapboxGeocoding mapboxGeocoding = MapboxGeocoding.builder()
.accessToken(ACCESS_TOKEN)
.query("wahsington")
.baseUrl(mockUrl.toString())
.build();
assertNotNull(mapboxGeocoding);
assertFalse(mapboxGeocoding.cloneCall().request().url().toString()
.contains("routing="));
}

@Test
public void routing_getsAddedToUrlCorrectly() {
MapboxGeocoding mapboxGeocoding = MapboxGeocoding.builder()
.accessToken(ACCESS_TOKEN)
.query("wahsington")
.routing(true)
.baseUrl(mockUrl.toString())
.build();
assertNotNull(mapboxGeocoding);
assertTrue(mapboxGeocoding.cloneCall().request().url().toString()
.contains("routing=true"));
}

@Test
public void intersectionSearchSanity() throws IOException {
MapboxGeocoding mapboxGeocoding = MapboxGeocoding.builder()
Expand Down Expand Up @@ -382,7 +421,7 @@ public void intersectionSearch_AutoSetGeocodingType() {
}

@Test
public void intersectionSearch_EmptyPromixity() {
public void intersectionSearch_EmptyProximity() {
thrown.expect(ServicesException.class);
thrown.expectMessage(
startsWith("Geocoding proximity must be set for intersection search."));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
public class CarmenContextTest extends GeocodingTestUtils {

@Test
public void sanity() throws Exception {
public void sanity() {
CarmenContext carmenContext = CarmenContext.builder().build();
assertNotNull(carmenContext);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public class CarmenFeatureTest extends GeocodingTestUtils {
private static final String FORWARD_FEATURE_VALID = "forward_feature_valid.json";

@Test
public void sanity() throws Exception {
public void sanity() {
CarmenFeature carmenFeature = CarmenFeature.builder()
.properties(new JsonObject())
.address("1234")
Expand Down Expand Up @@ -104,6 +104,32 @@ public void fromJson_handlesConversionCorrectly() throws Exception {
assertThat(feature.center().latitude(), equalTo(38.897702));
assertThat(feature.center().longitude(), equalTo(-77.036543));
assertThat(feature.language(), nullValue());
assertThat(feature.routablePoints(), notNullValue());
}

@Test
public void routablePoints_handlesCorrectly() throws IOException {
MapboxGeocoding mapboxGeocoding = MapboxGeocoding.builder()
.accessToken(ACCESS_TOKEN)
.query("1600 pennsylvania ave nw")
.baseUrl(mockUrl.toString())
.build();
GeocodingResponse response = mapboxGeocoding.executeCall().body();
assertNotNull(response);

assertThat(response.features().size(), equalTo(5));

CarmenFeature feature = response.features().get(0);
assertThat(feature.routablePoints(), notNullValue());
assertThat(feature.routablePoints().points(), notNullValue());
assertThat(feature.routablePoints().points().size(), equalTo(1));
assertThat(feature.routablePoints().points().get(0).name(), equalTo("default_routable_point"));
assertThat(feature.routablePoints().points().get(0).coordinate(), equalTo(Point.fromLngLat(-77.036544, 38.897703)));

assertThat(response.features().get(1).routablePoints(), notNullValue());
assertThat(response.features().get(1).routablePoints().points(), nullValue());

assertThat(response.features().get(2).routablePoints(), nullValue());
}

@Test
Expand Down
Loading