Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement `MergeOption` as an option #4851

Merged
merged 11 commits into from Apr 10, 2018

Conversation

@chemelnucfin
Copy link
Contributor

chemelnucfin commented Feb 7, 2018

Should close #4111
The first and second commits is #4654.

@chemelnucfin chemelnucfin requested a review from lukesneeringer as a code owner Feb 7, 2018

@googlebot googlebot added the cla: yes label Feb 7, 2018

@chemelnucfin chemelnucfin force-pushed the chemelnucfin:firestore_merge_option branch from 77795e1 to 2efc83f Feb 7, 2018

@chemelnucfin chemelnucfin reopened this Feb 8, 2018

@chemelnucfin chemelnucfin force-pushed the chemelnucfin:firestore_merge_option branch from 2efc83f to 16c9588 Feb 8, 2018

@chemelnucfin chemelnucfin removed the type: bug label Feb 8, 2018

@chemelnucfin chemelnucfin force-pushed the chemelnucfin:firestore_merge_option branch from 16c9588 to c5e2ed4 Feb 8, 2018

@chemelnucfin chemelnucfin requested a review from tseaver as a code owner Feb 8, 2018

@googlebot googlebot added cla: no and removed cla: yes labels Feb 8, 2018

@chemelnucfin chemelnucfin force-pushed the chemelnucfin:firestore_merge_option branch from c5e2ed4 to 03dc67f Feb 8, 2018

@chemelnucfin chemelnucfin added cla: yes and removed cla: no labels Feb 8, 2018

@googleapis googleapis deleted a comment from googlebot Feb 8, 2018

@chemelnucfin chemelnucfin changed the title Firestore merge option Implement `MergeOption` as an option Feb 20, 2018

@chemelnucfin chemelnucfin self-assigned this Feb 20, 2018

@chemelnucfin chemelnucfin force-pushed the chemelnucfin:firestore_merge_option branch 2 times, most recently from 08f0ee5 to e0d9b28 Mar 25, 2018

@@ -768,6 +763,22 @@ def get_doc_id(document_pb, expected_prefix):
return document_id


def get_field_paths(update_data):

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Mar 28, 2018

I named this extract_field_paths in the other SDKs. I think that's a much more descriptive name.

Furthermore, update_data is something different in the Firestore SDKs. Updata data is
{ fieldPath1 : value, fieldPath2: value }. I think you are just trying to iterate a map of data here { field : value, field: value }. That's document_data.

This comment has been minimized.

Copy link
@chemelnucfin

chemelnucfin Mar 28, 2018

Author Contributor

I'm actually recursing the get_field_paths (see line 774), so I want the actual fieldPath to the value as an api_repr. So I think this is actually an update_data?

I think I wrote another method to extract both, but for this PR and what I needed here, the field path itself suffices.

It seems like there are a few ways to represent the same thing: In dictionary form {'a': {'b': {1.2 : value}}}, in api_repr form {'a.b.(bt)1.2(bt) : value}, and in python form {FieldPath("a", "b", "1.2"): value}, and I think at some point I will need to clean up all these functions.

But it will probably be easier to make PRs one at a time for easier reviewing and then clean up later instead of writing them all in one PR.

@@ -880,15 +891,16 @@ def pbs_for_set(document_path, document_data, option):
or two ``Write`` protobuf instances for ``set()``.
"""
transform_paths, actual_data = remove_server_timestamp(document_data)

update_pb = write_pb2.Write(
update=document_pb2.Document(
name=document_path,
fields=encode_dict(actual_data),
),
)
if option is not None:

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Mar 28, 2018

This should only be true for option.merge = true

This comment has been minimized.

Copy link
@chemelnucfin

chemelnucfin Mar 28, 2018

Author Contributor

field_paths is ignored in all other options, and I had merge take and ignore any argument from what we discussed earlier. Do you only want it to work for merge=True and ignore all other values?

This comment has been minimized.

Copy link
@chemelnucfin

chemelnucfin Apr 2, 2018

Author Contributor

@schmidt-sebastian I have passed all of the conformance tests locally. I still have cleanups to do though.

Do you want me to do the entire thing in one new PR or would you like to merge this first and then work on the diffs from this one?

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Apr 3, 2018

This PR still seems very reasonable it in size. You can create one PR that combines the merge of this + the conformance test changes.



DEFAULT_DATABASE = '(default)'
"""str: The default database used in a :class:`~.firestore.client.Client`."""
_BAD_OPTION_ERR = (
'Exactly one of ``create_if_missing``, ``last_update_time`` '
'and ``exists`` must be provided.')
'Exactly one of ``last_update_time`` or ``exists`` must be provided.')

This comment has been minimized.

Copy link
@schmidt-sebastian

This comment has been minimized.

Copy link
@chemelnucfin

chemelnucfin Mar 28, 2018

Author Contributor

ok.

elif not self._create_if_missing:
current_doc = types.Precondition(exists=True)
write_pb.current_document.CopyFrom(current_doc)
mask = common_pb2.DocumentMask(field_paths=sorted(field_paths))

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Mar 28, 2018

Are you sorting for testing purposes? Would be good to point that out in a comment.

This comment has been minimized.

Copy link
@chemelnucfin

chemelnucfin Mar 28, 2018

Author Contributor

ok.

@chemelnucfin chemelnucfin force-pushed the chemelnucfin:firestore_merge_option branch 4 times, most recently from 7eec589 to 1af5b12 Mar 28, 2018

@chemelnucfin chemelnucfin force-pushed the chemelnucfin:firestore_merge_option branch 3 times, most recently from fa1e5d0 to 53159e9 Apr 6, 2018

chemelnucfin added some commits Dec 22, 2017

@chemelnucfin chemelnucfin force-pushed the chemelnucfin:firestore_merge_option branch from 2ea0832 to 93f0441 Apr 6, 2018

@chemelnucfin chemelnucfin force-pushed the chemelnucfin:firestore_merge_option branch from 93f0441 to 138111b Apr 6, 2018

@schmidt-sebastian
Copy link

schmidt-sebastian left a comment

This looks pretty close from a functional perspective. Some comments (mostly small nits) remaining.

Can you send this over to someone that understands Python after you resolved these last comments? Thanks!

for sub_path in sub_transform_paths:
field_path = FieldPath.from_string(field_name)
field_path.parts = field_path.parts + sub_path.parts
transform_paths.extend([field_path])

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Apr 6, 2018

Nit: Change all the naming in here to use 'transform' somewhere. That way, it is easy to tell the difference between this block and the one just below.

if split_on_dots:
transform_paths.append(FieldPath(*field_name.split(".")))
else:
transform_paths.append(FieldPath.from_string(field_name))

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Apr 6, 2018

Can you move the FieldPath logic much further up? If you have transform the field_name into a FieldPath right as you enter the loop, then you can re-use it everywhere here.

else:
return field_paths, document_data
field_paths.append(FieldPath(field_name))
if not transform_paths:

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Apr 6, 2018

Is this needed or just an optimization?

This comment has been minimized.

Copy link
@chemelnucfin

chemelnucfin Apr 6, 2018

Author Contributor

just a simplification.

@@ -39,8 +39,8 @@
DEFAULT_DATABASE = '(default)'
"""str: The default database used in a :class:`~.firestore.client.Client`."""
_BAD_OPTION_ERR = (
'Exactly one of ``create_if_missing``, ``last_update_time`` '
'and ``exists`` must be provided.')
'Exactly one of ``last_update_time``, ``exists`` '

This comment has been minimized.

Copy link
@schmidt-sebastian

# 1. Use ``set()`` to create the document (using an option).
# 1. Use ``create()`` to create the document (using an option).

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Apr 6, 2018

Remove "(using an option)"


data2 = {'1a.ab': '4d', '6f.7g': '9h'}
option2 = client.write_option(create_if_missing=True)
option2 = client.write_option(exists=True)

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Apr 6, 2018

Passing 'exists=true' to create should be disallowed. Can you add a TODO somewhere?

This comment has been minimized.

Copy link
@chemelnucfin

chemelnucfin Apr 6, 2018

Author Contributor

This is an update. I just checked that document.create cannot take options.

snapshot = document.get()
assert not snapshot.exists

# 1. Use ``set()`` to create the document (using an option).

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Apr 6, 2018

Please update these comments. You are using "create" here with no option.

self.run_write_test(test_proto, desc)
except (AssertionError, Exception) as error:
count += 1

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Apr 6, 2018

What does 'count' stand for? Can the name be more descriptive?

elif kind == "set":
tp = test_proto.set
client, doc = self.setup(firestore_api, tp)
data = convert_data(json.loads(tp.json_data))
# TODO: call doc.set.
if tp.HasField("option"):
option = True

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Apr 6, 2018

Is option the SetOptions or just "merge" (in which case it should be mergeOption)?

@chemelnucfin chemelnucfin force-pushed the chemelnucfin:firestore_merge_option branch from 10d612b to b166f06 Apr 6, 2018

@tseaver tseaver merged commit dd4b646 into googleapis:master Apr 10, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed

@chemelnucfin chemelnucfin deleted the chemelnucfin:firestore_merge_option branch Apr 10, 2018

@tseaver tseaver referenced this pull request Apr 10, 2018

Closed

Add 'FieldPath.__repr__' #5112

@mikedh

This comment has been minimized.

Copy link

mikedh commented Oct 16, 2018

Hey, this finally made it to Pypi in a release, and unfortunately broke some things for me. This PR removes firestore.CreateIfMissingOption, which is still recommended in the Firebase docs. The docs:

If the document does not exist, it will be created. If the document does exist, its contents will be overwritten with the newly provided data, unless you specify that the data should be merged into the existing document, as follows:

# The option to merge data is not yet available for Python. Instead, call the
# update method and pass the option to create the document if it's missing.

city_ref = db.collection(u'cities').document(u'BJ')

city_ref.update({
    u'capital': True
}, firestore.CreateIfMissingOption(True))

It looks like the migration here is (?):

city_ref.update({
    u'capital': True}, merge=True)

Given the arg ordering, an ugly backwards compatibility patch could be:

CreateIfMissingOption = lambda x : bool(x)
@tseaver

This comment has been minimized.

Copy link
Contributor

tseaver commented Oct 16, 2018

@mikedh The Firestore API team explicitly asked us to remove CreateIfMissingOption from the API surface of google-cloud-firestore.

@schmidt-sebastian Can you comment here?

@samtstern

This comment has been minimized.

Copy link

samtstern commented Oct 16, 2018

That's the correct API change, just looks like it took a bit longer for it to land in Python than the other SDKs and we didn't time the doc updates with the release. I'll get the docs updated.

@samtstern

This comment has been minimized.

Copy link

samtstern commented Oct 16, 2018

@mikedh

This comment has been minimized.

Copy link

mikedh commented Oct 16, 2018

Thanks! The API change seems like an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.