More minor cleanup, lint fixes, and coverage improvements

Author: mkruskal-google

index.d.ts

@@ -182,9 +182,6 @@ export class Enum extends ReflectionObject {
     /** Resolved values features, if any */
     public _valuesFeatures?: { [k: string]: { [k: string]: any } };
 
-    /** Unresolved values features, if any */
-    public _valuesProtoFeatures?: { [k: string]: { [k: string]: any } };
-
     /** Reserved ranges, if any. */
     public reserved: (number[]|string)[];
 

src/enum.js

@@ -62,12 +62,6 @@ function Enum(name, values, options, comment, comments, valuesOptions) {
      */
     this._valuesFeatures = {};
 
-    /**
-     * Unresolved values features, if any
-     * @type {Object<string, Object<string, *>>|undefined}
-     */
-    this._valuesProtoFeatures = {};
-
     /**
      * Reserved ranges, if any.
      * @type {Array.<number[]|string>}
@@ -88,12 +82,12 @@ function Enum(name, values, options, comment, comments, valuesOptions) {
  * @override
  */
 Enum.prototype._resolveFeatures = function _resolveFeatures(edition) {
-    var edition = this._edition || edition;
+    edition = this._edition || edition;
     ReflectionObject.prototype._resolveFeatures.call(this, edition);
 
-    Object.keys(this._valuesProtoFeatures).forEach(key => {
+    Object.keys(this.values).forEach(key => {
         var parentFeaturesCopy = Object.assign({}, this._features);
-        this._valuesFeatures[key] = Object.assign(parentFeaturesCopy, this._valuesProtoFeatures[key] || {});
+        this._valuesFeatures[key] = Object.assign(parentFeaturesCopy, this.valuesOptions && this.valuesOptions[key] && this.valuesOptions[key].features);
     });
 
     return this;
@@ -179,21 +173,6 @@ Enum.prototype.add = function add(name, id, comment, options) {
         if (this.valuesOptions === undefined)
             this.valuesOptions = {};
         this.valuesOptions[name] = options || null;
-
-        for (var key of Object.keys(this.valuesOptions)) {
-            var features = Array.isArray(this.valuesOptions[key]) ? this.valuesOptions[key].find(x => {return Object.prototype.hasOwnProperty.call(x, "features");}) : this.valuesOptions[key] === "features";
-            if (features) {
-                this._valuesProtoFeatures[key] = features.features;
-            } else {
-                this._valuesProtoFeatures[key] = {};
-            }
-        }
-    }
-
-    for (var enumValue of Object.keys(this.values)) {
-        if (!this._valuesProtoFeatures[enumValue]) {
-            this._valuesProtoFeatures[enumValue] = {};
-        }
     }
 
     this.comments[name] = comment || null;

src/field.js

@@ -366,7 +366,7 @@ Field.prototype.resolve = function resolve() {
  */
 Field.prototype._inferLegacyProtoFeatures = function _inferLegacyProtoFeatures(edition) {
     if (edition !== "proto2" && edition !== "proto3") {
-        return;
+        return {};
     }
 
     var features = {};

src/index-light.js

@@ -98,7 +98,7 @@ protobuf.types            = require("./types");
 protobuf.util             = require("./util");
 
 // Set up possibly cyclic reflection dependencies
-protobuf.ReflectionObject._configure(protobuf.Root, protobuf.Namespace);
+protobuf.ReflectionObject._configure(protobuf.Root);
 protobuf.Namespace._configure(protobuf.Type, protobuf.Service, protobuf.Enum);
 protobuf.Root._configure(protobuf.Type);
 protobuf.Field._configure(protobuf.Type);

src/namespace.js

@@ -324,7 +324,7 @@ Namespace.prototype.resolveAll = function resolveAll() {
  * @override
  */
 Namespace.prototype._resolveFeaturesRecursive = function _resolveFeaturesRecursive(edition) {
-    var edition = this._edition || edition;
+    edition = this._edition || edition;
 
     ReflectionObject.prototype._resolveFeaturesRecursive.call(this, edition);
     this.nestedArray.forEach(nested => {

src/object.js

@@ -6,7 +6,7 @@ ReflectionObject.className = "ReflectionObject";
 const OneOf = require("./oneof");
 var util = require("./util");
 
-var Root, Namespace; // cyclic
+var Root; // cyclic
 
 /* eslint-disable no-warning-comments */
 // TODO: Replace with embedded proto.
@@ -196,6 +196,7 @@ ReflectionObject.prototype._resolveFeaturesRecursive = function _resolveFeatures
 ReflectionObject.prototype._resolveFeatures = function _resolveFeatures(edition) {
     var defaults = {};
 
+    /* istanbul ignore if */
     if (!edition) {
         throw new Error("Unknown edition for " + this.fullName);
     }
@@ -205,6 +206,7 @@ ReflectionObject.prototype._resolveFeatures = function _resolveFeatures(edition)
 
     if (this._edition) {
         // For a namespace marked with a specific edition, reset defaults.
+        /* istanbul ignore else */
         if (edition === "proto2") {
             defaults = Object.assign({}, proto2Defaults);
         } else if (edition === "proto3") {
@@ -220,6 +222,7 @@ ReflectionObject.prototype._resolveFeatures = function _resolveFeatures(edition)
 
     // fields in Oneofs aren't actually children of them, so we have to
     // special-case it
+    /* istanbul ignore else */
     if (this.partOf instanceof OneOf) {
         var lexicalParentFeaturesCopy = Object.assign({}, this.partOf._features);
         this._features = Object.assign(lexicalParentFeaturesCopy, protoFeatures || {});
@@ -229,7 +232,7 @@ ReflectionObject.prototype._resolveFeatures = function _resolveFeatures(edition)
         var parentFeaturesCopy = Object.assign({}, this.parent._features);
         this._features = Object.assign(parentFeaturesCopy, protoFeatures || {});
     } else {
-        this._features = Object.assign({}, protoFeatures);
+        throw new Error("Unable to find a parent for " + this.fullName);
     }
     if (this.extensionField) {
         // Sister fields should have the same features as their extensions.
@@ -268,13 +271,7 @@ ReflectionObject.prototype.getOption = function getOption(name) {
 ReflectionObject.prototype.setOption = function setOption(name, value, ifNotSet) {
     if (!this.options)
         this.options = {};
-    if (name === "features") {
-        if (ifNotSet) {
-            this.options.features = Object.assign(Object.assign({}, value), this.options.features || {});
-        } else {
-            this.options.features = Object.assign(this.options.features || {}, value);
-        }
-    } else if (/^features\./.test(name)) {
+    if (/^features\./.test(name)) {
         util.setProperty(this.options, name, value, ifNotSet);
     } else if (!ifNotSet || this.options[name] === undefined) {
         if (this.getOption(name) !== value) this.resolved = false;
@@ -295,7 +292,6 @@ ReflectionObject.prototype.setParsedOption = function setParsedOption(name, valu
     if (!this.parsedOptions) {
         this.parsedOptions = [];
     }
-    var isFeature = /^features$/.test(name);
     var parsedOptions = this.parsedOptions;
     if (propName) {
         // If setting a sub property of an option then try to merge it
@@ -360,10 +356,9 @@ ReflectionObject.prototype._editionToJSON = function _editionToJSON() {
         return undefined;
     }
     return this._edition;
-}
+};
 
 // Sets up cyclic dependencies (called in index-light)
-ReflectionObject._configure = function(Root_, Namespace_) {
+ReflectionObject._configure = function(Root_) {
     Root = Root_;
-    Namespace = Namespace_;
 };

src/parse.js

@@ -96,7 +96,7 @@ function parse(source, root, options) {
             obj._edition = edition;
             Object.keys(topLevelOptions).forEach(opt => {
                 if (obj.getOption(opt) !== undefined) return;
-                obj.setOption(opt, topLevelOptions[opt]);
+                obj.setOption(opt, topLevelOptions[opt], true);
             });
         });
     }
@@ -628,18 +628,13 @@ function parse(source, root, options) {
             dummy = {
                 options: undefined
             };
+        dummy.getOption = function(name) {
+            return this.options[name];
+        };
         dummy.setOption = function(name, value) {
-            if (this.options === undefined)
-                this.options = {};
-
-            this.options[name] = value;
+            ReflectionObject.prototype.setOption.call(dummy, name, value);
         };
-        dummy.setParsedOption = function(name, value, propName) {
-            // In order to not change existing behavior, only calling
-            // this for features
-            if (/^features$/.test(name)) {
-                return ReflectionObject.prototype.setParsedOption.call(dummy, name, value, propName);
-            }
+        dummy.setParsedOption = function() {
             return undefined;
         };
         ifBlock(dummy, function parseEnumValue_block(token) {

src/service.js

@@ -121,11 +121,11 @@ Service.prototype.resolveAll = function resolveAll() {
  * @override
  */
 Service.prototype._resolveFeaturesRecursive = function _resolveFeaturesRecursive(edition) {
-    var edition = this._edition || edition;
+    edition = this._edition || edition;
 
     Namespace.prototype._resolveFeaturesRecursive.call(this, edition);
     this.methodsArray.forEach(method => {
-        method._resolveFeaturesRecursive(edition);        
+        method._resolveFeaturesRecursive(edition);
     });
     return this;
 };

src/type.js

@@ -234,7 +234,7 @@ function clearCache(type) {
  * @param {IType} json Message type descriptor
  * @returns {Type} Created message type
  */
-Type.fromJSON = function fromJSON(name, json, nested) {
+Type.fromJSON = function fromJSON(name, json) {
     var type = new Type(name, json.options);
     type.extensions = json.extensions;
     type.reserved = json.reserved;
@@ -317,14 +317,14 @@ Type.prototype.resolveAll = function resolveAll() {
  * @override
  */
 Type.prototype._resolveFeaturesRecursive = function _resolveFeaturesRecursive(edition) {
-    var edition = this._edition || edition;
+    edition = this._edition || edition;
 
     Namespace.prototype._resolveFeaturesRecursive.call(this, edition);
     this.oneofsArray.forEach(oneof => {
-        oneof._resolveFeatures(edition);        
+        oneof._resolveFeatures(edition);
     });
     this.fieldsArray.forEach(field => {
-        field._resolveFeatures(edition);        
+        field._resolveFeatures(edition);
     });
     return this;
 };

tests/api_enum.js

@@ -6,7 +6,7 @@ tape.test("reflected enums", function(test) {
 
     var enm = new protobuf.Enum("Test", {
         a: 1,
-        b: 2
+        b: 2,
     });
 
     var enm_allow_alias = new protobuf.Enum( 'AliasTest',
@@ -86,6 +86,17 @@ tape.test("reflected enums", function(test) {
             '(test_option)': 'test_value'
         }
     });
+    enm.remove("e");
+    test.same( enm.valuesOptions, {}, "should clean up value options");
+
+    enm.reserved = [[100,200], "BAD_NAME"];
+    test.throws(function() {
+        enm.add("d", 101);
+    }, Error, "should throw if id is a reserved number");
+
+    test.throws(function() {
+        enm.add("BAD_NAME", 5);
+    }, Error, "should throw if id is a reserved name");
 
     test.end();
 });

tests/comp_packed-repeated.js

@@ -95,3 +95,17 @@ tape.test("packed repeated values encode", function(top) {
 
     top.end()
 });
+
+tape.test("packed unpackable fields", function(test) {
+    var root = protobuf.parse(`message Test {
+      repeated Test a = 1 [packed = true];
+      repeated Test b = 2 [packed = true, foo = true];
+    }`).root.resolveAll();
+    var Test = root.lookup("Test");
+
+    test.equal(Test.fields.a.options, undefined, "should have no options")
+    test.equal(Test.fields.b.options.packed, undefined, "should have no packed option")
+    test.equal(Test.fields.b.options.foo, true, "should retain other option")
+
+    test.end();
+});

tests/data/uncommon.proto

@@ -71,6 +71,17 @@ enum Test4_1{
     OPTION = 1;
 }
 
+enum ReservedEnum{
+    RESERVED_UNKNOWN = 0;
+
+    reserved 1 to 5;
+    reserved "abc", "INVALID";
+    reserved 10;
+    reserved 11 to 20 {
+        option foo = true;
+    }
+}
+
 service Test5;
 
 service Test6 { option (custom).bar = "";

tests/feature_resolution_editions.js

@@ -115,12 +115,16 @@ tape.test("unresolved feature options", function(test) {
 tape.test("aggregate feature parsing", function(test) {
     var rootEditionsOverriden = protobuf.parse(`edition = "2023";
     option features = {
+        utf8_validation: VERIFY
         json_format: LEGACY_BEST_EFFORT
         field_presence: IMPLICIT
     };
 
     message Message {
-        option features = { utf8_validation: NONE };
+        option features = {
+            utf8_validation: NONE
+            enum_type: OPEN
+        };
         string string_val = 1;
         string string_repeated = 2 [features = { enum_type: CLOSED field_presence: LEGACY_REQUIRED }];
     }`).root.resolveAll();
@@ -557,15 +561,17 @@ tape.test("feature resolution inferred proto2 presence", function(test) {
     message Message {
         optional int32 default = 1;
         required int32 required = 2;
+        repeated int32 repeated = 3;
     }`).root.resolveAll();
 
     var msg = root.lookup("Message");
-    test.ok(msg.fields.default.optional)
-    test.notOk(msg.fields.default.required)
-    test.equal(msg.fields.default._features.field_presence, "EXPLICIT")
-    test.notOk(msg.fields.required.optional)
-    test.ok(msg.fields.required.required)
-    test.equal(msg.fields.required._features.field_presence, "LEGACY_REQUIRED")
+    test.ok(msg.fields.default.optional);
+    test.notOk(msg.fields.default.required);
+    test.ok(msg.fields.default.hasPresence);
+    test.notOk(msg.fields.required.optional);
+    test.ok(msg.fields.required.required);
+    test.equal(msg.fields.required._features.field_presence, "LEGACY_REQUIRED");
+    test.notOk(msg.fields.repeated.hasPresence, "repeated fields never have presence");
 
     test.end();
 });