Optional fields in nested `oneof` set to default value left out of field mask

[BenRKarl]

Environment details

• OS type and version: glinux
• Python version: 3.7.0
• pip version: 10.0.1
• google-api-core version: 1.31.0

Steps to reproduce

Run the below code example.

Code example

import proto
from google.api_core.protobuf_helpers import field_mask

class ManualCpc(proto.Message):
    enhanced_cpc_enabled = proto.Field(proto.BOOL, number=2, optional=True,)

class ManualCpm(proto.Message):
    pass

class Campaign(proto.Message):
    manual_cpc = proto.Field(
        proto.MESSAGE,
        number=1,
        oneof="campaign_bidding_strategy",
        message=ManualCpc,
    )

    manual_cpm = proto.Field(
        proto.MESSAGE,
        number=2,
        oneof="campaign_bidding_strategy",
        message=ManualCpm,
    )

campaign = Campaign()
# Since this is an optional bool value the default is `False` so the
# field mask helper can't seem to distinguish it from an unset field.
# This use case is important for users who want to _unset_ a field
# in the API and don't want to manually add the field path to
# the update mask.
campaign.manual_cpc.enhanced_cpc_enabled = False

fm = field_mask(None, campaign._pb)

assert fm.paths == ["manual_cpc.enhanced_cpc_enabled"]

Stack trace

Traceback (most recent call last):
  File "meow.py", line 30, in <module>
    assert fm.paths == ["manual_cpc.enhanced_cpc_enabled"]
AssertionError

[tseaver]

@BenRKarl Your example confuses me a bit: you supply an original value of None, which implies an empty message of the same type as modified, with only default values. Then, in the modified message, you want a field which matches the default value to be included in the field mask, but in effect, that field hasn't changed from the implied original value, and so should not be in the mask Am I misunderstanding your intent? Users who want to clear a field which is set to a non-default value need to pass in the original message which has the value they want cleared, e.g.:

original_campaign = Campaign()
original_campaign.manual_cpc.enhanced_cpc_enabled = True

modified_campaign = Campaign()
modified_campaign.manual_cpc.enhanced_cpc_enabled = False

fm = field_mask(original_campaign._pb, modified_campaign._pb)

assert fm.paths == ["manual_cpc.enhanced_cpc_enabled"]

[parthea]

Hi @BenRKarl ,

I'm going to close this issue due to inactivity but please feel free to re-open it with more information.

[devchas]

@parthea just chiming in here with some additional context and clarify what I think the case @BenRKarl was trying to demonstrate with that example.

It is possible that supplying an original value of None isn't the right way to approach this problem. However, what we are trying to do is compare the populated modified message with a default instance for that type (an instance of the type that has no fields set).

I am coming from Java, so I'll explain how we approach this there as a parallel.

Rather than comparing modified with None, we compare the modified message with the value returned by calling getDefaultInstanceForType for the given message type. Here is an example.

Then, when determining if the the message sub-field (e.g. enhanced_cpc_enabled on the message manual_cpc) value has changed, we first perform a check to see if the parent message field (e.g. enhanced_cpc_enabled) is set on either the original or modified but not the other by calling hasField like this.

If the message is set on either the original or modified message, but not the other, we add the complete path to the update mask to get the expected result.

I think this is effectively what we're trying to accomplish here. I'm just not familiar enough with these utilities in Python to no how to go about it.

[parthea]

Thanks for providing an example @devchas. I've re-opened this issue as a feature request.

[bobhancock]

https://team-review.git.corp.google.com/c/ads-api-devrel/google-ads-java/+/1302988/8/google-ads/src/main/java/com/google/ads/googleads/lib/utils/FieldMasks.java

Adds context to the full functionality required. We want to ensure that if a blank field with populated sub-fields is supplied that the error is raised before it reaches the server.