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

Api Core: add a method that calculates a fieldmask from two messages #5320

Merged
merged 8 commits into from
Jul 25, 2018

Conversation

landrito
Copy link
Contributor

What

Given two messages of the same type, this function will produce a field mask representing the diffs between the two messages.

Example Usage (with a fictional client)

from google.api_core import protobuf_helpers
from copy import deepcopy
...
resource = client.GetResource(name)
modified = deepcopy(resource)
modified.thing = 'new thing'
client.UpdateResource(modified, protobuf_helpers.fieldmask(resource, modified))

cc: @jbolinger

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 11, 2018
from google.protobuf.message import Message
from google.protobuf.descriptor import FieldDescriptor

This comment was marked as spam.

This comment was marked as spam.



def fieldmask(original, modified):
"""Constructs a field mask from two proto messages.

This comment was marked as spam.

This comment was marked as spam.

modified (~google.protobuf.message.Message): the modified message.

Returns:
FieldMask: returns a FieldMask object representing the differences

This comment was marked as spam.

This comment was marked as spam.

between made from the original message to the modified message.

Raises:
ValueError: If the ``original`` or ``modified`` are not subclasses of

This comment was marked as spam.

This comment was marked as spam.

raise ValueError('The parameters passed must be of the same type.')
answer = []
seen = []
for field, _ in original.ListFields():

This comment was marked as spam.

This comment was marked as spam.

@@ -0,0 +1,28 @@
// Copyright 2018 Google LLC

This comment was marked as spam.

This comment was marked as spam.

@landrito landrito force-pushed the api-core-field-mask branch from c515aca to 012c70d Compare May 11, 2018 22:41
Copy link
Contributor

@theacodes theacodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good. We can merge once you address my final comments and we can get CI to pass.


Returns:
google.protobuf.field_mask_pb2.FieldMask: returns a FieldMask instance
representing the differences between between two messages.

This comment was marked as spam.

This comment was marked as spam.

ValueError: If the ``original`` or ``modified`` are not the same type.
"""
if type(original) != type(modified):
raise ValueError('fieldmask() expects parameters must be of the same type.')

This comment was marked as spam.

This comment was marked as spam.

@landrito
Copy link
Contributor Author

Hold off on merging. Working out some details with internal feature requester.

@@ -247,3 +252,45 @@ def setdefault(msg_or_dict, key, value):
"""
if not get(msg_or_dict, key, default=None):
set(msg_or_dict, key, value)


def fieldmask(original, modified):

This comment was marked as spam.

This comment was marked as spam.

@landrito
Copy link
Contributor Author

landrito commented May 15, 2018

Update

I added an update to support the case where a user simply wants a field mask for every non-zero field by adding the ability to use None as a parameter:

Example getting all set fields.

from google.api_core import protobuf_helpers
...
# All set fields mask
mask = protobuf_helpers.field_mask(None, Resource)

# And conversely
mask = protobuf_helpers.field_mask(Resource, None)

@tseaver tseaver added api: core type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels May 16, 2018
@msaniscalchi
Copy link

Now that I'm working with some examples that require some slightly more sophisticated field masks, I've found an issue with the utility.

Consider the following case where we want to update a campaign:

def main(client, customer_id, campaign_id):
    campaign_service = client.get_service('CampaignService')
 
    # Create campaign operation.
    campaign_operation = client.get_type('CampaignOperation')
    campaign = campaign_operation.update
    campaign.resource_name = campaign_service.campaign_path(
        customer_id, campaign_id)
    campaign.status = client.get_type('CampaignStatusEnum').PAUSED
    campaign.network_settings.target_search_network.value = False
    fm = protobuf_helpers.field_mask(None, campaign)
    campaign_operation.update_mask.CopyFrom(fm)
 
    # Update the campaign.
    campaign_response = campaign_service.mutate_campaigns(
        customer_id, [campaign_operation])

It produces the following CampaignOperation:

update {
  resource_name: "customers/9185018835/campaigns/1455799401"
  status: PAUSED
  network_settings {
    target_search_network {
    }
  }
}
update_mask {
  paths: "resource_name"
  paths: "status"
  paths: "network_settings"
}

Note that the update_mask is incorrect, it includes "network_settings", but this value should really be "network_settings.target_search_network".

An API error results, trailing metadata includes the following raw description:

<type 'tuple'>: (_Metadatum(key='google.ads.googleads.v0.errors.googleadsfailure-bin', value='\nL\n\x02@\x03\x12BThe field mask updated a field with subfields: \'network_settings\'."\x02\n\x00'), _Metadatum(key='google.rpc.debuginfo-bin', value='\x12\xea\x02[ORIGINAL ERROR] generic::invalid_argument: The field mask updated a field with subfields: \'network_settings\'., in operation 0 [google.rpc.error_details_ext] { details { type_url: "type.googleapis.com/google.ads.googleads.v0.errors.GoogleAdsFailure" value: "\\nL\\n\\002@\\003\\022BThe field mask updated a field with subfields: \\\'network_settings\\\'.\\"\\002\\n\\000" } }'), _Metadatum(key='grpc-status-details-bin', value='\x08\x03\x12%Request contains an invalid argument.\x1a\x95\x01\nCtype.googleapis.com/google.ads.googleads.v0.errors.GoogleAdsFailure\x12N\nL\n\x02@\x03\x12BThe field mask updated a field with subfields: \'network_settings\'."\x02\n\x00\x1a\x9a\x03\n(type.googleapis.com/google.rpc.DebugInfo\x12\xed\x02\x12\xea\x02[ORIGINAL ERROR] generic::invalid_argument: The field mask updated a field with subfields: \'network_settings\'., in operation 0 [google.rpc.error_details_ext] { details { type_url: "type.googleapis.com/google.ads.googleads.v0.errors.GoogleAdsFailure" value: "\\nL\\n\\002@\\003\\022BThe field mask updated a field with subfields: \\\'network_settings\\\'.\\"\\002\\n\\000" } }'), _Metadatum(key='request-id', value='vRNsSNL_6p08Cx0-Lvem2Q'))

In contrast, setting this manually works as expected:

***
Code
***
def main(client, customer_id, campaign_id):
    campaign_service = client.get_service('CampaignService')
 
    # Create campaign operation.
    campaign_operation = client.get_type('CampaignOperation')
    campaign = campaign_operation.update
    campaign.resource_name = campaign_service.campaign_path(
        customer_id, campaign_id)
    campaign.status = client.get_type('CampaignStatusEnum').PAUSED
    campaign.network_settings.target_search_network.value = False
    #fm = protobuf_helpers.field_mask(None, campaign)
    #campaign_operation.update_mask.CopyFrom(fm)
    fm = campaign_operation.update_mask
    fm.paths.append('resource_name')
    fm.paths.append('status')
    fm.paths.append('network_settings.target_search_network')
 
    # Update the campaign.
    campaign_response = campaign_service.mutate_campaigns(
        customer_id, [campaign_operation])
 
***
CampaignOperation (w/ correct Field Mask)
***
update {
  resource_name: "customers/9185018835/campaigns/1455799401"
  status: PAUSED
  network_settings {
    target_search_network {
    }
  }
}
update_mask {
  paths: "resource_name"
  paths: "status"
  paths: "network_settings.target_search_network"
}

@landrito
Copy link
Contributor Author

Added a commit that should fix the issues raised by @msaniscalchi.

@theacodes can you take another look?

@landrito landrito removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 24, 2018
@theacodes theacodes merged commit 814e75b into googleapis:master Jul 25, 2018
@theacodes
Copy link
Contributor

Thank you, @landrito!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: core cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants