Skip to content

Commit 554ede5

Browse files
wu-huiwu-hui
committed
addressing comments
1 parent a5d91bb commit 554ede5

File tree

2 files changed

+91
-111
lines changed

2 files changed

+91
-111
lines changed

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

Lines changed: 75 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -588,8 +588,8 @@ private static class BeanMapper<T> {
588588
private final HashSet<String> serverTimestamps;
589589

590590
// A set of property names that were annotated with @DocumentId. These properties will be
591-
// populated with
592-
// document ID values during deserialization, and be skipped during serialization.
591+
// populated with document ID values during deserialization, and be skipped during
592+
// serialization.
593593
private final HashSet<String> documentIdPropertyNames;
594594

595595
BeanMapper(Class<T> clazz) {
@@ -648,49 +648,47 @@ private static class BeanMapper<T> {
648648
do {
649649
// Add any setters
650650
for (Method method : currentClass.getDeclaredMethods()) {
651-
if (!shouldIncludeSetter(method)) {
652-
continue;
653-
}
654-
String propertyName = propertyName(method);
655-
String existingPropertyName = properties.get(propertyName.toLowerCase(Locale.US));
656-
if (existingPropertyName != null && !existingPropertyName.equals(propertyName)) {
657-
throw new RuntimeException(
658-
"Found setter on "
659-
+ currentClass.getName()
660-
+ " with invalid case-sensitive name: "
661-
+ method.getName());
662-
}
663-
664-
Method existingSetter = setters.get(propertyName);
665-
666-
// Raise exception if a conflict between setters is found.
667-
if (existingSetter != null && !isSetterOverride(method, existingSetter)) {
668-
if (currentClass == clazz) {
669-
// TODO: Should we support overloads?
670-
throw new RuntimeException(
671-
"Class "
672-
+ clazz.getName()
673-
+ " has multiple setter overloads with name "
674-
+ method.getName());
675-
} else {
676-
throw new RuntimeException(
677-
"Found conflicting setters "
678-
+ "with name: "
679-
+ method.getName()
680-
+ " (conflicts with "
681-
+ existingSetter.getName()
682-
+ " defined on "
683-
+ existingSetter.getDeclaringClass().getName()
684-
+ ")");
651+
if (shouldIncludeSetter(method)) {
652+
String propertyName = propertyName(method);
653+
String existingPropertyName = properties.get(propertyName.toLowerCase(Locale.US));
654+
if (existingPropertyName != null) {
655+
if (!existingPropertyName.equals(propertyName)) {
656+
throw new RuntimeException(
657+
"Found setter on "
658+
+ currentClass.getName()
659+
+ " with invalid case-sensitive name: "
660+
+ method.getName());
661+
} else {
662+
Method existingSetter = setters.get(propertyName);
663+
if (existingSetter == null) {
664+
method.setAccessible(true);
665+
setters.put(propertyName, method);
666+
applySetterAnnotations(method);
667+
} else if (!isSetterOverride(method, existingSetter)) {
668+
// We require that setters with conflicting property names are
669+
// overrides from a base class
670+
if (currentClass == clazz) {
671+
// TODO: Should we support overloads?
672+
throw new RuntimeException(
673+
"Class "
674+
+ clazz.getName()
675+
+ " has multiple setter overloads with name "
676+
+ method.getName());
677+
} else {
678+
throw new RuntimeException(
679+
"Found conflicting setters "
680+
+ "with name: "
681+
+ method.getName()
682+
+ " (conflicts with "
683+
+ existingSetter.getName()
684+
+ " defined on "
685+
+ existingSetter.getDeclaringClass().getName()
686+
+ ")");
687+
}
688+
}
689+
}
685690
}
686691
}
687-
688-
// Make it accessible and process annotations if not yet.
689-
if (existingSetter == null) {
690-
method.setAccessible(true);
691-
setters.put(propertyName, method);
692-
applySetterAnnotations(method);
693-
}
694692
}
695693

696694
for (Field field : currentClass.getDeclaredFields()) {
@@ -717,15 +715,14 @@ private static class BeanMapper<T> {
717715

718716
// Make sure we can write to @DocumentId annotated properties before proceeding.
719717
for (String docIdProperty : documentIdPropertyNames) {
720-
if (setters.containsKey(docIdProperty) || fields.containsKey(docIdProperty)) {
721-
continue;
718+
if (!setters.containsKey(docIdProperty) && !fields.containsKey(docIdProperty)) {
719+
throw new RuntimeException(
720+
"@DocumentId is annotated on property "
721+
+ docIdProperty
722+
+ " of class "
723+
+ clazz.getName()
724+
+ " but no field or public setter was found");
722725
}
723-
724-
throw new RuntimeException(
725-
"@DocumentId is annotated on property "
726-
+ docIdProperty
727-
+ " which provides no way to write to on class "
728-
+ clazz.getName());
729726
}
730727
}
731728

@@ -798,10 +795,19 @@ T deserialize(
798795
}
799796
}
800797
}
798+
populateDocumentIdProperties(types, context, instance, deserialzedProperties);
799+
800+
return instance;
801+
}
801802

802-
// Populate @DocumentId annotated fields. If there is a conflict (@DocumentId annotation is
803-
// applied to a property that is already deserialized from the firestore document)
804-
// a runtime exception will be thrown.
803+
// Populate @DocumentId annotated fields. If there is a conflict (@DocumentId annotation is
804+
// applied to a property that is already deserialized from the firestore document)
805+
// a runtime exception will be thrown.
806+
private void populateDocumentIdProperties(
807+
Map<TypeVariable<Class<T>>, Type> types,
808+
DeserializeContext context,
809+
T instance,
810+
HashSet<String> deserialzedProperties) {
805811
for (String docIdPropertyName : documentIdPropertyNames) {
806812
if (deserialzedProperties.contains(docIdPropertyName)) {
807813
String message =
@@ -839,7 +845,6 @@ T deserialize(
839845
}
840846
}
841847
}
842-
return instance;
843848
}
844849

845850
private Type resolveType(Type type, Map<TypeVariable<Class<T>>, Type> types) {
@@ -915,14 +920,7 @@ private void applyFieldAnnotations(Field field) {
915920

916921
if (field.isAnnotationPresent(DocumentId.class)) {
917922
Class<?> fieldType = field.getType();
918-
if (fieldType != String.class && fieldType != DocumentReference.class) {
919-
throw new IllegalArgumentException(
920-
"Field "
921-
+ field.getName()
922-
+ " is annotated with @DocumentId but is "
923-
+ fieldType
924-
+ " instead of String or DocumentReference.");
925-
}
923+
ensureValidDocumentIdType("Field", "is", fieldType);
926924
documentIdPropertyNames.add(propertyName(field));
927925
}
928926
}
@@ -944,14 +942,7 @@ private void applyGetterAnnotations(Method method) {
944942
// Even though the value will be skipped, we still check for type matching for consistency.
945943
if (method.isAnnotationPresent(DocumentId.class)) {
946944
Class<?> returnType = method.getReturnType();
947-
if (returnType != String.class && returnType != DocumentReference.class) {
948-
throw new IllegalArgumentException(
949-
"Method "
950-
+ method.getName()
951-
+ " is annotated with @DocumentId but returns "
952-
+ returnType
953-
+ " instead of String or DocumentReference.");
954-
}
945+
ensureValidDocumentIdType("Method", "returns", returnType);
955946
documentIdPropertyNames.add(propertyName(method));
956947
}
957948
}
@@ -967,18 +958,23 @@ private void applySetterAnnotations(Method method) {
967958

968959
if (method.isAnnotationPresent(DocumentId.class)) {
969960
Class<?> paramType = method.getParameterTypes()[0];
970-
if (paramType != String.class && paramType != DocumentReference.class) {
971-
throw new IllegalArgumentException(
972-
"Method "
973-
+ method.getName()
974-
+ " is annotated with @DocumentId but sets "
975-
+ paramType
976-
+ " instead of String or DocumentReference.");
977-
}
961+
ensureValidDocumentIdType("Method", "sets", paramType);
978962
documentIdPropertyNames.add(propertyName(method));
979963
}
980964
}
981965

966+
private void ensureValidDocumentIdType(String fieldDescription, String operation, Type type) {
967+
if (type != String.class && type != DocumentReference.class) {
968+
throw new IllegalArgumentException(
969+
fieldDescription
970+
+ " is annotated with @DocumentId but "
971+
+ operation
972+
+ " "
973+
+ type
974+
+ " instead of String or DocumentReference.");
975+
}
976+
}
977+
982978
private static boolean shouldIncludeGetter(Method method) {
983979
if (!method.getName().startsWith("get") && !method.getName().startsWith("is")) {
984980
return false;

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

Lines changed: 16 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -907,7 +907,7 @@ public void setValue(String value) {
907907
}
908908

909909
private static <T> T deserialize(String jsonString, Class<T> clazz) {
910-
return deserialize(jsonString, clazz, null);
910+
return deserialize(jsonString, clazz, /*docRef=*/ null);
911911
}
912912

913913
private static <T> T deserialize(String jsonString, Class<T> clazz, DocumentReference docRef) {
@@ -2280,15 +2280,6 @@ private static class FieldWithDocumentIdOnWrongTypeBean {
22802280
@DocumentId public Integer intField;
22812281
}
22822282

2283-
private static class SetterWithDocumentIdOnWrongTypeBean {
2284-
private int intField = 100;
2285-
2286-
@DocumentId
2287-
public void setIntField(int value) {
2288-
intField = value;
2289-
}
2290-
}
2291-
22922283
private static class GetterWithDocumentIdOnWrongTypeBean {
22932284
private int intField = 100;
22942285

@@ -2306,32 +2297,23 @@ private static class PropertyWithDocumentIdOnWrongTypeBean {
23062297

23072298
@Test
23082299
public void documentIdAnnotateWrongTypeThrows() {
2300+
final String expectedErrorMessage = "instead of String or DocumentReference";
23092301
assertExceptionContains(
2310-
"instead of String or DocumentReference",
2311-
() -> serialize(new FieldWithDocumentIdOnWrongTypeBean()));
2302+
expectedErrorMessage, () -> serialize(new FieldWithDocumentIdOnWrongTypeBean()));
23122303
assertExceptionContains(
2313-
"instead of String or DocumentReference",
2304+
expectedErrorMessage,
23142305
() -> deserialize("{'intField': 1}", FieldWithDocumentIdOnWrongTypeBean.class));
23152306

23162307
assertExceptionContains(
2317-
"instead of String or DocumentReference",
2318-
() -> serialize(new SetterWithDocumentIdOnWrongTypeBean()));
2319-
assertExceptionContains(
2320-
"instead of String or DocumentReference",
2321-
() -> deserialize("{'intField': 1}", SetterWithDocumentIdOnWrongTypeBean.class));
2322-
2323-
assertExceptionContains(
2324-
"instead of String or DocumentReference",
2325-
() -> serialize(new GetterWithDocumentIdOnWrongTypeBean()));
2308+
expectedErrorMessage, () -> serialize(new GetterWithDocumentIdOnWrongTypeBean()));
23262309
assertExceptionContains(
2327-
"instead of String or DocumentReference",
2310+
expectedErrorMessage,
23282311
() -> deserialize("{'intField': 1}", GetterWithDocumentIdOnWrongTypeBean.class));
23292312

23302313
assertExceptionContains(
2331-
"instead of String or DocumentReference",
2332-
() -> serialize(new PropertyWithDocumentIdOnWrongTypeBean()));
2314+
expectedErrorMessage, () -> serialize(new PropertyWithDocumentIdOnWrongTypeBean()));
23332315
assertExceptionContains(
2334-
"instead of String or DocumentReference",
2316+
expectedErrorMessage,
23352317
() -> deserialize("{'intField': 1}", PropertyWithDocumentIdOnWrongTypeBean.class));
23362318
}
23372319

@@ -2344,14 +2326,15 @@ public String getDocId() {
23442326

23452327
@Test
23462328
public void documentIdAnnotateReadOnlyThrows() {
2329+
final String expectedErrorMessage = "but no field or public setter was found";
23472330
// Serialization.
23482331
GetterWithoutBackingFieldOnDocumentIdBean bean =
23492332
new GetterWithoutBackingFieldOnDocumentIdBean();
2350-
assertExceptionContains("provides no way to write", () -> serialize(bean));
2333+
assertExceptionContains(expectedErrorMessage, () -> serialize(bean));
23512334

23522335
// Deserialization.
23532336
assertExceptionContains(
2354-
"provides no way to write",
2337+
expectedErrorMessage,
23552338
() -> deserialize("{'docId': 'id'}", GetterWithoutBackingFieldOnDocumentIdBean.class));
23562339
}
23572340

@@ -2427,7 +2410,7 @@ public void documentIdsDeserialize() {
24272410

24282411
@Test
24292412
public void documentIdsRoundTrip() {
2430-
// Implicitly verifies @DocumentId is ignore during serialization.
2413+
// Implicitly verifies @DocumentId is ignored during serialization.
24312414

24322415
DocumentReference ref = TestUtil.documentReference("coll/doc123");
24332416

@@ -2453,22 +2436,23 @@ public void documentIdsRoundTrip() {
24532436

24542437
@Test
24552438
public void documentIdsDeserializeConflictThrows() {
2439+
final String expectedErrorMessage = "cannot apply @DocumentId on this property";
24562440
DocumentReference ref = TestUtil.documentReference("coll/doc123");
24572441

24582442
assertExceptionContains(
2459-
"cannot apply @DocumentId on this property",
2443+
expectedErrorMessage,
24602444
() -> deserialize("{'docId': 'toBeOverwritten'}", DocumentIdOnStringField.class, ref));
24612445

24622446
assertExceptionContains(
2463-
"cannot apply @DocumentId on this property",
2447+
expectedErrorMessage,
24642448
() ->
24652449
deserialize(
24662450
"{'docIdProperty': 'toBeOverwritten', 'anotherProperty': 100}",
24672451
DocumentIdOnStringFieldAsProperty.class,
24682452
ref));
24692453

24702454
assertExceptionContains(
2471-
"cannot apply @DocumentId on this property",
2455+
expectedErrorMessage,
24722456
() ->
24732457
deserialize(
24742458
"{'nestedDocIdHolder': {'docId': 'toBeOverwritten'}}",

0 commit comments

Comments
 (0)