Firestore: implement set such that it passes conformance testing

[mcdonc]

No master branch version of firestore has ever passed the class of conformance tests for the "set" command, which means someone (me) needs to deeply understand the Firestore spec and either make changes to the current impl to make those conformance tests pass or reimplement.

#6290 (comment)

[mcdonc]

I'm doing this work in https://github.com/mcdonc/google-cloud-python/tree/6307-implement-set-conformance

[mcdonc]

We'll merge the https://github.com/mcdonc/google-cloud-python/tree/6307-implement-set-conformance branch back into Tres' 6290-firestore-refactor_conformance_tests branch represented by #6291 once it has a positive review.

[mcdonc]

The https://github.com/mcdonc/google-cloud-python/tree/6307-implement-set-conformance branch is slightly better now; 22 set-related failures as opposed to 30.

However, save a couple, the remainder of the conformance failures are related to features that are just not implemented currently in the Python client. In particular, the conformance tests assert behavior related to several sentinel values that may be used in the request json payload: "ArrayRemove", "ArrayUnion", and "Delete". As far as I can tell this is just bridge-out; these strings in an appropriate context appear nowhere in the current source.

Thus, if we want to pass the conformance tests, this task is no longer really about "fixing conformance", it's about extending the client in fundamental ways.

[mcdonc]

We decided to blacklist the tests that are related to ArrayRemove, ArrayUnion and Delete transforms. We will reenable once we have a plan for implementing those features.

After the blacklisting, the branch currently has 6 4 failures. These fall into three two classes:

1) update-st-nested.textproto, update-st-multi.textproto

pbs_for_update doesn't populate update_mask properly when a ServerTimestamp is nested (not at the top level). This appears to be a problem with the way process_server_timestamp returns field paths.

2. create-st-alone.textproto, update-st-alone.textproto

stray update field in write protobuf. trying to make it do something different here breaks other tests, just haven't been able to pin the right juke down, probably should look more closely at the go and node impls. Note that there is a set-st-alone that passes, but it wants the update field, and removing it in the various impls causes the set tests to start to fail.

3. set-st-merge-nonleaf-alone.textproto, update-st-dot.textproto

stray update field and update_mask field in write protobuf. similar to 2.

Now that I look more closely at the failure mode of set-st-merge-nonleaf-alone, I think it's actually the same failure mode as 1 (parent of nested field path should be included in update mask).

update-st-dot.textproto has the docstring: "Like other uses of ServerTimestamp, the data is pruned and the field does not appear in the update mask, because it is in the transform. In this case an update operation is produced just to hold the precondition." but no update operation is present in the expected payload, only a transform, so it's difficult to determine what the intent is here.

[mcdonc]

This clowny diff fixes set-st-merge-nonleaf-alone.textproto but it's clearly just working around the problem and there is some more fundamental way to fix it.

diff --git a/firestore/google/cloud/firestore_v1beta1/_helpers.py b/firestore/google/cloud/firestore_v1beta1/_helpers.py
index 59110aeb16..135dd5f2a1 100644
@@ -1153,7 +1153,18 @@ def _pbs_for_set_with_merge(document_path, document_data, merge, exists):
     write_pbs = []
     update_pb = write_pb2.Write()
 
-    if actual_data or create_empty:
+    update_paths = data_merge[:]
+
+    # for whatever reason, the conformance tests want to see the parent
+    # of nested transform paths in the update mask
+    # (see set-st-merge-nonleaf-alone.textproto)
+    for transform_path in transform_paths:
+        if len(transform_path.parts) > 1:
+            parent_fp = FieldPath(*transform_path.parts[:-1])
+            if not parent_fp in update_paths:
+                update_paths.append(parent_fp)
+    
+    if actual_data or create_empty or update_paths:
         update = document_pb2.Document(
             name=document_path,
             fields=encode_dict(actual_data),
@@ -1161,7 +1172,9 @@ def _pbs_for_set_with_merge(document_path, document_data, merge, exists):
         update_pb.update.CopyFrom(update)
 
         mask_paths = [
-            fp.to_api_repr() for fp in merge if fp not in transform_merge
+            fp.to_api_repr() for fp in merge if (
+                (fp not in transform_merge) or (fp in update_paths)
+                )
         ]
 
         if mask_paths or create_empty:

[chemelnucfin]

@schmidt-sebastian, @jba, @mcdonc I have converted the merge from an Option class to the merge and exists parameters passing all the tests that were there before. In effect this is about the place I should have at least left the code at (before cleanup).

After rebasing to master it seems that there are more tests that need to be done to pass the conformance tests than before and it seems like I'm about 15 tests behind @MCONC even after taking away the ArrayUnion/ArrayRemove/Delete tests. Some of which seem easy to fix, and I will try to clean up the code a bit and perhaps there will be some commonality to compare and contrast to fix the remaining bugs.

[mcdonc]

@chemelnucfin the recent comments above refer to https://github.com/mcdonc/google-cloud-python/tree/6307-implement-set-conformance which evolved to not have a MergeOption class, instead using the master pattern of merge= to set. Its unclear where you branched from but you may want to base your work on that branch as it has all but 4 tests passing. I received some of your code for apirepr parsing in there too.

Edit: ah, it seems you must have looked at that branch from the "15 tests behind mcdonc" comment, I just got confused when you mentioned you "converted the merge from an options class" because that branch doesn't have a merge options class anymore. So I'm not sure where you started from, but it doesn't appear to be the latest revision.

[tseaver]

@mcdonc I've merged your https://github.com/mcdonc/google-cloud-python/tree/6307-implement-set-conformance branch into https://github.com/googleapis/google-cloud-python/tree/6290-firestore-refactor_conformance_tests, and will work on reducing the remaining 4 failures.