fix: fromObject should not initialize oneof members (#1597)

* test: adding test for pbjs static code generation * fix: fromObject should not initialize oneof members
protobufjs · Apr 29, 2021 · 90afe44 · 90afe44
1 parent 6c4d307
commit 90afe44
Show file tree

Hide file tree

Showing 4 changed files with 96 additions and 5 deletions.
diff --git a/cli/targets/static.js b/cli/targets/static.js
@@ -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 + ".",
@@ -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) + ","

diff --git a/package.json b/package.json
@@ -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",
@@ -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"

diff --git a/tests/cli.js b/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;
+});
diff --git a/tests/data/cli/test.proto b/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;
+}