Better POJO Support for Document Fields

* Allow passing POJOs as field values throughout API.
  * As values in a Map<> passed to set().
  * As values to a update() call.
  * As elements in an array transform operation.
  * As values in query field filters.
* Removed set(Map<>) overloads since they are now redundant with set(Object).
* Added `T get(..., Class<T>, ...)` overloads for extracting fields as a custom
  class (POJO) type.

Author: mikelehen

firebase-firestore/CHANGELOG.md

@@ -1,4 +1,10 @@
 # Unreleased
+- [feature] Custom objects (POJOs) can now be passed as a field value in
+  update(), within `Map<>` objects passed to set(), in array transform
+  operations, and in query filters.
+- [feature] DocumentSnapshot.get() now supports retrieving fields as
+  custom objects (POJOs) by passing a Class<T> instance, e.g.
+  `snapshot.get("field", CustomType.class)`.
 
 # 17.1.2
 - [changed] Changed the internal handling for locally updated documents that

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

@@ -15,13 +15,18 @@
 package com.google.firebase.firestore;
 
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testCollection;
+import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testDocument;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.waitFor;
 import static com.google.firebase.firestore.testutil.TestUtil.expectError;
+import static com.google.firebase.firestore.testutil.TestUtil.map;
 import static junit.framework.Assert.assertEquals;
 
 import android.support.test.runner.AndroidJUnit4;
+import com.google.android.gms.tasks.Task;
+import com.google.android.gms.tasks.Tasks;
 import com.google.firebase.Timestamp;
 import com.google.firebase.firestore.testutil.IntegrationTestUtil;
+import java.util.ArrayList;
 import java.util.Date;
 import org.junit.After;
 import org.junit.Test;
@@ -175,7 +180,7 @@ public void testWriteAndRead() {
   }
 
   @Test
-  public void testUpdate() {
+  public void testSetMerge() {
     CollectionReference collection = testCollection();
     POJO data = new POJO(1.0, "a", collection.document());
     DocumentReference reference = waitFor(collection.add(data));
@@ -190,6 +195,55 @@ public void testUpdate() {
     assertEquals(expected, doc.toObject(POJO.class));
   }
 
+  // General smoke test that makes sure APIs accept POJOs.
+  @Test
+  public void testAPIsAcceptPOJOsForFields() {
+    DocumentReference ref = testDocument();
+    ArrayList<Task<?>> tasks = new ArrayList<>();
+
+    // as Map<> entries in a set() call.
+    POJO data = new POJO(1.0, "a", ref);
+    tasks.add(ref.set(map("a", data, "b", map("c", data))));
+
+    // as Map<> entries in an update() call.
+    tasks.add(ref.update(map("a", data)));
+
+    // as field values in an update() call.
+    tasks.add(ref.update("c", data));
+
+    // as values in arrayUnion() / arrayRemove().
+    tasks.add(ref.update("c", FieldValue.arrayUnion(data)));
+    tasks.add(ref.update("c", FieldValue.arrayRemove(data)));
+
+    // as Query parameters.
+    data.setBlob(null); // blobs are broken, see b/117680212
+    tasks.add(testCollection().whereEqualTo("field", data).get());
+
+    waitFor(Tasks.whenAll(tasks));
+  }
+
+  @Test
+  public void testDocumentSnapshotGetWithPOJOs() {
+    DocumentReference ref = testDocument();
+
+    // Go offline so that we can verify server timestamp behavior overload.
+    ref.getFirestore().disableNetwork();
+
+    POJO pojo = new POJO(1.0, "a", ref);
+    ref.set(map("field", pojo));
+
+    DocumentSnapshot snap = waitFor(ref.get());
+
+    assertEquals(pojo, snap.get("field", POJO.class));
+    assertEquals(pojo, snap.get(FieldPath.of("field"), POJO.class));
+    assertEquals(
+        pojo, snap.get("field", POJO.class, DocumentSnapshot.ServerTimestampBehavior.DEFAULT));
+    assertEquals(
+        pojo,
+        snap.get(
+            FieldPath.of("field"), POJO.class, DocumentSnapshot.ServerTimestampBehavior.DEFAULT));
+  }
+
   @Test
   public void setFieldMaskMustHaveCorrespondingValue() {
     CollectionReference collection = testCollection();

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

@@ -248,6 +248,28 @@ public void testServerTimestampsUsesPreviousValueFromLocalMutation() {
     assertThat(remoteSnapshot.get("a")).isInstanceOf(Timestamp.class);
   }
 
+  @Test
+  public void testServerTimestampBehaviorOverloadsOfDocumentSnapshotGet() {
+    writeInitialData();
+    waitFor(docRef.update(updateData));
+    DocumentSnapshot snap = accumulator.awaitLocalEvent();
+
+    // Default behavior should return null timestamp (via any overload).
+    assertNull(snap.get("when"));
+    assertNull(snap.get(FieldPath.of("when")));
+    assertNull(snap.get("when", Timestamp.class));
+    assertNull(snap.get(FieldPath.of("when"), Timestamp.class));
+
+    // Estimate should return a Timestamp object (via any overload).
+    assertThat(snap.get("when", ServerTimestampBehavior.ESTIMATE)).isInstanceOf(Timestamp.class);
+    assertThat(snap.get(FieldPath.of("when"), ServerTimestampBehavior.ESTIMATE))
+        .isInstanceOf(Timestamp.class);
+    assertThat(snap.get("when", Timestamp.class, ServerTimestampBehavior.ESTIMATE))
+        .isInstanceOf(Timestamp.class);
+    assertThat(snap.get(FieldPath.of("when"), Timestamp.class, ServerTimestampBehavior.ESTIMATE))
+        .isInstanceOf(Timestamp.class);
+  }
+
   @Test
   public void testServerTimestampsWorkViaTransactionSet() {
     waitFor(

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

@@ -257,22 +257,16 @@ public void writesMustNotContainReservedFieldNames() {
 
   @Test
   public void setsMustNotContainFieldValueDelete() {
-    // PORTING NOTE: We avoid using expectSetError(), since it hits the POJO overload which
-    // can't handle FieldValue.delete().
-    DocumentReference ref = testDocument();
-    expectError(
-        () -> ref.set(map("foo", FieldValue.delete())),
+    expectSetError(
+        map("foo", FieldValue.delete()),
         "Invalid data. FieldValue.delete() can only be used with update() and set() with "
             + "SetOptions.merge() (found in field foo)");
   }
 
   @Test
   public void updatesMustNotContainNestedFieldValueDeletes() {
-    // PORTING NOTE: We avoid using expectSetError(), since it hits the POJO overload which
-    // can't handle FieldValue.delete().
-    DocumentReference ref = testDocument();
-    expectError(
-        () -> ref.update(map("foo", map("bar", FieldValue.delete()))),
+    expectUpdateError(
+        map("foo", map("bar", FieldValue.delete())),
         "Invalid data. FieldValue.delete() can only appear at the top level of your update data "
             + "(found in field foo.bar)");
   }
@@ -376,8 +370,8 @@ public void arrayTransformsFailInQueries() {
   @Test
   public void arrayTransformsRejectInvalidElements() {
     DocumentReference doc = testDocument();
-    String reason = "Invalid data. Unsupported type: com.google.firebase.firestore.ValidationTest";
-    // TODO: If we get more permissive with POJOs, perhaps we should make this work.
+    String reason =
+        "No properties to serialize found on class com.google.firebase.firestore.ValidationTest";
     expectError(() -> doc.set(map("x", FieldValue.arrayUnion(1, this))), reason);
     expectError(() -> doc.set(map("x", FieldValue.arrayRemove(1, this))), reason);
   }
@@ -558,15 +552,8 @@ private static void expectWriteError(
     DocumentReference ref = testDocument();
 
     if (includeSets) {
-      if (data instanceof Map) {
-        @SuppressWarnings("unchecked")
-        Map<String, Object> setMap = (Map<String, Object>) data;
-        expectError(() -> ref.set(setMap), reason);
-        expectError(() -> ref.getFirestore().batch().set(ref, setMap), reason);
-      } else {
-        expectError(() -> ref.set(data), reason);
-        expectError(() -> ref.getFirestore().batch().set(ref, data), reason);
-      }
+      expectError(() -> ref.set(data), reason);
+      expectError(() -> ref.getFirestore().batch().set(ref, data), reason);
     }
 
     if (includeUpdates) {
@@ -593,13 +580,7 @@ private static void expectWriteError(
                 (Function<Void>)
                     transaction -> {
                       if (includeSets) {
-                        if (data instanceof Map) {
-                          @SuppressWarnings("unchecked")
-                          Map<String, Object> setMap = (Map<String, Object>) data;
-                          expectError(() -> transaction.set(ref, setMap), reason);
-                        } else {
-                          expectError(() -> transaction.set(ref, data), reason);
-                        }
+                        expectError(() -> transaction.set(ref, data), reason);
                       }
                       if (includeUpdates) {
                         assertTrue("update() only support Maps.", data instanceof Map);

firebase-firestore/src/main/java/com/google/firebase/firestore/CollectionReference.java

@@ -23,7 +23,6 @@
 import com.google.firebase.firestore.model.ResourcePath;
 import com.google.firebase.firestore.util.Executors;
 import com.google.firebase.firestore.util.Util;
-import java.util.Map;
 import javax.annotation.Nullable;
 
 /**
@@ -117,12 +116,13 @@ public DocumentReference document(@NonNull String documentPath) {
    * Adds a new document to this collection with the specified data, assigning it a document ID
    * automatically.
    *
-   * @param data A Map containing the data for the new document.
+   * @param data The data to write to the document (e.g. a Map or a POJO containing the desired
+   *     document contents).
    * @return A Task that will be resolved with the DocumentReference of the newly created document.
    */
   @NonNull
   @PublicApi
-  public Task<DocumentReference> add(@NonNull Map<String, Object> data) {
+  public Task<DocumentReference> add(@NonNull Object data) {
     checkNotNull(data, "Provided data must not be null.");
     final DocumentReference ref = document();
     return ref.set(data)
@@ -134,17 +134,4 @@ public Task<DocumentReference> add(@NonNull Map<String, Object> data) {
               return ref;
             });
   }
-
-  /**
-   * Adds a new document to this collection with the specified POJO as contents, assigning it a
-   * document ID automatically.
-   *
-   * @param pojo The POJO that will be used to populate the contents of the document
-   * @return A Task that will be resolved with the DocumentReference of the newly created document.
-   */
-  @NonNull
-  @PublicApi
-  public Task<DocumentReference> add(@NonNull Object pojo) {
-    return add(firestore.getDataConverter().convertPOJO(pojo));
-  }
 }

firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java

@@ -143,12 +143,13 @@ public CollectionReference collection(@NonNull String collectionPath) {
    * Overwrites the document referred to by this DocumentReference. If the document does not yet
    * exist, it will be created. If a document already exists, it will be overwritten.
    *
-   * @param data A map of the fields and values for the document.
+   * @param data The data to write to the document (e.g. a Map or a POJO containing the desired
+   *     document contents).
    * @return A Task that will be resolved when the write finishes.
    */
   @NonNull
   @PublicApi
-  public Task<Void> set(@NonNull Map<String, Object> data) {
+  public Task<Void> set(@NonNull Object data) {
     return set(data, SetOptions.OVERWRITE);
   }
 
@@ -157,13 +158,14 @@ public Task<Void> set(@NonNull Map<String, Object> data) {
    * exist, it will be created. If you pass {@link SetOptions}, the provided data can be merged into
    * an existing document.
    *
-   * @param data A map of the fields and values for the document.
+   * @param data The data to write to the document (e.g. a Map or a POJO containing the desired
+   *     document contents).
    * @param options An object to configure the set behavior.
    * @return A Task that will be resolved when the write finishes.
    */
   @NonNull
   @PublicApi
-  public Task<Void> set(@NonNull Map<String, Object> data, @NonNull SetOptions options) {
+  public Task<Void> set(@NonNull Object data, @NonNull SetOptions options) {
     checkNotNull(data, "Provided data must not be null.");
     checkNotNull(options, "Provided options must not be null.");
     ParsedSetData parsed =
@@ -176,34 +178,6 @@ public Task<Void> set(@NonNull Map<String, Object> data, @NonNull SetOptions opt
         .continueWith(Executors.DIRECT_EXECUTOR, voidErrorTransformer());
   }
 
-  /**
-   * Overwrites the document referred to by this DocumentReference. If the document does not yet
-   * exist, it will be created. If a document already exists, it will be overwritten.
-   *
-   * @param pojo The POJO that will be used to populate the document contents
-   * @return A Task that will be resolved when the write finishes.
-   */
-  @NonNull
-  @PublicApi
-  public Task<Void> set(@NonNull Object pojo) {
-    return set(firestore.getDataConverter().convertPOJO(pojo), SetOptions.OVERWRITE);
-  }
-
-  /**
-   * Writes to the document referred to by this DocumentReference. If the document does not yet
-   * exist, it will be created. If you pass {@link SetOptions}, the provided data can be merged into
-   * an existing document.
-   *
-   * @param pojo The POJO that will be used to populate the document contents
-   * @param options An object to configure the set behavior.
-   * @return A Task that will be resolved when the write finishes.
-   */
-  @NonNull
-  @PublicApi
-  public Task<Void> set(@NonNull Object pojo, @NonNull SetOptions options) {
-    return set(firestore.getDataConverter().convertPOJO(pojo), options);
-  }
-
   /**
    * Updates fields in the document referred to by this DocumentReference. If no document exists
    * yet, the update will fail.

firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java

@@ -289,6 +289,69 @@ public Object get(
             firestore.getFirestoreSettings().areTimestampsInSnapshotsEnabled()));
   }
 
+  /**
+   * Returns the value at the field, converted to a POJO, or null if the field or document doesn't
+   * exist.
+   *
+   * @param field The path to the field
+   * @param valueType The Java class to convert the field value to.
+   * @return The value at the given field or null.
+   */
+  @Nullable
+  public <T> T get(@NonNull String field, @NonNull Class<T> valueType) {
+    return get(FieldPath.fromDotSeparatedPath(field), valueType, ServerTimestampBehavior.DEFAULT);
+  }
+
+  /**
+   * Returns the value at the field, converted to a POJO, or null if the field or document doesn't
+   * exist.
+   *
+   * @param field The path to the field
+   * @param valueType The Java class to convert the field value to.
+   * @param serverTimestampBehavior Configures the behavior for server timestamps that have not yet
+   *     been set to their final value.
+   * @return The value at the given field or null.
+   */
+  @Nullable
+  public <T> T get(
+      @NonNull String field,
+      @NonNull Class<T> valueType,
+      @NonNull ServerTimestampBehavior serverTimestampBehavior) {
+    return get(FieldPath.fromDotSeparatedPath(field), valueType, serverTimestampBehavior);
+  }
+
+  /**
+   * Returns the value at the field, converted to a POJO, or null if the field or document doesn't
+   * exist.
+   *
+   * @param fieldPath The path to the field
+   * @param valueType The Java class to convert the field value to.
+   * @return The value at the given field or null.
+   */
+  @Nullable
+  public <T> T get(@NonNull FieldPath fieldPath, @NonNull Class<T> valueType) {
+    return get(fieldPath, valueType, ServerTimestampBehavior.DEFAULT);
+  }
+
+  /**
+   * Returns the value at the field, converted to a POJO, or null if the field or document doesn't
+   * exist.
+   *
+   * @param fieldPath The path to the field
+   * @param valueType The Java class to convert the field value to.
+   * @param serverTimestampBehavior Configures the behavior for server timestamps that have not yet
+   *     been set to their final value.
+   * @return The value at the given field or null.
+   */
+  @Nullable
+  public <T> T get(
+      @NonNull FieldPath fieldPath,
+      @NonNull Class<T> valueType,
+      @NonNull ServerTimestampBehavior serverTimestampBehavior) {
+    Object data = get(fieldPath, serverTimestampBehavior);
+    return data == null ? null : CustomClassMapper.convertToCustomClass(data, valueType);
+  }
+
   /**
    * Returns the value of the field as a boolean. If the value is not a boolean this will throw a
    * runtime exception.