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

Implement Field Presence #1406

Open
drungrin opened this issue May 12, 2020 · 7 comments
Open

Implement Field Presence #1406

drungrin opened this issue May 12, 2020 · 7 comments

Comments

@drungrin
Copy link

Protobuf release 3.12 adds experimental support for optional fields in proto3. Proto3 optional fields track presence like in proto2.

https://github.com/protocolbuffers/protobuf/blob/master/docs/field_presence.md

https://github.com/protocolbuffers/protobuf/blob/master/docs/implementing_proto3_presence.md

@yinzara
Copy link

yinzara commented Apr 22, 2021

We'd REALLY love if this was implemented :) optional fields is the biggest drawback we've had with implementing protobuf and without this implementation we have to use wrapper types which also don't have the nicest implementation either

@kskalski
Copy link

Note that currently the field presence kind of works already even though there is not special API or official support in the library. In my project I use optional fields extensively and the messages parsed by protobufjs seem to follow the semantics of field presence:

  • non-set optional fields get the null value
  • optional fields set to default value (e.g. 0 for int32) do get their value propagated and the message get an initialized field with that value

@alexander-fenster
Copy link
Contributor

alexander-fenster commented Apr 28, 2021

Hi folks, I added this feature in #1584, please feel free to npm install protobufjs@next and check if it works for you. To be released soon.

@kskalski
Copy link

After this change, is it expected that .toObject(from_pb, { defaults: true }) will not populate the optional fields that are not explicitly set on the from_pb message? I'm observing a change in semantics compared to previous library version... though it's not completely clear what the behavior should be.

In other languages unset optional field will still return the default value when accessed (through getter) and it has separate API for checking whether it is set or not.
I suppose it is not the way it works in Javascript, where the implementation is using undefined for the unset fields. I think this is fine, though it would still be useful to provide a way to access the defaults as it's possible in other languages. In the previous version I relied on the way optional fields were treated:

  • PB.create() or deserializing the message did not provide any values for those fields
  • but .toObject(from_pb, { defaults: true }) did populate the default values for both optional and regular fields

I imagine the easiest way to provide both semantics to the user would be to introduce another option to toObject method, like optionalDefaults that would allow us to control which defaults are populated (alternatively bring back previous semantics of defualts: true populating all fields by making it imply optonalDefaults: true when it is not provided).

What are your thoughts? Or maybe protobufjs should follow instead the way other languages approach optonal API and create separate methods for checking presence?

@kskalski
Copy link

One more question - I noticed that for optional fields there are _fieldname: "fieldname" members generated in the API. Is this really necessary? I suppose the syntheric oneofs should be invisible to the end user, the proto3 docs seem to suggest it too:
"Avoid generating any oneof-based accessors for the synthetic oneof. Its only purpose is to make reflection-based algorithms work properly if they are not aware of proto3 presence. The synthetic oneof should not appear anywhere in the generated API."

Addition of those fields brings a lot of clutter. They double the footprint of the objects with no apparent value, especially that they are kind of trivial constant value accessors.

@alexander-fenster
Copy link
Contributor

The spec only asks not to generate accessors (getFoo, setFoo in other languages) which I believe we don't do anyway. I'm not sure if it's possible to hide those synthetic oneofs completely :(

@alexander-fenster
Copy link
Contributor

As for the methods to check presence, this would obviously be a cleaner solution than the one we have now but I don't think I'll be able to find time to contribute it. PRs are welcome! :)

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

4 participants