Reject non-standard Number classes during POJO serialization

firebase-database/CHANGELOG.md

@@ -2,6 +2,8 @@
 - [changed] Added `@RestrictTo` annotations to discourage the use of APIs that
   are not public. This affects internal APIs that were previously obfuscated
   and are not mentioned in our documentation.
+- [changed] Improved error messages for certain Number types that are not
+  supported by our serialization layer (#272).
 
 # 16.0.6  
 - [fixed] Fixed an issue that could cause a NullPointerException during the

firebase-database/src/main/java/com/google/firebase/database/core/utilities/encoding/CustomClassMapper.java

@@ -118,13 +118,13 @@ private static <T> Object serialize(T o) {
           return ((Number) o).longValue();
         }
         return doubleValue;
-      } else if (o instanceof Short) {
-        throw new DatabaseException("Shorts are not supported, please use int or long");
-      } else if (o instanceof Byte) {
-        throw new DatabaseException("Bytes are not supported, please use int or long");
-      } else {
-        // Long, Integer
+      } else if (o instanceof Long || o instanceof Integer) {
         return o;
+      } else {
+        throw new DatabaseException(
+            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;
@@ -278,14 +278,9 @@ private static <T> T deserializeToPrimitive(Object o, Class<T> clazz) {
       return (T) convertLong(o);
     } else if (Float.class.isAssignableFrom(clazz) || float.class.isAssignableFrom(clazz)) {
       return (T) (Float) convertDouble(o).floatValue();
-    } else if (Short.class.isAssignableFrom(clazz) || short.class.isAssignableFrom(clazz)) {
-      throw new DatabaseException("Deserializing to shorts is not supported");
-    } else if (Byte.class.isAssignableFrom(clazz) || byte.class.isAssignableFrom(clazz)) {
-      throw new DatabaseException("Deserializing to bytes is not supported");
-    } else if (Character.class.isAssignableFrom(clazz) || char.class.isAssignableFrom(clazz)) {
-      throw new DatabaseException("Deserializing to char is not supported");
     } else {
-      throw new IllegalArgumentException("Unknown primitive type: " + clazz);
+      throw new DatabaseException(
+          String.format("Deserializing values to %s is not supported", clazz.getSimpleName()));
     }
   }
 

firebase-firestore/CHANGELOG.md

@@ -11,6 +11,8 @@
 - [changed] Added `@RestrictTo` annotations to discourage the use of APIs that
   are not public. This affects internal APIs that were previously obfuscated
   and are not mentioned in our documentation.
+- [changed] Improved error messages for certain Number types that are not
+  supported by our serialization layer (#272).
 
 # 18.0.1
 - [fixed] Fixed an issue where Firestore would crash if handling write batches

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/POJOTest.java

@@ -26,8 +26,12 @@
 import com.google.android.gms.tasks.Tasks;
 import com.google.firebase.Timestamp;
 import com.google.firebase.firestore.testutil.IntegrationTestUtil;
+import java.math.BigInteger;
 import java.util.ArrayList;
 import java.util.Date;
+import java.util.HashMap;
+import java.util.Map;
+import javax.annotation.Nullable;
 import org.junit.After;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -164,6 +168,39 @@ public int hashCode() {
     }
   }
 
+  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;
+    }
+  }
+
   @After
   public void tearDown() {
     IntegrationTestUtil.tearDown();
@@ -253,4 +290,61 @@ public void setFieldMaskMustHaveCorrespondingValue() {
         () -> reference.set(new POJO(), SetOptions.mergeFields("str", "missing")),
         "Field 'missing' is specified in your field mask but not in your input data.");
   }
+
+  @Test
+  public void testCantWriteNonStandardNumberTypes() {
+    DocumentReference ref = testDocument();
+
+    Map<InvalidPOJO, String> expectedErrorMessages = new HashMap<>();
+
+    InvalidPOJO pojo = new InvalidPOJO();
+    pojo.bigIntegerValue = new BigInteger("0");
+    expectedErrorMessages.put(
+        pojo,
+        "Could not serialize object. BigInteger is 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. Byte is 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. Short is not supported, please use an int, long, float or double (found in field 'shortValue')");
+
+    for (Map.Entry<InvalidPOJO, String> testCase : expectedErrorMessages.entrySet()) {
+      expectError(() -> ref.set(testCase.getKey()), testCase.getValue());
+    }
+  }
+
+  @Test
+  public void testCantReadBigInteger() {
+    DocumentReference ref = testDocument();
+
+    Map<String, Object> invalidData =
+        map(
+            "bigIntegerValue",
+            map("bigIntegerValue", 0),
+            "byteValue",
+            map("byteValue", 0),
+            "shortValue",
+            map("shortValue", 0));
+    waitFor(ref.set(invalidData));
+    DocumentSnapshot snap = waitFor(ref.get());
+
+    expectError(
+        () -> snap.get("bigIntegerValue", InvalidPOJO.class),
+        "Could not deserialize object. Deserializing to BigInteger is not supported (found in field 'bigIntegerValue')");
+
+    expectError(
+        () -> snap.get("byteValue", InvalidPOJO.class),
+        "Could not deserialize object. Deserializing to Byte is not supported (found in field 'byteValue')");
+
+    expectError(
+        () -> snap.get("shortValue", InvalidPOJO.class),
+        "Could not deserialize object. Deserializing to Short is not supported (found in field 'shortValue')");
+  }
 }

firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java

@@ -113,22 +113,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()) {
@@ -313,14 +312,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()));
     }
   }
 

firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java

@@ -1442,7 +1442,9 @@ public void serializeBooleanBean() {
   public void serializeFloatBean() {
     FloatBean bean = new FloatBean();
     bean.value = 0.5f;
-    assertJson("{'value': 0.5}", serialize(bean));
+
+    // We don't use assertJson as it converts all floating point numbers to Double.
+    assertEquals(map("value", 0.5f), serialize(bean));
   }
 
   @Test
@@ -1696,7 +1698,7 @@ public void shortsCantBeSerialized() {
     ShortBean bean = new ShortBean();
     bean.value = 1;
     assertExceptionContains(
-        "Shorts are not supported, please use int or long (found in field 'value')",
+        "Numbers of type Short are not supported, please use an int, long, float or double (found in field 'value')",
         () -> serialize(bean));
   }
 
@@ -1705,7 +1707,7 @@ public void bytesCantBeSerialized() {
     ByteBean bean = new ByteBean();
     bean.value = 1;
     assertExceptionContains(
-        "Bytes are not supported, please use int or long (found in field 'value')",
+        "Numbers of type Byte are not supported, please use an int, long, float or double (found in field 'value')",
         () -> serialize(bean));
   }
 
@@ -1714,7 +1716,7 @@ public void charsCantBeSerialized() {
     CharBean bean = new CharBean();
     bean.value = 1;
     assertExceptionContains(
-        "Characters are not supported, please use Strings. (found in field 'value')",
+        "Characters are not supported, please use Strings (found in field 'value')",
         () -> serialize(bean));
   }
 
@@ -1741,21 +1743,21 @@ public void objectArraysCantBeSerialized() {
   @Test
   public void shortsCantBeDeserialized() {
     assertExceptionContains(
-        "Deserializing to shorts is not supported (found in field 'value')",
+        "Deserializing values to short is not supported (found in field 'value')",
         () -> deserialize("{'value': 1}", ShortBean.class));
   }
 
   @Test
   public void bytesCantBeDeserialized() {
     assertExceptionContains(
-        "Deserializing to bytes is not supported (found in field 'value')",
+        "Deserializing values to byte is not supported (found in field 'value')",
         () -> deserialize("{'value': 1}", ByteBean.class));
   }
 
   @Test
   public void charsCantBeDeserialized() {
     assertExceptionContains(
-        "Deserializing to chars is not supported (found in field 'value')",
+        "Deserializing values to char is not supported (found in field 'value')",
         () -> deserialize("{'value': '1'}", CharBean.class));
   }
 
@@ -1862,21 +1864,21 @@ public void passingInMapTopLevelThrows() {
   @Test
   public void passingInCharacterTopLevelThrows() {
     assertExceptionContains(
-        "Deserializing to chars is not supported",
+        "Deserializing values to Character is not supported",
         () -> CustomClassMapper.convertToCustomClass('1', Character.class));
   }
 
   @Test
   public void passingInShortTopLevelThrows() {
     assertExceptionContains(
-        "Deserializing to shorts is not supported",
+        "Deserializing values to Short is not supported",
         () -> CustomClassMapper.convertToCustomClass(1, Short.class));
   }
 
   @Test
   public void passingInByteTopLevelThrows() {
     assertExceptionContains(
-        "Deserializing to bytes is not supported",
+        "Deserializing values to Byte is not supported",
         () -> CustomClassMapper.convertToCustomClass(1, Byte.class));
   }
 
@@ -2220,8 +2222,8 @@ public void serializationFailureIncludesPath() {
       fail("should have thrown");
     } catch (RuntimeException e) {
       assertEquals(
-          "Could not serialize object. Shorts are not supported, please use int or "
-              + "long (found in field 'value.inner.value.short')",
+          "Could not serialize object. Numbers of type Short are not supported, please use an int, "
+              + "long, float or double (found in field 'value.inner.value.short')",
           e.getMessage());
     }
   }
@@ -2235,7 +2237,7 @@ public void deserializationFailureIncludesPath() {
       fail("should have thrown");
     } catch (RuntimeException e) {
       assertEquals(
-          "Could not deserialize object. Deserializing to shorts is not supported "
+          "Could not deserialize object. Deserializing values to short is not supported "
               + "(found in field 'value')",
           e.getMessage());
     }