firestore: add a FieldPath class

[jba]

The firestore client conflates dot-separated strings with field paths in the update methods. There should be two cases for top-level dict keys passed to update:

• If the key is a string, it is split on dots and cannot contain any of the special characters ~*/[]. There also can't be any empty parts. One reasonable approach is:

  # validate that there are no special characters in s
  fp = FieldPath(s.split('.'))
  

• If the key is a FieldPath, it is treated as-is.

Where FieldPath is something like:

class FieldPath(object):
    def __init__(self, *parts):
        # Verify that each part is a string, and that no part is the empty string.

    def _to_api_repr(self):
        # Apply Firestore escaping rules, using backticks and backslashes for parts
        # that do not match ^[A-Za-z_][A-Za-z_0-9]*$.
        # E.g. FieldPath("a", "~", "`").to_api_repr() => "a.`~`.`\``"

    def __eq__(self): # and __ne__ and whatever else is needed to be a dict key.

Since it is now possible to pass a dict where the same effective path occurs twice, that needs to be checked explicitly.
E.g.

{"a": 1, FieldPath("a"): 2}

should be an error.

@schmidt-sebastian Confirm this is accurate.

[schmidt-sebastian]

Yes, this is accurate.

[chemelnucfin]

@jba I would like some clarifications on the FieldPath (FP).

1. It seems like I should be able to take either a string, or an iterable and FP will figure out what to do. So either FP("a.b.c"), FP("a", "b", "c") and FP(["a", "b", "c"]) should all work and return the same thing, but I assume FP(["a"], "b", "c") will raise ValueError. Is this correct?

2. You mentioned above that ~ is an invalid character, but you allowed FieldPath to have "~" in the constructor. I would like to confirm that that "~" is only invalid inside a string such as FP("a.~.c"), but valid in FP("a", "~", "b")

3. How do backslashes get handled in api_repr()? Would "\\" (single slash) return "\\" or "`\\`"? I assume that "\\\\" (double slash) doesn't happen in paths, so how do you want that to be handled? An error, or "`\\``\\`", or something else?

4. Do unicode characters fit into your regex requirements?

5. Not exactly sure where to check for the same effective path, but I haven't looked at that part of the code yet.

Thanks in advance.

[jba]

Sorry, just saw this. I think we figured out all the answers on the PR.

[schmidt-sebastian]

@dhermes / @chemelnucfin: Can you give us an estimate for the completion of this change?

[schmidt-sebastian]

I might be late to this, but let me answer:

1 - FieldPath("a.b.c") is not the same as FieldPath("a", "b', "c"). In fact, it is a.b.c. String("a.b.c") is the same as FieldPath("a", "b', "c"). FieldPath(string) should not split on dots.
2. The set of invalid characters only applies to String field paths.
3.-5.: Please look at how it is done in Node: https://github.com/googleapis/nodejs-firestore/blob/master/src/path.js#L518

[schmidt-sebastian]

@chemelnucfin Can you provide an estimate for this issue?

[chemelnucfin]

@schmidt-sebastian #4392 has been approved by jba, but dhermes would like to review it first before I merge it.

[chemelnucfin]

@schmidt-sebastian Danny has been helping me quite a bit on spanner reviews and getting pull requests in for pubsub and a lot of others. I can understand that he's quite busy with other tasks.

[schmidt-sebastian]

@schmidt-sebastian Danny has been helping me quite a bit on spanner reviews and getting pull requests in for pubsub and a lot of others. I can understand that he's quite busy with other tasks.

That's fine, just wanted to make sure that this doesn't get dropped.

[schmidt-sebastian]

@chemelnucfin Is there any work remaining on this, now that #4392 is closed?

[chemelnucfin]

@schmidt-sebastian Could you please review #4466?
This can be closed after that is merged. Thanks.

[chemelnucfin]

Closed in #4392 and #4466.