Enhance Message Validation #1341
Conversation
|
No idea why it's only failing in humble pipelines |
|
Thanks for submitting the PR, will take a look! |
No worries, sometimes the pipeline fails. I will re-run it. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive message validation system for rclnodejs, allowing developers to validate ROS 2 messages before publishing or sending them through services and actions. The implementation includes schema introspection capabilities, structured error reporting, and flexible validation options with both opt-in and per-call override mechanisms.
Key Changes:
- Added message validation API with schema introspection functions (
getMessageSchema,getFieldNames,getFieldType) - Implemented validation for Publishers, Service Clients, and Action Clients with opt-in configuration
- Introduced
MessageValidationErrorclass with detailed issue reporting - Fixed existing bugs in
message_translator.jsfor proper type checking
Reviewed changes
Copilot reviewed 19 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
types/message_validation.d.ts |
New TypeScript definitions for validation API including enums, interfaces, and function signatures |
types/errors.d.ts |
Added MessageValidationError and MessageValidationIssue type definitions |
types/publisher.d.ts |
Added PublishOptions interface and validation-related methods/properties for Publisher |
types/client.d.ts |
Added SendRequestOptions interface and validation support for service clients |
types/action_client.d.ts |
Added SendGoalOptions interface and validation support for action clients |
types/index.d.ts |
Added reference to new message validation type definitions |
lib/message_validation.js |
Core validation implementation with schema introspection and message validation logic |
lib/errors.js |
Implemented MessageValidationError class with issue filtering methods |
lib/publisher.js |
Integrated validation into publisher with opt-in and per-publish override support |
lib/client.js |
Integrated validation into service client for request validation |
lib/action/client.js |
Integrated validation into action client for goal validation |
index.js |
Exported new validation functions and error classes to public API |
rosidl_gen/message_translator.js |
Fixed bugs: corrected property access, added wstring support, improved int64/uint64 handling |
src/macros.h |
Code formatting improvements for consistency |
test/test-message-validation.js |
Comprehensive test suite covering all validation features |
example/topics/publisher/publisher-validation-example.js |
Example demonstrating publisher validation features |
example/services/client/client-validation-example.js |
Example demonstrating service client validation |
example/actions/action_client/action-client-validation-example.js |
Example demonstrating action client validation |
example/topics/README.md |
Updated documentation to include validation examples |
example/services/README.md |
Updated documentation with validation example details |
example/actions/README.md |
Updated documentation with action validation examples |
example/rate/README.md |
Minor formatting improvements |
example/lifecycle/README.md |
Minor formatting improvements |
example/graph/README.md |
Minor formatting improvements |
CONTRIBUTORS.md |
Added credit for message validation enhancement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
the failure on humble pipleline is casued by mis-submitted the prebuilt binary, please remove it and try again, thanks!
There was a problem hiding this comment.
That's very weird I'm not sure how this file ended up here
There was a problem hiding this comment.
Now a third one is failing 😞
mahmoud-ghalayini
force-pushed
the
enhance-message-validation-1340
branch
from
eb1a32d to
e3710d3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 25 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## Troubleshooting | ||
|
|
||
| ### Common Issues | ||
|
|
There was a problem hiding this comment.
This change removes a blank line that appears to be unrelated to the message validation feature. This formatting change should not be included in this PR as it modifies content outside the scope of the PR's purpose (message validation).
There was a problem hiding this comment.
Do I need to revert these removed lines? @minggangw
There was a problem hiding this comment.
Ah, by the way I noticed that these changes in the readme happens when I do npm run format
| ## Troubleshooting | ||
|
|
||
| ### Common Issues | ||
|
|
There was a problem hiding this comment.
This change removes a blank line that appears to be unrelated to the message validation feature. This formatting change should not be included in this PR as it modifies content outside the scope of the PR's purpose (message validation).
| - Rate limiting causes message drops | ||
| - Expected behavior in the example (only every 200th message processed) | ||
| - Use `ros2 topic echo` to see all messages | ||
|
|
There was a problem hiding this comment.
This change removes a blank line that appears to be unrelated to the message validation feature. This formatting change should not be included in this PR as it modifies content outside the scope of the PR's purpose (message validation).
| ## Troubleshooting | ||
|
|
||
| ### Common Issues | ||
|
|
There was a problem hiding this comment.
This change removes a blank line that appears to be unrelated to the message validation feature. This formatting change should not be included in this PR as it modifies content outside the scope of the PR's purpose (message validation).
| #### Functionality | ||
|
|
||
| This example creates a complete ROS 2 system with multiple nodes and then introspects the graph to display: | ||
|
|
There was a problem hiding this comment.
This change removes a blank line that appears to be unrelated to the message validation feature. This formatting change should not be included in this PR as it modifies content outside the scope of the PR's purpose (message validation).
| - Ensure action server is running before starting client | ||
| - Check that both use the same action name (`fibonacci`) | ||
| - Verify action type matches (`test_msgs/action/Fibonacci`) | ||
|
|
There was a problem hiding this comment.
This change removes a blank line that appears to be unrelated to the message validation feature. This formatting change should not be included in this PR as it modifies content outside the scope of the PR's purpose (message validation).
| - Check server's `goalCallback` return value | ||
| - Verify goal message structure is correct | ||
| - Ensure server is properly initialized | ||
|
|
There was a problem hiding this comment.
This change removes a blank line that appears to be unrelated to the message validation feature. This formatting change should not be included in this PR as it modifies content outside the scope of the PR's purpose (message validation).
| - Processing time exceeds rate period | ||
| - Solution: Optimize processing or reduce rate frequency | ||
| - Monitor actual vs. target frequency | ||
|
|
There was a problem hiding this comment.
This change removes a blank line that appears to be unrelated to the message validation feature. This formatting change should not be included in this PR as it modifies content outside the scope of the PR's purpose (message validation).
| - Ensure lifecycle publisher is activated in `onActivate()` | ||
| - Check that node is in active state | ||
| - Verify publisher is created in `onConfigure()` | ||
|
|
There was a problem hiding this comment.
This change removes a blank line that appears to be unrelated to the message validation feature. This formatting change should not be included in this PR as it modifies content outside the scope of the PR's purpose (message validation).
| - Create timer in `onActivate()`, not `onConfigure()` | ||
| - Cancel timer in `onDeactivate()` | ||
| - Check timer interval format (BigInt nanoseconds) | ||
|
|
There was a problem hiding this comment.
This change removes a blank line that appears to be unrelated to the message validation feature. This formatting change should not be included in this PR as it modifies content outside the scope of the PR's purpose (message validation).
rosidl_gen/message_translator.js
Outdated
| } | ||
|
|
||
| function verifyMessage(message, obj) { | ||
| function verifyMessage(message, inputObj) { |
There was a problem hiding this comment.
This function was design to do some basic verification long time ago, will your PR replace its functionality? If yes, I think we can remove it becasue it doesn't get called anymore.
I go through this PR roughly and will try to understand more deeply, thanks!
There was a problem hiding this comment.
Yes we can remove it 😃
3 participants
Public API Changes
New Exports from
rclnodejs:ValidationProblem- Enum for validation issue typesMessageValidationError- Error class with structured issue reportinggetMessageSchema(typeClass)- Get message schemagetFieldNames(typeClass)- Get field names arraygetFieldType(typeClass, fieldName)- Get field type infovalidateMessage(obj, typeClass, options)- Validate without throwingassertValidMessage(obj, typeClass, options)- Validate and throw on failurecreateMessageValidator(typeClass, options)- Create reusable validatorNew Options:
PublisherOptions.validateMessages- Enable publisher validationClientOptions.validateRequests- Enable service client validationActionClientOptions.validateGoals- Enable action client validationNew Methods:
Publisher.setValidation(enable)- Toggle validation dynamicallyPublisher.validationEnabled- Check validation stateClient.setValidation(enable)/Client.validationEnabledActionClient.setValidation(enable)/ActionClient.validationEnabledPer-call Validation Override:
publisher.publish(msg, { validate: true/false })client.sendRequest(req, callback, { validate: true/false })client.sendRequestAsync(req, { validate: true/false })actionClient.sendGoal(goal, feedbackCb, uuid, { validate: true/false })#1340