Cleanup: remove protoFeatures, move editions out of options, handle unresolved options for fromJSON, fix aggregate feature handling

Author: mkruskal-google

cli/targets/proto.js

@@ -102,7 +102,7 @@ function buildRoot(root) {
     if (pkg.length)
         out.push("", "package " + pkg.join(".") + ";");
 
-    buildOptions(ptr, ["edition"]);
+    buildOptions(ptr);
     ptr.nestedArray.forEach(build);
 }
 
@@ -235,7 +235,7 @@ function buildFieldOptions(field) {
     }
     var sb = [];
     keys.forEach(function(key) {
-        if (key === "proto3_optional" || key === "packed" || key.startsWith("features.")) return;
+        if (key === "proto3_optional" || key === "packed" || key === "features") return;
 
         var val = field.options[key];
         switch (key) {
@@ -330,7 +330,7 @@ function buildOptions(object, ignore = []) {
         return;
     first = true;
     Object.keys(object.options).forEach(function(key) {
-        if (ignore.includes(key) || key.startsWith("features.") || key === "edition") return;
+        if (ignore.includes(key) || key === "features") return;
         if (first) {
             first = false;
             push("");

index.d.ts

@@ -188,12 +188,6 @@ export class Enum extends ReflectionObject {
     /** Reserved ranges, if any. */
     public reserved: (number[]|string)[];
 
-    /**
-     * Resolves value features
-     * @returns `this`
-     */
-    public resolve(): Enum;
-
     /**
      * Constructs an enum from an enum descriptor.
      * @param name Enum name
@@ -906,11 +900,17 @@ export abstract class ReflectionObject {
     /** Unique name within its namespace. */
     public name: string;
 
-    /** Resolved Features. */
-    public _features: any;
+    /** The edition specified for this object.  Only relevant for top-level objects. */
+    public _edition: string;
 
-    /** Unresolved Features. */
-    public _protoFeatures: any;
+    /**
+     * The default edition to use for this object if none is specified.  For legacy reasons,
+     * this is proto2 except in the JSON parsing case where it was proto3.
+     */
+    public _defaultEdition: string;
+
+    /** Resolved Features. */
+    public _features: object;
 
     /** Parent namespace. */
     public parent: (Namespace|null);
@@ -954,8 +954,18 @@ export abstract class ReflectionObject {
      */
     public resolve(): ReflectionObject;
 
-    /** Resolves child features from parent features */
-    public _resolveFeatures(): void;
+    /**
+     * Resolves this objects editions features.
+     * @param edition The edition we're currently resolving for.
+     * @returns `this`
+     */
+    public _resolveFeaturesRecursive(edition: string): ReflectionObject;
+
+    /**
+     * Resolves child features from parent features
+     * @param edition The edition we're currently resolving for.
+     */
+    public _resolveFeatures(edition: string): void;
 
     /**
      * Infers features from legacy syntax that may have been specified differently.
@@ -979,7 +989,7 @@ export abstract class ReflectionObject {
      * @param [ifNotSet] Sets the option only if it isn't currently set
      * @returns `this`
      */
-    public setOption(name: string, value: any, ifNotSet?: boolean): ReflectionObject;
+    public setOption(name: string, value: any, ifNotSet?: (boolean|undefined)): ReflectionObject;
 
     /**
      * Sets a parsed option.
@@ -1003,6 +1013,12 @@ export abstract class ReflectionObject {
      * @returns Class name[, space, full name]
      */
     public toString(): string;
+
+    /**
+     * Converts the edition this object is pinned to for JSON format.
+     * @returns The edition string for JSON representation
+     */
+    public _editionToJSON(): (string|undefined);
 }
 
 /** Reflected oneof. */
@@ -2244,10 +2260,10 @@ export namespace util {
      * @param dst Destination object
      * @param path dot '.' delimited path of the property to set
      * @param value the value to set
-     * @param overWrite whether or not to concatenate the values into an array or overwrite; defaults to false.
+     * @param [ifNotSet] Sets the option only if it isn't currently set
      * @returns Destination object
      */
-    function setProperty(dst: { [k: string]: any }, path: string, value: object, overWrite: boolean): { [k: string]: any };
+    function setProperty(dst: { [k: string]: any }, path: string, value: object, ifNotSet?: (boolean|undefined)): { [k: string]: any };
 
     /** Decorator root (TypeScript). */
     let decorateRoot: Root;

src/object.js

@@ -67,17 +67,6 @@ function ReflectionObject(name, options) {
      */
     this._features = {};
 
-    /**
-     * Whether or not features have been resolved.
-     * @type {boolean}
-     */
-    this._featuresResolved = false;
-
-    /**
-     * Unresolved Features.
-     */
-    this._protoFeatures = null;
-
     /**
      * Parent namespace.
      * @type {Namespace|null}
@@ -205,17 +194,14 @@ ReflectionObject.prototype._resolveFeaturesRecursive = function _resolveFeatures
  * @returns {undefined}
  */
 ReflectionObject.prototype._resolveFeatures = function _resolveFeatures(edition) {
-    if (this._featuresResolved) {
-        return;
-    }
-
     var defaults = {};
 
     if (!edition) {
         throw new Error("Unknown edition for " + this.fullName);
     }
 
-    var protoFeatures = Object.assign(Object.assign({}, this._protoFeatures), this._inferLegacyProtoFeatures(edition));
+    var protoFeatures = Object.assign(this.options ? Object.assign({},  this.options.features) : {},
+        this._inferLegacyProtoFeatures(edition));
 
     if (this._edition) {
         // For a namespace marked with a specific edition, reset defaults.
@@ -248,9 +234,7 @@ ReflectionObject.prototype._resolveFeatures = function _resolveFeatures(edition)
     if (this.extensionField) {
         // Sister fields should have the same features as their extensions.
         this.extensionField._features = this._features;
-        this.extensionField._featuresResolved = true;
     }
-    this._featuresResolved = true;
 };
 
 /**
@@ -278,14 +262,25 @@ ReflectionObject.prototype.getOption = function getOption(name) {
  * Sets an option.
  * @param {string} name Option name
  * @param {*} value Option value
- * @param {boolean} [ifNotSet] Sets the option only if it isn't currently set
+ * @param {boolean|undefined} [ifNotSet] Sets the option only if it isn't currently set
  * @returns {ReflectionObject} `this`
  */
 ReflectionObject.prototype.setOption = function setOption(name, value, ifNotSet) {
-    if (!ifNotSet || !this.options || this.options[name] === undefined) {
+    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)) {
+        util.setProperty(this.options, name, value, ifNotSet);
+    } else if (!ifNotSet || this.options[name] === undefined) {
         if (this.getOption(name) !== value) this.resolved = false;
-        (this.options || (this.options = {}))[name] = value;
+        this.options[name] = value;
     }
+
     return this;
 };
 
@@ -326,12 +321,6 @@ ReflectionObject.prototype.setParsedOption = function setParsedOption(name, valu
         parsedOptions.push(newOpt);
     }
 
-
-    if (isFeature) {
-        var features = parsedOptions.find(x => {return Object.prototype.hasOwnProperty.call(x, "features");});
-        this._protoFeatures = features.features || {};
-    }
-
     return this;
 };
 

src/parse.js

@@ -88,7 +88,6 @@ function parse(source, root, options) {
 
     var topLevelObjects = [];
     var topLevelOptions = {};
-    var topLevelParsedOptions = {};
 
     var applyCase = options.keepCase ? function(name) { return name; } : util.camelCase;
 
@@ -98,9 +97,6 @@ function parse(source, root, options) {
             Object.keys(topLevelOptions).forEach(opt => {
                 if (obj.getOption(opt) !== undefined) return;
                 obj.setOption(opt, topLevelOptions[opt]);
-                if (topLevelParsedOptions[opt.substring("features.".length)]) {
-                    obj.setParsedOption("features", topLevelParsedOptions[opt.substring("features.".length)], opt.substring("features.".length));
-                }
             });
         });
     }
@@ -769,10 +765,6 @@ function parse(source, root, options) {
     }
 
     function setParsedOption(parent, name, value, propName) {
-        if (ptr === parent && /^features$/.test(name)) {
-            topLevelParsedOptions[propName] = value;
-            return;
-        }
         if (parent.setParsedOption)
             parent.setParsedOption(name, value, propName);
     }

src/root.js

@@ -51,7 +51,7 @@ Root.fromJSON = function fromJSON(json, root) {
         root = new Root();
     if (json.options)
         root.setOptions(json.options);
-    return root.addJSON(json.nested);
+    return root.addJSON(json.nested).resolveAll();
 };
 
 /**

src/util.js

@@ -171,10 +171,10 @@ util.decorateEnum = function decorateEnum(object) {
  * @param {Object.<string,*>} dst Destination object
  * @param {string} path dot '.' delimited path of the property to set
  * @param {Object} value the value to set
- * @param {boolean} overWrite whether or not to concatenate the values into an array or overwrite; defaults to false.
+ * @param {boolean|undefined} [ifNotSet] Sets the option only if it isn't currently set
  * @returns {Object.<string,*>} Destination object
  */
-util.setProperty = function setProperty(dst, path, value) {
+util.setProperty = function setProperty(dst, path, value, ifNotSet) {
     function setProp(dst, path, value) {
         var part = path.shift();
         if (part === "__proto__" || part === "prototype") {
@@ -184,6 +184,8 @@ util.setProperty = function setProperty(dst, path, value) {
             dst[part] = setProp(dst[part] || {}, path, value);
         } else {
             var prevValue = dst[part];
+            if (prevValue && ifNotSet)
+                return dst;
             if (prevValue)
                 value = [].concat(prevValue).concat(value);
             dst[part] = value;

tests/api_enum.js

@@ -89,3 +89,101 @@ tape.test("reflected enums", function(test) {
 
     test.end();
 });
+
+tape.test("feature resolution legacy proto3", function(test) {
+    var json = {
+        values: {
+            a: 0, b: 1
+        }
+    };
+    var messageJson = {
+        fields: {},
+        nested: { Enum: { values: {
+            a: 0, b:1
+        } } }
+    };
+    var root = new protobuf.Root();
+    var Enum = protobuf.Enum.fromJSON("Enum", json);
+    var Message = protobuf.Type.fromJSON("Message", messageJson)
+    var Nested = Message.nested.Enum;
+    root.add(Enum).add(Message).resolveAll();
+
+    test.same(Enum.toJSON(), json, "JSON should roundtrip");
+    test.same(Message.toJSON(), messageJson, "container JSON should roundtrip");
+    test.same(Nested.toJSON(), messageJson.nested.Enum, "nested JSON should roundtrip");
+
+    test.equal(Enum._edition, "proto3", "should infer proto3 syntax");
+    test.equal(Enum._features.enum_type, "OPEN", "should be open by default");
+
+    test.equal(Nested._edition, null, "should not set edition");
+    test.equal(Nested._features.enum_type, "OPEN", "should be open by default");
+
+    test.end();
+});
+
+tape.test("feature resolution proto2", function(test) {
+    var json = {
+        edition: "proto2",
+        values: {
+            a: 0, b: 1
+        }
+    };
+    var messageJson = {
+        edition: "proto2",
+        fields: {},
+        nested: { Enum: { values: {
+            a: 0, b: 1
+        } } }
+    };
+    var root = new protobuf.Root();
+    var Enum = protobuf.Enum.fromJSON("Enum", json);
+    var Message = protobuf.Type.fromJSON("Message", messageJson)
+    var Nested = Message.nested.Enum;
+    root.add(Enum).add(Message).resolveAll();
+
+    test.same(Enum.toJSON(), json, "JSON should roundtrip");
+    test.same(Message.toJSON(), messageJson, "container JSON should roundtrip");
+    test.same(Nested.toJSON(), messageJson.nested.Enum, "nested JSON should roundtrip");
+
+    test.equal(Enum._edition, "proto2", "should set edition");
+    test.equal(Enum._features.enum_type, "CLOSED", "should be closed by default");
+
+    test.equal(Nested._edition, null, "should not set edition");
+    test.equal(Nested._features.enum_type, "CLOSED", "should be closed by default");
+
+    test.end();
+});
+
+tape.test("feature resolution legacy proto3", function(test) {
+    var json = {
+        edition: "2023",
+        values: {
+            a: 0, b: 1
+        }
+    };
+    var messageJson = {
+        edition: "2023",
+        options: { features: { enum_type: "CLOSED" } },
+        fields: {},
+        nested: { Enum: { values: {
+            a: 0, b: 1
+        } } }
+    };
+    var root = new protobuf.Root();
+    var Enum = protobuf.Enum.fromJSON("Enum", json);
+    var Message = protobuf.Type.fromJSON("Message", messageJson)
+    var Nested = Message.nested.Enum;
+    root.add(Enum).add(Message).resolveAll();
+
+    test.same(Enum.toJSON(), json, "JSON should roundtrip");
+    test.same(Message.toJSON(), messageJson, "container JSON should roundtrip");
+    test.same(Nested.toJSON(), messageJson.nested.Enum, "nested JSON should roundtrip");
+
+    test.equal(Enum._edition, "2023", "should set edition");
+    test.equal(Enum._features.enum_type, "OPEN", "should be open by default");
+
+    test.equal(Nested._edition, null, "should not set edition");
+    test.equal(Nested._features.enum_type, "CLOSED", "should inherit override");
+
+    test.end();
+});