Skip to content

Geocoding V6 preview #1550

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

Closed
DzmitryFomchyn wants to merge 3 commits into from
Closed

Geocoding V6 preview #1550

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
525276f
V6 preview
DzmitryFomchyn Feb 20, 2023
eb5084d
V6 full support
DzmitryFomchyn Jun 16, 2023
255ffa3
Code review
DzmitryFomchyn Jun 21, 2023
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
93 changes: 93 additions & 0 deletions samples/src/main/java/com/mapbox/samples/BasicV6BatchGeocoding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package com.mapbox.samples;

import com.mapbox.api.geocoding.v6.MapboxV6BatchGeocoding;
import com.mapbox.api.geocoding.v6.V6RequestOptions;
import com.mapbox.api.geocoding.v6.V6StructuredInputQuery;
import com.mapbox.api.geocoding.v6.V6ForwardGeocodingRequestOptions;
import com.mapbox.api.geocoding.v6.models.V6BatchResponse;
import com.mapbox.api.geocoding.v6.models.V6Feature;
import com.mapbox.api.geocoding.v6.models.V6FeatureType;
import com.mapbox.api.geocoding.v6.models.V6Response;
import com.mapbox.api.geocoding.v6.V6ReverseGeocodingRequestOptions;
import com.mapbox.geojson.BoundingBox;
import com.mapbox.geojson.Point;
import com.mapbox.sample.BuildConfig;

import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;

import retrofit2.Call;
import retrofit2.Callback;
import retrofit2.Response;

public class BasicV6BatchGeocoding {

private static List<V6RequestOptions> requestOptions() {
final V6ForwardGeocodingRequestOptions forwardOptions = V6ForwardGeocodingRequestOptions
.builder("1600 Pennsylvania Avenue NW, Washington, DC 20500, United States")
.types(V6FeatureType.ADDRESS)
.limit(1)
.boundingBox(BoundingBox.fromLngLats(-80, 35, -70, 40))
.build();

final V6StructuredInputQuery structuredInputQuery = V6StructuredInputQuery.builder()
.addressNumber("1600")
.street("Pennsylvania Avenue NW")
.postcode("20500")
.place("Washington, DC")
.build();

final V6ForwardGeocodingRequestOptions structuredInputOptions = V6ForwardGeocodingRequestOptions
.builder(structuredInputQuery)
.country("us")
.build();

final V6ReverseGeocodingRequestOptions reverseOptions = V6ReverseGeocodingRequestOptions
.builder(Point.fromLngLat(-73.986136, 40.748895))
.types(V6FeatureType.ADDRESS)
.build();

return Arrays.asList(forwardOptions, structuredInputOptions, reverseOptions);
}

public static void main(String[] args) {
final MapboxV6BatchGeocoding geocoding = MapboxV6BatchGeocoding
.builder(
BuildConfig.MAPBOX_ACCESS_TOKEN,
requestOptions()
)
.build();

geocoding.enqueueCall(new Callback<V6BatchResponse>() {
@Override
public void onResponse(Call<V6BatchResponse> call, Response<V6BatchResponse> response) {
System.out.println("Response code: " + response.code());
System.out.println("Response message: " + response.message());

final V6BatchResponse body = response.body();
if (body == null) {
System.out.println("Response body is null");
return;
}

System.out.println("Number of responses: " + body.responses().size());

for (V6Response v6Response : body.responses()) {
System.out.println("Number of results: " + v6Response.features().size());

final String results = v6Response.features().stream()
.map(V6Feature::toString)
.collect(Collectors.joining(", \n"));

System.out.println("Features: " + results);
}
}

@Override
public void onFailure(Call<V6BatchResponse> call, Throwable t) {
System.out.println(t);
}
});
}
}
52 changes: 52 additions & 0 deletions samples/src/main/java/com/mapbox/samples/BasicV6ForwardGeocoding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package com.mapbox.samples;

import com.mapbox.api.geocoding.v6.MapboxV6Geocoding;
import com.mapbox.api.geocoding.v6.V6ForwardGeocodingRequestOptions;
import com.mapbox.api.geocoding.v6.models.V6Response;
import com.mapbox.api.geocoding.v6.models.V6Feature;
import com.mapbox.api.geocoding.v6.models.V6FeatureType;
import com.mapbox.sample.BuildConfig;

import java.util.stream.Collectors;

import retrofit2.Call;
import retrofit2.Callback;
import retrofit2.Response;

public class BasicV6ForwardGeocoding {

public static void main(String[] args) {
final V6ForwardGeocodingRequestOptions requestOptions = V6ForwardGeocodingRequestOptions
.builder("740 15th St NW, Washington, DC 20005")
.types(V6FeatureType.ADDRESS)
.build();

final MapboxV6Geocoding geocoding = MapboxV6Geocoding
.builder(BuildConfig.MAPBOX_ACCESS_TOKEN, requestOptions)
.build();

geocoding.enqueueCall(new Callback<V6Response>() {
@Override
public void onResponse(Call<V6Response> call, Response<V6Response> response) {
final V6Response body = response.body();
if (body == null) {
System.out.println("Response body is null");
return;
}

System.out.println("Number of results: " + body.features().size());

final String results = body.features().stream()
.map(V6Feature::toString)
.collect(Collectors.joining(", \n"));

System.out.println("Features: " + results);
}

@Override
public void onFailure(Call<V6Response> call, Throwable t) {
System.out.println(t);
}
});
}
}
51 changes: 51 additions & 0 deletions samples/src/main/java/com/mapbox/samples/BasicV6ReverseGeocoding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package com.mapbox.samples;

import com.mapbox.api.geocoding.v6.MapboxV6Geocoding;
import com.mapbox.api.geocoding.v6.models.V6Feature;
import com.mapbox.api.geocoding.v6.models.V6Response;
import com.mapbox.api.geocoding.v6.V6ReverseGeocodingRequestOptions;
import com.mapbox.geojson.Point;
import com.mapbox.sample.BuildConfig;

import java.util.stream.Collectors;

import retrofit2.Call;
import retrofit2.Callback;
import retrofit2.Response;

public class BasicV6ReverseGeocoding {

public static void main(String[] args) {
final V6ReverseGeocodingRequestOptions requestOptions = V6ReverseGeocodingRequestOptions
.builder(Point.fromLngLat(-77.03397315668123, 38.89991317162873))
.build();

final MapboxV6Geocoding geocoding = MapboxV6Geocoding
.builder(BuildConfig.MAPBOX_ACCESS_TOKEN, requestOptions)
.build();

geocoding.enqueueCall(new Callback<V6Response>() {
@Override
public void onResponse(Call<V6Response> call, Response<V6Response> response) {
final V6Response body = response.body();
if (body == null) {
System.out.println("Response body is null");
return;
}

System.out.println("Number of results: " + body.features().size());

final String results = body.features().stream()
.map(V6Feature::toString)
.collect(Collectors.joining(", \n"));

System.out.println("Features: " + results);
}

@Override
public void onFailure(Call<V6Response> call, Throwable t) {
System.out.println(t);
}
});
}
}
62 changes: 62 additions & 0 deletions samples/src/main/java/com/mapbox/samples/BasicV6StructuredInputForwardGeocoding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package com.mapbox.samples;

import com.mapbox.api.geocoding.v6.V6StructuredInputQuery;
import com.mapbox.api.geocoding.v6.MapboxV6Geocoding;
import com.mapbox.api.geocoding.v6.V6ForwardGeocodingRequestOptions;
import com.mapbox.api.geocoding.v6.models.V6Feature;
import com.mapbox.api.geocoding.v6.models.V6FeatureType;
import com.mapbox.api.geocoding.v6.models.V6Response;
import com.mapbox.sample.BuildConfig;

import java.util.stream.Collectors;

import retrofit2.Call;
import retrofit2.Callback;
import retrofit2.Response;

public class BasicV6StructuredInputForwardGeocoding {

public static void main(String[] args) {
final V6StructuredInputQuery query = V6StructuredInputQuery.builder()
.addressNumber("740")
.street("15th St")
.place("Washington")
.postcode("20005")
.build();

final V6ForwardGeocodingRequestOptions requestOptions = V6ForwardGeocodingRequestOptions
.builder(query)
.country("United States")
.types(V6FeatureType.ADDRESS)
.autocomplete(false)
.build();

final MapboxV6Geocoding geocoding = MapboxV6Geocoding
.builder(BuildConfig.MAPBOX_ACCESS_TOKEN, requestOptions)
.build();

geocoding.enqueueCall(new Callback<V6Response>() {
@Override
public void onResponse(Call<V6Response> call, Response<V6Response> response) {
final V6Response body = response.body();
if (body == null) {
System.out.println("Response body is null");
return;
}

System.out.println("Number of results: " + body.features().size());

final String results = body.features().stream()
.map(V6Feature::toString)
.collect(Collectors.joining(", \n"));

System.out.println("Features: " + results);
}

@Override
public void onFailure(Call<V6Response> call, Throwable t) {
System.out.println(t);
}
});
}
}
5 changes: 4 additions & 1 deletion ...geocoding/src/main/java/com/mapbox/api/geocoding/v5/SingleElementSafeListTypeAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@
*/
class SingleElementSafeListTypeAdapter<E> extends TypeAdapter<List<E>> {

/**
* A type adapter factory instance for serialization/deserialization.
*/
static final TypeAdapterFactory FACTORY = new TypeAdapterFactory() {

@Override
Expand All @@ -44,7 +47,7 @@ public <T> TypeAdapter<T> create(final Gson gson, final TypeToken<T> typeToken)

@SuppressWarnings("unchecked")
final TypeAdapter<T> adapter =
(TypeAdapter<T>) new SingleElementSafeListTypeAdapter<>(elementTypeAdapter);
(TypeAdapter<T>) new SingleElementSafeListTypeAdapter<>(elementTypeAdapter);
return adapter;
}
};
Expand Down
66 changes: 66 additions & 0 deletions services-geocoding/src/main/java/com/mapbox/api/geocoding/v6/MapboxV6BaseGeocoding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package com.mapbox.api.geocoding.v6;

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;

import com.mapbox.core.MapboxService;

/**
* Base class for entry points to Mapbox V6 geocoding: forward geocoding, reverse geocoding
* and batch geocoding. See derived classes for more information.
* @param <T> response type.
*/
public abstract class MapboxV6BaseGeocoding<T> extends MapboxService<T, V6GeocodingService> {

@dzinad dzinad Jun 20, 2023

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 have multiple options on how to distinguish v6 and v5 classes.

  1. Like you did: v6 package + v6 in the class names;
  2. Just the v6 package;
  3. New major services-geocoding release that would remove all v5 classes (for v5 use prev major release);
  4. maybe sth else?

What I don't like so much about option 1 is having v6 in class names (also in different places: sometimes at the beginning, sometimes in the middle). It just doesn't look right as part of the class name. Maybe it's subjective, idk.
I understand that with option 2 the user will be running into wrong imports, but there are not a lot of conflicting classes, right?
With option 3 it's not very convenient for us, because we'd have to support to 2 major trains and some of the modules are reused, so we can't just bump the major only for geocoding.

So I wonder why you chose this specific approach and which ones you've also considered. Also why not option 2.

@DzmitryFomchyn DzmitryFomchyn Jun 21, 2023

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.

Thanks for raising this question, I was going to discuss it because I don't like this option either.

  1. New major services-geocoding release that would remove all v5 classes (for v5 use prev major release);

v5 is not deprecated and I'm not sure if it's going to be, so it's not an option for now.

  1. Just the v6 package;

I like this option but I had the same concern as you described.

but there are not a lot of conflicting classes, right?

Yes, right. We can leave v6 prefix for entrypoint classes (V6ForwardGeocodingRequestOptions, and MapboxV6BatchGeocoding), and for few more cases like V6Context, V6ContextElement. What do you think?

@dzinad dzinad Jun 22, 2023

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.

v5 is not deprecated and I'm not sure if it's going to be, so it's not an option for now.

I'm not suggesting to deprecate it, rather use a different major of mapbox-java. However, having 2 active major trains is not a very good idea.

Yes, right. We can leave v6 prefix for entrypoint classes (V6ForwardGeocodingRequestOptions, and MapboxV6BatchGeocoding), and for few more cases like V6Context, V6ContextElement. What do you think?

Why these specific classes? What's the criteria? IMO that would be: MapboxV6Geocoding, V6GeocodingService, V6Context, V6ContextElement, V6Feature, V6FeatureType`. These classes have conflicts with v5 and I added ContextElement and FeatureType for consistency (because we have Context and Feature).


@NonNull
protected abstract String accessToken();

@dzinad dzinad Jun 20, 2023

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.

Why are all methods protected, but the base class is public? What are we actually exposing to the user with this approach?

@DzmitryFomchyn DzmitryFomchyn Jun 21, 2023 •
edited
Loading

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.

These fields are supposed to be set by a user via Builder class which is public. We use these fields later in sub classes when we make http request. It's like a public set-function and protected get function, but in this case set function is a builder class.

I think that in general we should keep everything non-public until proven otherwise, so they're with minimal possible visibility level.

@dzinad dzinad Jun 22, 2023 •
edited
Loading

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.

I see your point and it makes sense.
However, I see multiple problems with this approach:

  1. Inconsistency: user created the object but can't "see" it: how will they test it, for example? They'd want something like: "when I receive this input data I create these options".
  2. User can't modify the existing object based on the actual data. You can imagine creating an interceptor that would, for example, check if clientAppName is null and, if it is, set it to some value.


@Nullable
protected abstract Boolean permanent();

@Nullable
protected abstract String clientAppName();

@NonNull
@Override
protected abstract String baseUrl();

protected MapboxV6BaseGeocoding() {
super(V6GeocodingService.class);
}

/**
* Base class for Mapbox V6 Geocoding Builders.
* @param <T> type of Builder
*/
public abstract static class BaseBuilder<T> {

protected abstract T accessToken(@NonNull String accessToken);

/**
* Specify whether you intend to store the results of the query. Backend default is false.
*
* @param permanent specify whether you intend to store the results
* @return this builder for chaining options together
*
* @see <a href="https://docs.mapbox.com/api/search/geocoding-v6/#data-storage">Data storage</a>
*/
public abstract T permanent(@NonNull Boolean permanent);

@dzinad dzinad Jun 20, 2023

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.

Why is the corresponding method in the MapboxV6BaseGeocoding protected and here it's public?

@DzmitryFomchyn DzmitryFomchyn Jun 21, 2023 •
edited
Loading

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.

Builder functions are public because they're supposed to be called by a user. See code example and this comment for more information

@dzinad dzinad Jun 22, 2023

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.

Looks like this thread is the same as #1550 (comment), let's continue there.


/**
* Base package name or other simple string identifier. Used inside the calls user agent header.
*
* @param clientAppName base package name or other simple string identifier
* @return this builder for chaining options together
*/
public abstract T clientAppName(@NonNull String clientAppName);

/**
* Optionally change the APIs base URL to something other then the default Mapbox one.
*
* @param baseUrl base url used as end point
* @return this builder for chaining options together
*/
public abstract T baseUrl(@NonNull String baseUrl);
}
}
Loading