fix: handle nullability for optional fields

cli/README.md

@@ -68,6 +68,7 @@ Translates between file formats and generates static code.
   --force-long     Enforces the use of 'Long' for s-/u-/int64 and s-/fixed64 fields.
   --force-number   Enforces the use of 'number' for s-/u-/int64 and s-/fixed64 fields.
   --force-message  Enforces the use of message instances instead of plain objects.
+  --force-optional Enforces nullable types for fields marked as optional (proto2 and proto3)
 
 usage: pbjs [options] file1.proto file2.json ...  (or pipe)  other | pbjs [options] -
 ```

cli/pbjs.js

@@ -41,7 +41,7 @@ exports.main = function main(args, callback) {
             "force-message": "strict-message"
         },
         string: [ "target", "out", "path", "wrap", "dependency", "root", "lint", "filter" ],
-        boolean: [ "create", "encode", "decode", "verify", "convert", "delimited", "typeurl", "beautify", "comments", "service", "es6", "sparse", "keep-case", "alt-comment", "force-long", "force-number", "force-enum-string", "force-message", "null-defaults" ],
+        boolean: [ "create", "encode", "decode", "verify", "convert", "delimited", "typeurl", "beautify", "comments", "service", "es6", "sparse", "keep-case", "alt-comment", "force-long", "force-number", "force-enum-string", "force-message", "force-optional", "null-defaults"],
         default: {
             target: "json",
             create: true,
@@ -62,7 +62,8 @@ exports.main = function main(args, callback) {
             "force-number": false,
             "force-enum-string": false,
             "force-message": false,
-            "null-defaults": false,
+            "force-optional": false,
+            "null-defaults": false
         }
     });
 
@@ -146,8 +147,9 @@ exports.main = function main(args, callback) {
                 "  --force-long     Enforces the use of 'Long' for s-/u-/int64 and s-/fixed64 fields.",
                 "  --force-number   Enforces the use of 'number' for s-/u-/int64 and s-/fixed64 fields.",
                 "  --force-message  Enforces the use of message instances instead of plain objects.",
+                "  --force-optional Enforces nullable types for fields marked as optional (proto2 and proto3)",
                 "",
-                "  --null-defaults  Default value for optional fields is null instead of zero value.",
+                "  --null-defaults  Default value for optional fields is null instead of zero value (deprecated, use --force-optional instead)",
                 "",
                 "usage: " + chalk.bold.green("pbjs") + " [options] file1.proto file2.json ..." + chalk.gray("  (or pipe)  ") + "other | " + chalk.bold.green("pbjs") + " [options] -",
                 ""

cli/targets/static.js

@@ -354,8 +354,48 @@
     return type;
 }
 
+function syntaxForType(type) {
+
+    var syntax = null;
+    var namespace = type;
+
+    while (syntax === null && namespace !== null) {
+        if (namespace.options != null && "syntax" in namespace.options)
+            syntax = namespace.options["syntax"];
+        else
+            namespace = namespace.parent;
+    }
+
+    return (syntax === "proto3") ? "proto3" : "proto2";
+}
+
+function isOptional(field, syntax) {
+
+    // In proto3, optional fields are explicit
+    if (syntax === "proto3")
+        return field.options != null && field.options["proto3_optional"] === true;
+
+    // In proto2, fields are explicitly optional if they are not part of a map, array or oneOf group
+    return field.optional && !(field.partOf || field.repeated || field.map);
+}
+
+function isOptionalOneOf(oneof, syntax) {
+
+    if (syntax !== "proto3")
+        return false;
+
+    if (oneof.fieldsArray == null || oneof.fieldsArray.length !== 1)
+        return false;
+
+    var field = oneof.fieldsArray[0];
+
+    return field.options != null && field.options["proto3_optional"] === true;
+}
+
 function buildType(ref, type) {
 
+    var syntax = syntaxForType(type);
+
     if (config.comments) {
         var typeDef = [
             "Properties of " + aOrAn(type.name) + ".",
@@ -366,9 +406,24 @@
             var prop = util.safeProp(field.name); // either .name or ["name"]
             prop = prop.substring(1, prop.charAt(0) === "[" ? prop.length - 1 : prop.length);
             var jsType = toJsType(field);
-            if (field.optional)
-                jsType = jsType + "|null";
-            typeDef.push("@property {" + jsType + "} " + (field.optional ? "[" + prop + "]" : prop) + " " + (field.comment || type.name + " " + field.name));
+            var nullable = false;
+
+            // New behaviour - respect explicit optional semantics in both proto2 and proto3
+            if (config["force-optional"]) {
+                if (isOptional(field, syntax) || field.partOf || field.repeated || field.map) {
+                    jsType = jsType + "|null";
+                    nullable = true;
+                }
+            }
+            // Old behaviour - field.optional is true for all fields in proto3
+            else {
+                if (field.optional) {
+                    jsType = jsType + "|null";
+                    nullable = true;
+                }
+            }
+
+            typeDef.push("@property {" + jsType + "} " + (nullable ? "[" + prop + "]" : prop) + " " + (field.comment || type.name + " " + field.name));
         });
         push("");
         pushComment(typeDef);
@@ -394,8 +449,19 @@
         if (config.comments) {
             push("");
             var jsType = toJsType(field);
-            if (field.optional && !field.map && !field.repeated && (field.resolvedType instanceof Type || config["null-defaults"]) || field.partOf)
-                jsType = jsType + "|null|undefined";
+
+            // New behaviour - fields explicitly marked as optional and members of a one-of are nullable
+            // Maps and repeated fields are not nullable, they default to empty instances
+            if (config["force-optional"]) {
+                if (isOptional(field, syntax) || field.partOf)
+                    jsType = jsType + "|null|undefined";
+            }
+            // Old behaviour - field.optional is true for all fields in proto3
+            else {
+                if (field.optional && !field.map && !field.repeated && (field.resolvedType instanceof Type || config["null-defaults"]) || field.partOf)
+                    jsType = jsType + "|null|undefined";
+            }
+
             pushComment([
                 field.comment || type.name + " " + field.name + ".",
                 "@member {" + jsType + "} " + field.name,
@@ -406,11 +472,16 @@
             push("");
             firstField = false;
         }
+        // New behaviour sets a null default when the optional keyword is used explicitly
+        // Old behaviour considers all proto3 fields optional and uses the null-defaults config flag
+        var nullDefault = config["force-optional"]
+            ? isOptional(field, syntax)
+            : field.optional && config["null-defaults"];
         if (field.repeated)
             push(escapeName(type.name) + ".prototype" + prop + " = $util.emptyArray;"); // overwritten in constructor
         else if (field.map)
             push(escapeName(type.name) + ".prototype" + prop + " = $util.emptyObject;"); // overwritten in constructor
-        else if (field.partOf || field.optional && config["null-defaults"])
+        else if (field.partOf || nullDefault)
             push(escapeName(type.name) + ".prototype" + prop + " = null;"); // do not set default value for oneof members
         else if (field.long)
             push(escapeName(type.name) + ".prototype" + prop + " = $util.Long ? $util.Long.fromBits("
@@ -436,12 +507,17 @@
         }
         oneof.resolve();
         push("");
-        pushComment([
-            oneof.comment || type.name + " " + oneof.name + ".",
-            "@member {" + oneof.oneof.map(JSON.stringify).join("|") + "|undefined} " + escapeName(oneof.name),
-            "@memberof " + exportName(type),
-            "@instance"
-        ]);
+        if (isOptionalOneOf(oneof, syntax)) {
+            push("// Virtual OneOf for proto3 optional field");
+        }
+        else {
+            pushComment([
+                oneof.comment || type.name + " " + oneof.name + ".",
+                "@member {" + oneof.oneof.map(JSON.stringify).join("|") + "|undefined} " + escapeName(oneof.name),
+                "@memberof " + exportName(type),
+                "@instance"
+            ]);
+        }
         push("Object.defineProperty(" + escapeName(type.name) + ".prototype, " + JSON.stringify(oneof.name) +", {");
         ++indent;
             push("get: $util.oneOfGetter($oneOfFields = [" + oneof.oneof.map(JSON.stringify).join(", ") + "]),");

src/parse.js

@@ -263,6 +263,10 @@ function parse(source, root, options) {
         if (!isProto3 && syntax !== "proto2")
             throw illegal(syntax, "syntax");
 
+        // Syntax is needed to understand the meaning of the optional field rule
+        // Otherwise the meaning is ambiguous between proto2 and proto3
+        root.setOption("syntax", syntax);
+
         skip(";");
     }
 

tests/cli.js

@@ -165,6 +165,72 @@ tape.test("with null-defaults, absent optional fields have null values", functio
 });
 
 
+tape.test("with force-optional, optional fields are handled correctly in proto2", function(test) {
+    cliTest(test, function() {
+        var root = protobuf.loadSync("tests/data/cli/null-defaults.proto");
+        root.resolveAll();
+
+        var staticTarget = require("../cli/targets/static");
+
+        staticTarget(root, {
+            create: true,
+            decode: true,
+            encode: true,
+            convert: true,
+            comments: true,
+            "force-optional": true,
+        }, function(err, jsCode) {
+
+            test.error(err, 'static code generation worked');
+
+            test.ok(jsCode.includes("@property {number|null|undefined} [c] OptionalFields c"), "Property for c should be nullable")
+            test.ok(jsCode.includes("@member {number|null|undefined} c"), "Member for c should be nullable")
+            test.ok(jsCode.includes("OptionalFields.prototype.c = null;"), "Initializer for c should be null")
+
+            test.ok(jsCode.includes("@property {number} d OptionalFields d"), "Property for d should not be nullable")
+            test.ok(jsCode.includes("@member {number} d"), "Member for d should not be nullable")
+            test.ok(jsCode.includes("OptionalFields.prototype.d = 0;"), "Initializer for d should be zero")
+
+            test.end();
+        });
+    });
+});
+
+
+tape.test("with force-optional, optional fields are handled correctly in proto3", function(test) {
+    cliTest(test, function() {
+        var root = protobuf.loadSync("tests/data/cli/null-defaults-proto3.proto");
+        root.resolveAll();
+
+        var staticTarget = require("../cli/targets/static");
+
+        staticTarget(root, {
+            create: true,
+            decode: true,
+            encode: true,
+            convert: true,
+            comments: true,
+            "force-optional": true,
+        }, function(err, jsCode) {
+
+            console.log(jsCode);
+
+            test.error(err, 'static code generation worked');
+
+            test.ok(jsCode.includes("@property {number|null|undefined} [c] OptionalFields c"), "Property for c should be nullable")
+            test.ok(jsCode.includes("@member {number|null|undefined} c"), "Member for c should be nullable")
+            test.ok(jsCode.includes("OptionalFields.prototype.c = null;"), "Initializer for c should be null")
+
+            test.ok(jsCode.includes("@property {number} d OptionalFields d"), "Property for d should not be nullable")
+            test.ok(jsCode.includes("@member {number} d"), "Member for d should not be nullable")
+            test.ok(jsCode.includes("OptionalFields.prototype.d = 0;"), "Initializer for d should be zero")
+
+            test.end();
+        });
+    });
+});
+
+
 tape.test("pbjs generates static code with message filter", function (test) {
     cliTest(test, function () {
         var root = protobuf.loadSync("tests/data/cli/test-filter.proto");

tests/data/cli/null-defaults-proto3.proto

@@ -0,0 +1,12 @@
+syntax = "proto3";
+
+message OptionalFields {
+  message SubMessage {
+    string a = 1;
+  }
+
+  optional SubMessage a = 1;
+  optional string b = 2;
+  optional uint32 c = 3;
+  uint32 d = 4;
+}

tests/data/cli/null-defaults.proto

@@ -8,4 +8,5 @@ message OptionalFields {
   optional SubMessage a = 1;
   optional string b = 2;
   optional uint32 c = 3;
+  required uint32 d = 4;
 }