From 04261f8d0607428396c6245b7af089c1e0ad9e28 Mon Sep 17 00:00:00 2001
From: Rich Gowman <rgowman@google.com>
Date: Thu, 22 Nov 2018 17:08:35 -0500
Subject: [PATCH] Forbid queries endAt an uncommitted server timestamp.

---
 .../firebase/firestore/ValidationTest.java    | 53 +++++++++++++++++++
 .../com/google/firebase/firestore/Query.java  | 13 ++++-
 2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java
index dd9ae258d67..1c526440a2f 100644
--- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java
+++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java
@@ -14,6 +14,8 @@
 
 package com.google.firebase.firestore;
 
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.firebase.firestore.testutil.Assert.assertThrows;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testAlternateFirestore;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testCollection;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testCollectionWithDocs;
@@ -30,6 +32,7 @@
 
 import android.support.test.InstrumentationRegistry;
 import android.support.test.runner.AndroidJUnit4;
+import com.google.android.gms.tasks.TaskCompletionSource;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.FirebaseOptions;
 import com.google.firebase.firestore.Transaction.Function;
@@ -432,6 +435,56 @@ public void queriesCannotBeCreatedFromDocumentsMissingSortValues() {
     expectError(() -> query.endAt(snapshot), reason);
   }
 
+  @Test
+  public void queriesCannotBeSortedByAnUncommittedServerTimestamp() {
+    CollectionReference collection = testCollection();
+
+    // Ensure the server timestamp stays uncommitted for the first half of the test
+    waitFor(collection.firestore.getClient().disableNetwork());
+
+    TaskCompletionSource<Void> offlineCallbackDone = new TaskCompletionSource<>();
+    TaskCompletionSource<Void> onlineCallbackDone = new TaskCompletionSource<>();
+
+    collection.addSnapshotListener(
+        (snapshot, error) -> {
+          assertNotNull(snapshot);
+
+          // Skip the initial empty snapshot.
+          if (snapshot.isEmpty()) return;
+
+          assertThat(snapshot.getDocuments()).hasSize(1);
+          DocumentSnapshot docSnap = snapshot.getDocuments().get(0);
+
+          if (snapshot.getMetadata().hasPendingWrites()) {
+            // Offline snapshot. Since the server timestamp is uncommitted, we shouldn't be able to
+            // query by it.
+            assertThrows(
+                IllegalArgumentException.class,
+                () ->
+                    collection
+                        .orderBy("timestamp")
+                        .endAt(docSnap)
+                        .addSnapshotListener((snapshot2, error2) -> {}));
+            offlineCallbackDone.setResult(null);
+          } else {
+            // Online snapshot. Since the server timestamp is committed, we should be able to query
+            // by it.
+            collection
+                .orderBy("timestamp")
+                .endAt(docSnap)
+                .addSnapshotListener((snapshot2, error2) -> {});
+            onlineCallbackDone.setResult(null);
+          }
+        });
+
+    DocumentReference document = collection.document();
+    document.set(map("timestamp", FieldValue.serverTimestamp()));
+    waitFor(offlineCallbackDone.getTask());
+
+    waitFor(collection.firestore.getClient().enableNetwork());
+    waitFor(onlineCallbackDone.getTask());
+  }
+
   @Test
   public void queriesMustNotHaveMoreComponentsThanOrderBy() {
     CollectionReference collection = testCollection();
diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java
index bb6143edcce..6008bb00f82 100644
--- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java
+++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java
@@ -39,6 +39,7 @@
 import com.google.firebase.firestore.model.ResourcePath;
 import com.google.firebase.firestore.model.value.FieldValue;
 import com.google.firebase.firestore.model.value.ReferenceValue;
+import com.google.firebase.firestore.model.value.ServerTimestampValue;
 import com.google.firebase.firestore.util.ExecutorEventListener;
 import com.google.firebase.firestore.util.Executors;
 import com.google.firebase.firestore.util.ListenerRegistrationImpl;
@@ -582,7 +583,8 @@ public Query endAt(Object... fieldValues) {
    * <p>Note that the Bound will always include the key of the document and so only the provided
    * document will compare equal to the returned position.
    *
-   * <p>Will throw if the document does not contain all fields of the order by of the query.
+   * <p>Will throw if the document does not contain all fields of the order by of the query or if
+   * any of the fields in the order by are an uncommitted server timestamp.
    */
   private Bound boundFromDocumentSnapshot(
       String methodName, DocumentSnapshot snapshot, boolean before) {
@@ -606,7 +608,14 @@ private Bound boundFromDocumentSnapshot(
         components.add(ReferenceValue.valueOf(firestore.getDatabaseId(), document.getKey()));
       } else {
         FieldValue value = document.getField(orderBy.getField());
-        if (value != null) {
+        if (value instanceof ServerTimestampValue) {
+          throw new IllegalArgumentException(
+              "Invalid query. You are trying to start or end a query using a document for which "
+                  + "the field '"
+                  + orderBy.getField()
+                  + "' is an uncommitted server timestamp. (Since the value of this field is "
+                  + "unknown, you cannot start/end a query with it.)");
+        } else if (value != null) {
           components.add(value);
         } else {
           throw new IllegalArgumentException(