Sanitize temporary node name used in rdf_loader

[tokoro10g]

Description

Names of parameters do not have naming rules yet, whereas those for nodes do. Therefore the temporary node name containing the parameter name must be sanitized.

Checklist

• [x] Required by CI: Code is auto formatted using clang-format
• [ ] Extend the tutorials / documentation reference
• [ ] Document API changes relevant to the user in the MIGRATION.md notes
• [ ] Create tests, which fail without this PR reference
• [ ] Include a screenshot if changing a GUI
• [ ] While waiting for someone to review your request, please help review another open pull request to support the maintainers

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 50.34%. Comparing base (0e1e376) to head (8fa3c41). Report is 52 commits behind head on main.

:exclamation: Current head 8fa3c41 differs from pull request most recent head f09bff4. Consider uploading reports for the commit f09bff4 to get more accurate results

Files	Patch %	Lines
...g/rdf_loader/src/synchronized_string_parameter.cpp	0.00%	6 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2386      +/-   ##
==========================================
- Coverage   50.99%   50.34%   -0.65%     
==========================================
  Files         387      385       -2     
  Lines       32308    31801     -507     
==========================================
- Hits        16473    16007     -466     
+ Misses      15835    15794      -41     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[henningkayser]

I'm not following. What's wrong with the node name in rdf_loader in particular? The code doesn't look like it's producing invalid names to me.

[tokoro10g]

@henningkayser Thanks for your attention to this PR!! I could have explained it more clearly...

For example, abc.def containing . is a valid name for a parameter, https://github.com/ros2/rclcpp/blob/7f7ffcf6ed1143b4d4a9e0cf248e190731b2e5f8/rclcpp/include/rclcpp/node.hpp#L502-L504 but not for a node: https://github.com/ros2/rmw/blob/83445be486deae8c78d275e092eafb4bf380bd49/rmw/src/validate_node_name.c#L55-L71

In the current code, such a parameter name is directly used in the node name, and the node cannot be started because of this.

[henningkayser]

@henningkayser Thanks for your attention to this PR!! I could have explained it more clearly...

For example, abc.def containing . is a valid name for a parameter, https://github.com/ros2/rclcpp/blob/7f7ffcf6ed1143b4d4a9e0cf248e190731b2e5f8/rclcpp/include/rclcpp/node.hpp#L502-L504 but not for a node: https://github.com/ros2/rmw/blob/83445be486deae8c78d275e092eafb4bf380bd49/rmw/src/validate_node_name.c#L55-L71

In the current code, such a parameter name is directly used in the node name, and the node cannot be started because of this.

thanks for the pointers. What part of the node name in particular has an invalid character? Is it name_?

[tokoro10g]

@henningkayser Yes, exactly.

[tokoro10g]

@henningkayser Hi! I really appreciate your review! Please let me know if you have any further questions or suggestions on this change!

[tokoro10g]

@henningkayser Friendly ping! cc: @DLu

[DLu]

Is there no more-central resource for santizing names? It seems odd to have to do the same thing in each package.

[tokoro10g]

@DLu Thanks for the kind reply!

It seems odd to have to do the same thing in each package.

Agreed, and there are none as far as I could find. If we are to add the sanitization function in, let's say for example, rmw, it means that we need to change the API. The ROS2 versioning policy is strict on such changes, and we might need to wait for a major release.

I do not think that this simple fix is worth waiting for years. While this specific usage of the node name is rare, it should be reasonable to have it fixed now and get replaced by the "more-central resource" afterwards. What do you say?

[github-actions[bot]]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.