Firestore: conformance tests hiding failures

[tseaver]

Since dd4b646 (PR #4851), the test_cross_language testcase has been swallowing errors.

And therefore we have a bunch of bugs to fix. :(

[tseaver]

/cc @schmidt-sebastian

[schmidt-sebastian]

The cross platform test cases - especially those that only verify that an error has thrown - are not a replacement for unit tests. There are a very helpful tool when it comes to ensuring API conformance, but even without these tests, we should aim for high test coverage.

[tseaver]

@schmidt-sebastian: we do have good test coverage, but the failures masked by this bug are cases where our tests are asserting wrong behavior.

Edit: I just checked, and we have 100% unit test coverage (including branches) even with the conformance tests not running.

[mcdonc]

At least some subset of the failures are related to the decision to not pass "options" to document.set, instead electing to pass a merge boolean, which was an API change I guess. This is a problem because some set tests pass options in cases where merge is true and those options get ignored.

This is the commit that introduced the API change:

421efcd

[mcdonc]

Here's sample failure output from the conformance tests demonstrating this (via set-17.testproto). Note that the expected "update mask" and payload is only "a" but the actual update mask and payload contain both "a" and "b". This is because set-17.testproto contains a "fields: a" option that gets ignored.

E   AssertionError: Expected call: commit('projects/projectID/databases/(default)', [update {
E     name: "projects/projectID/databases/(default)/documents/C/d"
E     fields {
E       key: "a"
E       value {
E         integer_value: 1
E       }
E     }
E   }
E   update_mask {
E     field_paths: "a"
E   }
E   ], metadata=[('google-cloud-resource-prefix', 'projects/projectID/databases/(default)')], transaction=None)
E   Actual call: commit('projects/projectID/databases/(default)', [update {
E     name: "projects/projectID/databases/(default)/documents/C/d"
E     fields {
E       key: "a"
E       value {
E         integer_value: 1
E       }
E     }
E     fields {
E       key: "b"
E       value {
E         integer_value: 2
E       }
E     }
E   }
E   update_mask {
E     field_paths: "a"
E     field_paths: "b"
E   }
E   ], metadata=[('google-cloud-resource-prefix', 'projects/projectID/databases/(default)')], transaction=None)

[tseaver]

See some discussion of the MergeOption bit in #4111.

[mcdonc]

Good find. It looks like the decision to use a merge= boolean instead of a MergeOption was cosmetic.

#4111 (comment)

I suspect we should just revert 421efcd but I'm not sure how an API change like this should be shepherded.

[mcdonc]

It's not quite as simple as reverting the effect of that commit, unfortunately. I think I understand now why the commit was applied.

Various methods of DocumentReference accept an "option" argument. An option is a "write option", which is an instance of a class that inherits from a common base class. These are supplied only on write ops like set and update. The state of the code prior to the commit attempted to characterize a merge as an option, but only one type of option may be supplied to any of the methods (there are other types of options such as an exists option, and a last_update option). This wasn't a problem in the past, because there were only two options previously (exists and last_update) and they were mutually exclusive. However, with the introduction of merge as an option, I think it broke the "all options are mutually exclusive from one another" assumption that had been in effect until then. This is evidenced by e.g. https://github.com/googleapis/google-cloud-python/blob/master/firestore/google/cloud/firestore_v1beta1/_helpers.py#L930 where it was changed to accept two arguments, exists and merge, which had each once been options; that code duplicates for "exists" what the client.ExistsOption.modify_write method is doing, doing an end run around the options system entirely.

So I think we either need to:

• Allow DocumentReference.set/update/etc and its callees to accept multiple options (e.g. add back option= paramter to those methods and allow option to be a list and do duck typing on it further down the stack). We can do a sanity check before we apply the options that they aren't mutually exclusive.

• Do away with the concept of "options" entirely; instead, hardcode the updates to the protobuf that are implied by options based on flags like the referenced commit did for the combination of exists and merge.

I am partial to the first option.

[schmidt-sebastian]

Please follow the APIs as outlined in the specification and as implemented by the other SDKs.

Set - the Protobuf contains only Write.Document
Set with merge:true - the Protobuf contains Write.Document and Write.UpdateMask, where the update mask contains the field paths of all leaf nodes.
Set with mergePaths - the Protobuf contains Write.Document and Write.UpdateMask, where the update mask contains the field paths as defined in mergePaths.
Update - the Protobuf contains Write.Document (with fields split on dots), Write.UpdateMask with the field paths specified in the input, and Precondition.exists
Update with Precondition lastUpdateTime - the Protobuf contains Write.Document (with fields split on dots), Write.UpdateMaskwith the field paths specified in the input, and Precondition.lastUpdateTime

Please take a look at the reference implementation in Node or in Java if there are any concerns.