Add batch_get_documents method to fetch many documents at once#42
Conversation
|
Just a small example from our GraphQL API to showcase just how big the difference can be: For just 348 users:
I expect the difference to be even more stark with a larger query set. ~350 documents is not that much, so this PR may provide some very nice low-latency value. |
WalkthroughAdds batch-get document APIs to FirestoreClient, introduces ordered batch retrieval, and refactors path handling by adding a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Caller
participant FC as FirestoreClient
participant FS as Firestore API
App->>FC: batch_get_documents(doc_refs)
FC->>FC: Build full doc names from database_path
FC->>FS: BatchGetDocumentsRequest(database=database_path, documents=...)
FS-->>FC: Stream items (Found | Missing | Non-doc)
alt Found
FC->>FC: Deserialize fields -> FirestoreDocument<T>
FC-->>App: Stream Ok(FirestoreDocument<T>)
else Missing
FC-->>App: Stream Ok(Err(missing_path String))
else Non-doc
FC->>FC: Ignore
end
sequenceDiagram
autonumber
participant App as Caller
participant FC as FirestoreClient
App->>FC: batch_get_documents_ordered(doc_refs[])
FC->>FC: Build index map: doc_name -> position
FC->>FC: Consume batch_get_documents stream
loop For each streamed item
alt Found
FC->>FC: Place deserialized T at original index
else Missing
FC->>FC: Place None at original index
end
end
FC-->>App: Vec<Option<T>> in input order
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
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(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/firestore/client/mod.rs (2)
src/firestore/reference.rs (3)
doc(46-51)new(39-44)id(140-142)src/firestore/serde/serialize.rs (9)
new(25-32)new(277-285)new(314-323)new(391-396)new(431-436)new(481-487)new(533-539)new(577-582)new(610-615)
🔇 Additional comments (2)
src/firestore/client/mod.rs (2)
1-1: Import looks good.Needed for the ordered index map later.
15-18: New Firestore gRPC imports are appropriate.
batch_get_documents_responseandBatchGetDocumentsRequestare used correctly below.
| grpc_channel: Channel, | ||
| project_id: String, | ||
| token_provider: FirestoreTokenProvider, | ||
| database_path: String, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Storing database_path is a solid refactor.
Nice separation between database_path and root_resource_path. Consider future-proofing by letting FirestoreClientOptions optionally carry a non-default database id.
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around line 53, add an optional non-default
database id to FirestoreClientOptions so callers can override the implicit
database when constructing database_path; change the struct to carry
Option<String> (e.g., database_id: Option<String>), update any
new()/from_env()/builder/constructor APIs to accept or set this option (falling
back to the current default when None), and propagate that value into the logic
that builds database_path/root_resource_path so the path uses the provided id
when present; also update docs and any unit/integration tests to cover supplying
a custom database id.
| let database_path = format!("projects/{project_id}/databases/(default)"); | ||
| let root_resource_path = format!("{database_path}/documents"); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Deriving root_resource_path from database_path is clean.
Good centralization. If/when supporting non-default databases, this spot can read the db id from options.
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around lines 146 to 148, currently the database
path and root resource path hardcode the default database id; change this to
derive the database id from configuration/options (falling back to "(default)"
when none provided) and build database_path and root_resource_path using that db
id so future non-default database support reads the db id from options instead
of being fixed.
| /// The returned stream will contain `Ok(FirestoreDocument<T>)` for each | ||
| /// document that was found, and `Err(String)` for each document that was not | ||
| /// found, with the string being the path of the missing document. | ||
| pub async fn batch_get_documents<'de, 'a, T: Deserialize<'de>>( | ||
| &'a mut self, | ||
| documents: impl IntoIterator<Item = &'a DocumentReference>, | ||
| ) -> Result< | ||
| FirebaseStream<'a, Result<FirestoreDocument<T>, String>, FirebaseError>, | ||
| FirebaseError, | ||
| > { | ||
| let doc_refs = documents | ||
| .into_iter() | ||
| .map(|doc_ref| self.get_name_with(doc_ref)) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let res = self | ||
| .client | ||
| .batch_get_documents(BatchGetDocumentsRequest { | ||
| database: self.database_path.clone(), | ||
| documents: doc_refs, | ||
| mask: None, | ||
| consistency_selector: None, | ||
| }) | ||
| .await |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Heads-up: very large batches may hit gRPC message-size limits.
You’re sending all paths in a single request. If callers pass thousands of docs, the request header can exceed limits. Consider chunking the documents into reasonably sized groups (e.g., 500–1000), issuing multiple batch_get_documents calls, and flattening the result streams.
Do you want me to sketch a chunked version that preserves your current API and merges streams?
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around lines 1546 to 1569, the current
implementation sends all document paths in a single BatchGetDocumentsRequest
which can exceed gRPC message-size limits for very large inputs; change the
method to split the incoming documents iterator into fixed-size chunks (e.g.,
500–1000 document names), call batch_get_documents for each chunk, and then
merge/flatten the per-chunk response streams into one FirebaseStream so the
public API remains unchanged; ensure you handle/propagate per-call
FirebaseError(s), preserve the per-document Ok/Err items, and choose whether to
run chunk requests sequentially or limited-concurrency to avoid client-side
resource spikes.
| /// Fetches all documents from the given slice of document references. | ||
| /// Just like [`batch_get_documents`](Self::batch_get_documents), but returns | ||
| /// the documents in the same order as they were given. | ||
| /// | ||
| /// This has the downside of not being able to return a stream since the | ||
| /// Firestore API does not support ordering the results. Instead, this method | ||
| /// will fetch all the specified documents and return them in a `Vec` of | ||
| /// `Option<T>`, where `None` indicates that the document at that index could | ||
| /// not be found. | ||
| pub async fn batch_get_documents_ordered<'de, 'a, T: Deserialize<'de>>( | ||
| &'a mut self, | ||
| documents: &[DocumentReference], | ||
| ) -> Result<Vec<Option<T>>, FirebaseError> { | ||
| let indices = documents | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, doc_ref)| (self.get_name_with(doc_ref), i)) | ||
| .collect::<HashMap<_, _>>(); | ||
|
|
||
| let mut doc_stream = self.batch_get_documents(documents).await?; | ||
|
|
||
| // Cannot use vec![None; documents.len()] because it would require T: Clone | ||
| let mut results = Vec::new(); | ||
| results.resize_with(documents.len(), || None); | ||
|
|
||
| while let Some(result) = doc_stream.try_next().await? { | ||
| let doc = match result { | ||
| Ok(doc) => doc, | ||
| Err(_) => continue, | ||
| }; | ||
|
|
||
| let index = indices.get(&doc.id).with_context(|| { | ||
| format!("Firestore returned an unexpected document: {}", doc.id) | ||
| })?; | ||
|
|
||
| results[*index] = Some(doc.data); | ||
| } | ||
|
|
||
| Ok(results) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Duplicate input doc refs produce surprising results.
You map each returned doc to a single index via HashMap<name, index>. With duplicates in documents, the last occurrence wins, and earlier positions stay None. Either (a) reject duplicates up front, or (b) map to the first occurrence deterministically.
Minimal fix to prefer the first index (stable behavior, no new bounds):
- let indices = documents
- .iter()
- .enumerate()
- .map(|(i, doc_ref)| (self.get_name_with(doc_ref), i))
- .collect::<HashMap<_, _>>();
+ let mut indices = HashMap::new();
+ for (i, doc_ref) in documents.iter().enumerate() {
+ indices.entry(self.get_name_with(doc_ref)).or_insert(i);
+ }Optional: detect duplicates and error out to make behavior explicit.
📝 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.
| /// Fetches all documents from the given slice of document references. | |
| /// Just like [`batch_get_documents`](Self::batch_get_documents), but returns | |
| /// the documents in the same order as they were given. | |
| /// | |
| /// This has the downside of not being able to return a stream since the | |
| /// Firestore API does not support ordering the results. Instead, this method | |
| /// will fetch all the specified documents and return them in a `Vec` of | |
| /// `Option<T>`, where `None` indicates that the document at that index could | |
| /// not be found. | |
| pub async fn batch_get_documents_ordered<'de, 'a, T: Deserialize<'de>>( | |
| &'a mut self, | |
| documents: &[DocumentReference], | |
| ) -> Result<Vec<Option<T>>, FirebaseError> { | |
| let indices = documents | |
| .iter() | |
| .enumerate() | |
| .map(|(i, doc_ref)| (self.get_name_with(doc_ref), i)) | |
| .collect::<HashMap<_, _>>(); | |
| let mut doc_stream = self.batch_get_documents(documents).await?; | |
| // Cannot use vec![None; documents.len()] because it would require T: Clone | |
| let mut results = Vec::new(); | |
| results.resize_with(documents.len(), || None); | |
| while let Some(result) = doc_stream.try_next().await? { | |
| let doc = match result { | |
| Ok(doc) => doc, | |
| Err(_) => continue, | |
| }; | |
| let index = indices.get(&doc.id).with_context(|| { | |
| format!("Firestore returned an unexpected document: {}", doc.id) | |
| })?; | |
| results[*index] = Some(doc.data); | |
| } | |
| Ok(results) | |
| } | |
| let mut indices = HashMap::new(); | |
| for (i, doc_ref) in documents.iter().enumerate() { | |
| indices.entry(self.get_name_with(doc_ref)).or_insert(i); | |
| } |
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around lines 1600 to 1639, the indices HashMap
currently maps a document name to the last occurrence index which causes earlier
duplicate positions to remain None; change construction to prefer the first
occurrence (or optionally detect and return an error) by inserting each (name,
index) only if the name is not already present (i.e., check contains_key or use
entry API to only set on Vacant) so earlier indices win and results are
deterministic.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
src/firestore/client/mod.rs (5)
53-55: Don’t hardcode the default DB — make it configurable.Storing
database_pathis good, but it’s fixed to “(default)”. Please letFirestoreClientOptionscarry an optionaldatabase_idand derivedatabase_pathfrom it. This avoids future churn for multi-database setups.I can wire this through
FirestoreClientOptionsand update constructors/tests if you want.
146-148: Builddatabase_pathfrom options, not a literal "(default)".Swap to read
database_idfrom options (fallback to "(default)"):- let database_path = format!("projects/{project_id}/databases/(default)"); - let root_resource_path = format!("{database_path}/documents"); + let database_id = options.database_id.as_deref().unwrap_or("(default)"); + let database_path = format!("projects/{project_id}/databases/{database_id}"); + let root_resource_path = format!("{database_path}/documents");This needs
FirestoreClientOptions { database_id: Option<String>, .. }.Also applies to: 154-156
1623-1629: Loosen the lifetime ondocumentsto avoid borrow coupling with&mut self.Right now
documents: impl IntoIterator<Item = &'a DocumentReference>ties docs’ lifetime to the stream’s&'a mut self. Introduce a separate lifetime:-pub async fn batch_get_documents<'de, 'a, T: Deserialize<'de>>( - &'a mut self, - documents: impl IntoIterator<Item = &'a DocumentReference>, +pub async fn batch_get_documents<'de, 'a, 'docs, T: Deserialize<'de>>( + &'a mut self, + documents: impl IntoIterator<Item = &'docs DocumentReference>,This matches how the function only uses the refs to build owned paths.
1635-1643: Large inputs may hit gRPC message-size limits — consider chunking.Sending thousands of paths in one
BatchGetDocumentsRequestcan exceed limits. Chunkdoc_refs(e.g., 500–1000), issue multiple requests, and flatten the streams.I can sketch a chunked implementation that preserves your API and merges streams with limited concurrency.
1744-1749: Duplicate doc refs cause earlier positions to stayNone. Prefer first-occurrence or reject duplicates.Your map overwrites with the last index. Make behavior stable:
- let indices = documents - .iter() - .enumerate() - .map(|(i, doc_ref)| (self.get_name_with(doc_ref), i)) - .collect::<HashMap<_, _>>(); + let mut indices = HashMap::with_capacity(documents.len()); + for (i, doc_ref) in documents.iter().enumerate() { + indices.entry(self.get_name_with(doc_ref)).or_insert(i); + }Optionally detect duplicates up front and error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/firestore/client/mod.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/firestore/client/mod.rs (2)
src/firestore/reference.rs (3)
doc(46-51)new(39-44)id(140-142)src/firestore/serde/serialize.rs (9)
new(25-32)new(277-285)new(314-323)new(391-396)new(431-436)new(481-487)new(533-539)new(577-582)new(610-615)
⏰ 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
| /// Fetches all documents from the given iterator of document references. | ||
| /// | ||
| /// Documents are not guaranteed to be returned in the same order as they | ||
| /// were given in the iterator. If you need that guarantee, use | ||
| /// [`batch_get_documents_ordered`](Self::batch_get_documents_ordered). | ||
| /// | ||
| /// The returned stream will contain `Ok(FirestoreDocument<T>)` for each | ||
| /// document that was found, and `Err(String)` for each document that was not | ||
| /// found, with the string being the path of the missing document. | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// # #[tokio::main] | ||
| /// # async fn main() -> Result<(), Box<dyn std::error::Error>> { | ||
| /// # use serde::{Serialize, Deserialize}; | ||
| /// # use fireplace::firestore::collection; | ||
| /// # let mut client = fireplace::firestore::test_helpers::initialise().await.unwrap(); | ||
| /// # | ||
| /// use futures::TryStreamExt; | ||
| /// | ||
| /// #[derive(Debug, Serialize, Deserialize, PartialEq)] | ||
| /// struct Movie { | ||
| /// title: String, | ||
| /// year: u32, | ||
| /// } | ||
| /// | ||
| /// let movies_collection = collection("movies"); | ||
| /// | ||
| /// // Create some movies in the database | ||
| /// let movie1 = Movie { | ||
| /// title: "The Matrix".to_string(), | ||
| /// year: 1999, | ||
| /// }; | ||
| /// let movie2 = Movie { | ||
| /// title: "Inception".to_string(), | ||
| /// year: 2010, | ||
| /// }; | ||
| /// | ||
| /// client.set_document(&movies_collection.doc("matrix"), &movie1).await?; | ||
| /// client.set_document(&movies_collection.doc("inception"), &movie2).await?; | ||
| /// | ||
| /// // Batch fetch documents (order not guaranteed) | ||
| /// let doc_refs = [ | ||
| /// movies_collection.doc("matrix"), | ||
| /// movies_collection.doc("inception"), | ||
| /// movies_collection.doc("nonexistent"), | ||
| /// ]; | ||
| /// | ||
| /// let mut found_movies = Vec::new(); | ||
| /// let mut missing_paths = Vec::new(); | ||
| /// | ||
| /// let mut stream = client | ||
| /// .batch_get_documents::<Movie>(doc_refs.iter()) | ||
| /// .await?; | ||
| /// | ||
| /// // Process the stream results | ||
| /// while let Some(result) = stream.try_next().await? { | ||
| /// match result { | ||
| /// Ok(doc) => { | ||
| /// // Document was found | ||
| /// found_movies.push(doc.data); | ||
| /// } | ||
| /// Err(missing_path) => { | ||
| /// // Document was not found | ||
| /// missing_paths.push(missing_path); | ||
| /// } | ||
| /// } | ||
| /// } | ||
| /// | ||
| /// // We should have found 2 movies and 1 missing document | ||
| /// assert_eq!(found_movies.len(), 2); | ||
| /// assert_eq!(missing_paths.len(), 1); | ||
| /// | ||
| /// // Sort movies by title for predictable testing (since order isn't guaranteed) | ||
| /// found_movies.sort_by(|a, b| a.title.cmp(&b.title)); | ||
| /// assert!(found_movies.contains(&movie2)); // Inception | ||
| /// assert!(found_movies.contains(&movie1)); // The Matrix | ||
| /// | ||
| /// // The missing document path should contain "nonexistent" | ||
| /// assert!(missing_paths[0].contains("nonexistent")); | ||
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub async fn batch_get_documents<'de, 'a, T: Deserialize<'de>>( | ||
| &'a mut self, | ||
| documents: impl IntoIterator<Item = &'a DocumentReference>, | ||
| ) -> Result< | ||
| FirebaseStream<'a, Result<FirestoreDocument<T>, String>, FirebaseError>, | ||
| FirebaseError, | ||
| > { | ||
| let doc_refs = documents | ||
| .into_iter() | ||
| .map(|doc_ref| self.get_name_with(doc_ref)) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let res = self | ||
| .client | ||
| .batch_get_documents(BatchGetDocumentsRequest { | ||
| database: self.database_path.clone(), | ||
| documents: doc_refs, | ||
| mask: None, | ||
| consistency_selector: None, | ||
| }) | ||
| .await | ||
| .context("Failed to run batch get documents request")?; | ||
|
|
||
| let doc_stream = res | ||
| .into_inner() | ||
| // Some of the "results" coming from the gRPC stream don't represent | ||
| // search hits but rather information about "the transaction". We just | ||
| // ignore those items. | ||
| .filter_map(|res| future::ready(res.map(|inner| inner.result).transpose())) | ||
| .map(|batch_get_res| { | ||
| let doc = match batch_get_res.map_err(|e| anyhow!(e))? { | ||
| batch_get_documents_response::Result::Found(document) => document, | ||
| batch_get_documents_response::Result::Missing(doc_path) => { | ||
| return Ok(Err(doc_path)) | ||
| } | ||
| }; | ||
|
|
||
| let deserialised = FirestoreDocument { | ||
| data: deserialize_firestore_document_fields::<T>(doc.fields) | ||
| .map_err(|e| serde_err_with_doc(e, &doc.name))?, | ||
| id: doc.name, | ||
| create_time: doc.create_time.map(|t| t.seconds), | ||
| update_time: doc.update_time.map(|t| t.seconds), | ||
| }; | ||
|
|
||
| Ok(Ok(deserialised)) | ||
| }); | ||
|
|
||
| Ok(doc_stream.boxed()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add tests for the new batch APIs.
Please add happy-path, missing-doc, duplicate-input, and “very large list” (chunking) cases. I can draft these.
I can generate integration-style tests using the existing test helpers to seed data and assert ordering/missing semantics. Want me to open a follow-up PR with tests?
Also applies to: 1740-1770
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around lines 1539-1672 (also applies to
1740-1770): the review requests adding integration tests for the new
batch_get_documents APIs covering happy-path, missing-doc, duplicate-input, and
very-large-list (chunking) cases; please add tests in the existing tests module
using test_helpers to seed a test collection with documents, create document
references via collection(...).doc(...), call client.batch_get_documents (and
batch_get_documents_ordered where appropriate), and assert results by consuming
the returned stream: for happy-path assert all documents returned and
deserialised correctly; for missing-doc assert Err(String) entries contain the
missing path; for duplicate-input assert duplicates in input produce the
expected duplicate results or missing markers consistently; for very-large-list
seed more documents than the service/client chunk limit and verify all documents
are returned (and ordering only when using the ordered variant); use
TryStreamExt to collect results and deterministic assertions (sort where order
is not guaranteed).
| let doc = match batch_get_res.map_err(|e| anyhow!(e))? { | ||
| batch_get_documents_response::Result::Found(document) => document, | ||
| batch_get_documents_response::Result::Missing(doc_path) => { | ||
| return Ok(Err(doc_path)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Return a consistent, relative path for missing docs.
Other APIs strip the resource prefix in errors. Consider aligning:
- batch_get_documents_response::Result::Missing(doc_path) => {
- return Ok(Err(doc_path))
- }
+ batch_get_documents_response::Result::Missing(doc_path) => {
+ let missing = super::serde::strip_reference_prefix(&doc_path).to_string();
+ return Ok(Err(missing));
+ }📝 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.
| let doc = match batch_get_res.map_err(|e| anyhow!(e))? { | |
| batch_get_documents_response::Result::Found(document) => document, | |
| batch_get_documents_response::Result::Missing(doc_path) => { | |
| return Ok(Err(doc_path)) | |
| } | |
| let doc = match batch_get_res.map_err(|e| anyhow!(e))? { | |
| batch_get_documents_response::Result::Found(document) => document, | |
| batch_get_documents_response::Result::Missing(doc_path) => { | |
| let missing = super::serde::strip_reference_prefix(&doc_path).to_string(); | |
| return Ok(Err(missing)); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around lines 1653 to 1657, the Missing arm
currently returns the raw doc_path which includes the full resource prefix;
change it to return the consistent relative document path used by other APIs by
stripping the "projects/{project}/databases/{database}/documents/" prefix (or
use the existing helper that converts full resource names to relative paths)
before returning Ok(Err(...)), ensuring the returned value matches other error
paths that expose relative document paths.
| let deserialised = FirestoreDocument { | ||
| data: deserialize_firestore_document_fields::<T>(doc.fields) | ||
| .map_err(|e| serde_err_with_doc(e, &doc.name))?, | ||
| id: doc.name, | ||
| create_time: doc.create_time.map(|t| t.seconds), | ||
| update_time: doc.update_time.map(|t| t.seconds), | ||
| }; | ||
|
|
||
| Ok(Ok(deserialised)) | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Nit: use US spelling for consistency (deserialized).
Tiny naming cleanup:
- let deserialised = FirestoreDocument {
+ let deserialized = FirestoreDocument {
data: deserialize_firestore_document_fields::<T>(doc.fields)
.map_err(|e| serde_err_with_doc(e, &doc.name))?,
id: doc.name,
create_time: doc.create_time.map(|t| t.seconds),
update_time: doc.update_time.map(|t| t.seconds),
};
-
- Ok(Ok(deserialised))
+ Ok(Ok(deserialized))📝 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.
| let deserialised = FirestoreDocument { | |
| data: deserialize_firestore_document_fields::<T>(doc.fields) | |
| .map_err(|e| serde_err_with_doc(e, &doc.name))?, | |
| id: doc.name, | |
| create_time: doc.create_time.map(|t| t.seconds), | |
| update_time: doc.update_time.map(|t| t.seconds), | |
| }; | |
| Ok(Ok(deserialised)) | |
| }); | |
| let deserialized = FirestoreDocument { | |
| data: deserialize_firestore_document_fields::<T>(doc.fields) | |
| .map_err(|e| serde_err_with_doc(e, &doc.name))?, | |
| id: doc.name, | |
| create_time: doc.create_time.map(|t| t.seconds), | |
| update_time: doc.update_time.map(|t| t.seconds), | |
| }; | |
| Ok(Ok(deserialized)) | |
| }); |
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around lines 1660 to 1669, rename the local
variable and any occurrences of "deserialised" to the US-spelled "deserialized"
for consistency (e.g., let deserialised -> let deserialized, and return
Ok(Ok(deserialized))); update the identifier inside the FirestoreDocument
construction and any matching references in this block to the new name, ensuring
the code compiles after the rename.
| pub async fn batch_get_documents_ordered<'de, 'a, T: Deserialize<'de>>( | ||
| &'a mut self, | ||
| documents: &[DocumentReference], | ||
| ) -> Result<Vec<Option<T>>, FirebaseError> { | ||
| let indices = documents | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, doc_ref)| (self.get_name_with(doc_ref), i)) | ||
| .collect::<HashMap<_, _>>(); | ||
|
|
||
| let mut doc_stream = self.batch_get_documents(documents).await?; | ||
|
|
||
| // Cannot use vec![None; documents.len()] because it would require T: Clone | ||
| let mut results = Vec::new(); | ||
| results.resize_with(documents.len(), || None); | ||
|
|
||
| while let Some(result) = doc_stream.try_next().await? { | ||
| let doc = match result { | ||
| Ok(doc) => doc, | ||
| Err(_) => continue, | ||
| }; | ||
|
|
||
| let index = indices.get(&doc.id).with_context(|| { | ||
| format!("Firestore returned an unexpected document: {}", doc.id) | ||
| })?; | ||
|
|
||
| results[*index] = Some(doc.data); | ||
| } | ||
|
|
||
| Ok(results) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider preserving metadata in the ordered variant.
If callers need timestamps or IDs, returning Vec<Option<FirestoreDocument<T>>> would avoid a second fetch.
🤖 Prompt for AI Agents
In src/firestore/client/mod.rs around lines 1740 to 1770, the ordered batch
getter currently returns Vec<Option<T>> and only stores doc.data, losing
metadata like timestamps and IDs; change the function to return
Result<Vec<Option<FirestoreDocument<T>>>, FirebaseError> (or the appropriate
wrapper type used elsewhere), update the results vector to hold
Option<FirestoreDocument<T>> and when populating use the full document (not just
doc.data), adjust the Deserialize bounds if necessary (e.g. Deserialize for T
inside FirestoreDocument), and update any call sites to handle the new return
type so callers can access metadata without a second fetch.
1 participant
Refer to the documentation on each of the methods for explanations and examples of usages of the methods.
Note
(Old note)
This PR has kind of left behind. It's still a nice update to the library, and it's ready to be merged as soon as tests have been added.However, we no longer needed the new functions, so we haven't spent time finishing the PR.Summary by CodeRabbit
New Features
Refactor