fix: ensure that types are always resolved (#2068)

* run a full resolveAll eagerly now that it's optimized

* add caching to prevent excessive recursion on resolveAll

* build ts file

* minor 20% optimization to lookup

* Cleanup to mark fields private

* further 40% optimization to lookup that bails out early on fully-qualified types

* lint fix

* Include fields in global fullname cache for custom options

Author: mkruskal-google

index.d.ts

@@ -753,6 +753,9 @@ export abstract class NamespaceBase extends ReflectionObject {
     /** Whether or not objects contained in this namespace need feature resolution. */
     protected _needsRecursiveFeatureResolution: boolean;
 
+    /** Whether or not objects contained in this namespace need a resolve. */
+    protected _needsRecursiveResolve: boolean;
+
     /** Nested objects of this namespace as an array for iteration. */
     public readonly nestedArray: ReflectionObject[];
 
@@ -900,21 +903,6 @@ export abstract class ReflectionObject {
     /** Unique name within its namespace. */
     public name: string;
 
-    /** The edition specified for this object.  Only relevant for top-level objects. */
-    public _edition: string;
-
-    /**
-     * 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;
-
-    /** Whether or not features have been resolved. */
-    public _featuresResolved: boolean;
-
     /** Parent namespace. */
     public parent: (Namespace|null);
 

src/namespace.js

@@ -124,6 +124,13 @@ function Namespace(name, options) {
      * @protected
      */
     this._needsRecursiveFeatureResolution = true;
+
+    /**
+     * Whether or not objects contained in this namespace need a resolve.
+     * @type {boolean}
+     * @protected
+     */
+    this._needsRecursiveResolve = true;
 }
 
 function clearCache(namespace) {
@@ -273,11 +280,13 @@ Namespace.prototype.add = function add(object) {
     }
 
     this._needsRecursiveFeatureResolution = true;
+    this._needsRecursiveResolve = true;
 
     // Also clear parent caches, since they need to recurse down.
     var parent = this;
     while(parent = parent.parent) {
         parent._needsRecursiveFeatureResolution = true;
+        parent._needsRecursiveResolve = true;
     }
 
     object.onAdd(this);
@@ -341,13 +350,16 @@ Namespace.prototype.define = function define(path, json) {
  * @returns {Namespace} `this`
  */
 Namespace.prototype.resolveAll = function resolveAll() {
+    if (!this._needsRecursiveResolve) return this;
+
     var nested = this.nestedArray, i = 0;
     this.resolve();
     while (i < nested.length)
         if (nested[i] instanceof Namespace)
             nested[i++].resolveAll();
         else
             nested[i++].resolve();
+    this._needsRecursiveResolve = false;
     return this;
 };
 
@@ -389,29 +401,47 @@ Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChe
     } else if (!path.length)
         return this;
 
+    var flatPath = path.join(".");
+
     // Start at root if path is absolute
     if (path[0] === "")
         return this.root.lookup(path.slice(1), filterTypes);
 
-    var found = this._lookupImpl(path);
+    // Early bailout for objects with matching absolute paths
+    var found = this.root._fullyQualifiedObjects["." + flatPath];
+    if (found && (!filterTypes || filterTypes.indexOf(found.constructor) > -1)) {
+        return found;
+    }
+
+    // Do a regular lookup at this namespace and below
+    found = this._lookupImpl(path, flatPath);
     if (found && (!filterTypes || filterTypes.indexOf(found.constructor) > -1)) {
         return found;
     }
 
-    // If there hasn't been a match, try again at the parent
-    if (this.parent === null || parentAlreadyChecked)
+    if (parentAlreadyChecked)
         return null;
-    return this.parent.lookup(path, filterTypes);
+
+    // If there hasn't been a match, walk up the tree and look more broadly
+    var current = this;
+    while (current.parent) {
+        found = current.parent._lookupImpl(path, flatPath);
+        if (found && (!filterTypes || filterTypes.indexOf(found.constructor) > -1)) {
+            return found;
+        }
+        current = current.parent;
+    }
+    return null;
 };
 
 /**
  * Internal helper for lookup that handles searching just at this namespace and below along with caching.
  * @param {string[]} path Path to look up
+ * @param {string} flatPath Flattened version of the path to use as a cache key
  * @returns {ReflectionObject|null} Looked up object or `null` if none could be found
  * @private
  */
-Namespace.prototype._lookupImpl = function lookup(path) {
-    var flatPath = path.join(".");
+Namespace.prototype._lookupImpl = function lookup(path, flatPath) {
     if(Object.prototype.hasOwnProperty.call(this._lookupCache, flatPath)) {
         return this._lookupCache[flatPath];
     }
@@ -422,13 +452,15 @@ Namespace.prototype._lookupImpl = function lookup(path) {
     if (found) {
         if (path.length === 1) {
             exact = found;
-        } else if (found instanceof Namespace && (found = found._lookupImpl(path.slice(1))))
-            exact = found;
+        } else if (found instanceof Namespace) {
+            path = path.slice(1);
+            exact = found._lookupImpl(path, path.join("."));
+        }
 
     // 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]._lookupImpl(path)))
+            if (this._nestedArray[i] instanceof Namespace && (found = this._nestedArray[i]._lookupImpl(path, flatPath)))
                 exact = found;
     }
 

src/object.js

@@ -51,25 +51,29 @@ function ReflectionObject(name, options) {
     /**
      * The edition specified for this object.  Only relevant for top-level objects.
      * @type {string}
+     * @private
      */
     this._edition = null;
 
     /**
      * 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.
      * @type {string}
+     * @private
      */
     this._defaultEdition = "proto2";
 
     /**
      * Resolved Features.
      * @type {object}
+     * @private
      */
     this._features = {};
 
     /**
      * Whether or not features have been resolved.
      * @type {boolean}
+     * @private
      */
     this._featuresResolved = false;
 

src/root.js

@@ -36,8 +36,19 @@ function Root(options) {
      */
     this.files = [];
 
-    // Default to proto2 if unspecified.
+    /**
+     * Edition, defaults to proto2 if unspecified.
+     * @type {string}
+     * @private
+     */
     this._edition = "proto2";
+
+    /**
+     * Global lookup cache of fully qualified names.
+     * @type {Object.<string,ReflectionObject>}
+     * @private
+     */
+    this._fullyQualifiedObjects = {};
 }
 
 /**
@@ -51,7 +62,7 @@ Root.fromJSON = function fromJSON(json, root) {
         root = new Root();
     if (json.options)
         root.setOptions(json.options);
-    return root.addJSON(json.nested)._resolveFeaturesRecursive();
+    return root.addJSON(json.nested).resolveAll();
 };
 
 /**
@@ -100,7 +111,7 @@ Root.prototype.load = function load(filename, options, callback) {
     // Finishes loading by calling the callback (exactly once)
     function finish(err, root) {
         if (root) {
-            root._resolveFeaturesRecursive();
+            root.resolveAll();
         }
         /* istanbul ignore if */
         if (!callback) {
@@ -219,7 +230,7 @@ Root.prototype.load = function load(filename, options, callback) {
         if (resolved = self.resolvePath("", filename[i]))
             fetch(resolved);
     if (sync) {
-        self._resolveFeaturesRecursive();
+        self.resolveAll();
         return self;
     }
     if (!queued) {
@@ -268,6 +279,8 @@ Root.prototype.loadSync = function loadSync(filename, options) {
  * @override
  */
 Root.prototype.resolveAll = function resolveAll() {
+    if (!this._needsRecursiveResolve) return this;
+
     if (this.deferred.length)
         throw Error("unresolvable extensions: " + this.deferred.map(function(field) {
             return "'extend " + field.extend + "' in " + field.parent.fullName;
@@ -335,6 +348,11 @@ Root.prototype._handleAdd = function _handleAdd(object) {
             object.parent[object.name] = object; // expose namespace as property of its parent
     }
 
+    if (object instanceof Type || object instanceof Enum || object instanceof Field) {
+        // Only store types and enums for quick lookup during resolve.
+        this._fullyQualifiedObjects[object.fullName] = object;
+    }
+
     // The above also adds uppercased (and thus conflict-free) nested types, services and enums as
     // properties of namespaces just like static code does. This allows using a .d.ts generated for
     // a static module with reflection-based solutions where the condition is met.
@@ -375,6 +393,8 @@ Root.prototype._handleRemove = function _handleRemove(object) {
             delete object.parent[object.name]; // unexpose namespaces
 
     }
+
+    delete this._fullyQualifiedObjects[object.fullName];
 };
 
 // Sets up cyclic dependencies (called in index-light)

src/service.js

@@ -110,6 +110,8 @@ Service.prototype.get = function get(name) {
  * @override
  */
 Service.prototype.resolveAll = function resolveAll() {
+    if (!this._needsRecursiveResolve) return this;
+
     Namespace.prototype.resolve.call(this);
     var methods = this.methodsArray;
     for (var i = 0; i < methods.length; ++i)

src/type.js

@@ -303,6 +303,8 @@ Type.prototype.toJSON = function toJSON(toJSONOptions) {
  * @override
  */
 Type.prototype.resolveAll = function resolveAll() {
+    if (!this._needsRecursiveResolve) return this;
+
     Namespace.prototype.resolveAll.call(this);
     var oneofs = this.oneofsArray; i = 0;
     while (i < oneofs.length)