fix: handle nullability for optional fields #2011
Conversation
…this only sets the default of optional fields, it doesn't control whether a field is optional or not)
|
This PR is to address #1779. I believe it is a more complete solution than my old PR, #1780. The expected behaviour IMO is that fields marked as optional (or fields in a one-of) should have nullable type hints and a null default, while fields marked as required (proto2) or with no annotation (proto3) should be marked not-nullable and have a zero default. In proto3, although technically everything is optional, the protobuf system should provide a default value making it not-null in practice if the optional flag is not set, which is consistent with other implementations. After getting more into the code, the real culprit here is the Field rule. A rule of "proto3_optional" or "optional" can get set in the parser, but these both get converted to "optional" and then ultimately discarded, before reaching the generator. To avoid losing this information in proto3, I added a proto3Optional member to the Field class which gets set before the rule is discarded. I also made the parser store the proto syntax as an option on the Root object during the parse. With these two bits of information together, there is enough info to determine what is optional in the static generator. The new functionality is behind a --force-optional flag, which sets nullability based on the optional keyword. The default is false for now, but it could be changed later. I added a couple of tests to show this is working as expected. I chose that name to match the existing flags but happy to change to whatever you think will be most clear / consistent. Lastly, I have a question about JSON descriptors. Are these always created by running the parser and storing the result? If so they should load back in with the syntax option and work as expected. If there is another way of creating these objects that doesn't go through the parser, the syntax option will be missing and the static generator would fall back on proto2 syntax. Please do let me know your thoughts on this when you get time. I'm still very keen to get this fix into my project, if there are things you want tweaking / adding I'm happy to do it. |
|
Thank you to whoever approved the CI workflows. There were a few lint errors, I have fixed and re-pushed, could you approve the workflows to run again? |
|
Thanks for all the feedback. I believe I have addressed all the comments, changes are pushed. I believe all the tests / linting should be up to date now if you want to re-run CI. After thinking about the "new behaviour" / "old behaviour" comment, I changed the guard flag to --null-semantics. I think this is more descriptive. With this flag, the nullability of fields in messages / interfaces reflects the semantics described in the proto file. This allows bringing in various fixes under the one flag. In particular, I have folded in the change from PR #2013 (issue #2012) regarding nullability when message types are composed. It gets away from having multiple flags, keeps backwards compatibility and keeps open the option to make semantic nulls the default in future if people think that is the right thing to do. If you're happy to take this as one change for Semantic Nulls, I can close issue #2012 and PR #2013 as these would no longer be needed. Lastly on the coming changes for edition syntax. These methods will for sure need updating. Happy to help at that point if that is useful. Also I left an issue on the protobuf GitHub page requesting the optional keyword be maintained as syntactic sugar for [features.field_presence = EXPLICIT]. After spending some time working with this, I believe nullability is a core part of the semantic type definition, not a technical annotation for how a type should be handled. Time will tell, whether anyone on the protobuf team agrees! Here is the link: protocolbuffers/protobuf#17624 |
There was a problem hiding this comment.
LGTM, I'd like a second opinion from @dcodeIO, if possible. I'm still familiarizing myself with the library.
Co-authored-by: Daniel Bankhead <[email protected]>
|
Thanks @danielbankhead - I committed the update you suggested. |
…n properties / interfaces
|
One more update. I've let implicit optional fields (proto3), repeated fields and maps be omitted from the properties. However, if specified, these fields must have a non-null value. I think this is more consistent with protobuf semantics, that assigns default values to these fields if they are missing. This only affects calls to create() / new, implicit fields are non-null once they are constructed. |
|
Thanks @sofisl, I have updated all the code touched by this change to use braces for every if/else statement |
|
Hello - I have closed PR #2013 as that change is now fully covered by this PR. Also I have marked all the conversations resolved since those changes are all pushed. I believe we just need a final review now? @dcodeIO - Please let me know if there is anything you would like added / removed / changed, very happy to do the work and get this closed off! |
|
Thank you @danielbankhead for merging this! Was very happy yesterday when I saw it had gone in. Do you have any idea yet what the schedule is for the next release? |
sofisl
changed the title
|
@martin-traverse we're working on cutting a release on Monday: |
|
Hi @danielbankhead - it looks like this release didn't get to NPM yet, is there another step that needs to happen? I saw the Github release went out on Friday, but NPM is still showing v7.3.2 as the latest version. |
|
Hey @martin-traverse, we just published: |
|
Thank you so much! I have updated our mainline with the new package. |
BEGIN_COMMIT_OVERRIDE
fix: handle nullability for optional fields
END_COMMIT_OVERRIDE