Optional types nullability - not working correctly for TS

[martin-traverse]

protobuf.js version: 7.0.0

I don't think optional types are working correctly. My understanding is that optional types (including proto3 optionals) should have a type signature of "|null|undefined", while non-optional types should just have the base type signature. Currently, this is being applied to member definitions in JS, but not to the property type hints set at the class level. This cascades through in the TS, where optional types are respected in the concrete type definitions, but not the interface definitions where everything is nullable.

Is this diference in the type definitions intentional, or not? Even though concrete classes are returned from API calls, the nested types they are built from use interfaces. So, the net result is that our web developers still have to treat everything as nullable, regardless of what is defined as optional in the original proto files. The behviour we would like is that optional fields show as nullable, while non-optional fields do not and are supplied a default value if they are missing. I have include some example outputs below.

The root cause seems to be in static.js in the CLI, where nullability for class property comments is defined differently than for members. I will link a PR to change this, which is fairly simple. However, I realise this is a change to API and may not be desirable for everyone! So my questions are:

1. Does this all make sense and some sort of patch is needed, or have I missed something obvious?!

and

2. Assuming a fix is helpful in some cases, should it be guarded behind a CLI option? If so what that be?

Sample proto file

syntax = "proto3";
package tracdap.api;
...

message FileWriteRequest {

    /// Tenant code for the requested operation, always required
    string tenant = 1;

    /// Prior object/tag version to use for update operations
    optional metadata.TagSelector priorVersion = 2;
    ...

Sample output which shows the problem

JS file has "tenant" as nullable in the object properties, but not the member definition

        api.FileWriteRequest = (function() {

            /**
             * Properties of a FileWriteRequest.
             * @memberof tracdap.api
             * @interface IFileWriteRequest
             * @property {string|null} [tenant] Tenant code for the requested operation, always required
             * @property {tracdap.metadata.ITagSelector|null} [priorVersion] Prior object/tag version to use for update operations
            
            ...
            
            /**
             * Tenant code for the requested operation, always required
             * @member {string} tenant
             * @memberof tracdap.api.FileWriteRequest
             * @instance
             */
            FileWriteRequest.prototype.tenant = "";

            /**
             * Prior object/tag version to use for update operations
             * @member {tracdap.metadata.ITagSelector|null|undefined} priorVersion
             * @memberof tracdap.api.FileWriteRequest
             * @instance
             */
            FileWriteRequest.prototype.priorVersion = null;

TS file has "tenant" nullable in the interface, but not the concrete class

    interface IFileWriteRequest {

        /** Tenant code for the requested operation, always required */
        tenant?: (string|null);

        /** Prior object/tag version to use for update operations */
        priorVersion?: (tracdap.metadata.ITagSelector|null);
   
   ...
   
    class FileWriteRequest implements IFileWriteRequest {

        /** Tenant code for the requested operation, always required */
        public tenant: string;

        /** Prior object/tag version to use for update operations */
        public priorVersion?: (tracdap.metadata.ITagSelector|null);

After the change, the JS properties line up with the member definition and the TS interface lines up with the concrete class.

[martin-traverse]

Relevant pr is #1780

[alexander-fenster]

Hi @martin-traverse,

Thank you for the PR! I'm indeed afraid that this type change might be breaking for some users, even though the prior types were indeed incorrect. If it's easy to put this behind an option until the next major release, that would be the safest way to go. Otherwise, let me try to play with the code and see how bad the impact could be :)

[hxsf]

@alexander-fenster Maybe we can add an option like --pb3-optional to cli.

[martin-traverse]

Hello,

Apologies for the delay, I have updated the PR to use the --pb3-optional flag to guard the new functionality as suggested. I have tested on my own project with and without the flag and it all works as expected, without the flag the generated code is the same as before.

I believe this should be OK to merge now? Do you agree or is there anything else you would like me to do?

[martin-traverse]

Hi - just an update to say I have rebased the PR for this issue, #1780, as there were some conflicting changes on master. Are you still happy to merge? As discussed above the --pb3-optional flag is in place.

[davidmcclelland]

Any updates on this PR being accepted?

[nicholasngai]

@alexander-fenster Can we take a look at merging this in soon? We would like to be able to use proto3 field presence semantics in our code using this library, and it looks like the linked PR is exactly what we need to support proto3 properly!

[martin-traverse]

Hi @alexander-fenster, @nicholasngai, @davidmcclelland,

I have rebased and pushed the code. As discussed above, the change is protected by the --pb3-optional flag, so it will be non-breaking for existing users. I think all it needs is review and approval.

We'd still really like this fix for our project as well!

[martin-traverse]

Hello @alexander-fenster & @Asondo,

I took another look at this today and have come up with a new implementation. Please see PR #2011.

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.

[martin-traverse]

Hi, I spent more time on this today. There was an issue with virtual oneOf groups showing up in client code for proto3, also I found proto3_optional in the field options which saves the need to add a member to Field. PR is updated.

Perhaps more interesting, I think there is an issue with the behaviour of --force-message that could be confusing things. I will raise a separate ticket / PR to discuss that point.

Apologies for the flurry of activity after so long - we're working on our web bindings so I thought I'd try to bottom out some of these issues, which have been long-standing for us.

[martin-traverse]

After review comments on the PR, I changed the guard flag for this feature 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 issue #2012 regarding nullability when message types are composed. It gets away from having multiple flags and still keeps backwards compatibility. If people are happy with that approach I can close #2012 and the associated PR.

[martin-traverse]

The fix for this is now released! It is available in the new 7.3.3 / CLI 1.1.3 packages on NPMJS and I've been able to use the fix in the project I look after.

Closing this issue as fixed!