Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize name resolution for performance and spec compliance #2031

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -800,9 +800,10 @@ export abstract class NamespaceBase extends ReflectionObject {
* @param path Path to look up
* @param filterTypes Filter types, any combination of the constructors of `protobuf.Type`, `protobuf.Enum`, `protobuf.Service` etc.
* @param [parentAlreadyChecked=false] If known, whether the parent has already been checked
* @param [checkNested=false] @deprecated Whether to include nested objects in the lookup (for backwards-compatibility)
* @returns Looked up object or `null` if none could be found
*/
public lookup(path: (string|string[]), filterTypes: (any|any[]), parentAlreadyChecked?: boolean): (ReflectionObject|null);
public lookup(path: (string|string[]), filterTypes: (any|any[]), parentAlreadyChecked?: boolean, checkNested?: boolean): (ReflectionObject|null);

/**
* Looks up the reflection object at the specified path, relative to this namespace.
Expand Down
19 changes: 13 additions & 6 deletions src/namespace.js
Original file line number Diff line number Diff line change
Expand Up @@ -315,9 +315,10 @@ Namespace.prototype.resolveAll = function resolveAll() {
* @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
* @param {boolean} [checkNested=false] @deprecated Whether to include nested objects in the lookup (for backwards-compatibility)
* @returns {ReflectionObject|null} Looked up object or `null` if none could be found
*/
Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChecked) {
Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChecked, checkNested) {

/* istanbul ignore next */
if (typeof filterTypes === "boolean") {
Expand All @@ -326,6 +327,12 @@ Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChe
} else if (filterTypes && !Array.isArray(filterTypes))
filterTypes = [ filterTypes ];

if (typeof checkNested !== 'boolean')
// Note: adding backwards-compatibility support for resolving non-qualified names using nested namespaces,
// which goes against the protobuf specification. This will only be supported for lookups originating from
// the root namespace now, and fully qualified names should be used instead to avoid ambiguity.
checkNested = this === this.root && path[0] !== '.';

if (util.isString(path) && path.length) {
if (path === ".")
return this.root;
Expand All @@ -335,27 +342,27 @@ 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);
return this.root.lookup(path.slice(1), filterTypes, true, false);

// 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) {
if (path.length === 1) {
if (!filterTypes || filterTypes.indexOf(found.constructor) > -1)
return found;
} else if (found instanceof Namespace && (found = found.lookup(path.slice(1), filterTypes, true)))
} else if (found instanceof Namespace && (found = found.lookup(path.slice(1), filterTypes, true, false)))
return found;

// Otherwise try each nested namespace
} else
} else if (checkNested)
for (var i = 0; i < this.nestedArray.length; ++i)
if (this._nestedArray[i] instanceof Namespace && (found = this._nestedArray[i].lookup(path, filterTypes, true)))
if (this._nestedArray[i] instanceof Namespace && (found = this._nestedArray[i].lookup(path, filterTypes, true, true)))
return found;

// If there hasn't been a match, try again at the parent
if (this.parent === null || parentAlreadyChecked)
return null;
return this.parent.lookup(path, filterTypes);
return this.parent.lookup(path, filterTypes, false, false);
};

/**
Expand Down
5 changes: 5 additions & 0 deletions tests/api_namespace.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ enum Enm {\
}\
message Msg {\
message Enm {}\
message Inner {}\
}\
service Svc {}";

Expand All @@ -35,6 +36,10 @@ tape.test("reflected namespaces", function(test) {

test.equal(ns.get("Msg").lookupTypeOrEnum("Enm"), ns.lookup(".ns.Msg.Enm"), "should lookup the nearest type or enum");

test.throws(function() {
ns.lookupType("Inner");
}, Error, "should throw when looking up an inner nested type without specifying its path");

test.throws(function() {
ns.lookupType("Enm");
}, Error, "should throw when looking up an enum as a type");
Expand Down