From 3a20968c6d676312e4f2a510f7e079e0e0819daf Mon Sep 17 00:00:00 2001 From: dcodeIO Date: Thu, 13 Apr 2017 19:11:17 +0200 Subject: [PATCH] Other: Properly remove unnecessary (packed) options from JSON descriptors --- cli/targets/proto.js | 2 +- ext/descriptor/index.js | 13 ++-- ext/descriptor/test.js | 15 +++-- google/protobuf/descriptor.json | 105 ++++++++++---------------------- src/decoder.js | 2 +- src/encoder.js | 2 +- src/field.js | 10 ++- src/namespace.js | 5 +- src/parse.js | 5 +- src/type.js | 2 +- 10 files changed, 66 insertions(+), 95 deletions(-) diff --git a/cli/targets/proto.js b/cli/targets/proto.js index c3de65bc2..eb8ee6a6b 100644 --- a/cli/targets/proto.js +++ b/cli/targets/proto.js @@ -229,7 +229,7 @@ function buildFieldOptions(field) { var sb = []; keys.forEach(function(key) { var val = field.options[key]; - var wireType = types.packed[field.resolvedType instanceof Enum ? "uint32" : field.type]; + var wireType = types.packed[field.resolvedType instanceof Enum ? "int32" : field.type]; switch (key) { case "packed": val = Boolean(val); diff --git a/ext/descriptor/index.js b/ext/descriptor/index.js index 2f7191604..e2eedd502 100644 --- a/ext/descriptor/index.js +++ b/ext/descriptor/index.js @@ -234,16 +234,16 @@ Type.prototype.toDescriptor = function toDescriptor(syntax) { if (this._fieldsArray[i] instanceof MapField) { // map fields are repeated FieldNameEntry var keyType = toDescriptorType(this._fieldsArray[i].keyType, this._fieldsArray[i].resolvedKeyType), valueType = toDescriptorType(this._fieldsArray[i].type, this._fieldsArray[i].resolvedType), - valueTypeName = valueType === 11 || valueType === 14 + valueTypeName = valueType === /* type */ 11 || valueType === /* enum */ 14 ? this._fieldsArray[i].resolvedType && this._fieldsArray[i].resolvedType.fullName || this._fieldsArray[i].type : undefined; descriptor.nestedType.push(exports.DescriptorProto.create({ - name: descriptor.field[descriptor.field.length - 1].type_name, + name: descriptor.field[descriptor.field.length - 1].typeName, field: [ - exports.FieldDescriptorProto.create({ name: "key", number: 1, label: 1, type: keyType }), // can't be message + exports.FieldDescriptorProto.create({ name: "key", number: 1, label: 1, type: keyType }), // can't reference a type or enum exports.FieldDescriptorProto.create({ name: "value", number: 2, label: 1, type: valueType, typeName: valueTypeName }) ], - options: exports.MessageOptions({ mapEntry: true }) + options: exports.MessageOptions.create({ mapEntry: true }) })); } } @@ -482,7 +482,7 @@ Field.prototype.toDescriptor = function toDescriptor(syntax) { case 10: // group case 11: // type case 14: // enum - descriptor.typeName = this.resolvedType ? this.resolvedType.fullName : this.type; + descriptor.typeName = this.resolvedType ? this.resolvedType.fullName : this.type; // TODO: shorten break; } @@ -567,7 +567,8 @@ Enum.fromDescriptor = function fromDescriptor(descriptor) { return new Enum( descriptor.name && descriptor.name.length ? descriptor.name : "Enum" + unnamedEnumIndex++, - values + values, + descriptor.options && descriptor.options.allowAlias ? { allowAlias: true } : undefined ); }; diff --git a/ext/descriptor/test.js b/ext/descriptor/test.js index f704c0328..43a92fbfd 100644 --- a/ext/descriptor/test.js +++ b/ext/descriptor/test.js @@ -27,15 +27,22 @@ var proto = require("../../google/protobuf/descriptor.json")/*{ } }*/; -var root = protobuf.Root.fromJSON(proto); +var root = protobuf.Root.fromJSON(proto).resolveAll(); -console.log("Original proto", JSON.stringify(root, null, 2)); +// console.log("Original proto", JSON.stringify(root, null, 2)); var msg = root.toDescriptor(); -console.log("\nDescriptor", JSON.stringify(msg.toObject(), null, 2)); +// console.log("\nDescriptor", JSON.stringify(msg.toObject(), null, 2)); var buf = descriptor.FileDescriptorSet.encode(msg).finish(); var root2 = protobuf.Root.fromDescriptor(buf, "proto2"); -console.log("\nDecoded proto", JSON.stringify(root2, null, 2)); +// console.log("\nDecoded proto", JSON.stringify(root2, null, 2)); + +require("deep-diff").diff(root.toJSON(), root2.toJSON()).forEach(function(diff) { + console.log(diff.kind + " @ " + diff.path.join(".")); + console.log("lhs:", diff.lhs); + console.log("rhs:", diff.rhs); + console.log(); +}); diff --git a/google/protobuf/descriptor.json b/google/protobuf/descriptor.json index 941209e3d..f6c5c1123 100644 --- a/google/protobuf/descriptor.json +++ b/google/protobuf/descriptor.json @@ -9,8 +9,7 @@ "file": { "rule": "repeated", "type": "FileDescriptorProto", - "id": 1, - "options": {} + "id": 1 } } }, @@ -27,10 +26,7 @@ "dependency": { "rule": "repeated", "type": "string", - "id": 3, - "options": { - "packed": false - } + "id": 3 }, "publicDependency": { "rule": "repeated", @@ -51,26 +47,22 @@ "messageType": { "rule": "repeated", "type": "DescriptorProto", - "id": 4, - "options": {} + "id": 4 }, "enumType": { "rule": "repeated", "type": "EnumDescriptorProto", - "id": 5, - "options": {} + "id": 5 }, "service": { "rule": "repeated", "type": "ServiceDescriptorProto", - "id": 6, - "options": {} + "id": 6 }, "extension": { "rule": "repeated", "type": "FieldDescriptorProto", - "id": 7, - "options": {} + "id": 7 }, "options": { "type": "FileOptions", @@ -95,38 +87,32 @@ "field": { "rule": "repeated", "type": "FieldDescriptorProto", - "id": 2, - "options": {} + "id": 2 }, "extension": { "rule": "repeated", "type": "FieldDescriptorProto", - "id": 6, - "options": {} + "id": 6 }, "nestedType": { "rule": "repeated", "type": "DescriptorProto", - "id": 3, - "options": {} + "id": 3 }, "enumType": { "rule": "repeated", "type": "EnumDescriptorProto", - "id": 4, - "options": {} + "id": 4 }, "extensionRange": { "rule": "repeated", "type": "ExtensionRange", - "id": 5, - "options": {} + "id": 5 }, "oneofDecl": { "rule": "repeated", "type": "OneofDescriptorProto", - "id": 8, - "options": {} + "id": 8 }, "options": { "type": "MessageOptions", @@ -135,16 +121,12 @@ "reservedRange": { "rule": "repeated", "type": "ReservedRange", - "id": 9, - "options": {} + "id": 9 }, "reservedName": { "rule": "repeated", "type": "string", - "id": 10, - "options": { - "packed": false - } + "id": 10 } }, "nested": { @@ -270,8 +252,7 @@ "value": { "rule": "repeated", "type": "EnumValueDescriptorProto", - "id": 2, - "options": {} + "id": 2 }, "options": { "type": "EnumOptions", @@ -304,8 +285,7 @@ "method": { "rule": "repeated", "type": "MethodDescriptorProto", - "id": 2, - "options": {} + "id": 2 }, "options": { "type": "ServiceOptions", @@ -408,8 +388,7 @@ "uninterpretedOption": { "rule": "repeated", "type": "UninterpretedOption", - "id": 999, - "options": {} + "id": 999 } }, "extensions": [ @@ -455,8 +434,7 @@ "uninterpretedOption": { "rule": "repeated", "type": "UninterpretedOption", - "id": 999, - "options": {} + "id": 999 } }, "extensions": [ @@ -507,8 +485,7 @@ "uninterpretedOption": { "rule": "repeated", "type": "UninterpretedOption", - "id": 999, - "options": {} + "id": 999 } }, "extensions": [ @@ -545,8 +522,7 @@ "uninterpretedOption": { "rule": "repeated", "type": "UninterpretedOption", - "id": 999, - "options": {} + "id": 999 } }, "extensions": [ @@ -569,8 +545,7 @@ "uninterpretedOption": { "rule": "repeated", "type": "UninterpretedOption", - "id": 999, - "options": {} + "id": 999 } }, "extensions": [ @@ -589,8 +564,7 @@ "uninterpretedOption": { "rule": "repeated", "type": "UninterpretedOption", - "id": 999, - "options": {} + "id": 999 } }, "extensions": [ @@ -609,8 +583,7 @@ "uninterpretedOption": { "rule": "repeated", "type": "UninterpretedOption", - "id": 999, - "options": {} + "id": 999 } }, "extensions": [ @@ -629,8 +602,7 @@ "uninterpretedOption": { "rule": "repeated", "type": "UninterpretedOption", - "id": 999, - "options": {} + "id": 999 } }, "extensions": [ @@ -645,8 +617,7 @@ "name": { "rule": "repeated", "type": "NamePart", - "id": 2, - "options": {} + "id": 2 }, "identifierValue": { "type": "string", @@ -695,8 +666,7 @@ "location": { "rule": "repeated", "type": "Location", - "id": 1, - "options": {} + "id": 1 } }, "nested": { @@ -705,18 +675,12 @@ "path": { "rule": "repeated", "type": "int32", - "id": 1, - "options": { - "packed": true - } + "id": 1 }, "span": { "rule": "repeated", "type": "int32", - "id": 2, - "options": { - "packed": true - } + "id": 2 }, "leadingComments": { "type": "string", @@ -729,10 +693,7 @@ "leadingDetachedComments": { "rule": "repeated", "type": "string", - "id": 6, - "options": { - "packed": false - } + "id": 6 } } } @@ -743,8 +704,7 @@ "annotation": { "rule": "repeated", "type": "Annotation", - "id": 1, - "options": {} + "id": 1 } }, "nested": { @@ -753,10 +713,7 @@ "path": { "rule": "repeated", "type": "int32", - "id": 1, - "options": { - "packed": true - } + "id": 1 }, "sourceFile": { "type": "string", diff --git a/src/decoder.js b/src/decoder.js index bac7ee862..ab2085eed 100644 --- a/src/decoder.js +++ b/src/decoder.js @@ -31,7 +31,7 @@ function decoder(mtype) { var i = 0; for (; i < /* initializes */ mtype.fieldsArray.length; ++i) { var field = mtype._fieldsArray[i].resolve(), - type = field.resolvedType instanceof Enum ? "uint32" : field.type, + type = field.resolvedType instanceof Enum ? "int32" : field.type, ref = "m" + util.safeProp(field.name); gen ("case %d:", field.id); diff --git a/src/encoder.js b/src/encoder.js index 538ff6b89..0348c27d1 100644 --- a/src/encoder.js +++ b/src/encoder.js @@ -39,7 +39,7 @@ function encoder(mtype) { for (var i = 0; i < fields.length; ++i) { var field = fields[i].resolve(), index = mtype._fieldsArray.indexOf(field), - type = field.resolvedType instanceof Enum ? "uint32" : field.type, + type = field.resolvedType instanceof Enum ? "int32" : field.type, wireType = types.basic[type]; ref = "m" + util.safeProp(field.name); diff --git a/src/field.js b/src/field.js index 412940a4c..68cb06bdc 100644 --- a/src/field.js +++ b/src/field.js @@ -273,9 +273,13 @@ Field.prototype.resolve = function resolve() { this.typeDefault = this.resolvedType.values[this.typeDefault]; } - // remove unnecessary packed option (parser adds this) if not referencing an enum - if (this.options && this.options.packed !== undefined && this.resolvedType && !(this.resolvedType instanceof Enum)) - delete this.options.packed; + // remove unnecessary options + if (this.options) { + if (this.options.packed === true || this.options.packed !== undefined && this.resolvedType && !(this.resolvedType instanceof Enum)) + delete this.options.packed; + if (Object.keys(this.options).length === 0) + this.options = undefined; + } // convert to internal data type if necesssary if (this.long) { diff --git a/src/namespace.js b/src/namespace.js index 399c5aa99..edc6f3cb6 100644 --- a/src/namespace.js +++ b/src/namespace.js @@ -290,7 +290,7 @@ Namespace.prototype.resolveAll = function resolveAll() { }; /** - * Looks up the reflection object at the specified path, relative to this namespace. + * Recursively looks up the reflection object matching the specified path in the scope of this namespace. * @param {string|string[]} path Path to look up * @param {*|Array.<*>} filterTypes Filter types, any combination of the constructors of `protobuf.Type`, `protobuf.Enum`, `protobuf.Service` etc. * @param {boolean} [parentAlreadyChecked=false] If known, whether the parent has already been checked @@ -315,6 +315,7 @@ Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChe // Start at root if path is absolute if (path[0] === "") return this.root.lookup(path.slice(1), filterTypes); + // Test if the first part matches any nested object, and if so, traverse if path contains more var found = this.get(path[0]); if (found) { @@ -323,11 +324,13 @@ Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChe return found; } else if (found instanceof Namespace && (found = found.lookup(path.slice(1), filterTypes, true))) return found; + // Otherwise try each nested namespace } else for (var i = 0; i < this.nestedArray.length; ++i) if (this._nestedArray[i] instanceof Namespace && (found = this._nestedArray[i].lookup(path, filterTypes, true))) return found; + // If there hasn't been a match, try again at the parent if (this.parent === null || parentAlreadyChecked) return null; diff --git a/src/parse.js b/src/parse.js index c22a38b77..7b851d809 100644 --- a/src/parse.js +++ b/src/parse.js @@ -383,9 +383,8 @@ function parse(source, root, options) { // JSON defaults to packed=true if not set so we have to set packed=false explicity when // parsing proto2 descriptors without the option, where applicable. This must be done for - // any type (not just packable types) because enums also use varint encoding and it is not - // yet known whether a type is an enum or not. - if (!isProto3 && field.repeated) + // all known packable types and anything that could be an enum (= is not a basic type). + if (!isProto3 && field.repeated && (types.packed[type] !== undefined || types.basic[type] === undefined)) field.setOption("packed", false, /* ifNotSet */ true); } diff --git a/src/type.js b/src/type.js index 21c72e459..1d5648ce2 100644 --- a/src/type.js +++ b/src/type.js @@ -302,7 +302,7 @@ Type.prototype.resolveAll = function resolveAll() { var oneofs = this.oneofsArray; i = 0; while (i < oneofs.length) oneofs[i++].resolve(); - return Namespace.prototype.resolve.call(this); + return Namespace.prototype.resolveAll.call(this); }; /**