From 47876e85a5af0a663b96e45a2a51ad2b4a1f9c63 Mon Sep 17 00:00:00 2001
From: Brian Chen <chenbrian@google.com>
Date: Mon, 24 May 2021 13:57:58 -0500
Subject: [PATCH] add comments, fix preconditions check

---
 .../main/java/com/google/cloud/firestore/RecursiveDelete.java | 3 ++-
 .../java/com/google/cloud/firestore/RecursiveDeleteTest.java  | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/RecursiveDelete.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/RecursiveDelete.java
index bbec4a798..b14eb1983 100644
--- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/RecursiveDelete.java
+++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/RecursiveDelete.java
@@ -189,8 +189,9 @@ private Query getAllDescendantsQuery() {
       // The parent is the closest ancestor document to the location we're deleting. Since we are
       // deleting a document, the parent is the path of that document.
       parentPath = path;
+      Preconditions.checkState(
+          path.getParent() != null, "Parent of a document should not be null.");
       collectionId = path.getParent().getId();
-      Preconditions.checkState(collectionId != null, "Parent of a document should not be null.");
     } else {
       // The parent is the closest ancestor document to the location we're deleting. Since we are
       // deleting a collection, the parent is the path of the document containing that collection
diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/RecursiveDeleteTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/RecursiveDeleteTest.java
index 5143714bb..a83842779 100644
--- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/RecursiveDeleteTest.java
+++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/RecursiveDeleteTest.java
@@ -389,6 +389,10 @@ public RunQueryResponse answer(InvocationOnMock invocation) throws Throwable {
               public ApiFuture<BatchWriteResponse> answer(InvocationOnMock mock) throws Exception {
                 if (numDeletesBuffered[0] < cutoff) {
                   numDeletesBuffered[0] += batchWriteResponse.size();
+                  // By waiting for `bufferFuture` to complete, we can guarantee that the writes
+                  // complete after all documents are streamed. Without this future, the test can
+                  // race and complete the writes before the stream is finished, which is a
+                  // different scenario this test is not for.
                   return ApiFutures.transformAsync(
                       bufferFuture,
                       new ApiAsyncFunction<Void, BatchWriteResponse>() {