Skip to content

fix(cli): resolve --filter dependencies at any namespace depth - #2417

Open
twoRoger wants to merge 1 commit into
protobufjs:masterfrom
twoRoger:fix/cli-filter-nested-namespaces
Open

fix(cli): resolve --filter dependencies at any namespace depth#2417
twoRoger wants to merge 1 commit into
protobufjs:masterfrom
twoRoger:fix/cli-filter-nested-namespaces

Conversation

@twoRoger

Copy link
Copy Markdown

Fixes #2416

Summary

--filter could not resolve a dependency whose package has more than one segment, so any schema using a package like example.values ended up producing a completely unfiltered root.

Root cause

Dependency names were rebuilt from the immediate parent of the resolved type:

var packageName = field.resolvedType.parent.name;
var typeName = field.resolvedType.name;
var fullName = packageName ? `${packageName}.${typeName}` : typeName;

For .example.values.Value the immediate parent is only values, producing the bogus name values.Value. That name was then consumed by a lookup that splits into exactly two segments and indexes root.nested directly, so nothing below package.Message could be addressed.

Pruning had the same one-level limitation: it filtered root._nestedArray and then a single level of ns._nestedArray.

Fix

The filter now works on reflection objects instead of reconstructed names:

  • each configured name is resolved with root.lookup(name, [protobuf.Type]), which handles full names of any namespace depth;
  • the transitive closure is built over field.resolvedType, with a Set of reflection objects as the cycle guard. fieldsArray covers plain, repeated, map value, oneof and group fields alike;
  • pruning is recursive and goes through the public Namespace#remove and Namespace#add rather than assigning to the private _nestedArray. This keeps Root#_fullyQualifiedObjects in sync, so pruned types are no longer reachable through lookup — previously they were, because that cache is a lookup fallback;
  • namespaces that retained objects need for their full name are kept. An unselected Type or Service that only holds retained descendants is reduced to a plain Namespace, so no message or service code is emitted for it.

One subtlety worth calling out: Namespace#add stamps direct children of a plain namespace with the default edition. A type nested inside another type has no explicit edition of its own, so moving it into the replacement namespace as-is would silently change it from proto3 to proto2 and flip field presence from IMPLICIT to EXPLICIT. The edition in effect before the replacement is therefore pinned on the replacement and on each moved child.

The configuration is also validated up front and rejected unless it is a non-empty array of non-empty strings — otherwise an empty selection would now silently produce an empty root. Names are resolved before anything is removed, so a filter that cannot be applied leaves the root untouched for the existing fallback in pbjs.

No behavior outside --filter changes; cli/pbjs.js is untouched.

Tests

Added to tests/cli-pbjs.js:

  • the regression above, with a dependency in example.values;
  • cyclic references, repeated message and enum fields, map values, and oneof branches;
  • nested types that are used are kept while unused ones are dropped;
  • a root selected inside an unselected parent type: the parent is reduced to a namespace, its own fields are gone, and the retained nested type keeps its edition;
  • invalid configurations, including enum, service and package names, which are not valid roots.

Every filter assertion also reloads the pruned root through Root.fromJSON(root.toJSON()).resolveAll(). Resolving the pruned root in place is not sufficient — its objects are already resolved and Namespace#remove does not invalidate them — so reloading is what actually proves no retained field points at a pruned type.

The existing --filter test, which covers a single-segment package and a message without a package, is unchanged and still passes.

Commands run

npm install
npm --prefix cli install
npm run build
npm run build:tests
npm run lint:sources -- --max-warnings 0
npm run lint:types
npm test

npm run build:tests produces an unrelated diff in tests/data/test.js that also reproduces on a clean master, so it is not included here.

--filter only understood names of the form `package.Message`. Dependencies
were rebuilt as `resolvedType.parent.name + "." + resolvedType.name`, so a
message in a deeper namespace such as `example.values.Value` produced the
bogus name `values.Value` and the filter threw, which pbjs reports before
falling back to the unfiltered root.

Replace the string-based lookup with the reflection API:

- resolve each configured name with `root.lookup(name, [protobuf.Type])`,
  which handles full names of any namespace depth;
- build the transitive closure over `field.resolvedType`, using a Set of
  reflection objects as the cycle guard, which covers repeated, map value,
  oneof and group fields alike;
- prune the tree recursively through the public `Namespace#remove` and
  `Namespace#add` instead of assigning to the private `_nestedArray`, so
  that the root's fully qualified object cache stays in sync and removed
  types are no longer reachable through lookup;
- keep namespaces that retained objects need for their full name, reducing
  an unselected `Type` or `Service` to a plain `Namespace` so no message or
  service code is emitted for it. The edition in effect before the
  replacement is pinned on the moved children, otherwise `Namespace#add`
  stamps them with the default edition and changes field presence.

Validate the configuration up front and reject anything that is not a
non-empty array of non-empty strings, so that an empty selection no longer
silently produces an empty root. Names are resolved before the tree is
touched, leaving the root intact for the existing fallback in pbjs when a
filter cannot be applied.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pbjs --filter fails to resolve dependencies in packages deeper than one segment

1 participant