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

Manage optional field in Message body #579

Closed
Dufgui opened this issue Mar 5, 2024 · 3 comments
Closed

Manage optional field in Message body #579

Dufgui opened this issue Mar 5, 2024 · 3 comments

Comments

@Dufgui
Copy link

Dufgui commented Mar 5, 2024

Optional field are well manage in constructor but not in the class field declaration. I think it"s a quick fix to do in writeMessage as I can see but not sure. Today generation is:

class MyRequest(google.protobuf.message.Message):
    DESCRIPTOR: google.protobuf.descriptor.Descriptor

    field1: builtins.str
    optionalfield1: builtins.str
    optionalfield2: builtins.str
    def __init__(
        self,
        *,
        field1: builtins.str = ...,
        optionalfield1: builtins.str | None = ...,
        optionalfield2: builtins.str | None = ...,
    ) -> None: ...

But it must be:

class MyRequest(google.protobuf.message.Message):
    DESCRIPTOR: google.protobuf.descriptor.Descriptor

    field1: builtins.str
    optionalfield1: builtins.str | None
    optionalfield2: builtins.str | None
    def __init__(
        self,
        *,
        field1: builtins.str = ...,
        optionalfield1: builtins.str | None = ...,
        optionalfield2: builtins.str | None = ...,
    ) -> None: ...
@nipunn1313
Copy link
Owner

my understanding is that for proto3, scalar non-repeated fields are not actually optional on the wire, nor on the generated class. IE - if you construct it with None, my understanding is that you will get the 0-value.

if is_scalar(field) and field.label != d.FieldDescriptorProto.LABEL_REPEATED:

It's possible my understanding is wrong. I'd be welcome to a fix - as long as it includes a unit test that proves that this behavior is expected - eg

def test_constructor_proto3() -> None:

@RonaldGalea
Copy link

With the newest version of protobuf, it seems that it's possible to explicitly mark scalar fields as optional and therefore track set/not set information (see this SO post). This would allow generating the optional types, right?

@nipunn1313
Copy link
Owner

I made an error in my previous message

my understanding is that for proto3, scalar non-repeated fields are not actually optional on the wire, nor on the generated class. IE - if you construct it with None, my understanding is that you will get the 0-value.

Actually, the fields are optional on the wire, however if you access them in the generated class, you will still get the 0-value. The behavior is documented here.

https://protobuf.dev/reference/python/python-generated/#singular-fields-proto3
image

I was able to validate this with an improvement to the unit test in #655. You can check it out to prove the behavior returns the 0-value rather than None on field access.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants