Skip to content

Lazy initialisation of default values can cause unexpected behaviour #347

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

Closed
abn opened this issue Mar 2, 2022 · 2 comments · Fixed by #348
Closed

Lazy initialisation of default values can cause unexpected behaviour #347

abn opened this issue Mar 2, 2022 · 2 comments · Fixed by #348
Labels
bug Something isn't working

Comments

@abn
Copy link
Collaborator

abn commented Mar 2, 2022

I have the following code genereated from the openconfig/gnmi proto files.

@dataclass(eq=False, repr=False)
class Update(betterproto.Message):
    """
    Update is a re-usable message that is used to store a particular Path,
    Value pair. Reference: gNMI Specification Section 2.1
    """

    path: "Path" = betterproto.message_field(1)
    value: "Value" = betterproto.message_field(2)
    val: "TypedValue" = betterproto.message_field(3)
    duplicates: int = betterproto.uint32_field(4)

    def __post_init__(self) -> None:
        super().__post_init__()
        if self.value:
            warnings.warn("Update.value is deprecated", DeprecationWarning)

Field value is deprecated in favour of val. See relevant content from proto files.

message Update {
  Path path = 1;                      // The path (key) for the update.
  Value value = 2 [deprecated=true];  // The value (value) for the update.
  TypedValue val = 3;                 // The explicitly typed update value.
  uint32 duplicates = 4;              // Number of coalesced duplicates.
}

message Value {
  option deprecated = true;
  bytes value = 1;      // Value of the variable being transmitted.
  Encoding type = 2;    // Encoding used for the value field.
}

Due to the changes introduced in #130, specifically in Message.__getattribute__, when the deprecation logic checks value (Update.value), this value gets set via this logic.

def __getattribute__(self, name: str) -> Any:
"""
Lazily initialize default values to avoid infinite recursion for recursive
message types
"""
value = super().__getattribute__(name)
if value is not PLACEHOLDER:
return value
value = self._get_field_default(name)
super().__setattr__(name, value)
return value

The side-effects of this are the following:

  1. The deprecation warning for Update.value is not raised.
  2. A new instance (default value) of Value() is initialised. This raises a deprecation warning for the message itself. This happens in cases where this is not set explicitly.
  3. In cases where implementations check both Update.value and Update.val, we could end up with unexpected behaviour as the values contained in these might differ as one is default and the other is not.

Expected Behaviour

  • Access to deprecated field when not set should be None.
  • No deprecation warnings are raised when fields are not explicitly set.
@abn
Copy link
Collaborator Author

abn commented Mar 2, 2022

cc: @Gobot1234 @nat-n @chris-chambers as this stems from the code introduced in #130 to handle recursive message types as far as I can tell.

@Gobot1234 Gobot1234 added the bug Something isn't working label Mar 2, 2022
abn added a commit to abn/python-betterproto that referenced this issue Mar 2, 2022
This change ensures that deprecation warnings are only raised when
either a deprecated field is explicitly set or a deprecated message is
initialised.

Resolves: danielgtaylor#347
@abn
Copy link
Collaborator Author

abn commented Mar 2, 2022

@Gobot1234 I have proposed a fixe for this at b1fd73d. However, will wait till #345 and #346 to be merged before creating the pull request as CI will fail without #345.

abn added a commit to abn/python-betterproto that referenced this issue Mar 3, 2022
This change ensures that deprecation warnings are only raised when
either a deprecated field is explicitly set or a deprecated message is
initialised.

Resolves: danielgtaylor#347
abn added a commit to abn/python-betterproto that referenced this issue Mar 3, 2022
This change ensures that deprecation warnings are only raised when
either a deprecated field is explicitly set or a deprecated message is
initialised.

Resolves: danielgtaylor#347
Gobot1234 pushed a commit that referenced this issue Mar 11, 2022
This change ensures that deprecation warnings are only raised when
either a deprecated field is explicitly set or a deprecated message is
initialised.

Resolves: #347
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants