-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: add null-defaults option #1611
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
syntax = "proto2"; | ||
|
||
message OptionalFields { | ||
message SubMessage { | ||
required string a = 1; | ||
} | ||
|
||
optional SubMessage a = 1; | ||
optional string b = 2; | ||
optional uint32 c = 3; | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be only for
proto2
?does it make sense to test it against
proto3
too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--null-defaults
is already the default behavior for proto3 according to @alexander-fenster :There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it's not exactly how it works. Internally missing values might be represented by undefined or null, but the getters pervert his behavior, returning non-null defaults.
protobuf.js/src/field.js
Line 313 in 6c4d307
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting this @castarco. We mostly use static objects (generated by
pbjs -t static-module
) and the default values are set tonull
there for optional fields; I haven't seen any bad consequences of thefield.js
code but I guess we just don't have this use case in our code base. I'll look into this, I believe it should not be hard to fix it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I was talking about (
proto3
optional fields are internallyoneof
members so getnull
as their default values):protobuf.js/cli/targets/static.js
Line 414 in 90afe44
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm maintaining a typescript code generator ( https://github.com/join-com/protoc-gen-ts/ , quite opinionated, for the specific uses of my company, although it would be usable for others too ), and I had to implement a fix today because of this:
https://github.com/join-com/protoc-gen-ts/pull/37/files#diff-9e3e664b405f7e977363c9421812babe65c6c2ad5f5715f8cab534dd7ee2be78R108
(btw now I realize that the PR did not include the generator changes 😅 , only the results...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
today I learned: you need to link to the specific commit SHA to make GitHub magic work :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this how it is supposed to work? To always use
oneOf
fields if we want truly nullable fields in proto3? 😢This is an example of a generated file:
https://github.com/join-com/protoc-gen-ts/blob/master/tests/__tests__/generated/Test.ts
As in
proto3
all fields are "optional" by default, I did not add theoptional
parameter to the@Field.d
decorator (example: https://github.com/join-com/protoc-gen-ts/blob/master/tests/__tests__/generated/Test.ts#L342), would it make a difference? (I thoughtoptional
andrequired
are only applicable toproto2
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is how this is supposed to work in proto3. Original design: https://github.com/protocolbuffers/protobuf/blob/master/docs/implementing_proto3_presence.md - I just implemented it as is.
The thing is, message fields are always optional. The main reason of having optional primitive fields is to distinguish between integer 0 and "unset" value, same for strings and booleans. Putting them into a hidden
oneof
is apparently the easiest non-breaking solution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional
fields are supported in proto3 starting fromprotoc
3.12 (behind the flag) and IIRC 3.15 without the flag.