Add ParameterEventHandler node filtering support#1474
Add ParameterEventHandler node filtering support#1474minggangw merged 3 commits intoRobotWebTools:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds node-based filtering to ParameterEventHandler by leveraging DDS content filters on the /parameter_events subscription, enabling consumers to limit received events to specific nodes (with relative names resolved to the handler node’s namespace).
Changes:
- Add
ParameterEventHandler.configureNodesFilter(nodeNames?)to apply/clear a/parameter_eventssubscription content filter. - Implement relative node-name resolution against the handler node namespace (and add a private resolver helper).
- Add TypeScript declarations and focused unit tests for node filtering behavior and validation.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/parameter_event_handler.js | Implements configureNodesFilter() and node path resolution logic used to build subscription content filters. |
| types/parameter_event_handler.d.ts | Adds TS declaration and docs for configureNodesFilter(). |
| test/test-parameter-event-handler.js | Adds unit tests validating absolute/relative filters, clearing behavior, and invalid input handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const resolvedNodeNames = nodeNames.map((nodeName, index) => { | ||
| if (typeof nodeName !== 'string' || nodeName.trim() === '') { | ||
| throw new TypeValidationError( | ||
| `nodeNames[${index}]`, | ||
| nodeName, | ||
| 'non-empty string', | ||
| { | ||
| entityType: 'parameter event handler', | ||
| } | ||
| ); | ||
| } | ||
| return this.#resolvePath(nodeName.trim()); | ||
| }); | ||
|
|
||
| const contentFilter = { | ||
| expression: resolvedNodeNames | ||
| .map((_, index) => `node = %${index}`) | ||
| .join(' OR '), | ||
| parameters: resolvedNodeNames.map((nodeName) => `'${nodeName}'`), | ||
| }; |
There was a problem hiding this comment.
configureNodesFilter() wraps each node name in single quotes for the content-filter parameter ('${nodeName}') but does not escape or reject ' characters in nodeNames. Passing a value containing a quote will produce an invalid filter expression (or allow filter-expression injection). Consider validating node names against ROS name rules (or at least escaping/rejecting ') before building contentFilter.parameters.
lib/parameter_event_handler.js
Outdated
| if (!nodePath) { | ||
| return this.#node.getFullyQualifiedName(); | ||
| } | ||
|
|
There was a problem hiding this comment.
#resolvePath() has an if (!nodePath) branch returning this.#node.getFullyQualifiedName(), but configureNodesFilter() rejects empty/whitespace-only node names before calling it. This branch is currently unreachable and can be removed (or, if empty strings are intended to clear/target the handler node, adjust the validation/docs accordingly).
| if (!nodePath) { | |
| return this.#node.getFullyQualifiedName(); | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .join(' OR '), | ||
| parameters: resolvedNodeNames.map((nodeName) => `'${nodeName}'`), | ||
| }; | ||
|
|
There was a problem hiding this comment.
configureNodesFilter() calls setContentFilter() unconditionally. Content-filtered topics are only supported on some ROS distros/RMWs (see Subscription.isContentFilterSupported()), and setContentFilter() will throw a native exception when unsupported. Consider checking this.#subscription.isContentFilterSupported() (when available) and either returning false or throwing an OperationError with a clear message before attempting to set a filter.
| if ( | |
| typeof this.#subscription.isContentFilterSupported === 'function' && | |
| !this.#subscription.isContentFilterSupported() | |
| ) { | |
| throw new OperationError( | |
| 'Node filtering requires content-filtered topic support, which is not available in the current ROS distro/RMW implementation', | |
| { entityType: 'parameter event handler' } | |
| ); | |
| } |
| ); | ||
| } | ||
|
|
||
| const resolvedNodeName = this.#resolvePath(nodeName.trim()); |
There was a problem hiding this comment.
#validateFullyQualifiedNodePath() strips trailing slashes only for validation, but resolvedNodeName is used unchanged in the filter parameters. Inputs like /ns/node/ can pass validation yet generate a filter that won’t match ParameterEvent.node (published without trailing slashes), causing missed events. Normalize resolvedNodeName (strip trailing / except for /, and ideally collapse repeated /) before building the filter.
| const resolvedNodeName = this.#resolvePath(nodeName.trim()); | |
| const resolvedNodeName = normalizeNodeName( | |
| this.#resolvePath(nodeName.trim()) | |
| ); |
- add `ParameterEventHandler.configureNodesFilter(nodeNames?)` to apply or clear `/parameter_events` subscription content filters for selected nodes - resolve relative node names against the handler node namespace - reuse `Node.getFullyQualifiedName()` for handler node fully qualified name resolution instead of duplicating that logic - add TypeScript declarations for `configureNodesFilter()` - add focused `ParameterEventHandler` tests for: - absolute node filters - relative node name resolution - clearing filters when `nodeNames` is omitted - clearing filters when `nodeNames` is empty - invalid `nodeNames` validation Testing: - `npx mocha test/test-parameter-event-handler.js` - `npx tsd` Fix: #1473
ParameterEventHandler.configureNodesFilter(nodeNames?)to apply or clear/parameter_eventssubscription content filters for selected nodesNode.getFullyQualifiedName()for handler node fully qualified name resolution instead of duplicating that logicconfigureNodesFilter()ParameterEventHandlertests for:nodeNamesis omittednodeNamesis emptynodeNamesvalidationTesting:
npx mocha test/test-parameter-event-handler.jsnpx tsdFix: #1473