Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 175 additions & 5 deletions src/firestore/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,20 @@ use std::pin::Pin;

use anyhow::{Context, anyhow};
use firestore_grpc::tonic;
use firestore_grpc::v1::firestore_client::FirestoreClient as GrpcFirestoreClient;
use firestore_grpc::v1::precondition::ConditionType;
use firestore_grpc::v1::run_query_request::QueryType;
use firestore_grpc::v1::structured_aggregation_query::aggregation;
use firestore_grpc::v1::structured_query::CollectionSelector;
use firestore_grpc::v1::value::ValueType;
use firestore_grpc::v1::{
BatchGetDocumentsRequest, CreateDocumentRequest, DeleteDocumentRequest, DocumentMask,
Precondition, RunAggregationQueryRequest, RunQueryRequest, StructuredAggregationQuery,
StructuredQuery, UpdateDocumentRequest, batch_get_documents_response,
run_aggregation_query_request, structured_aggregation_query,
};
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};
Comment on lines +15 to +21

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

use firestore_grpc::{
tonic::{
Request, Status, codegen::InterceptedService, metadata::MetadataValue, transport::Channel,
Expand Down Expand Up @@ -827,6 +829,174 @@ impl FirestoreClient {
Ok(())
}

/// 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")?;
Comment on lines +894 to +900

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


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<_>, _>>()?;
Comment on lines +902 to +908

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 rust

Length 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"
fi

Length 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.


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)
}
Comment on lines +832 to +938

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


/// 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())
}
Comment on lines +940 to +998

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


/// Query a collection for documents that fulfill the given criteria.
///
/// Returns a [`Stream`] of query results, allowing you to process results as they are coming in.
Expand Down
Loading