Skip to content

Commit 9e80030

Browse files
dcodeIOdcodeIO
authored
fix(cli): Consistently handle derived names (#2293)
1 parent f8c489b commit 9e80030

13 files changed

Lines changed: 208 additions & 35 deletions

File tree

‎cli/targets/json-module.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ function escapeName(name) {
3838

3939
function json_module(root, options, callback) {
4040
try {
41-
var rootProp = protobuf.util.safeProp(options.root || "default");
41+
var rootProp = "[" + JSON.stringify(String(options.root || "default")) + "]";
4242
var output = [
4343
(options.es6 ? "const" : "var") + " $root = ($protobuf.roots" + rootProp + " || ($protobuf.roots" + rootProp + " = new $protobuf.Root()))\n"
4444
];

‎cli/targets/static.js‎

Lines changed: 45 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ function static_target(root, options, callback) {
4141
}
4242
push("// Exported root namespace");
4343
}
44-
var rootProp = util.safeProp(config.root || "default");
44+
var rootProp = "[" + JSON.stringify(String(config.root || "default")) + "]";
4545
push((config.es6 ? "const" : "var") + " $root = $protobuf.roots" + rootProp + " || ($protobuf.roots" + rootProp + " = {});");
4646
buildNamespace(null, root);
4747
return callback(null, out.join("\n"));
@@ -79,18 +79,24 @@ function pushComment(lines) {
7979
push(" */");
8080
}
8181

82+
function objectPath(object) {
83+
var parts = [];
84+
while (object && object.name !== "") {
85+
parts.unshift(escapeName(object.name));
86+
object = object.parent;
87+
}
88+
return parts;
89+
}
90+
8291
function exportName(object, asInterface) {
8392
if (asInterface) {
8493
if (object.__interfaceName)
8594
return object.__interfaceName;
8695
} else if (object.__exportName)
8796
return object.__exportName;
88-
var parts = object.fullName.substring(1).split("."),
89-
i = 0;
90-
while (i < parts.length)
91-
parts[i] = escapeName(parts[i++]);
92-
if (asInterface)
93-
parts[i - 1] = "I" + parts[i - 1];
97+
var parts = objectPath(object);
98+
if (asInterface && parts.length)
99+
parts[parts.length - 1] = "I" + parts[parts.length - 1];
94100
return object[asInterface ? "__interfaceName" : "__exportName"] = parts.join(".");
95101
}
96102

@@ -141,7 +147,15 @@ function buildNamespace(ref, ns) {
141147
push((config.es6 ? "const" : "var") + " " + escapeName(ns.name) + " = {};");
142148
}
143149

150+
var seenNames = new Set();
144151
ns.nestedArray.forEach(function(nested) {
152+
// Only check names of elements that are emitted below
153+
if (!(nested instanceof Enum || nested instanceof Namespace) || nested instanceof Service && !config.service)
154+
return;
155+
var name = escapeName(nested.name);
156+
if (seenNames.has(name))
157+
throw Error("duplicate generated name '" + name + "'");
158+
seenNames.add(name);
145159
if (nested instanceof Enum)
146160
buildEnum(ns.name, nested);
147161
else if (nested instanceof Namespace)
@@ -237,8 +251,27 @@ var renameVars = {
237251

238252
function buildFunction(type, functionName, gen, scope) {
239253
var code = gen.toString(functionName);
240-
241254
var ast = espree.parse(code);
255+
256+
function rootMemberRef(object) {
257+
var ref = {
258+
"type": "Identifier",
259+
"name": "$root"
260+
};
261+
var parts = objectPath(object);
262+
for (var i = 0; i < parts.length; ++i)
263+
ref = {
264+
"type": "MemberExpression",
265+
"computed": false,
266+
"object": ref,
267+
"property": {
268+
"type": "Identifier",
269+
"name": parts[i]
270+
}
271+
};
272+
return ref;
273+
}
274+
242275
/* eslint-disable no-extra-parens */
243276
estraverse.replace(ast, {
244277
enter: function(node, parent) {
@@ -263,10 +296,7 @@ function buildFunction(type, functionName, gen, scope) {
263296
|| (parent.type === "BinaryExpression" && parent.operator === "instanceof" && parent.right === node)
264297
)
265298
)
266-
return {
267-
"type": "Identifier",
268-
"name": "$root" + type.fullName
269-
};
299+
return rootMemberRef(type);
270300
// replace types[N].ctor with the field's actual type constructor
271301
if (
272302
node.type === "MemberExpression"
@@ -275,10 +305,7 @@ function buildFunction(type, functionName, gen, scope) {
275305
&& node.object.property.type === "Literal"
276306
&& node.property.type === "Identifier" && node.property.name === "ctor"
277307
)
278-
return {
279-
"type": "Identifier",
280-
"name": "$root" + type.fieldsArray[node.object.property.value].resolvedType.fullName
281-
};
308+
return rootMemberRef(type.fieldsArray[node.object.property.value].resolvedType);
282309
// replace types[N].values with the field's actual enum object
283310
if (
284311
node.type === "MemberExpression"
@@ -287,20 +314,14 @@ function buildFunction(type, functionName, gen, scope) {
287314
&& node.object.property.type === "Literal"
288315
&& node.property.type === "Identifier" && node.property.name === "values"
289316
)
290-
return {
291-
"type": "Identifier",
292-
"name": "$root" + type.fieldsArray[node.object.property.value].resolvedType.fullName
293-
};
317+
return rootMemberRef(type.fieldsArray[node.object.property.value].resolvedType);
294318
// replace types[N] with the field's actual type
295319
if (
296320
node.type === "MemberExpression"
297321
&& node.object.type === "Identifier" && node.object.name === "types"
298322
&& node.property.type === "Literal"
299323
)
300-
return {
301-
"type": "Identifier",
302-
"name": "$root" + type.fieldsArray[node.property.value].resolvedType.fullName
303-
};
324+
return rootMemberRef(type.fieldsArray[node.property.value].resolvedType);
304325
return undefined;
305326
}
306327
});

‎cli/util.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ exports.wrap = function(OUTPUT, options) {
126126
return $1.length ? OUTPUT.replace(/^/mg, $1) : OUTPUT;
127127
});
128128
if (options.lint !== "")
129-
wrap = "/*" + options.lint + "*/\n" + wrap;
129+
wrap = "/*" + String(options.lint).replace(/\*\//g, "* /") + "*/\n" + wrap;
130130
return wrap.replace(/\r?\n/g, "\n");
131131
};
132132

‎src/roots.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
"use strict";
2-
module.exports = {};
2+
module.exports = Object.create(null);
33

44
/**
55
* Named roots.

‎tests/cli-pbjs.js‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,81 @@ tape.test("pbjs keeps es6 as an ES module wrapper alias", function(test) {
223223
});
224224
});
225225

226+
tape.test("pbjs escapes wrapper lint comments", function(test) {
227+
cliTest(test, function() {
228+
var root = new protobuf.Root();
229+
var staticModuleTarget = require("../cli/targets/static-module");
230+
231+
staticModuleTarget(root, {
232+
wrap: "commonjs",
233+
lint: "note */ text"
234+
}, function(err, jsCode) {
235+
test.error(err, "static-module code generation worked");
236+
test.equal(jsCode.indexOf("note */ text"), -1, "does not emit raw comment terminator");
237+
test.ok(jsCode.indexOf("note * / text") >= 0, "escapes comment terminator");
238+
test.doesNotThrow(function() {
239+
new Function("require", "module", "exports", jsCode); // eslint-disable-line no-new-func
240+
}, "should generate parseable output");
241+
test.end();
242+
});
243+
});
244+
});
245+
246+
tape.test("pbjs supports dictionary generated root names", function(test) {
247+
cliTest(test, function() {
248+
var staticTarget = require("../cli/targets/static");
249+
var jsonModuleTarget = require("../cli/targets/json-module");
250+
var root = protobuf.Root.fromJSON({
251+
nested: {
252+
M: {
253+
fields: {}
254+
}
255+
}
256+
});
257+
258+
test.equal(Object.getPrototypeOf(protobuf.roots), null, "roots uses dictionary semantics");
259+
260+
delete protobuf.roots.__proto__;
261+
delete protobuf.roots.constructor;
262+
staticTarget(root, {
263+
root: "__proto__"
264+
}, function(staticErr, staticCode) {
265+
test.error(staticErr, "static target accepts dictionary root name");
266+
test.ok(staticCode.indexOf("$protobuf.roots[\"__proto__\"]") >= 0, "static target uses bracket root access");
267+
var $protobuf = protobuf;
268+
test.doesNotThrow(function() {
269+
eval(staticCode);
270+
}, "static output should execute");
271+
test.ok(Object.prototype.hasOwnProperty.call(protobuf.roots, "__proto__"), "static target creates own root property");
272+
273+
jsonModuleTarget(root, {
274+
wrap: "commonjs",
275+
root: "constructor",
276+
lint: ""
277+
}, function(jsonErr, jsonCode) {
278+
test.error(jsonErr, "json-module target accepts dictionary root name");
279+
test.ok(jsonCode.indexOf("$protobuf.roots[\"constructor\"]") >= 0, "json-module target uses bracket root access");
280+
281+
var module = { exports: {} };
282+
function localRequire(request) {
283+
if (request.indexOf("protobufjs") === 0)
284+
return protobuf;
285+
throw Error("unexpected require: " + request);
286+
}
287+
288+
test.doesNotThrow(function() {
289+
new Function("require", "module", "exports", jsonCode)(localRequire, module, module.exports); // eslint-disable-line no-new-func
290+
}, "json-module output should execute");
291+
test.ok(Object.prototype.hasOwnProperty.call(protobuf.roots, "constructor"), "json-module target creates own root property");
292+
test.equal(module.exports, protobuf.roots.constructor, "json-module exports dictionary root");
293+
delete protobuf.roots.__proto__;
294+
delete protobuf.roots.constructor;
295+
test.end();
296+
});
297+
});
298+
});
299+
});
300+
226301
tape.test("pbjs supports custom target paths", function(test) {
227302
cliTest(test, function() {
228303
var pbjs = require("../cli/pbjs");
@@ -748,6 +823,83 @@ tape.test("pbjs escapes static target names", function(test) {
748823
});
749824
});
750825

826+
tape.test("pbjs rejects static target escaped name collisions", function(test) {
827+
cliTest(test, function() {
828+
var root = protobuf.Root.fromJSON({
829+
nested: {
830+
"pkg-name": {
831+
nested: {}
832+
},
833+
pkgname: {
834+
nested: {}
835+
}
836+
}
837+
});
838+
var staticTarget = require("../cli/targets/static");
839+
840+
staticTarget(root, {}, function(err) {
841+
test.match(err && err.message, /duplicate generated name 'pkgname'/, "rejects ambiguous generated names");
842+
test.end();
843+
});
844+
});
845+
});
846+
847+
tape.test("pbjs builds static references from escaped path segments", function(test) {
848+
cliTest(test, function() {
849+
var root = protobuf.Root.fromJSON({
850+
nested: {
851+
"pkg.name": {
852+
nested: {
853+
Child: {
854+
fields: {
855+
value: { type: "string", id: 1 }
856+
}
857+
},
858+
Kind: {
859+
values: {
860+
UNKNOWN: 0,
861+
READY: 1
862+
}
863+
},
864+
Parent: {
865+
fields: {
866+
child: { type: "Child", id: 1 },
867+
kind: { type: "Kind", id: 2 }
868+
}
869+
},
870+
TestService: {
871+
methods: {
872+
Call: {
873+
requestType: "Child",
874+
responseType: "Parent"
875+
}
876+
}
877+
}
878+
}
879+
}
880+
}
881+
});
882+
var staticTarget = require("../cli/targets/static");
883+
884+
staticTarget(root, {
885+
create: true,
886+
service: true,
887+
verify: true,
888+
convert: true
889+
}, function(err, jsCode) {
890+
test.error(err, "static code generation worked");
891+
test.equal(jsCode.indexOf("$root.pkg.name"), -1, "does not split dotted descriptor names");
892+
test.ok(jsCode.indexOf("$root.pkgname.Child") >= 0, "uses escaped path segment for request type");
893+
test.ok(jsCode.indexOf("$root.pkgname.Parent") >= 0, "uses escaped path segment for response type");
894+
test.doesNotThrow(function() {
895+
new Function("$protobuf", jsCode); // eslint-disable-line no-new-func
896+
}, "should generate parseable output");
897+
898+
test.end();
899+
});
900+
});
901+
});
902+
751903
tape.test("pbjs generates static code with message filter", function (test) {
752904
cliTest(test, function () {
753905
var root = protobuf.loadSync("tests/data/cli/test-filter.proto");

‎tests/data/comments.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ var $protobuf = require("../../minimal");
77
var $Reader = $protobuf.Reader, $Writer = $protobuf.Writer, $util = $protobuf.util;
88

99
// Exported root namespace
10-
var $root = $protobuf.roots.test_comments || ($protobuf.roots.test_comments = {});
10+
var $root = $protobuf.roots["test_comments"] || ($protobuf.roots["test_comments"] = {});
1111

1212
$root.Test1 = (function() {
1313

‎tests/data/convert.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ var $protobuf = require("../../minimal");
77
var $Reader = $protobuf.Reader, $Writer = $protobuf.Writer, $util = $protobuf.util;
88

99
// Exported root namespace
10-
var $root = $protobuf.roots.test_convert || ($protobuf.roots.test_convert = {});
10+
var $root = $protobuf.roots["test_convert"] || ($protobuf.roots["test_convert"] = {});
1111

1212
$root.Message = (function() {
1313

‎tests/data/mapbox/vector_tile.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ var $protobuf = require("../../../minimal");
77
var $Reader = $protobuf.Reader, $Writer = $protobuf.Writer, $util = $protobuf.util;
88

99
// Exported root namespace
10-
var $root = $protobuf.roots.test_vector_tile || ($protobuf.roots.test_vector_tile = {});
10+
var $root = $protobuf.roots["test_vector_tile"] || ($protobuf.roots["test_vector_tile"] = {});
1111

1212
$root.vector_tile = (function() {
1313

‎tests/data/package.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ var $protobuf = require("../../minimal");
77
var $Reader = $protobuf.Reader, $Writer = $protobuf.Writer, $util = $protobuf.util;
88

99
// Exported root namespace
10-
var $root = $protobuf.roots.test_package || ($protobuf.roots.test_package = {});
10+
var $root = $protobuf.roots["test_package"] || ($protobuf.roots["test_package"] = {});
1111

1212
$root.Package = (function() {
1313

‎tests/data/rpc-es6.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import $protobuf from "protobufjs/minimal.js";
55
const $Reader = $protobuf.Reader, $Writer = $protobuf.Writer, $util = $protobuf.util;
66

77
// Exported root namespace
8-
const $root = $protobuf.roots.test_rpc || ($protobuf.roots.test_rpc = {});
8+
const $root = $protobuf.roots["test_rpc"] || ($protobuf.roots["test_rpc"] = {});
99

1010
export const MyService = $root.MyService = (() => {
1111

0 commit comments

Comments
 (0)