Skip to content
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 3 commits into from
May 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions cli/pbjs.js
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
syntax = "proto2";
Copy link

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?

Copy link
Contributor Author

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 :

For proto3 optional, I think I got it right in #1584 and #1597 (always assigning them to null).

Copy link

@castarco castarco May 6, 2021

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.

this.defaultValue = this.typeDefault;

Copy link
Contributor

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 to null there for optional fields; I haven't seen any bad consequences of the field.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.

Copy link
Contributor

@alexander-fenster alexander-fenster May 6, 2021

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 internally oneof members so get null as their default values):

push(escapeName(type.name) + ".prototype" + prop + " = null;"); // do not set default value for oneof members

Copy link

@castarco castarco May 6, 2021

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...)

Copy link
Contributor

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 :)

Copy link

@castarco castarco May 6, 2021

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 the optional 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 thought optional and required are only applicable to proto2)

Copy link
Contributor

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.

Copy link
Contributor

@alexander-fenster alexander-fenster May 6, 2021

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 from protoc 3.12 (behind the flag) and IIRC 3.15 without the flag.


message OptionalFields {
message SubMessage {
required string a = 1;
}

optional SubMessage a = 1;
optional string b = 2;
optional uint32 c = 3;
}