Skip to content

Commit

Permalink
feat: add null-defaults option (#1611)
Browse files Browse the repository at this point in the history 
* feat: add null-defaults option

* fix: linting

Co-authored-by: Alexander Fenster <github@fenster.name>
Co-authored-by: Alexander Fenster <fenster@google.com>
  • Loading branch information
@sloonz @alexander-fenster
3 people committed May 8, 2021
1 parent 9011aac commit 6e713ba
Show file tree
Hide file tree
Showing 4 changed files with 145 additions and 52 deletions.
7 changes: 5 additions & 2 deletions cli/pbjs.js
View file
Expand Up @@ -41,7 +41,7 @@ exports.main = function main(args, callback) {
"force-message": "strict-message"
},
string: [ "target", "out", "path", "wrap", "dependency", "root", "lint" ],
boolean: [ "create", "encode", "decode", "verify", "convert", "delimited", "beautify", "comments", "service", "es6", "sparse", "keep-case", "force-long", "force-number", "force-enum-string", "force-message" ],
boolean: [ "create", "encode", "decode", "verify", "convert", "delimited", "beautify", "comments", "service", "es6", "sparse", "keep-case", "force-long", "force-number", "force-enum-string", "force-message", "null-defaults" ],
default: {
target: "json",
create: true,
Expand All @@ -59,7 +59,8 @@ exports.main = function main(args, callback) {
"force-long": false,
"force-number": false,
"force-enum-string": false,
"force-message": false
"force-message": false,
"null-defaults": false,
}
});

Expand Down Expand Up @@ -139,6 +140,8 @@ exports.main = function main(args, callback) {
" --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.",
"",
" --null-defaults Default value for optional fields is null instead of zero value.",
"",
"usage: " + chalk.bold.green("pbjs") + " [options] file1.proto file2.json ..." + chalk.gray(" (or pipe) ") + "other | " + chalk.bold.green("pbjs") + " [options] -",
""
].join("\n"));
Expand Down
4 changes: 2 additions & 2 deletions cli/targets/static.js
View file
Expand Up @@ -394,7 +394,7 @@ function buildType(ref, type) {
if (config.comments) {
push("");
var jsType = toJsType(field);
if (field.optional && !field.map && !field.repeated && field.resolvedType instanceof Type || field.partOf)
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 + ".",
Expand All @@ -410,7 +410,7 @@ function buildType(ref, type) {
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)
else if (field.partOf || field.optional && config["null-defaults"])
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("
Expand Down
175 changes: 127 additions & 48 deletions tests/cli.js
View file
Expand Up @@ -5,7 +5,7 @@ var path = require("path");
var Module = require("module");
var protobuf = require("..");

tape.test("pbjs generates static code", function(test) {
function cliTest(test, testFunc) {
// pbjs does not seem to work with Node v4, so skip this test if we're running on it
if (process.versions.node.match(/^4\./)) {
test.end();
Expand All @@ -23,54 +23,133 @@ tape.test("pbjs generates static code", function(test) {
};
require.cache.protobufjs = require.cache[path.resolve("index.js")];

var staticTarget = require("../cli/targets/static");

var root = protobuf.loadSync("tests/data/cli/test.proto");
root.resolveAll();

staticTarget(root, {
create: true,
decode: true,
encode: true,
convert: true,
}, function(err, jsCode) {
test.error(err, 'static code generation worked');

// jsCode is the generated code; we'll eval it
// (since this is what we normally does with the code, right?)
// This is a test code. Do not use this in production.
var $protobuf = protobuf;
eval(jsCode);

var OneofContainer = protobuf.roots.default.OneofContainer;
var Message = protobuf.roots.default.Message;
test.ok(OneofContainer, "type is loaded");
test.ok(Message, "type is loaded");

// Check that fromObject and toObject work for plain object
var obj = {
messageInOneof: {
value: 42,
},
regularField: "abc",
};
var obj1 = OneofContainer.toObject(OneofContainer.fromObject(obj));
test.deepEqual(obj, obj1, "fromObject and toObject work for plain object");

// Check that dynamic fromObject and toObject work for static instance
try {
testFunc();
} finally {
// Rollback all the require() related mess we made
delete require.cache.protobufjs;
Module._resolveFilename = savedResolveFilename;
}
}

tape.test("pbjs generates static code", function(test) {
cliTest(test, function() {
var root = protobuf.loadSync("tests/data/cli/test.proto");
var OneofContainerDynamic = root.lookup("OneofContainer");
var instance = new OneofContainer();
instance.messageInOneof = new Message();
instance.messageInOneof.value = 42;
instance.regularField = "abc";
var instance1 = OneofContainerDynamic.toObject(OneofContainerDynamic.fromObject(instance));
test.deepEqual(instance, instance1, "fromObject and toObject work for instance of the static type");

test.end();
root.resolveAll();

var staticTarget = require("../cli/targets/static");

staticTarget(root, {
create: true,
decode: true,
encode: true,
convert: true,
}, function(err, jsCode) {
test.error(err, 'static code generation worked');

// jsCode is the generated code; we'll eval it
// (since this is what we normally does with the code, right?)
// This is a test code. Do not use this in production.
var $protobuf = protobuf;
eval(jsCode);

var OneofContainer = protobuf.roots.default.OneofContainer;
var Message = protobuf.roots.default.Message;
test.ok(OneofContainer, "type is loaded");
test.ok(Message, "type is loaded");

// Check that fromObject and toObject work for plain object
var obj = {
messageInOneof: {
value: 42,
},
regularField: "abc",
};
var obj1 = OneofContainer.toObject(OneofContainer.fromObject(obj));
test.deepEqual(obj, obj1, "fromObject and toObject work for plain object");

// Check that dynamic fromObject and toObject work for static instance
var root = protobuf.loadSync("tests/data/cli/test.proto");
var OneofContainerDynamic = root.lookup("OneofContainer");
var instance = new OneofContainer();
instance.messageInOneof = new Message();
instance.messageInOneof.value = 42;
instance.regularField = "abc";
var instance1 = OneofContainerDynamic.toObject(OneofContainerDynamic.fromObject(instance));
test.deepEqual(instance, instance1, "fromObject and toObject work for instance of the static type");

test.end();
});
});
});

tape.test("without null-defaults, absent optional fields have zero values", function(test) {
cliTest(test, function() {
var root = protobuf.loadSync("tests/data/cli/null-defaults.proto");
root.resolveAll();

var staticTarget = require("../cli/targets/static");

// Rollback all the require() related mess we made
delete require.cache.protobufjs;
Module._resolveFilename = savedResolveFilename;
staticTarget(root, {
create: true,
decode: true,
encode: true,
convert: true,
}, function(err, jsCode) {
test.error(err, 'static code generation worked');

// jsCode is the generated code; we'll eval it
// (since this is what we normally does with the code, right?)
// This is a test code. Do not use this in production.
var $protobuf = protobuf;
eval(jsCode);

var OptionalFields = protobuf.roots.default.OptionalFields;
test.ok(OptionalFields, "type is loaded");

// Check default values
var msg = OptionalFields.fromObject({});
test.equal(msg.a, null, "default submessage is null");
test.equal(msg.b, "", "default string is empty");
test.equal(msg.c, 0, "default integer is 0");

test.end();
});
});
});

tape.test("with null-defaults, absent optional fields have null values", 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,
"null-defaults": true,
}, function(err, jsCode) {
test.error(err, 'static code generation worked');

// jsCode is the generated code; we'll eval it
// (since this is what we normally does with the code, right?)
// This is a test code. Do not use this in production.
var $protobuf = protobuf;
eval(jsCode);

var OptionalFields = protobuf.roots.default.OptionalFields;
test.ok(OptionalFields, "type is loaded");

// Check default values
var msg = OptionalFields.fromObject({});
test.equal(msg.a, null, "default submessage is null");
test.equal(msg.b, null, "default string is null");
test.equal(msg.c, null, "default integer is null");

test.end();
});
});
});
11 changes: 11 additions & 0 deletions tests/data/cli/null-defaults.proto
View file
@@ -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;
}

0 comments on commit 6e713ba

Please sign in to comment.