which_one_of returns incorrect value for nested assignments

[girtsf]

If a oneof contains a submessage, and a field is assigned to this submessage, which_one_of does not report the submessage. Here is a proto file, generated code and code to demonstrate the issue.

Proto:

syntax = "proto3";

message Inner {
  int32 a = 1;
}

message Outer {
  oneof which {
    Inner inner = 1;
  }
}

Generated code + reproduction of the bug:

from dataclasses import dataclass

import betterproto


@dataclass(eq=False, repr=False)
class Inner(betterproto.Message):
    a: int = betterproto.int32_field(1)


@dataclass(eq=False, repr=False)
class Outer(betterproto.Message):
    inner: "Inner" = betterproto.message_field(1, group="which")


test = Outer()
test.inner.a = 42
print(test)  # Outer(inner=Inner(a=42))
print(betterproto.which_one_of(test, "which"))  # ('', None)   <-- WRONG

# Round trip encode / parse.
test2 = Outer().parse(bytes(test))
print(test2)  # Outer(inner=Inner(a=42))
print(betterproto.which_one_of(test2, "which"))  # ('inner', Inner(a=42))   <-- correct

[girtsf]

Quick thinking on this: have Message track its parent (if one exists). Then in __setattr__ recursively ask the parent to see if it needs to modify _group_current.

Let me know if you would like me to take a stab at this.

[kalzoo]

Good find, @girtsf - and thank you for the test case.

My first inclination is the same as yours - what makes me uneasy is that the child messages will need to keep a handle to their parent messages and mutate them. Another option is to return a proxy from the parent's __get_attribute__ so that it intercepts __setattr__ calls itself before passing them through to the child Message, and the child Message can remain unaware of its parent structure. Thoughts?

[girtsf]

Another option is to return a proxy from the parent's __get_attribute__ so that it intercepts __setattr__ calls itself before passing them through to the child Message, and the child Message can remain unaware of its parent structure. Thoughts?

I commonly end up passing submessages to functions, so for many of my use cases it would be mostly these proxy objects being passed on and operated on. At which point it becomes not that different from passing the Message itself that knows how to get to its parent (probably via a weakref to prevent cycles for garbage collection).

Looking at how google protobufs does it: they have an internal listener for each message instance that gets set when the parent instantiates it. It is used both for marking messages as used (so that empty messages can be serialized) and for storing which oneof is set. They use weakrefs to keep track of parents, and if the reference goes away, they just silently ignore it, which seems the right thing to do.

Looks like there are other bugs related to field presence, e.g., #205, #199. It would be nice to try to fix all of these by improving how field presence is tracked and set in parents.

I could take a stab at a prototype for this if there's a chance this could (eventually) get merged.

[Kludex]

I think I'm facing the same issue. :thinking:

It would be cool if every oneOf field would accept None instead. :thinking: