[DRAFT] feat(firestore): add DML support to pipelines#16473
[DRAFT] feat(firestore): add DML support to pipelines#16473daniel-sanche wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces delete and update stages to the Firestore pipeline, enabling document deletion and in-place updates. The changes include new stage implementations, pipeline methods, and comprehensive system tests. Feedback highlights a logic error in the test assertions where empty documents were incorrectly identified as deleted, and suggests clarifying the update method's documentation to distinguish between partial updates and full document saves. Additionally, a more explicit syntax for field transformations in YAML tests was recommended for better consistency.
| for doc_path, expected_content in expected_state.items(): | ||
| doc_ref = client.document(doc_path) | ||
| snapshot = doc_ref.get() | ||
| if expected_content is None or expected_content == {}: |
There was a problem hiding this comment.
The condition or expected_content == {} could lead to incorrect test failures. An empty document is a valid state in Firestore (snapshot.exists would be true), but this logic treats it as a deleted document. To correctly test for document deletion, the check should only be for expected_content is None, which corresponds to null in the YAML file.
| if expected_content is None or expected_content == {}: | |
| if expected_content is None: |
| for doc_path, expected_content in expected_state.items(): | ||
| doc_ref = async_client.document(doc_path) | ||
| snapshot = await doc_ref.get() | ||
| if expected_content is None or expected_content == {}: |
There was a problem hiding this comment.
Similar to the synchronous test, this condition or expected_content == {} could lead to incorrect test failures. An empty document is a valid state, but this logic treats it as deleted. The check for deletion should likely only be against None.
| if expected_content is None or expected_content == {}: | |
| if expected_content is None: |
| Updates the documents with the given transformations. | ||
|
|
||
| This method updates the documents in place based on the data flowing through the pipeline. | ||
| To specify transformations, use `*transformed_fields`. |
There was a problem hiding this comment.
The current docstring can be confusing as it doesn't clearly distinguish between calling update() with and without arguments. This can make it hard to understand the behavior in different scenarios, like in Example 1. Clarifying the two modes of operation (partial update vs. full document save) would improve usability.
| Updates the documents with the given transformations. | |
| This method updates the documents in place based on the data flowing through the pipeline. | |
| To specify transformations, use `*transformed_fields`. | |
| Updates documents in the collection. | |
| When called with *transformed_fields, this method applies partial updates | |
| to documents. When called without arguments, it saves the documents as they | |
| exist in the pipeline stream, overwriting existing documents. This is useful | |
| for persisting changes from previous stages like add_fields or remove_fields. |
| - description: "Basic DML update" | ||
| pipeline: | ||
| - Collection: dml_update_coll | ||
| - Update: Field.of("status").set("active") |
There was a problem hiding this comment.
The expression Field.of("status").set("active") is a bit ambiguous. It's not clear if set() is a standard operation on a Field expression or a custom test helper. For setting a field to a constant value, using Constant.of("active").as_("status") would be more explicit and align better with the expression patterns seen in unit tests. This would improve the clarity and maintainability of the test.
- Update: Constant.of("active").as_("status")
WIP