Skip to content

Properties passed to Feature.fromGeometry should be Nullable #815

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
osana merged 1 commit into from
May 24, 2018
Merged

Properties passed to Feature.fromGeometry should be Nullable #815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 25 additions & 7 deletions services-geocoding/src/main/java/com/mapbox/api/geocoding/v5/models/CarmenFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import com.mapbox.api.geocoding.v5.GeocodingCriteria.GeocodingTypeCriteria;
import com.mapbox.geojson.BoundingBox;
import com.mapbox.geojson.Feature;
import com.mapbox.geojson.GeoJson;
import com.mapbox.geojson.Geometry;
import com.mapbox.geojson.Point;
import com.mapbox.geojson.gson.BoundingBoxDeserializer;
Expand All @@ -24,7 +25,7 @@

/**
* The Features key in the geocoding API response contains the majority of information you'll want
* to use. It extends the {@link Feature} object in GeoJSON and adds several additional attribute
* to use. It extends the {@link GeoJson} object in GeoJSON and adds several additional attribute
* which further describe the geocoding result.
* <p>
* A Geocoding id is a String in the form {@code {type}.{id}} where {@code {type}} is the lowest
Expand All @@ -41,7 +42,7 @@
* @since 1.0.0
*/
@AutoValue
public abstract class CarmenFeature implements Serializable {
public abstract class CarmenFeature implements GeoJson, Serializable {

private static final String TYPE = "Feature";

Expand All @@ -60,7 +61,12 @@ public static CarmenFeature fromJson(@NonNull String json) {
.registerTypeAdapter(BoundingBox.class, new BoundingBoxDeserializer())
.registerTypeAdapterFactory(GeocodingAdapterFactory.create())
.create();
return gson.fromJson(json, CarmenFeature.class);
CarmenFeature feature = gson.fromJson(json, CarmenFeature.class);
// Even thought properties are Nullable,
// Feature object will be created with properties set to an empty object,
return feature.properties() == null
? feature.toBuilder().properties(new JsonObject()).build()
: feature;
}

/**
Expand All @@ -72,12 +78,14 @@ public static CarmenFeature fromJson(@NonNull String json) {
@NonNull
public static Builder builder() {
return new AutoValue_CarmenFeature.Builder()
.type(TYPE);
.type(TYPE)
.properties(new JsonObject());
}

//
// Feature specific attributes
//
// Note that CarmenFeature cannot extend Feature due to AutoValue limitations

/**
* This describes the TYPE of GeoJson geometry this object is, thus this will always return
Expand All @@ -89,6 +97,7 @@ public static Builder builder() {
*/
@NonNull
@SerializedName("type")
@Override
public abstract String type();

/**
Expand All @@ -102,6 +111,7 @@ public static Builder builder() {
* @since 3.0.0
*/
@Nullable
@Override
public abstract BoundingBox bbox();

/**
Expand Down Expand Up @@ -132,7 +142,7 @@ public static Builder builder() {
* @return a {@link JsonObject} which holds this features current properties
* @since 1.0.0
*/
@NonNull
@Nullable
public abstract JsonObject properties();

//
Expand Down Expand Up @@ -282,14 +292,22 @@ public static TypeAdapter<CarmenFeature> typeAdapter(Gson gson) {
* @return a JSON string which represents this CarmenFeature
* @since 3.0.0
*/
@Override
@SuppressWarnings("unused")
public String toJson() {
Gson gson = new GsonBuilder()
.registerTypeAdapter(Geometry.class, new GeometryTypeAdapter())
.registerTypeAdapter(BoundingBox.class, new BoundingBoxSerializer())
.registerTypeAdapterFactory(GeocodingAdapterFactory.create())
.create();
return gson.toJson(this, CarmenFeature.class);

// Empty properties -> should not appear in json string
CarmenFeature feature = this;
if (properties() != null && properties().size() == 0) {
feature = toBuilder().properties(null).build();
}

return gson.toJson(feature, CarmenFeature.class);
}

/**
Expand Down Expand Up @@ -355,7 +373,7 @@ public abstract static class Builder {
* @return this builder for chaining options together
* @since 3.0.0
*/
public abstract Builder properties(@NonNull JsonObject properties);
public abstract Builder properties(@Nullable JsonObject properties);

/**
* A string representing the feature in the requested language.
Expand Down
30 changes: 30 additions & 0 deletions services-geocoding/src/test/java/com/mapbox/api/geocoding/v5/models/CarmenFeatureTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,24 @@
import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.core.IsEqual.equalTo;
import static org.hamcrest.junit.MatcherAssert.assertThat;
import static org.junit.Assert.assertFalse;

import com.google.gson.JsonObject;
import com.mapbox.api.geocoding.v5.GeocodingTestUtils;
import com.mapbox.api.geocoding.v5.MapboxGeocoding;
import com.mapbox.core.TestUtils;
import com.mapbox.geojson.CoordinateContainer;
import com.mapbox.geojson.Feature;
import com.mapbox.geojson.LineString;
import com.mapbox.geojson.Point;

import org.junit.Assert;
import org.junit.Test;
import retrofit2.Response;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Locale;

public class CarmenFeatureTest extends GeocodingTestUtils {
Expand Down Expand Up @@ -149,4 +156,27 @@ public void ForwardGeocode_withValidChineseResponse() throws Exception {
assertThat(feature.bbox().east(), equalTo(107.025123596));
assertThat(feature.bbox().north(), equalTo(39.6012458800001));
}

@Test
public void testNullProperties() {
CarmenFeature feature = CarmenFeature.builder()
.geometry(Point.fromLngLat(-77, 38))
.build();
String jsonString = feature.toJson();
assertFalse(jsonString.contains("\"properties\":{}"));

// Feature (empty Properties) -> Json (null Properties) -> Equavalent Feature
CarmenFeature featureFromJson = CarmenFeature.fromJson(jsonString);
assertEquals(featureFromJson, feature);
}

@Test
public void testNullPropertiesJson() {
String jsonString = "{\"type\":\"Feature\",\"geometry\":{\"type\":\"Point\",\"coordinates\":[-77.0, 38.0]}}";
CarmenFeature feature = CarmenFeature.fromJson(jsonString);

// Json( null Properties) -> Feature (empty Properties) -> Json(null Properties)
String fromFeatureJsonString = feature.toJson();
assertEquals(fromFeatureJsonString, jsonString);
}
}
2 changes: 1 addition & 1 deletion services-geocoding/src/test/resources/forward_feature_valid.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"id":"address.3982178573139850","type":"Feature","place_type":["address"],"relevance":1,"properties":{},"text":"Pennsylvania Ave NW","place_name":"1600 Pennsylvania Ave NW, Washington, District of Columbia 20006, United States","center":[-77.036543,38.897702],"geometry":{"type":"Point","coordinates":[-77.036543,38.897702]},"address":"1600","context":[{"id":"neighborhood.291451","text":"Downtown"},{"id":"postcode.8031694603652840","text":"20006"},{"id":"place.11387590027246050","wikidata":"Q61","text":"Washington"},{"id":"region.3403","short_code":"US-DC","wikidata":"Q61","text":"District of Columbia"},{"id":"country.3145","short_code":"us","wikidata":"Q30","text":"United States"}]}
{"id":"address.3982178573139850","type":"Feature","place_type":["address"],"relevance":1,"text":"Pennsylvania Ave NW","place_name":"1600 Pennsylvania Ave NW, Washington, District of Columbia 20006, United States","center":[-77.036543,38.897702],"geometry":{"type":"Point","coordinates":[-77.036543,38.897702]},"address":"1600","context":[{"id":"neighborhood.291451","text":"Downtown"},{"id":"postcode.8031694603652840","text":"20006"},{"id":"place.11387590027246050","wikidata":"Q61","text":"Washington"},{"id":"region.3403","short_code":"US-DC","wikidata":"Q61","text":"District of Columbia"},{"id":"country.3145","short_code":"us","wikidata":"Q30","text":"United States"}]}
35 changes: 27 additions & 8 deletions services-geojson/src/main/java/com/mapbox/geojson/Feature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,16 @@ public static Feature fromJson(@NonNull String json) {
gson.registerTypeAdapter(Point.class, new PointDeserializer());
gson.registerTypeAdapter(BoundingBox.class, new BoundingBoxDeserializer());
gson.registerTypeAdapter(Geometry.class, new GeometryDeserializer());
return gson.create().fromJson(json, Feature.class);
Feature feature = gson.create().fromJson(json, Feature.class);

// Even thought properties are Nullable,
// Feature object will be created with properties set to an empty object,
// so that addProperties() would work
if (feature.properties() != null) {
return feature;
}
return new AutoValue_Feature(TYPE, feature.bbox(),
feature.id(), feature.geometry(), new JsonObject());
}

/**
Expand Down Expand Up @@ -105,8 +114,9 @@ public static Feature fromGeometry(@Nullable Geometry geometry, @Nullable Boundi
* method
* @since 1.0.0
*/
public static Feature fromGeometry(@Nullable Geometry geometry, @NonNull JsonObject properties) {
return new AutoValue_Feature(TYPE, null, null, geometry, properties);
public static Feature fromGeometry(@Nullable Geometry geometry, @Nullable JsonObject properties) {
return new AutoValue_Feature(TYPE, null, null, geometry,
properties == null ? new JsonObject() : properties);
}

/**
Expand All @@ -120,9 +130,10 @@ public static Feature fromGeometry(@Nullable Geometry geometry, @NonNull JsonObj
* method
* @since 1.0.0
*/
public static Feature fromGeometry(@Nullable Geometry geometry, @NonNull JsonObject properties,
public static Feature fromGeometry(@Nullable Geometry geometry, @Nullable JsonObject properties,
@Nullable BoundingBox bbox) {
return new AutoValue_Feature(TYPE, bbox, null, geometry, properties);
return new AutoValue_Feature(TYPE, bbox, null, geometry,
properties == null ? new JsonObject() : properties);
}

/**
Expand All @@ -135,9 +146,10 @@ public static Feature fromGeometry(@Nullable Geometry geometry, @NonNull JsonObj
* @return {@link Feature}
* @since 1.0.0
*/
public static Feature fromGeometry(@Nullable Geometry geometry, @NonNull JsonObject properties,
public static Feature fromGeometry(@Nullable Geometry geometry, @Nullable JsonObject properties,
@Nullable String id) {
return new AutoValue_Feature(TYPE, null, id, geometry, properties);
return new AutoValue_Feature(TYPE, null, id, geometry,
properties == null ? new JsonObject() : properties);
}

/**
Expand Down Expand Up @@ -225,7 +237,14 @@ public String toJson() {
GsonBuilder gson = new GsonBuilder();
gson.registerTypeAdapter(Point.class, new PointSerializer());
gson.registerTypeAdapter(BoundingBox.class, new BoundingBoxSerializer());
return gson.create().toJson(this);

// Empty properties -> should not appear in json string
Feature feature = this;
if (properties().size() == 0) {
feature = new AutoValue_Feature(TYPE, bbox(), id(), geometry(), null);
}

return gson.create().toJson(feature);
}

/**
Expand Down
29 changes: 25 additions & 4 deletions services-geojson/src/test/java/com/mapbox/geojson/FeatureTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.mapbox.geojson;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
Expand Down Expand Up @@ -47,7 +48,7 @@ public void bbox_doesNotSerializeWhenNotPresent() throws Exception {
Feature feature = Feature.fromGeometry(lineString);
compareJson(feature.toJson(),
"{\"type\":\"Feature\",\"geometry\":{\"type\":"
+ "\"LineString\",\"coordinates\":[[1,2],[2,3]]},\"properties\":{}}");
+ "\"LineString\",\"coordinates\":[[1,2],[2,3]]}}");
}

@Test
Expand Down Expand Up @@ -76,7 +77,7 @@ public void bbox_doesSerializeWhenPresent() throws Exception {
BoundingBox bbox = BoundingBox.fromLngLats(1.0, 2.0, 3.0, 4.0);
Feature feature = Feature.fromGeometry(lineString, bbox);
compareJson("{\"type\":\"Feature\",\"bbox\":[1.0,2.0,3.0,4.0],\"geometry\":"
+ "{\"type\":\"LineString\",\"coordinates\":[[1,2],[2,3]]},\"properties\":{}}",
+ "{\"type\":\"LineString\",\"coordinates\":[[1,2],[2,3]]}}",
feature.toJson());
}

Expand All @@ -103,7 +104,12 @@ public void testNullProperties() {
coordinates.add(Point.fromLngLat(4.5, 6.7));
LineString line = LineString.fromLngLats(coordinates);
Feature feature = Feature.fromGeometry(line);
assertTrue(feature.toJson().contains("\"properties\":{}"));
String jsonString = feature.toJson();
assertFalse(jsonString.contains("\"properties\":{}"));

// Feature (empty Properties) -> Json (null Properties) -> Equavalent Feature
Feature featureFromJson = Feature.fromJson(jsonString);
assertEquals(featureFromJson, feature);
}

@Test
Expand All @@ -115,6 +121,21 @@ public void testNonNullProperties() {
JsonObject properties = new JsonObject();
properties.addProperty("key", "value");
Feature feature = Feature.fromGeometry(line, properties);
assertTrue(feature.toJson().contains("\"properties\":{\"key\":\"value\"}"));
String jsonString = feature.toJson();
assertTrue(jsonString.contains("\"properties\":{\"key\":\"value\"}"));

// Feature (non-empty Properties) -> Json (non-empty Properties) -> Equavalent Feature
assertEquals(Feature.fromJson(jsonString), feature);
}

@Test
public void testNullPropertiesJson() {
String jsonString = "{\"type\":\"Feature\",\"bbox\":[1.0,2.0,3.0,4.0],\"geometry\":"
+ "{\"type\":\"LineString\",\"coordinates\":[[1.0,2.0],[2.0,3.0]]}}";
Feature feature = Feature.fromJson(jsonString);

// Json( null Properties) -> Feature (empty Properties) -> Json(null Properties)
String fromFeatureJsonString = feature.toJson();
assertEquals(fromFeatureJsonString, jsonString);
}
}