Skip to content

Firestore: from_api_repr and parse_field_path methods - #5109

Closed
chemelnucfin wants to merge 3 commits into
googleapis:masterfrom
chemelnucfin:firestore_from_api_repr
Closed

Firestore: from_api_repr and parse_field_path methods#5109
chemelnucfin wants to merge 3 commits into
googleapis:masterfrom
chemelnucfin:firestore_from_api_repr

Conversation

@chemelnucfin

@chemelnucfin chemelnucfin commented Mar 24, 2018

Copy link
Copy Markdown
Contributor

No description provided.

@chemelnucfin chemelnucfin added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. api: firestore Issues related to the Firestore API. labels Mar 24, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 24, 2018
@tseaver tseaver removed the type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. label Mar 26, 2018
@tseaver

tseaver commented Mar 26, 2018

Copy link
Copy Markdown
Contributor

@chemelnucfin Although PRs may introduce bugs, we don't want to classify them as bugs. If this PR is fixing an existing bug (or even working toward it), please link that issue in the description.

@chemelnucfin

chemelnucfin commented Mar 27, 2018

Copy link
Copy Markdown
Contributor Author

@jba could you take a look at this one first?

Thanks.

jba
jba suggested changes Mar 27, 2018
View reviewed changes
Comment thread firestore/google/cloud/firestore_v1beta1/_helpers.py Outdated

This method always splits on dots. If the unicode string representation
includes dots or other invalid characters `~*/[]`, this method will fail
and the constructor must be used to determine nesting.

This comment was marked as spam.

Sign in to view

Comment thread firestore/google/cloud/firestore_v1beta1/_helpers.py Outdated
A tuple with the first field name and the api_repr
of the rest.
"""
if api_repr[0] == '.':

This comment was marked as spam.

Sign in to view

Comment thread firestore/google/cloud/firestore_v1beta1/_helpers.py Outdated
if api_repr[index-1] == '`': # skips escaped backticks
value = (
api_repr[:index+1],
'.'.join(api_repr[index+1:].split('.')))

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread firestore/google/cloud/firestore_v1beta1/_helpers.py Outdated
api_repr = api_repr[1:]
if api_repr[0] != '`': # first field name is simple
field_names = api_repr.split('.')
return field_names[0], '.'.join(field_names[1:])

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread firestore/google/cloud/firestore_v1beta1/_helpers.py
return field_names[0], '.'.join(field_names[1:])
else:
index = 1
while index < len(api_repr):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread firestore/google/cloud/firestore_v1beta1/_helpers.py
return cls(*string)

@classmethod
def _parse_field_name(cls, api_repr):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread firestore/tests/unit/test__helpers.py Outdated
@@ -133,64 +133,90 @@ def test_unicode(self):
self.assertEqual(field_path.parts, ('一', '二', '三'))

def test_to_api_repr_a(self):

This comment was marked as spam.

Sign in to view

@chemelnucfin
chemelnucfin force-pushed the firestore_from_api_repr branch from af7b563 to caf8a50 Compare March 27, 2018 21:41
@chemelnucfin
chemelnucfin force-pushed the firestore_from_api_repr branch from caf8a50 to 5c2d4a4 Compare March 27, 2018 21:44
jba
jba suggested changes Mar 27, 2018
View reviewed changes
Comment thread firestore/google/cloud/firestore_v1beta1/_helpers.py
The constructor must be used if the unicode string representation
consists of invalid characters `~*/[]`. This method always splits
on the character `.` for nesting. If any field names in this field
path includes the character `.`, the constructor must also be used

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread firestore/google/cloud/firestore_v1beta1/_helpers.py
return cls(*string)

@classmethod
def _parse_field_name(cls, api_repr):

This comment was marked as spam.

Sign in to view

@chemelnucfin

chemelnucfin commented Mar 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@jba I think I found the scenario where I needed this from_api_repr.

From this test (update-10)

description: "prefix #2"
update {
  doc_ref_path: "projects/projectID/databases/(default)/documents/C/d"
  json_data: "{\"a\": 1, \"a.b\": 2}"
  is_error: true
}

The only reason I know that that is actually FieldPath("a", "b") instead of FieldPath("a.b") is that since this test is an error I'm assuming this is an api_repr. I can't easily split on dots if it's an api_repr.
For example:

>>> json.loads("\"a.b\"")
'a.b'
>>> json.loads("\"`a.b`\"")
'`a.b`'
>>>

Maybe I missed something? How do you handle this in golang?

@chemelnucfin chemelnucfin reopened this Mar 28, 2018
@jba

jba commented Mar 28, 2018

Copy link
Copy Markdown
Contributor

The JSON in the tests should correspond exactly to a Python dict that is the argument to the update method. So that JSON should map to the Python {"a": 1, "a.b": 2}. If I understand correctly, in dynamic languages the keys of the dict passed to update can either be strings, which are split on dots, or FieldPaths, which are interpreted literally. @schmidt-sebastian can elaborate on that. You should also read the section on update in the DocumentReference section of the API doc. Note the sentence:

Thus, update( { ‘foo.bar’ : 42 } ) becomes an object with the nested hierarchy of { foo { bar : 42 }}.

@jba

jba commented Mar 28, 2018

Copy link
Copy Markdown
Contributor

Just to be clear: nothing in the tests requires you to parse the API format for field paths. You only need to generate that format, not parse it.

@chemelnucfin

chemelnucfin commented Mar 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Ahh, I think this makes more sense now.

Note that the server rejects documents that contain single field names with dots that prefix match other field names. Hence, a document cannot contain both ‘a’ and ‘`a.b`’. The Server SDKs can rely on the backend to perform this validation.

@chemelnucfin

chemelnucfin commented Mar 30, 2018

Copy link
Copy Markdown
Contributor Author

Closing this for now.

@chemelnucfin chemelnucfin mentioned this pull request Apr 10, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer
1 more reviewer
@jba jba jba requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: firestore Issues related to the Firestore API. cla: yes This human has signed the Contributor License Agreement. priority: p2 Moderately-important priority. Fix may not be included in next release.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chemelnucfin @tseaver @jba @googlebot