addressing comments

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

@@ -588,8 +588,8 @@ private static class BeanMapper<T> {
     private final HashSet<String> serverTimestamps;
 
     // A set of property names that were annotated with @DocumentId. These properties will be
-    // populated with
-    // document ID values during deserialization, and be skipped during serialization.
+    // populated with document ID values during deserialization, and be skipped during
+    // serialization.
     private final HashSet<String> documentIdPropertyNames;
 
     BeanMapper(Class<T> clazz) {
@@ -648,49 +648,47 @@ private static class BeanMapper<T> {
       do {
         // Add any setters
         for (Method method : currentClass.getDeclaredMethods()) {
-          if (!shouldIncludeSetter(method)) {
-            continue;
-          }
-          String propertyName = propertyName(method);
-          String existingPropertyName = properties.get(propertyName.toLowerCase(Locale.US));
-          if (existingPropertyName != null && !existingPropertyName.equals(propertyName)) {
-            throw new RuntimeException(
-                "Found setter on "
-                    + currentClass.getName()
-                    + " with invalid case-sensitive name: "
-                    + method.getName());
-          }
-
-          Method existingSetter = setters.get(propertyName);
-
-          // Raise exception if a conflict between setters is found.
-          if (existingSetter != null && !isSetterOverride(method, existingSetter)) {
-            if (currentClass == clazz) {
-              // TODO: Should we support overloads?
-              throw new RuntimeException(
-                  "Class "
-                      + clazz.getName()
-                      + " has multiple setter overloads with name "
-                      + method.getName());
-            } else {
-              throw new RuntimeException(
-                  "Found conflicting setters "
-                      + "with name: "
-                      + method.getName()
-                      + " (conflicts with "
-                      + existingSetter.getName()
-                      + " defined on "
-                      + existingSetter.getDeclaringClass().getName()
-                      + ")");
+          if (shouldIncludeSetter(method)) {
+            String propertyName = propertyName(method);
+            String existingPropertyName = properties.get(propertyName.toLowerCase(Locale.US));
+            if (existingPropertyName != null) {
+              if (!existingPropertyName.equals(propertyName)) {
+                throw new RuntimeException(
+                    "Found setter on "
+                        + currentClass.getName()
+                        + " with invalid case-sensitive name: "
+                        + method.getName());
+              } else {
+                Method existingSetter = setters.get(propertyName);
+                if (existingSetter == null) {
+                  method.setAccessible(true);
+                  setters.put(propertyName, method);
+                  applySetterAnnotations(method);
+                } else if (!isSetterOverride(method, existingSetter)) {
+                  // We require that setters with conflicting property names are
+                  // overrides from a base class
+                  if (currentClass == clazz) {
+                    // TODO: Should we support overloads?
+                    throw new RuntimeException(
+                        "Class "
+                            + clazz.getName()
+                            + " has multiple setter overloads with name "
+                            + method.getName());
+                  } else {
+                    throw new RuntimeException(
+                        "Found conflicting setters "
+                            + "with name: "
+                            + method.getName()
+                            + " (conflicts with "
+                            + existingSetter.getName()
+                            + " defined on "
+                            + existingSetter.getDeclaringClass().getName()
+                            + ")");
+                  }
+                }
+              }
             }
           }
-
-          // Make it accessible and process annotations if not yet.
-          if (existingSetter == null) {
-            method.setAccessible(true);
-            setters.put(propertyName, method);
-            applySetterAnnotations(method);
-          }
         }
 
         for (Field field : currentClass.getDeclaredFields()) {
@@ -717,15 +715,14 @@ private static class BeanMapper<T> {
 
       // Make sure we can write to @DocumentId annotated properties before proceeding.
       for (String docIdProperty : documentIdPropertyNames) {
-        if (setters.containsKey(docIdProperty) || fields.containsKey(docIdProperty)) {
-          continue;
+        if (!setters.containsKey(docIdProperty) && !fields.containsKey(docIdProperty)) {
+          throw new RuntimeException(
+              "@DocumentId is annotated on property "
+                  + docIdProperty
+                  + " of class "
+                  + clazz.getName()
+                  + " but no field or public setter was found");
         }
-
-        throw new RuntimeException(
-            "@DocumentId is annotated on property "
-                + docIdProperty
-                + " which provides no way to write to on class "
-                + clazz.getName());
       }
     }
 
@@ -798,10 +795,19 @@ T deserialize(
           }
         }
       }
+      populateDocumentIdProperties(types, context, instance, deserialzedProperties);
+
+      return instance;
+    }
 
-      // Populate @DocumentId annotated fields. If there is a conflict (@DocumentId annotation is
-      // applied to a property that is already deserialized from the firestore document)
-      // a runtime exception will be thrown.
+    // Populate @DocumentId annotated fields. If there is a conflict (@DocumentId annotation is
+    // applied to a property that is already deserialized from the firestore document)
+    // a runtime exception will be thrown.
+    private void populateDocumentIdProperties(
+        Map<TypeVariable<Class<T>>, Type> types,
+        DeserializeContext context,
+        T instance,
+        HashSet<String> deserialzedProperties) {
       for (String docIdPropertyName : documentIdPropertyNames) {
         if (deserialzedProperties.contains(docIdPropertyName)) {
           String message =
@@ -839,7 +845,6 @@ T deserialize(
           }
         }
       }
-      return instance;
     }
 
     private Type resolveType(Type type, Map<TypeVariable<Class<T>>, Type> types) {
@@ -915,14 +920,7 @@ private void applyFieldAnnotations(Field field) {
 
       if (field.isAnnotationPresent(DocumentId.class)) {
         Class<?> fieldType = field.getType();
-        if (fieldType != String.class && fieldType != DocumentReference.class) {
-          throw new IllegalArgumentException(
-              "Field "
-                  + field.getName()
-                  + " is annotated with @DocumentId but is "
-                  + fieldType
-                  + " instead of String or DocumentReference.");
-        }
+        ensureValidDocumentIdType("Field", "is", fieldType);
         documentIdPropertyNames.add(propertyName(field));
       }
     }
@@ -944,14 +942,7 @@ private void applyGetterAnnotations(Method method) {
       // Even though the value will be skipped, we still check for type matching for consistency.
       if (method.isAnnotationPresent(DocumentId.class)) {
         Class<?> returnType = method.getReturnType();
-        if (returnType != String.class && returnType != DocumentReference.class) {
-          throw new IllegalArgumentException(
-              "Method "
-                  + method.getName()
-                  + " is annotated with @DocumentId but returns "
-                  + returnType
-                  + " instead of String or DocumentReference.");
-        }
+        ensureValidDocumentIdType("Method", "returns", returnType);
         documentIdPropertyNames.add(propertyName(method));
       }
     }
@@ -967,18 +958,23 @@ private void applySetterAnnotations(Method method) {
 
       if (method.isAnnotationPresent(DocumentId.class)) {
         Class<?> paramType = method.getParameterTypes()[0];
-        if (paramType != String.class && paramType != DocumentReference.class) {
-          throw new IllegalArgumentException(
-              "Method "
-                  + method.getName()
-                  + " is annotated with @DocumentId but sets "
-                  + paramType
-                  + " instead of String or DocumentReference.");
-        }
+        ensureValidDocumentIdType("Method", "sets", paramType);
         documentIdPropertyNames.add(propertyName(method));
       }
     }
 
+    private void ensureValidDocumentIdType(String fieldDescription, String operation, Type type) {
+      if (type != String.class && type != DocumentReference.class) {
+        throw new IllegalArgumentException(
+            fieldDescription
+                + " is annotated with @DocumentId but "
+                + operation
+                + " "
+                + type
+                + " instead of String or DocumentReference.");
+      }
+    }
+
     private static boolean shouldIncludeGetter(Method method) {
       if (!method.getName().startsWith("get") && !method.getName().startsWith("is")) {
         return false;

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

@@ -907,7 +907,7 @@ public void setValue(String value) {
   }
 
   private static <T> T deserialize(String jsonString, Class<T> clazz) {
-    return deserialize(jsonString, clazz, null);
+    return deserialize(jsonString, clazz, /*docRef=*/ null);
   }
 
   private static <T> T deserialize(String jsonString, Class<T> clazz, DocumentReference docRef) {
@@ -2280,15 +2280,6 @@ private static class FieldWithDocumentIdOnWrongTypeBean {
     @DocumentId public Integer intField;
   }
 
-  private static class SetterWithDocumentIdOnWrongTypeBean {
-    private int intField = 100;
-
-    @DocumentId
-    public void setIntField(int value) {
-      intField = value;
-    }
-  }
-
   private static class GetterWithDocumentIdOnWrongTypeBean {
     private int intField = 100;
 
@@ -2306,32 +2297,23 @@ private static class PropertyWithDocumentIdOnWrongTypeBean {
 
   @Test
   public void documentIdAnnotateWrongTypeThrows() {
+    final String expectedErrorMessage = "instead of String or DocumentReference";
     assertExceptionContains(
-        "instead of String or DocumentReference",
-        () -> serialize(new FieldWithDocumentIdOnWrongTypeBean()));
+        expectedErrorMessage, () -> serialize(new FieldWithDocumentIdOnWrongTypeBean()));
     assertExceptionContains(
-        "instead of String or DocumentReference",
+        expectedErrorMessage,
         () -> deserialize("{'intField': 1}", FieldWithDocumentIdOnWrongTypeBean.class));
 
     assertExceptionContains(
-        "instead of String or DocumentReference",
-        () -> serialize(new SetterWithDocumentIdOnWrongTypeBean()));
-    assertExceptionContains(
-        "instead of String or DocumentReference",
-        () -> deserialize("{'intField': 1}", SetterWithDocumentIdOnWrongTypeBean.class));
-
-    assertExceptionContains(
-        "instead of String or DocumentReference",
-        () -> serialize(new GetterWithDocumentIdOnWrongTypeBean()));
+        expectedErrorMessage, () -> serialize(new GetterWithDocumentIdOnWrongTypeBean()));
     assertExceptionContains(
-        "instead of String or DocumentReference",
+        expectedErrorMessage,
         () -> deserialize("{'intField': 1}", GetterWithDocumentIdOnWrongTypeBean.class));
 
     assertExceptionContains(
-        "instead of String or DocumentReference",
-        () -> serialize(new PropertyWithDocumentIdOnWrongTypeBean()));
+        expectedErrorMessage, () -> serialize(new PropertyWithDocumentIdOnWrongTypeBean()));
     assertExceptionContains(
-        "instead of String or DocumentReference",
+        expectedErrorMessage,
         () -> deserialize("{'intField': 1}", PropertyWithDocumentIdOnWrongTypeBean.class));
   }
 
@@ -2344,14 +2326,15 @@ public String getDocId() {
 
   @Test
   public void documentIdAnnotateReadOnlyThrows() {
+    final String expectedErrorMessage = "but no field or public setter was found";
     // Serialization.
     GetterWithoutBackingFieldOnDocumentIdBean bean =
         new GetterWithoutBackingFieldOnDocumentIdBean();
-    assertExceptionContains("provides no way to write", () -> serialize(bean));
+    assertExceptionContains(expectedErrorMessage, () -> serialize(bean));
 
     // Deserialization.
     assertExceptionContains(
-        "provides no way to write",
+        expectedErrorMessage,
         () -> deserialize("{'docId': 'id'}", GetterWithoutBackingFieldOnDocumentIdBean.class));
   }
 
@@ -2427,7 +2410,7 @@ public void documentIdsDeserialize() {
 
   @Test
   public void documentIdsRoundTrip() {
-    // Implicitly verifies @DocumentId is ignore during serialization.
+    // Implicitly verifies @DocumentId is ignored during serialization.
 
     DocumentReference ref = TestUtil.documentReference("coll/doc123");
 
@@ -2453,22 +2436,23 @@ public void documentIdsRoundTrip() {
 
   @Test
   public void documentIdsDeserializeConflictThrows() {
+    final String expectedErrorMessage = "cannot apply @DocumentId on this property";
     DocumentReference ref = TestUtil.documentReference("coll/doc123");
 
     assertExceptionContains(
-        "cannot apply @DocumentId on this property",
+        expectedErrorMessage,
         () -> deserialize("{'docId': 'toBeOverwritten'}", DocumentIdOnStringField.class, ref));
 
     assertExceptionContains(
-        "cannot apply @DocumentId on this property",
+        expectedErrorMessage,
         () ->
             deserialize(
                 "{'docIdProperty': 'toBeOverwritten', 'anotherProperty': 100}",
                 DocumentIdOnStringFieldAsProperty.class,
                 ref));
 
     assertExceptionContains(
-        "cannot apply @DocumentId on this property",
+        expectedErrorMessage,
         () ->
             deserialize(
                 "{'nestedDocIdHolder': {'docId': 'toBeOverwritten'}}",