From 16c3aa02250f63ba30e4a652defa02426857f822 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 14 Mar 2019 09:44:44 +0800 Subject: [PATCH 1/3] Firestore: Update CustomClassMapper --- .../cloud/firestore/CustomClassMapper.java | 74 +++++++++++-------- 1 file changed, 44 insertions(+), 30 deletions(-) diff --git a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CustomClassMapper.java b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CustomClassMapper.java index 1ba2de65328f..fc9f009e8581 100644 --- a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CustomClassMapper.java +++ b/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 convertToPlainJavaTypes(Map update) * @param clazz The class of the object to convert to * @return The POJO object. */ - public static T convertToCustomClass(Object object, Class clazz) { + static T convertToCustomClass(Object object, Class clazz) { return deserializeToClass(object, clazz, ErrorPath.EMPTY); } - protected static Object serialize(T o) { + static Object serialize(T o) { return serialize(o, ErrorPath.EMPTY); } @@ -112,22 +112,21 @@ private static 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 result = new HashMap<>(); for (Map.Entry entry : ((Map) o).entrySet()) { @@ -155,7 +154,13 @@ private static 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 deserializeToClass(Object o, Class 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 deserializeToPrimitive(Object o, Class 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 deserializeToEnum(Object object, Class clazz, ErrorPath String value = (String) object; // We cast to Class without generics here since we can't prove the bound // T extends Enum 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 BeanMapper loadOrCreateBeanMapperForClass(Class clazz) @SuppressWarnings("unchecked") private static Map expectMap(Object object, ErrorPath path) { if (object instanceof Map) { - // TODO(dimond): runtime validation of keys? + // TODO: runtime validation of keys? return (Map) object; } else { throw deserializeError( @@ -508,12 +522,12 @@ private static T convertBean(Object o, Class 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 { // A list of any properties that were annotated with @ServerTimestamp. private final HashSet serverTimestamps; - public BeanMapper(Class clazz) { + BeanMapper(Class clazz) { this.clazz = clazz; throwOnUnknownProperties = clazz.isAnnotationPresent(ThrowOnExtraProperties.class); warnOnUnknownProperties = !clazz.isAnnotationPresent(IgnoreExtraProperties.class); @@ -614,7 +628,7 @@ public BeanMapper(Class 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 values, ErrorPath path) { + T deserialize(Map values, ErrorPath path) { return deserialize(values, Collections.>, Type>emptyMap(), path); } - public T deserialize( + T deserialize( Map values, Map>, Type> types, ErrorPath path) { if (constructor == null) { throw deserializeError( @@ -746,7 +760,7 @@ private Type resolveType(Type type, Map>, Type> types) { } } - public Map serialize(T object, ErrorPath path) { + Map 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); } From 7e3482f3f45281358dc1afba02f7cec56fba2f72 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 14 Mar 2019 19:10:55 +0800 Subject: [PATCH 2/3] Adding Unit tests --- .../firestore/DocumentReferenceTest.java | 66 +++++++++++++++++++ .../cloud/firestore/LocalFirestoreHelper.java | 36 ++++++++++ 2 files changed, 102 insertions(+) diff --git a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/DocumentReferenceTest.java b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/DocumentReferenceTest.java index a07dab86f326..10572a2c405a 100644 --- a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/DocumentReferenceTest.java +++ b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/DocumentReferenceTest.java @@ -65,6 +65,7 @@ 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; @@ -72,12 +73,14 @@ 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; @@ -165,6 +168,38 @@ public void serializeDocumentReference() throws Exception { assertCommitEquals(commit(set(documentReferenceFields)), commitCapture.getValue()); } + @Test + public void doesNotSerializeAdvancedNumberTypes() { + Map 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 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)) @@ -263,6 +298,37 @@ public void deserializesDates() throws Exception { assertEquals(TIMESTAMP, snapshot.getData().get("timestampValue")); } + @Test + public void doesNotDeserializeAdvancedNumberTypes() throws Exception { + Map fieldNamesToTypeNames = + map("bigIntegerValue", "BigInteger", "shortValue", "Short", "byteValue", "Byte"); + + for (Entry testCase : fieldNamesToTypeNames.entrySet()) { + String fieldName = testCase.getKey(); + String typeName = testCase.getValue(); + Map response = map(fieldName, Value.newBuilder().setIntegerValue(0).build()); + + doAnswer(getAllResponse(response)) + .when(firestoreMock) + .streamRequest( + getAllCapture.capture(), + streamObserverCapture.capture(), + Matchers.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 = diff --git a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java index 718064bc5feb..31afece8d319 100644 --- a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java +++ b/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,41 @@ 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 Map map(K key, V value, Object... moreKeysAndValues) { Map map = new HashMap<>(); map.put(key, value); From 5f3030feee6a21c27005c2de37c4b4ff6dc255cb Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 15 Mar 2019 09:50:15 +0800 Subject: [PATCH 3/3] Lint fix --- .../java/com/google/cloud/firestore/LocalFirestoreHelper.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java index 31afece8d319..92cad26ce4e8 100644 --- a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java +++ b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java @@ -149,8 +149,7 @@ class Inner { } public static class InvalidPOJO { - @Nullable - BigInteger bigIntegerValue = null; + @Nullable BigInteger bigIntegerValue = null; @Nullable Byte byteValue = null; @Nullable Short shortValue = null; @@ -182,7 +181,6 @@ public void setShortValue(@Nullable Short shortValue) { } } - public static Map map(K key, V value, Object... moreKeysAndValues) { Map map = new HashMap<>(); map.put(key, value);