Add recursive document deletion functionality#45
Merged
Conversation
WalkthroughAdds a recursive delete feature to FirestoreClient. Introduces delete_document_recursively to delete a document and all descendants via a kindless all_descendants query, batching deletes in chunks of 500 using BatchWrite. Includes a private get_all_descendants stream helper. The methods appear duplicated in the impl. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant FC as FirestoreClient
participant FS as Firestore API
Caller->>FC: delete_document_recursively(document)
FC->>FS: RunQuery (kindless, all_descendants)
FS-->>FC: Stream of descendant document names
Note over FC: Collect + convert to DocumentReference list (incl. root)
loop For each chunk of up to 500
FC->>FS: BatchWrite(Delete[..<=500])
FS-->>FC: BatchWriteResponse
end
FC-->>Caller: Vec<DocumentReference> of deleted docs
Note over Caller,FC: New flow adds recursive delete using batching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/firestore/client/mod.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/firestore/client/mod.rs (2)
src/firestore/reference.rs (6)
name(57-59)try_from(95-113)try_from(163-182)parent(53-55)parent(136-138)doc(46-51)src/firestore/serde/deserialize.rs (1)
strip_reference_prefix(282-285)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests
🔇 Additional comments (3)
src/firestore/client/mod.rs (3)
910-911: Good use of batching for API limitsNice work implementing the 500-document batch limit to stay within Firestore's API constraints. This prevents potential failures when dealing with large document trees.
913-914: Deletion order is correctGood implementation detail - deleting in reverse order (descendants first, then parent) ensures proper cleanup and avoids potential issues with deleting parent documents that still have children.
958-964: Clever use of kindless query techniqueSmart implementation using the empty
collection_idwithall_descendants: trueto fetch all descendant documents regardless of collection structure. The comment explaining this non-obvious technique from the Node.js SDK is helpful for future maintainers.
Comment on lines
+15
to
+21
| use firestore_grpc::v1::{BatchWriteRequest, structured_query::CollectionSelector}; | ||
| use firestore_grpc::v1::{Write, run_query_request::QueryType}; | ||
| use firestore_grpc::v1::{ | ||
| firestore_client::FirestoreClient as GrpcFirestoreClient, structured_query::Projection, | ||
| }; | ||
| use firestore_grpc::v1::{precondition::ConditionType, structured_query::FieldReference}; | ||
| use firestore_grpc::v1::{structured_aggregation_query::aggregation, write::Operation}; |
There was a problem hiding this comment.
🧹 Nitpick
Clean up redundant imports
The imports are split across multiple use statements for the same module. Consider consolidating them for better readability.
-use firestore_grpc::v1::{BatchWriteRequest, structured_query::CollectionSelector};
-use firestore_grpc::v1::{Write, run_query_request::QueryType};
-use firestore_grpc::v1::{
- firestore_client::FirestoreClient as GrpcFirestoreClient, structured_query::Projection,
-};
-use firestore_grpc::v1::{precondition::ConditionType, structured_query::FieldReference};
-use firestore_grpc::v1::{structured_aggregation_query::aggregation, write::Operation};
+use firestore_grpc::v1::{
+ BatchWriteRequest, Write,
+ firestore_client::FirestoreClient as GrpcFirestoreClient,
+ precondition::ConditionType,
+ run_query_request::QueryType,
+ structured_aggregation_query::aggregation,
+ structured_query::{CollectionSelector, FieldReference, Projection},
+ write::Operation,
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| use firestore_grpc::v1::{BatchWriteRequest, structured_query::CollectionSelector}; | |
| use firestore_grpc::v1::{Write, run_query_request::QueryType}; | |
| use firestore_grpc::v1::{ | |
| firestore_client::FirestoreClient as GrpcFirestoreClient, structured_query::Projection, | |
| }; | |
| use firestore_grpc::v1::{precondition::ConditionType, structured_query::FieldReference}; | |
| use firestore_grpc::v1::{structured_aggregation_query::aggregation, write::Operation}; | |
| use firestore_grpc::v1::{ | |
| BatchWriteRequest, Write, | |
| firestore_client::FirestoreClient as GrpcFirestoreClient, | |
| precondition::ConditionType, | |
| run_query_request::QueryType, | |
| structured_aggregation_query::aggregation, | |
| structured_query::{CollectionSelector, FieldReference, Projection}, | |
| write::Operation, | |
| }; |
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around lines 15 to 21, the same crate
firestore_grpc::v1 is imported across multiple use statements; consolidate these
into a single use firestore_grpc::v1::{ ... } statement (or nested grouped
imports) listing BatchWriteRequest, structured_query::CollectionSelector, Write,
run_query_request::QueryType, firestore_client::FirestoreClient as
GrpcFirestoreClient, structured_query::Projection, precondition::ConditionType,
structured_query::FieldReference, structured_aggregation_query::aggregation, and
write::Operation to improve readability and remove redundant use lines.
Comment on lines
+832
to
+938
| /// Recursively deletes a document and all its descendant documents from the database. | ||
| /// | ||
| /// Returns a vector of all document references that were deleted. | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// # #[tokio::main] | ||
| /// # async fn main() -> Result<(), Box<dyn std::error::Error>> { | ||
| /// # use fireplace::firestore::collection; | ||
| /// # let mut client = fireplace::firestore::test_helpers::initialise().await.unwrap(); | ||
| /// # | ||
| /// // Create a document | ||
| /// let startup_ref = collection("startups").doc("doomed-ventures"); | ||
| /// client | ||
| /// .set_document(&startup_ref, &serde_json::json!({ "name": "Doomed Ventures Inc", "status": "burning_cash" })) | ||
| /// .await?; | ||
| /// | ||
| /// // Add some employees | ||
| /// let dev_ref = startup_ref.collection("employees").doc("steve"); | ||
| /// client | ||
| /// .set_document(&dev_ref, &serde_json::json!({ "name": "Steve", "role": "10x developer", "languages": ["Rust", "Go", "Assembly"] })) | ||
| /// .await?; | ||
| /// | ||
| /// let pm_ref = startup_ref.collection("employees").doc("karen"); | ||
| /// client | ||
| /// .set_document(&pm_ref, &serde_json::json!({ "name": "Karen", "role": "Scrum Master Extraordinaire" })) | ||
| /// .await?; | ||
| /// | ||
| /// // Recursively delete everything | ||
| /// let deleted_docs = client | ||
| /// .delete_document_recursively(&startup_ref) | ||
| /// .await?; | ||
| /// | ||
| /// // Verify that the digital cremation was successful | ||
| /// assert_eq!(deleted_docs.len(), 3); | ||
| /// | ||
| /// // The root document should be gone | ||
| /// assert_eq!( | ||
| /// client.get_document::<serde_json::Value>(&startup_ref).await?, | ||
| /// None | ||
| /// ); | ||
| /// | ||
| /// // So should all the descendant documents | ||
| /// assert_eq!( | ||
| /// client.get_document::<serde_json::Value>(&dev_ref).await?, | ||
| /// None | ||
| /// ); | ||
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub async fn delete_document_recursively( | ||
| &mut self, | ||
| document: &DocumentReference, | ||
| ) -> Result<Vec<DocumentReference>, FirebaseError> { | ||
| let doc_name = self.get_name_with(document); | ||
|
|
||
| let descendants_stream = self | ||
| .get_all_descendants(document) | ||
| .await | ||
| .context("Failed to request descendant documents")?; | ||
|
|
||
| let mut docs_to_delete = descendants_stream | ||
| .try_fold(vec![doc_name], |mut acc, name| async move { | ||
| acc.push(name); | ||
| Ok(acc) | ||
| }) | ||
| .await | ||
| .context("Failed to stream descendant documents")?; | ||
|
|
||
| let document_references = docs_to_delete | ||
| .iter() | ||
| .map(|name| { | ||
| DocumentReference::try_from(strip_reference_prefix(name)) | ||
| .with_context(|| anyhow!("Failed to parse document reference from name {name}")) | ||
| }) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
|
|
||
| const BATCH_SIZE: usize = 500; | ||
|
|
||
| while !docs_to_delete.is_empty() { | ||
| let batch_range = docs_to_delete.len().saturating_sub(BATCH_SIZE)..; | ||
|
|
||
| let batch_writes = docs_to_delete | ||
| .drain(batch_range) | ||
| .map(|name| Write { | ||
| operation: Some(Operation::Delete(name)), | ||
| update_mask: None, | ||
| current_document: None, | ||
| update_transforms: vec![], | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let request = BatchWriteRequest { | ||
| database: self.database_path.clone(), | ||
| writes: batch_writes, | ||
| labels: HashMap::new(), | ||
| }; | ||
|
|
||
| self.client | ||
| .batch_write(request) | ||
| .await | ||
| .map_err(|e| anyhow!(e))?; | ||
| } | ||
|
|
||
| Ok(document_references) | ||
| } |
There was a problem hiding this comment.
Duplicate method definition detected
The delete_document_recursively method appears to be defined twice in the same impl block. This will cause a compilation error. You should remove one of the duplicate definitions.
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around lines 832-938 there are two identical
implementations of delete_document_recursively causing a duplicate-definition
compile error; remove the redundant duplicate block so only a single async pub
fn delete_document_recursively(&mut self, document: &DocumentReference) ->
Result<Vec<DocumentReference>, FirebaseError> remains, ensure any unique logic
or comments from either copy are preserved/merged into the kept version, remove
any now-unused imports if the deletion makes them dead, and run cargo
build/tests to verify the error is resolved.
Comment on lines
+894
to
+900
| let mut docs_to_delete = descendants_stream | ||
| .try_fold(vec![doc_name], |mut acc, name| async move { | ||
| acc.push(name); | ||
| Ok(acc) | ||
| }) | ||
| .await | ||
| .context("Failed to stream descendant documents")?; |
There was a problem hiding this comment.
🧹 Nitpick
Consider memory usage for very large document trees
The current implementation collects all descendant document names into a vector before processing deletions. For extremely large document trees (tens of thousands of documents), this could consume significant memory.
Consider processing deletions in a streaming fashion or implementing pagination if memory becomes a concern in production use cases.
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around lines 894 to 900, the code currently
collects all descendant document names into a single Vec which can blow up
memory for very large trees; instead, process the descendants as a stream (e.g.,
use try_for_each or try_for_each_concurrent with a bounded concurrency limit)
and perform deletions as each name is yielded, or accumulate fixed-size batches
(pagination) and delete each batch before collecting the next; modify the code
to remove the growing Vec, handle errors per-item or per-batch, and ensure
bounded memory usage by limiting concurrency/batch size.
Comment on lines
+902
to
+908
| let document_references = docs_to_delete | ||
| .iter() | ||
| .map(|name| { | ||
| DocumentReference::try_from(strip_reference_prefix(name)) | ||
| .with_context(|| anyhow!("Failed to parse document reference from name {name}")) | ||
| }) | ||
| .collect::<Result<Vec<_>, _>>()?; |
There was a problem hiding this comment.
🧹 Nitpick
🧩 Analysis chain
Verify error handling for invalid document references
The code maps document names to DocumentReference objects but could fail if strip_reference_prefix returns an invalid path. While you're using with_context for better error messages, ensure this handles edge cases properly.
🏁 Script executed:
#!/bin/bash
# Check if strip_reference_prefix is properly handling edge cases
ast-grep --pattern 'fn strip_reference_prefix($_) {
$$$
}'
# Look for any tests that cover strip_reference_prefix with invalid inputs
rg -A 5 "strip_reference_prefix.*test" --type rustLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for strip_reference_prefix definition/usages and nearby DocumentReference::try_from
set -euo pipefail
echo "1) Definition (fn strip_reference_prefix):"
rg -n --hidden --follow --no-ignore-vcs --type rust 'fn\s+strip_reference_prefix\b' || true
echo
echo "2) All usages of strip_reference_prefix(...):"
rg -n --hidden --follow --no-ignore-vcs --type rust 'strip_reference_prefix\(' || true
echo
echo "3) Usages of DocumentReference::try_from (context):"
rg -n --hidden --follow --no-ignore-vcs --type rust 'DocumentReference::try_from' -C3 || true
echo
echo "4) Show src/firestore/client/mod.rs around lines 860-920 (with numbers):"
if [ -f src/firestore/client/mod.rs ]; then
nl -ba src/firestore/client/mod.rs | sed -n '860,920p'
else
echo "File not found: src/firestore/client/mod.rs"
fiLength of output: 1942
Verify error handling for invalid document references
- Mapping at src/firestore/client/mod.rs:902–908 already wraps DocumentReference::try_from(...) with with_context — will include the original name on parse failure.
- document_reference() at src/firestore/client/mod.rs:85–90 calls DocumentReference::try_from(...) without context — add with_context or map_err to include the id.
- strip_reference_prefix is at src/firestore/serde/deserialize.rs:282 — confirm it won't return empty/invalid paths silently and add unit tests for empty/ malformed inputs.
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around lines 85 to 90 and 902 to 908, add
contextual error handling and validate stripped paths: update the
document_reference() function (lines ~85-90) to wrap the
DocumentReference::try_from(...) call with either with_context or map_err to
include the offending id/name in the error message (matching the existing
approach used at 902-908), and update/confirm strip_reference_prefix
(src/firestore/serde/deserialize.rs around line 282) so it does not silently
return empty or malformed paths (add explicit validation and return an Err when
input yields an empty or invalid path). Add unit tests covering empty,
malformed, and valid inputs for strip_reference_prefix and for
document_reference() to ensure errors include the original id/name.
Comment on lines
+940
to
+998
| /// Get a stream of all descendant documents of the given document. | ||
| /// | ||
| /// The returned strings are the full resource names of the documents. | ||
| /// | ||
| /// Based on https://github.com/googleapis/nodejs-firestore/blob/99918f1794adee706c4f2685cd3f8aea6dff895e/dev/src/recursive-delete.ts#L228 | ||
| async fn get_all_descendants( | ||
| &mut self, | ||
| document: &DocumentReference, | ||
| ) -> Result<FirebaseStream<'_, String, FirebaseError>, FirebaseError> { | ||
| let parent = self.get_name_with(document); | ||
|
|
||
| let structured_query = StructuredQuery { | ||
| // We don't want document data to be returned, so just select the document ID | ||
| select: Some(Projection { | ||
| fields: vec![FieldReference { | ||
| field_path: "__name__".to_string(), | ||
| }], | ||
| }), | ||
| from: vec![CollectionSelector { | ||
| // Must "remove" collection ID in order to select all descendant documents, which | ||
| // is an unintuitive hack, that I got from reading the Firestore Node.js SDK | ||
| // source. They use a custom notion of "kindless" queries, which removes the | ||
| // collection ID set by the getAllDescendants function linked above. | ||
| // Source: https://github.com/googleapis/nodejs-firestore/blob/99918f1794adee706c4f2685cd3f8aea6dff895e/dev/src/reference/query.ts#L1420-L1424 | ||
| collection_id: "".to_string(), | ||
| all_descendants: true, | ||
| }], | ||
| r#where: None, | ||
| order_by: vec![], | ||
| start_at: None, | ||
| end_at: None, | ||
| offset: 0, | ||
| limit: Some(i32::MAX), | ||
| find_nearest: None, | ||
| }; | ||
|
|
||
| let request = RunQueryRequest { | ||
| parent, | ||
| query_type: Some(QueryType::StructuredQuery(structured_query)), | ||
| consistency_selector: None, | ||
| explain_options: None, | ||
| }; | ||
|
|
||
| let res = self | ||
| .client | ||
| .run_query(request) | ||
| .await | ||
| .map_err(|e| anyhow!(e))?; | ||
|
|
||
| let stream = res | ||
| .into_inner() | ||
| .filter_map(|res| future::ready(res.map(|inner| inner.document).transpose())) | ||
| .map(|doc_res| { | ||
| let doc = doc_res.map_err(|e| anyhow!(e))?; | ||
| Ok(doc.name) | ||
| }); | ||
|
|
||
| Ok(stream.boxed()) | ||
| } |
There was a problem hiding this comment.
Duplicate helper method definition
The get_all_descendants helper method is also duplicated. Remove the duplicate definition to fix the compilation error.
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around lines 940 to 998 there is a duplicate
definition of the get_all_descendants helper method; remove the duplicate
function block (keeping the correctly implemented one) so only a single
get_all_descendants remains, ensuring any internal references and imports still
compile and no other duplicate symbols exist.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
delete_document_recursivelymethod to FirestoreClient that deletes a document and all its descendant documents in a single operation.Implementation Details
The implementation uses an unintuitive hack discovered in the Firestore Node.js SDK where setting
collection_idto an empty string withall_descendants: trueallows querying all descendant documents regardless of their collection structure. The Node.js SDK calls this a "kindless" query, though we just borrowed the technique.The deletion process works in reverse order - descendants are deleted first, then the parent document. Batch writes are used with a 500-document limit per batch to efficiently handle large document trees while staying within Firestore's API limits.
Summary by CodeRabbit
New Features
Documentation