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

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

Open
BenRKarl opened this issue Jul 20, 2021 · 5 comments
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@BenRKarl
Copy link

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
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jul 21, 2021
@busunkim96 busunkim96 added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Jul 22, 2021
@tseaver tseaver self-assigned this Sep 8, 2021
@tseaver
Copy link
Contributor

tseaver commented Sep 8, 2021

@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"]

@tseaver tseaver added needs more info This issue needs more information from the customer to proceed. status: investigating The issue is under investigation, which is determined to be non-trivial. and removed status: investigating The issue is under investigation, which is determined to be non-trivial. labels Sep 8, 2021
@tseaver tseaver removed their assignment Sep 8, 2021
@parthea
Copy link
Collaborator

parthea commented Nov 13, 2021

Hi @BenRKarl ,

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

@devchas
Copy link

devchas commented Feb 25, 2022

@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 parthea added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed 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. labels Mar 8, 2022
@parthea parthea reopened this Mar 8, 2022
@parthea parthea removed the needs more info This issue needs more information from the customer to proceed. label Mar 8, 2022
@parthea
Copy link
Collaborator

parthea commented Mar 8, 2022

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

@bobhancock
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

7 participants