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

proto2 default optional fields construction behaviour #319

Closed
frarees opened this issue Sep 18, 2015 · 6 comments
Closed

proto2 default optional fields construction behaviour #319

frarees opened this issue Sep 18, 2015 · 6 comments

Comments

@frarees
Copy link

frarees commented Sep 18, 2015

Hi,

Reading the official language guide, I came across the following clause:

If the default value is not specified for an optional element, a type-specific default value is used instead: for strings, the default value is the empty string. For bools, the default value is false. For numeric types, the default value is zero. For enums, the default value is the first value listed in the enum's type definition.

When using ProtoBuf.js, I construct via builder a message (new MyMessage ()). All optional fields are null, but I guess they should be set to its type's default value, as stated above i.e. integers as zero, booleans as false and strings as empty strings. Even after decoding, if optional fields were not in the wire data they will remain null instead of having their protocol default values.

Does this means ProtoBuf.js construction is not compliant with the protocol?

I've edited the description to better state the issue

@dcodeIO
Copy link
Member

dcodeIO commented Sep 19, 2015

It is compliant with how the wire format looks. The API might differ for a lot of reasons, though, most notably for the fact that this library has been built from scratch while having wire format compatibility in mint, but without the goal to be API-compatible to other implementations, which is inevitable anyway because this is JavaScript.

@frarees
Copy link
Author

frarees commented Sep 19, 2015

I see. Currently we're using this library to serve a REST API. Clients are implemented in C# using another implementation, so basically we need interop pretty much... Are there any downsides you spotted of using typed construction?

If my message field is defined as optional bool foo = 0 I expect js object m.foo to be true or false but not null. Otherwise, we'd have to write conditionals per field and assigning default values by ourselves when needed, making decoding dirtier and error prone.

Anyway, if you think it's not a good move I could always fork and manage that by ourselves I guess..

@seishun
Copy link
Contributor

seishun commented Sep 19, 2015

It's not compliant in the sense that optional bool foo = 0; should be equivalent to optional bool foo = 0 [default = false];, but they are treated differently by ProtoBuf.js.

Despite the fact that this is JavaScript, there is a way to have an API that works the same as the official implementations, which I described here: #200 (comment).

@frarees
Copy link
Author

frarees commented Sep 23, 2015

Explicitly exposing default values made it work for us. It's a sensible workaround ATM. #200 +1

@atomizer
Copy link

Looks like this can be fixed by changing this logic: https://github.com/dcodeIO/protobuf.js/blob/72bc66ccdfcf66c1645c5de311bf26078d411cac/src/ProtoBuf/Reflect/Message/Field.js#L148-L155
Line 151 must also run in case of proto2 syntax and optional field with no explicit default value.

@dcodeIO
Copy link
Member

dcodeIO commented Nov 28, 2016

Closing this for now.

protobuf.js 6.0.0

Feel free to send a pull request if this is still a requirement.

@dcodeIO dcodeIO closed this as completed Nov 28, 2016
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

Successfully merging a pull request may close this issue.

4 participants