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

fix: fromObject should not initialize oneof members #1597

Merged
merged 2 commits into from Apr 29, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 3 additions & 5 deletions cli/targets/static.js
Expand Up @@ -394,8 +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.options && field.options.proto3_optional)
if (field.optional && !field.map && !field.repeated && field.resolvedType instanceof Type || field.partOf)
jsType = jsType + "|null|undefined";
pushComment([
field.comment || type.name + " " + field.name + ".",
Expand All @@ -411,9 +410,8 @@ 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.options && field.options.proto3_optional) {
push(escapeName(type.name) + ".prototype" + prop + " = null;"); // do not set default value for proto3 optional fields
}
else if (field.partOf)
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("
+ JSON.stringify(field.typeDefault.low) + ","
Expand Down
4 changes: 4 additions & 0 deletions package.json
Expand Up @@ -56,7 +56,10 @@
"browserify-wrap": "^1.0.2",
"bundle-collapser": "^1.3.0",
"chalk": "^4.0.0",
"escodegen": "^1.13.0",
"eslint": "^7.0.0",
"espree": "^7.1.0",
"estraverse": "^5.1.0",
"gh-pages": "^3.0.0",
"git-raw-commits": "^2.0.3",
"git-semver-tags": "^4.0.0",
Expand All @@ -74,6 +77,7 @@
"tape": "^5.0.0",
"tslint": "^6.0.0",
"typescript": "^3.7.5",
"uglify-js": "^3.7.7",
"vinyl-buffer": "^1.0.1",
"vinyl-fs": "^3.0.3",
"vinyl-source-stream": "^2.0.0"
Expand Down
76 changes: 76 additions & 0 deletions tests/cli.js
@@ -0,0 +1,76 @@
// A minimal test for pbjs tool targets.

var tape = require("tape");
var path = require("path");
var Module = require("module");
var protobuf = require("..");

tape.test("pbjs generates static code", function(test) {
// 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();
return;
}

// Alter the require cache to make the cli/targets/static work since it requires "protobufjs"
// and we don't want to mess with "npm link"
var savedResolveFilename = Module._resolveFilename;
Module._resolveFilename = function(request, parent) {
if (request.startsWith("protobufjs")) {
return request;
}
return savedResolveFilename(request, parent);
};
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
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();
});

// Rollback all the require() related mess we made
delete require.cache.protobufjs;
Module._resolveFilename = savedResolveFilename;
});
13 changes: 13 additions & 0 deletions tests/data/cli/test.proto
@@ -0,0 +1,13 @@
syntax = "proto3";

message Message {
int32 value = 1;
}

message OneofContainer {
oneof some_oneof {
string string_in_oneof = 1;
Message message_in_oneof = 2;
}
string regular_field = 3;
}