Properties passed to Feature.fromGeometry and CarmenFeature.properties should be Nullable [#813] (#815)

Author: osana

services-geocoding/src/main/java/com/mapbox/api/geocoding/v5/models/CarmenFeature.java

@@ -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;
@@ -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
@@ -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";
 
@@ -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;
   }
 
   /**
@@ -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
@@ -89,6 +97,7 @@ public static Builder builder() {
    */
   @NonNull
   @SerializedName("type")
+  @Override
   public abstract String type();
 
   /**
@@ -102,6 +111,7 @@ public static Builder builder() {
    * @since 3.0.0
    */
   @Nullable
+  @Override
   public abstract BoundingBox bbox();
 
   /**
@@ -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();
 
   //
@@ -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);
   }
 
   /**
@@ -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.

services-geocoding/src/test/java/com/mapbox/api/geocoding/v5/models/CarmenFeatureTest.java

@@ -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 {
@@ -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);
+  }
 }

services-geocoding/src/test/resources/forward_feature_valid.json

@@ -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"}]}

services-geojson/src/main/java/com/mapbox/geojson/Feature.java

@@ -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());
   }
 
   /**
@@ -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);
   }
 
   /**
@@ -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);
   }
 
   /**
@@ -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);
   }
 
   /**
@@ -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);
   }
 
   /**

services-geojson/src/test/java/com/mapbox/geojson/FeatureTest.java

@@ -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;
@@ -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
@@ -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());
   }
 
@@ -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
@@ -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);
   }
 }