Firestore: Update CustomClassMapper (#4675)

* Firestore: Update CustomClassMapper

* Adding Unit tests

* Lint fix

google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CustomClassMapper.java

@@ -72,7 +72,7 @@ private static void hardAssert(boolean assertion, String message) {
    * @param object The representation of the JSON data
    * @return JSON representation containing only standard library Java types
    */
-  public static Object convertToPlainJavaTypes(Object object) {
+  static Object convertToPlainJavaTypes(Object object) {
     return serialize(object);
   }
 
@@ -92,11 +92,11 @@ public static Map<String, Object> convertToPlainJavaTypes(Map<?, Object> update)
    * @param clazz The class of the object to convert to
    * @return The POJO object.
    */
-  public static <T> T convertToCustomClass(Object object, Class<T> clazz) {
+  static <T> T convertToCustomClass(Object object, Class<T> clazz) {
     return deserializeToClass(object, clazz, ErrorPath.EMPTY);
   }
 
-  protected static <T> Object serialize(T o) {
+  static <T> Object serialize(T o) {
     return serialize(o, ErrorPath.EMPTY);
   }
 
@@ -112,22 +112,21 @@ private static <T> Object serialize(T o, ErrorPath path) {
     if (o == null) {
       return null;
     } else if (o instanceof Number) {
-      if (o instanceof Float) {
-        return ((Float) o).doubleValue();
-      } else if (o instanceof Short) {
-        throw serializeError(path, "Shorts are not supported, please use int or long");
-      } else if (o instanceof Byte) {
-        throw serializeError(path, "Bytes are not supported, please use int or long");
-      } else {
-        // Long, Integer, Double
+      if (o instanceof Long || o instanceof Integer || o instanceof Double || o instanceof Float) {
         return o;
+      } else {
+        throw serializeError(
+            path,
+            String.format(
+                "Numbers of type %s are not supported, please use an int, long, float or double",
+                o.getClass().getSimpleName()));
       }
     } else if (o instanceof String) {
       return o;
     } else if (o instanceof Boolean) {
       return o;
     } else if (o instanceof Character) {
-      throw serializeError(path, "Characters are not supported, please use Strings.");
+      throw serializeError(path, "Characters are not supported, please use Strings");
     } else if (o instanceof Map) {
       Map<String, Object> result = new HashMap<>();
       for (Map.Entry<Object, Object> entry : ((Map<Object, Object>) o).entrySet()) {
@@ -155,7 +154,13 @@ private static <T> Object serialize(T o, ErrorPath path) {
     } else if (o.getClass().isArray()) {
       throw serializeError(path, "Serializing Arrays is not supported, please use Lists instead");
     } else if (o instanceof Enum) {
-      return ((Enum<?>) o).name();
+      String enumName = ((Enum<?>) o).name();
+      try {
+        Field enumField = o.getClass().getField(enumName);
+        return BeanMapper.propertyName(enumField);
+      } catch (NoSuchFieldException ex) {
+        return enumName;
+      }
     } else if (o instanceof Date
         || o instanceof Timestamp
         || o instanceof GeoPoint
@@ -212,7 +217,7 @@ private static <T> T deserializeToClass(Object o, Class<T> clazz, ErrorPath path
         || Number.class.isAssignableFrom(clazz)
         || Boolean.class.isAssignableFrom(clazz)
         || Character.class.isAssignableFrom(clazz)) {
-      return (T) deserializeToPrimitive(o, clazz, path);
+      return deserializeToPrimitive(o, clazz, path);
     } else if (String.class.isAssignableFrom(clazz)) {
       return (T) convertString(o, path);
     } else if (Date.class.isAssignableFrom(clazz)) {
@@ -306,14 +311,10 @@ private static <T> T deserializeToPrimitive(Object o, Class<T> clazz, ErrorPath
       return (T) convertLong(o, path);
     } else if (Float.class.isAssignableFrom(clazz) || float.class.isAssignableFrom(clazz)) {
       return (T) (Float) convertDouble(o, path).floatValue();
-    } else if (Short.class.isAssignableFrom(clazz) || short.class.isAssignableFrom(clazz)) {
-      throw deserializeError(path, "Deserializing to shorts is not supported");
-    } else if (Byte.class.isAssignableFrom(clazz) || byte.class.isAssignableFrom(clazz)) {
-      throw deserializeError(path, "Deserializing to bytes is not supported");
-    } else if (Character.class.isAssignableFrom(clazz) || char.class.isAssignableFrom(clazz)) {
-      throw deserializeError(path, "Deserializing to chars is not supported");
     } else {
-      throw new IllegalArgumentException("Unknown primitive type: " + clazz);
+      throw deserializeError(
+          path,
+          String.format("Deserializing values to %s is not supported", clazz.getSimpleName()));
     }
   }
 
@@ -323,6 +324,19 @@ private static <T> T deserializeToEnum(Object object, Class<T> clazz, ErrorPath
       String value = (String) object;
       // We cast to Class without generics here since we can't prove the bound
       // T extends Enum<T> statically
+
+      // try to use PropertyName if exist
+      Field[] enumFields = clazz.getFields();
+      for (Field field : enumFields) {
+        if (field.isEnumConstant()) {
+          String propertyName = BeanMapper.propertyName(field);
+          if (value.equals(propertyName)) {
+            value = field.getName();
+            break;
+          }
+        }
+      }
+
       try {
         return (T) Enum.valueOf((Class) clazz, value);
       } catch (IllegalArgumentException e) {
@@ -355,7 +369,7 @@ private static <T> BeanMapper<T> loadOrCreateBeanMapperForClass(Class<T> clazz)
   @SuppressWarnings("unchecked")
   private static Map<String, Object> expectMap(Object object, ErrorPath path) {
     if (object instanceof Map) {
-      // TODO(dimond): runtime validation of keys?
+      // TODO: runtime validation of keys?
       return (Map<String, Object>) object;
     } else {
       throw deserializeError(
@@ -508,12 +522,12 @@ private static <T> T convertBean(Object o, Class<T> clazz, ErrorPath path) {
     }
   }
 
-  private static RuntimeException serializeError(ErrorPath path, String reason) {
+  private static IllegalArgumentException serializeError(ErrorPath path, String reason) {
     reason = "Could not serialize object. " + reason;
     if (path.getLength() > 0) {
       reason = reason + " (found in field '" + path.toString() + "')";
     }
-    return new RuntimeException(reason);
+    return new IllegalArgumentException(reason);
   }
 
   private static RuntimeException deserializeError(ErrorPath path, String reason) {
@@ -539,7 +553,7 @@ private static class BeanMapper<T> {
     // A list of any properties that were annotated with @ServerTimestamp.
     private final HashSet<String> serverTimestamps;
 
-    public BeanMapper(Class<T> clazz) {
+    BeanMapper(Class<T> clazz) {
       this.clazz = clazz;
       throwOnUnknownProperties = clazz.isAnnotationPresent(ThrowOnExtraProperties.class);
       warnOnUnknownProperties = !clazz.isAnnotationPresent(IgnoreExtraProperties.class);
@@ -614,7 +628,7 @@ public BeanMapper(Class<T> clazz) {
                   // We require that setters with conflicting property names are
                   // overrides from a base class
                   if (currentClass == clazz) {
-                    // TODO(mikelehen): Should we support overloads?
+                    // TODO: Should we support overloads?
                     throw new RuntimeException(
                         "Class "
                             + clazz.getName()
@@ -670,11 +684,11 @@ private void addProperty(String property) {
       }
     }
 
-    public T deserialize(Map<String, Object> values, ErrorPath path) {
+    T deserialize(Map<String, Object> values, ErrorPath path) {
       return deserialize(values, Collections.<TypeVariable<Class<T>>, Type>emptyMap(), path);
     }
 
-    public T deserialize(
+    T deserialize(
         Map<String, Object> values, Map<TypeVariable<Class<T>>, Type> types, ErrorPath path) {
       if (constructor == null) {
         throw deserializeError(
@@ -746,7 +760,7 @@ private Type resolveType(Type type, Map<TypeVariable<Class<T>>, Type> types) {
       }
     }
 
-    public Map<String, Object> serialize(T object, ErrorPath path) {
+    Map<String, Object> serialize(T object, ErrorPath path) {
       if (!clazz.isAssignableFrom(object.getClass())) {
         throw new IllegalArgumentException(
             "Can't serialize object of class "
@@ -992,7 +1006,7 @@ int getLength() {
       return length;
     }
 
-    public ErrorPath child(String name) {
+    ErrorPath child(String name) {
       return new ErrorPath(this, name, length + 1);
     }
 

google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/DocumentReferenceTest.java

@@ -66,19 +66,22 @@
 import com.google.api.gax.rpc.ServerStreamingCallable;
 import com.google.api.gax.rpc.UnaryCallable;
 import com.google.cloud.Timestamp;
+import com.google.cloud.firestore.LocalFirestoreHelper.InvalidPOJO;
 import com.google.cloud.firestore.spi.v1.FirestoreRpc;
 import com.google.common.collect.ImmutableList;
 import com.google.firestore.v1.BatchGetDocumentsRequest;
 import com.google.firestore.v1.BatchGetDocumentsResponse;
 import com.google.firestore.v1.CommitRequest;
 import com.google.firestore.v1.CommitResponse;
 import com.google.firestore.v1.Value;
+import java.math.BigInteger;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -166,6 +169,38 @@ public void serializeDocumentReference() throws Exception {
     assertCommitEquals(commit(set(documentReferenceFields)), commitCapture.getValue());
   }
 
+  @Test
+  public void doesNotSerializeAdvancedNumberTypes() {
+    Map<InvalidPOJO, String> expectedErrorMessages = new HashMap<>();
+
+    InvalidPOJO pojo = new InvalidPOJO();
+    pojo.bigIntegerValue = new BigInteger("0");
+    expectedErrorMessages.put(
+        pojo,
+        "Could not serialize object. Numbers of type BigInteger are not supported, please use an int, long, float or double (found in field 'bigIntegerValue')");
+
+    pojo = new InvalidPOJO();
+    pojo.byteValue = 0;
+    expectedErrorMessages.put(
+        pojo,
+        "Could not serialize object. Numbers of type Byte are not supported, please use an int, long, float or double (found in field 'byteValue')");
+
+    pojo = new InvalidPOJO();
+    pojo.shortValue = 0;
+    expectedErrorMessages.put(
+        pojo,
+        "Could not serialize object. Numbers of type Short are not supported, please use an int, long, float or double (found in field 'shortValue')");
+
+    for (Map.Entry<InvalidPOJO, String> testCase : expectedErrorMessages.entrySet()) {
+      try {
+        documentReference.set(testCase.getKey());
+        fail();
+      } catch (IllegalArgumentException e) {
+        assertEquals(testCase.getValue(), e.getMessage());
+      }
+    }
+  }
+
   @Test
   public void deserializeBasicTypes() throws Exception {
     doAnswer(getAllResponse(ALL_SUPPORTED_TYPES_PROTO))
@@ -264,6 +299,37 @@ public void deserializesDates() throws Exception {
     assertEquals(TIMESTAMP, snapshot.getData().get("timestampValue"));
   }
 
+  @Test
+  public void doesNotDeserializeAdvancedNumberTypes() throws Exception {
+    Map<String, String> fieldNamesToTypeNames =
+        map("bigIntegerValue", "BigInteger", "shortValue", "Short", "byteValue", "Byte");
+
+    for (Entry<String, String> testCase : fieldNamesToTypeNames.entrySet()) {
+      String fieldName = testCase.getKey();
+      String typeName = testCase.getValue();
+      Map<String, Value> response = map(fieldName, Value.newBuilder().setIntegerValue(0).build());
+
+      doAnswer(getAllResponse(response))
+          .when(firestoreMock)
+          .streamRequest(
+              getAllCapture.capture(),
+              streamObserverCapture.capture(),
+              Matchers.<ServerStreamingCallable>any());
+
+      DocumentSnapshot snapshot = documentReference.get().get();
+      try {
+        snapshot.toObject(InvalidPOJO.class);
+        fail();
+      } catch (RuntimeException e) {
+        assertEquals(
+            String.format(
+                "Could not deserialize object. Deserializing values to %s is not supported (found in field '%s')",
+                typeName, fieldName),
+            e.getMessage());
+      }
+    }
+  }
+
   @Test
   public void notFound() throws Exception {
     final BatchGetDocumentsResponse.Builder getDocumentResponse =

google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java

@@ -52,6 +52,7 @@
 import com.google.protobuf.Empty;
 import com.google.protobuf.NullValue;
 import com.google.type.LatLng;
+import java.math.BigInteger;
 import java.nio.charset.Charset;
 import java.text.ParseException;
 import java.text.SimpleDateFormat;
@@ -147,6 +148,39 @@ class Inner {
     }
   }
 
+  public static class InvalidPOJO {
+    @Nullable BigInteger bigIntegerValue = null;
+    @Nullable Byte byteValue = null;
+    @Nullable Short shortValue = null;
+
+    @Nullable
+    public BigInteger getBigIntegerValue() {
+      return bigIntegerValue;
+    }
+
+    public void setBigIntegerValue(@Nullable BigInteger bigIntegerValue) {
+      this.bigIntegerValue = bigIntegerValue;
+    }
+
+    @Nullable
+    public Byte getByteValue() {
+      return byteValue;
+    }
+
+    public void setByteValue(@Nullable Byte byteValue) {
+      this.byteValue = byteValue;
+    }
+
+    @Nullable
+    public Short getShortValue() {
+      return shortValue;
+    }
+
+    public void setShortValue(@Nullable Short shortValue) {
+      this.shortValue = shortValue;
+    }
+  }
+
   public static <K, V> Map<K, V> map(K key, V value, Object... moreKeysAndValues) {
     Map<K, V> map = new HashMap<>();
     map.put(key, value);