feat: add recursiveDelete() #1427

thebrianchen · 2021-02-17T23:59:18Z

First pass at adding recursive delete. Merging to a feature branch for iterative development.

Some things that I still have yet to add:

Firestore instance deletion
BulkWriter backoffs (extra for RESOURCE_EXHAUSTED)
More aggressive delete retry policy (ex: RST_STREAM errors).
Add toString() for document references
No throttling on deletes.

codecov · 2021-02-18T00:08:18Z

Codecov Report

Merging #1427 (800dfee) into bc/rc-main (685e75c) will increase coverage by 0.31%.
The diff coverage is 99.23%.

@@              Coverage Diff               @@
##           bc/rc-main    #1427      +/-   ##
==============================================
+ Coverage       98.20%   98.51%   +0.31%     
==============================================
  Files              32       32              
  Lines           19502    19623     +121     
  Branches         1363     1296      -67     
==============================================
+ Hits            19152    19332     +180     
+ Misses            346      286      -60     
- Partials            4        5       +1

Impacted Files	Coverage Δ
dev/src/index.ts	`97.76% <99.22%> (+2.31%)`	⬆️
dev/src/bulk-writer.ts	`100.00% <100.00%> (ø)`
dev/src/path.ts	`98.57% <0.00%> (+0.42%)`	⬆️
dev/src/reference.ts	`99.89% <0.00%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 685e75c...e24c591. Read the comment docs.

thebrianchen · 2021-02-18T01:18:00Z

dev/src/index.ts

+        writer.flush().then(async () => {
+          if (ref instanceof DocumentReference) {
+            try {
+              await ref.delete();


I can't think of a more elegant way to differentiate between the final document reference delete failing vs. the BulkWriter deletes. Any suggestions?

I haven't looked at the rest of the PR, but if you want to work on it before I do - couldn't you just let the BulkWriter handle this delete as well?

I still have a few more changes/cleanup I need to push upstream, so reviewing now might result in some wasted work.

Thanks for the suggestion! I originally figured a normal delete is less verbose than enqueuing another operation to BulkWriter and calling flush(), but using BulkWriter allows for any user callbacks to also run.

schmidt-sebastian · 2021-02-18T05:09:27Z

One more note: Can you ping the backend chat to determine whether we can retry deletes that fail with RST_STREAM? (I see this is already part of your TODOs, so please pretend that you never saw this note).

thebrianchen · 2021-02-18T05:35:18Z

dev/src/index.ts

+        err.message =
+          'Failed to fetch children documents. ' +
+          'The provided reference was not deleted.';
+        deleteCompleted.reject(wrapError(err, stack));


Is not deleting the provided reference in the case of a failed StructuredQuery the ideal, expected behavior? I need a quick sanity check. Here are the options:

Do not delete the provided reference if StructuredQuery stream ever errors.

Do not delete provided reference iff StructuredQuery stream errors before any deletes are enqueued.

Delete the reference regardless of what happens.

I think we originally agreed on Option 2. It makes logical sense -- if Firestore can't fetch any of the descendants, that's a failed precondition, so Firestore shouldn't delete the provided reference. However, what's tripping me up is if the StructuredQuery stream errors out midway through the deletion process. In this case, an argument can be made both ways -- Firestore shouldn't delete the provided reference because it couldn't fetch the descendants (failed precondition argument) OR Firestore should delete the provided reference to be consistent in behavior (the provided reference is deleted even if BulkWriter fails to delete some references).

Trying to document all these edge cases led me to consider Option 3 -- if I'm a developer who simply wants the collection or document deleted, I don't care about the provided reference, and Firestore should just nuke whatever it can get a hold of.

The query can error at any point in time. I would suggest we make no guarantees whatsoever, which makes the overall behavior of the API much more predictable. If any operation fails, the database tree is in an undefined state, and the user needs to try again.

Done. Changed it to always delete the provided reference when possible, but kept the error message in for query stream failures. Not sure if we want the FAILED_PRECONDITION status code though.

thebrianchen · 2021-02-18T23:13:42Z

dev/src/index.ts

+          );
+          if (lastError instanceof BulkWriterError) {
+            error.code = (lastError.code as number) as Status;
+          }


Not sure why these lines aren't covered by Codecov, but I'm pretty sure they're being called. Maybe it's b/c I put it in a lambda function to access lastError and errorCount. Is this an anti-pattern?

schmidt-sebastian

Feedback for your implementation. I hope that I can get to the tests soon. They look a bit scary in that you wrote a lot of them :)

dev/src/index.ts

schmidt-sebastian · 2021-02-19T04:15:29Z

dev/src/index.ts

+   *
+   * @private
+   */
+  private _bulkWriter: BulkWriter | undefined;


Could we move this to BulkWriter (kind of like a singleton pattern)? This could then be a shared instance that could be used by other operations in the future as well.

I also wonder whether the default ramp up makes sense here. It will slow down the delete (especially when compared to the CLI) and it also doesn't quite fit the use case for 5/5/5. We do not have to ramp up to give the database a chance to shard - the data is already sharded, we just want to delete it.

Could we move this to BulkWriter (kind of like a singleton pattern)? This could then be a shared instance that could be used by other operations in the future as well.

Do you mean that calling firestore.bulkWriter() would return the same instance each time, or having a Firestore-internal BulkWriter.bulkWriter() getter that lazy-loads?

I also wonder whether the default ramp up makes sense here.

Yeah, that's something I was thinking about as well to implement as part of the RST_STREAM retries. Deletes aren't subject to 555 throttling, which creates two possibilities. 1) We can add an internal override on BulkWriter to turn off throttling when recursive delete is called. However, this would mean that writes being performed at the same time won't get throttled. 2) We can add logic for BulkWriter to automatically ignore throttling limits for deletes, but then we'd have to batch deletes separately from writes (or think of another solution that allows writes to still be throttled).

Firestore internal getter that lazy-loads. I want to keep it similar to what you have, but try to move these internals out of Firestore.

Added a static BulkWriter.bulkWriter() method.

dev/src/index.ts

schmidt-sebastian · 2021-02-19T04:25:14Z

dev/src/index.ts

+        err.message =
+          'Failed to fetch children documents. ' +
+          'The provided reference was not deleted.';
+        incrementErrorCount(err);


This shouldn't bump up the error count that is used for the error message, should it?

Good point. This made me realize that the # deletes failed message won't be accurate if the stream fails since all the remaining docs not returned by the stream are unaccounted for. Should I special case the error message in the stream failure case to capture this fact?

dev/src/index.ts

schmidt-sebastian · 2021-02-19T04:26:55Z

dev/src/index.ts

+    const writer = bulkWriter ?? this.getBulkWriter();
+    writer._verifyNotClosed();
+    const docStream = this.getAllDescendants(ref);
+    const deleteCompletedDeferred = new Deferred<void>();


Should this just be result? While generic, it makes it clear that this is what the method returns.

Renamed to resultDeferred. Since the return type is a void promise, I want to make it clear that there is no real "result" being returned.

schmidt-sebastian · 2021-02-19T04:27:32Z

dev/src/index.ts

-  // TODO(chenbrian): Make this a private method after adding recursive delete.
-  _getAllDescendants(
-    ref: CollectionReference | DocumentReference
+  private getAllDescendants<T = DocumentData>(


This should still be underscore prefixed to prevent "usage by code completion".

Renamed. Aren't private methods hidden from code completion? How do they show up? Also, what's the difference between the _method() and method_() patterns in this file?

"private" is only hidden from TS developers. JS developers will still see it in code completion.

Underscore suffix is the same as underscore prefix, but from about 10 years ago. I am surprised this client uses underscore suffix.

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

schmidt-sebastian

Feedback for the tests. Generally looks great!

dev/test/index.ts

schmidt-sebastian · 2021-02-19T16:43:05Z

dev/test/index.ts

+        try {
+          expect(request.writes).to.deep.equal(expected.writes);
+        } catch (e) {
+          batchWriteError = e;


Do the tests not fail if we simply let the exception throw here? Ideally, the implementation should be able to handle if the batchWrite RPC throws an exception and propagate it as a failed BatchWrite.

I have a test that checks the error stack to make sure that the original error thrown in batchWrite is surfaced. BulkWriter currently doesn't do any special handling for batchWrite RPC failures, so recursive delete can't perform any handling logic for it. Do you think we should add logic to surface batchWrite RPC failures differently?

No, I think we can keep the logic as is. Individual RPC failures versus individual operations failures are not different from a user perspective. I was trying to see if we could simply bubble up the assertion failure as an individual RPC failure, which would surface these assertions in onWriteResult or via failed promises rather than via batchWriteError.

dev/test/index.ts

schmidt-sebastian · 2021-02-19T16:49:19Z

dev/test/index.ts

+      firestore = await instantiateInstance([
+        ['anna', 'bob', 'bob/children/charlie', 'bob/children/daniel'],
+      ]);
+      await firestore.recursiveDelete(firestore.collection('collectionId'));


I am not sure I follow this test. The documents are not part of the collection, but they are still getting returned from the RunQuery request if I am not mistaken.

The collectionId is implicit in instantiateInstance(), which follows the pattern that other test helpers use. So in this case, the documents under firestore.collection('collectionId') are ['collectionId/anna', 'collectionId/bob', ...].

Added a comment to clarify.

schmidt-sebastian · 2021-02-19T16:55:13Z

dev/test/index.ts

+      let callbackCount = 0;
+      const bulkWriter = firestore.bulkWriter();
+      bulkWriter.onWriteResult(() => {
+        callbackCount++;


Do we want to add a test that verifies that we return the expected list of DocumentReferences?

schmidt-sebastian · 2021-02-19T16:56:29Z

dev/test/index.ts

+        callbackCount++;
+      });
+      await firestore.recursiveDelete(firestore.collection('foo'), bulkWriter);
+      expect(callbackCount).to.equal(3);


We also don't seem to have tests that exercise the error handler.

schmidt-sebastian · 2021-02-19T16:57:23Z

dev/test/query.ts

+  queryEqualsHelper(actual, '', ...protoComponents);
+}
+
+function queryEqualsHelper(


I am not a huge fan of Helpers. Would it be possible to just make this queryEqualsWithParent?

A lot of tests use queryEquals() without a parent parameter, and I didn't want to pass in an empty string for parent in all of them or change the spread operator to an array. Is it possible to combine these two signatures in an overload:

export function queryEquals( actual: api.IRunQueryRequest | undefined, parent: string, ...protoComponents: api.IStructuredQuery[] ): void; export function queryEquals( actual: api.IRunQueryRequest | undefined, ...protoComponents: api.IStructuredQuery[] ): void;

You could do:

export function queryEquals( actual: api.IRunQueryRequest | undefined, parentOrProtoComponent: string|api.IStructuredQuery|undefined, ...protoComponents: api.IStructuredQuery[] ): void;

But that is not all that pretty.

I was actually thinking you keep both queryEquals and queryEqualsWithParent, but merge queryEqualsWithParent and queryEqualsHelper.

oops, that makes more sense. done.

schmidt-sebastian · 2021-02-19T16:57:45Z

types/firestore.d.ts

+     * containing the number of failed deletes and the stack trace of the last
+     * failed delete. The provided reference is deleted regardless of whether
+     * all deletes succeeded, except when Firestore fails to fetch the provided
+     * reference's descendants.


Please update according to the suggestions above.

schmidt-sebastian · 2021-02-19T16:58:12Z

types/firestore.d.ts

+     * The promise is rejected if any of the deletes fail.
+     */
+    recursiveDelete(
+      ref: CollectionReference | DocumentReference,


Please add <unknown>, otherwise this typing won't allow converters.

thebrianchen

Thanks for the detailed review! Addressed comments for the logic, going to work on test comments next.

dev/src/index.ts

thebrianchen · 2021-02-19T16:43:50Z

dev/src/index.ts

+    ref: CollectionReference<T> | DocumentReference<T>,
+    bulkWriter?: BulkWriter
+  ): Promise<void> {
+    const writer = bulkWriter ?? this.getBulkWriter();


writer has type BulkWriter, whereas bulkWriter always has type BulkWriter | undefined, which makes using the variable later messier.

thebrianchen · 2021-02-19T16:45:26Z

dev/src/index.ts

+  ): Promise<void> {
+    const writer = bulkWriter ?? this.getBulkWriter();
+    writer._verifyNotClosed();
+    const docStream = this.getAllDescendants(ref);


Hmm, I'm not really sure where to move it. Are you suggesting we encapsulate all the behavior in a separate class?

dev/src/index.ts

thebrianchen · 2021-02-19T17:07:07Z

dev/src/index.ts

+        err.message =
+          'Failed to fetch children documents. ' +
+          'The provided reference was not deleted.';
+        incrementErrorCount(err);


Good point. This made me realize that the # deletes failed message won't be accurate if the stream fails since all the remaining docs not returned by the stream are unaccounted for. Should I special case the error message in the stream failure case to capture this fact?

thebrianchen · 2021-02-19T17:12:10Z

dev/src/index.ts

+   * The promise is rejected if any of the deletes fail.
+   */
+  recursiveDelete<T = DocumentData>(
+    ref: CollectionReference<T> | DocumentReference<T>,


I had to change the signature to firestore.DocumentRef and add some casting logic from the firestore typed classes to our internal classes. Can you double check to see if there's a cleaner way? Thanks!

thebrianchen · 2021-02-19T17:59:48Z

dev/src/index.ts

+   * Firestore uses a BulkWriter instance with default settings to perform the
+   * deletes. To customize throttling rates or add success/error callbacks,
+   * pass in a custom BulkWriter instance.
+   *


Done. I included the default error handler to make it easier to paste. Do you think including it would confuse developers?

thebrianchen · 2021-02-19T18:08:42Z

dev/src/index.ts

+   *
+   * @private
+   */
+  private _bulkWriter: BulkWriter | undefined;


Could we move this to BulkWriter (kind of like a singleton pattern)? This could then be a shared instance that could be used by other operations in the future as well.

Do you mean that calling firestore.bulkWriter() would return the same instance each time, or having a Firestore-internal BulkWriter.bulkWriter() getter that lazy-loads?

I also wonder whether the default ramp up makes sense here.

Yeah, that's something I was thinking about as well to implement as part of the RST_STREAM retries. Deletes aren't subject to 555 throttling, which creates two possibilities. 1) We can add an internal override on BulkWriter to turn off throttling when recursive delete is called. However, this would mean that writes being performed at the same time won't get throttled. 2) We can add logic for BulkWriter to automatically ignore throttling limits for deletes, but then we'd have to batch deletes separately from writes (or think of another solution that allows writes to still be throttled).

thebrianchen

One more note: Can you ping the backend chat to determine whether we can retry deletes that fail with RST_STREAM? (I see this is already part of your TODOs, so please pretend that you never saw this note).

Checked in with with backend team, and it's fine to retry deletes since they're idempotent.

dev/test/index.ts

thebrianchen · 2021-02-19T19:39:29Z

types/firestore.d.ts

+     * containing the number of failed deletes and the stack trace of the last
+     * failed delete. The provided reference is deleted regardless of whether
+     * all deletes succeeded, except when Firestore fails to fetch the provided
+     * reference's descendants.


thebrianchen · 2021-02-19T19:56:32Z

dev/test/index.ts

+      let callbackCount = 0;
+      const bulkWriter = firestore.bulkWriter();
+      bulkWriter.onWriteResult(() => {
+        callbackCount++;


thebrianchen · 2021-02-19T20:01:19Z

dev/test/index.ts

+        callbackCount++;
+      });
+      await firestore.recursiveDelete(firestore.collection('foo'), bulkWriter);
+      expect(callbackCount).to.equal(3);


thebrianchen · 2021-02-19T20:06:44Z

dev/test/index.ts

+      firestore = await instantiateInstance([
+        ['anna', 'bob', 'bob/children/charlie', 'bob/children/daniel'],
+      ]);
+      await firestore.recursiveDelete(firestore.collection('collectionId'));


The collectionId is implicit in instantiateInstance(), which follows the pattern that other test helpers use. So in this case, the documents under firestore.collection('collectionId') are ['collectionId/anna', 'collectionId/bob', ...].

Added a comment to clarify.

thebrianchen · 2021-02-19T20:18:24Z

dev/test/query.ts

+  queryEqualsHelper(actual, '', ...protoComponents);
+}
+
+function queryEqualsHelper(


A lot of tests use queryEquals() without a parent parameter, and I didn't want to pass in an empty string for parent in all of them or change the spread operator to an array. Is it possible to combine these two signatures in an overload:

export function queryEquals( actual: api.IRunQueryRequest | undefined, parent: string, ...protoComponents: api.IStructuredQuery[] ): void; export function queryEquals( actual: api.IRunQueryRequest | undefined, ...protoComponents: api.IStructuredQuery[] ): void;

schmidt-sebastian

Looking better and better. Some small suggestions remain.

schmidt-sebastian · 2021-02-22T16:29:37Z

dev/src/index.ts

+   * bulkWriter
+   *   .onWriteError((error) => {
+   *     if (
+   *       error.code === GrpcStatus.UNAVAILABLE &&


I think we should drop this line. Deletes are almost always retyrable, and we should show this here and do a blind retry up to MAX_RETRY_ATTEMPTS times.

Regardless of what the developer does, we should apply a sensible backoff on top of these attempts. This means that we should backoff longer for RESOURCE_EXHAUSTED.

Done. Added the RESOURCE_EXHAUSTED note to the todo list at the top of the PR.

schmidt-sebastian · 2021-02-22T16:31:53Z

dev/src/index.ts

+   *     ) {
+   *       return true;
+   *     } else {
+   *       console.log('Failed write at document: ', error.documentRef);


We should probably add a toString() to make this output sensible.

"That sounds like a good idea, but it seems unrelated to this PR."

I came up with an answer for you to copy & paste.

I often rely on printing the javascript object for DocumentReference when debugging to view all the different attributes of the reference, so I'm afraid to replace that with a pretty-print.

Updated to use documentRef.path which should be sufficient. Thoughts?

schmidt-sebastian · 2021-02-22T16:35:53Z

dev/src/index.ts

+
+    docStream
+      .on('error', err => {
+        err.code = Status.FAILED_PRECONDITION;


Please take a look at https://developers.google.com/maps-booking/reference/grpc-api/status_codes

FAILED_PRECONDITION means that a simple retry won't work here. The developer has to change something about the state of the system before retrying the operation. This is likely not the case here. I would guess that we will have an error code here already when we hit this code path, but if we don't, I would suggest using UNAVAILABLE as the return code.

schmidt-sebastian · 2021-02-22T16:37:44Z

dev/src/index.ts

    let parentPath = ref._resourcePath;
    if (ref instanceof CollectionReference) {
      parentPath = parentPath.popLast();
    }
    const collectionId =
-      ref instanceof CollectionReference ? ref.id : ref.parent.id;
+      ref instanceof CollectionReference


CollectionReference means CollectionReference<DocumentData>. If you change this to ref instanceof CollectionReference<unknown> you should be able to remove the as DocumentReference cast (which should also be as DocumentReference<unknown>).

The TS compiler was not happy with that suggestion:
TS2348: Value of type 'typeof CollectionReference' is not callable. Did you mean to include 'new'

I think the TS compiler is right, since the generics support of TypeScript is pretty much like Java and "erased at compile time". I was able to get the code to transpiler even without the ref as DocumentReference though..

FWIW, an as DocumentReference<unknown> would still be the correct way to type this if we were to keep the cast. Right now, you are dropping the converter in the cast, when really we don't know anything about it.

Added the <unknown> type to the DocumentReference cast.

schmidt-sebastian · 2021-02-22T16:40:20Z

dev/test/query.ts

+  queryEqualsHelper(actual, '', ...protoComponents);
+}
+
+function queryEqualsHelper(


You could do:

export function queryEquals( actual: api.IRunQueryRequest | undefined, parentOrProtoComponent: string|api.IStructuredQuery|undefined, ...protoComponents: api.IStructuredQuery[] ): void;

But that is not all that pretty.

I was actually thinking you keep both queryEquals and queryEqualsWithParent, but merge queryEqualsWithParent and queryEqualsHelper.

schmidt-sebastian · 2021-02-22T16:42:27Z

dev/system-test/firestore.ts

+      });
+      await firestore.recursiveDelete(randomCol, bulkWriter);
+      expect(callbackCount).to.equal(6);
+    });


Can we also get a test that has "randomCollA/docA" and "randomCollB/docB" and only deletes randomCollA? There might be some nuances in how to delete parent collection (in other ports) and I want to make sure that all of our clients handle this case correctly.

good point, done.

schmidt-sebastian · 2021-02-22T16:47:06Z

dev/test/index.ts

+        try {
+          expect(request.writes).to.deep.equal(expected.writes);
+        } catch (e) {
+          batchWriteError = e;


No, I think we can keep the logic as is. Individual RPC failures versus individual operations failures are not different from a user perspective. I was trying to see if we could simply bubble up the assertion failure as an individual RPC failure, which would surface these assertions in onWriteResult or via failed promises rather than via batchWriteError.

schmidt-sebastian · 2021-02-23T21:42:37Z

dev/src/bulk-writer.ts

+   * @private
+   */
+  static _getInstance(firestore: Firestore): BulkWriter {
+    if (!this.instance || this.instance.firestore !== firestore) {


I didn't think about the fact that this would break if multiple Firestore instances are used together. I am sightly inclined to ask you to undo this change since it would simplify the instance creation and its lifetime management (at the cost of further increasing the size of Firestore).

I can see arguments either way, but at this point, I'm more in favor of leaving it on the Firestore instance, since trying to track the different firestore instances to pass into BulkWriter is more confusing than just having the BulkWriter instance available on the Firestore instance.

Reverted back to before.

schmidt-sebastian · 2021-02-23T21:50:11Z

dev/system-test/firestore.ts

+
+      expect(await countCollectionChildren(collA)).to.equal(0);
+      expect(await countCollectionChildren(randomCol)).to.equal(3);
+      expect(await countCollectionChildren(collB)).to.equal(1);


I think this is now a similar test to what you have above (something akin to on nested collection). I think the edge case is for root collections so something like randomCol.doc('bob') and a recursive delete on "otherCollection" (which could really be any name). We can then ensure than randomCol still has one document.

Thanks for clarifying what you meant. Added a test for that.

schmidt-sebastian · 2021-02-23T21:56:09Z

dev/src/index.ts

    let parentPath = ref._resourcePath;
    if (ref instanceof CollectionReference) {
      parentPath = parentPath.popLast();
    }
    const collectionId =
-      ref instanceof CollectionReference ? ref.id : ref.parent.id;
+      ref instanceof CollectionReference


I think the TS compiler is right, since the generics support of TypeScript is pretty much like Java and "erased at compile time". I was able to get the code to transpiler even without the ref as DocumentReference though..

FWIW, an as DocumentReference<unknown> would still be the correct way to type this if we were to keep the cast. Right now, you are dropping the converter in the cast, when really we don't know anything about it.

types/firestore.d.ts

schmidt-sebastian · 2021-02-23T21:57:35Z

types/firestore.d.ts

+     *       return false;
+     *     }
+     *   });
+     * await firestore.recursiveDelete(docRef, bulkWriter);


Please update this snippet as well (drop the error code, change the logging).

thanks for catching.

schmidt-sebastian · 2021-02-23T22:00:27Z

dev/src/index.ts

+   * Firestore uses a BulkWriter instance with default settings to perform the
+   * deletes. To customize throttling rates or add success/error callbacks,
+   * pass in a custom BulkWriter instance.
+   *


Looks good.

…e, comment updates

thebrianchen

Thanks for the quick feedback cycles!

thebrianchen · 2021-02-23T22:17:00Z

types/firestore.d.ts

+     *       return false;
+     *     }
+     *   });
+     * await firestore.recursiveDelete(docRef, bulkWriter);


thanks for catching.

thebrianchen · 2021-02-23T22:20:13Z

dev/src/index.ts

    let parentPath = ref._resourcePath;
    if (ref instanceof CollectionReference) {
      parentPath = parentPath.popLast();
    }
    const collectionId =
-      ref instanceof CollectionReference ? ref.id : ref.parent.id;
+      ref instanceof CollectionReference


Added the <unknown> type to the DocumentReference cast.

types/firestore.d.ts

thebrianchen · 2021-02-23T22:22:37Z

dev/src/bulk-writer.ts

+   * @private
+   */
+  static _getInstance(firestore: Firestore): BulkWriter {
+    if (!this.instance || this.instance.firestore !== firestore) {


I can see arguments either way, but at this point, I'm more in favor of leaving it on the Firestore instance, since trying to track the different firestore instances to pass into BulkWriter is more confusing than just having the BulkWriter instance available on the Firestore instance.

Reverted back to before.

thebrianchen · 2021-02-23T22:27:11Z

dev/system-test/firestore.ts

+
+      expect(await countCollectionChildren(collA)).to.equal(0);
+      expect(await countCollectionChildren(randomCol)).to.equal(3);
+      expect(await countCollectionChildren(collB)).to.equal(1);


Thanks for clarifying what you meant. Added a test for that.

thebrianchen self-assigned this Feb 17, 2021

thebrianchen requested review from a team as code owners February 17, 2021 23:59

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 17, 2021

product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Feb 17, 2021

init

3d1fa79

thebrianchen force-pushed the bc/rc-del branch from f99da27 to 3d1fa79 Compare February 18, 2021 00:00

thebrianchen commented Feb 18, 2021

View reviewed changes

delete provided ref with BulkWriter instead

4f40d8c

thebrianchen requested a review from schmidt-sebastian February 18, 2021 17:15

thebrianchen assigned schmidt-sebastian and thebrianchen and unassigned thebrianchen and schmidt-sebastian Feb 18, 2021

always delete provided reference

aa3c1cc

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Feb 18, 2021

remove extra check

e3aaea4

thebrianchen commented Feb 18, 2021

View reviewed changes

schmidt-sebastian reviewed Feb 19, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Feb 19, 2021

Apply suggestions from code review

8952a37

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

schmidt-sebastian reviewed Feb 19, 2021

View reviewed changes

resolve comments for logic

ec6be1c

thebrianchen commented Feb 19, 2021

View reviewed changes

resolve comments for tests

a0c1162

thebrianchen commented Feb 19, 2021

View reviewed changes

Brian Chen added 2 commits February 19, 2021 16:08

adding the test file

a8d1250

remove stack check in test

baee6fd

thebrianchen requested a review from schmidt-sebastian February 22, 2021 15:19

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Feb 22, 2021

schmidt-sebastian reviewed Feb 22, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Feb 22, 2021

bulkWriter static instance, additional tests

474e1c6

thebrianchen requested a review from schmidt-sebastian February 23, 2021 18:53

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Feb 23, 2021

schmidt-sebastian approved these changes Feb 23, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Feb 23, 2021

final revisions: revert back BulkWriter static instance into Firestor…

e24c591

…e, comment updates

thebrianchen commented Feb 23, 2021

View reviewed changes

thebrianchen merged commit eed3e6b into bc/rc-main Feb 23, 2021

thebrianchen deleted the bc/rc-del branch February 23, 2021 22:49

thebrianchen mentioned this pull request May 5, 2021

feat: add recursiveDelete() to Firestore googleapis/java-firestore#622

Merged

feat: add recursiveDelete() #1427

feat: add recursiveDelete() #1427

Conversation

thebrianchen commented Feb 17, 2021 • edited

codecov bot commented Feb 18, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Feb 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian Feb 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebrianchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebrianchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebrianchen commented Feb 17, 2021 •

edited

codecov bot commented Feb 18, 2021 •

edited

schmidt-sebastian commented Feb 18, 2021 •

edited

schmidt-sebastian Feb 22, 2021 •

edited