rcl_yaml_param_parser allows two // in node name

[sloretz]

Bug report

Required Info:

• Operating System:
  • Ubuntu Bionic
• Installation type:
  • source
• Version or commit hash:
  • master
• DDS implementation:
  • Fast-RTPS
• Client library (if applicable):
  • N/A

Steps to reproduce issue

Create a yaml file called empty_name_in_ns.yaml in rcl_yaml_param_parser/test with the following

lidar_ns//lidar_w:
  ros__parameters:
    id: 10
    name: front_lidar
    ports: [2438, 2439, 2440]
    driver1:
      dx: 4.56
      dy: 2.30
      fr_sensor_specs: [12, 3, 0, 7]
      bk_sensor_specs: [12.1, -2.3, 5.2, 9.0]
    is_front: true
    driver2:
      dx: 1.23
      dy: 0.45

Add this test block to test_parse_yaml.cpp

TEST(test_file_parser, empty_name_in_ns) {
  rcutils_reset_error();
  EXPECT_TRUE(rcutils_get_cwd(cur_dir, 1024));
  rcutils_allocator_t allocator = rcutils_get_default_allocator();
  char * test_path = rcutils_join_path(cur_dir, "test", allocator);
  char * path = rcutils_join_path(test_path, "empty_name_in_ns.yaml", allocator);
  fprintf(stderr, "cur_path: %s\n", path);
  EXPECT_TRUE(rcutils_exists(path));
  rcl_params_t * params_hdl = rcl_yaml_node_struct_init(allocator);
  EXPECT_FALSE(NULL == params_hdl);
  bool res = rcl_parse_yaml_file(path, params_hdl);
  fprintf(stderr, "%s\n", rcutils_get_error_string_safe());
  EXPECT_FALSE(res);
  rcl_yaml_node_struct_print(params_hdl);
  rcl_yaml_node_struct_fini(params_hdl);
  allocator.deallocate(test_path, allocator.state);
  allocator.deallocate(path, allocator.state);
}

Expected behavior

Parsing should fail to parse the yaml file.

Actual behavior

1:  Node Name                           Parameters
1: lidar_ns//lidar_w
1:                                                 id: 10
1:                                               name: front_lidar
1:                                              ports: 2438, 2439, 2440, 
1:                                         driver1.dx: 4.560000
1:                                         driver1.dy: 2.300000
1:                            driver1.fr_sensor_specs: 12, 3, 0, 7, 
1:                            driver1.bk_sensor_specs: 12.100000, -2.300000, 5.200000, 9.000000, 
1:                                           is_front: true
1:                                         driver2.dx: 1.230000
1:                                         driver2.dy: 0.450000

Additional information

See #299 for another bug about / in the namespace names.

[dhood]

alternative to failing to parse the yaml file would be to parse it and remove the duplicated separators so they are assigned to node lidar_ns/lidar_w (which is what would happen in rcl if you pass duplicated slashes, they will collapse I believe)

[iuhilnehc-ynos]

I'd like to contribute to this issue. I think that rcl_yaml_param_parser should notify user it's a error if there is a namespace named with an empty name inside //.

[iuhilnehc-ynos]

@sloretz

I have found a way to fix it by adding rmw_validate_namespace behind the following source code,

rcl/rcl_yaml_param_parser/src/namespace.c

Lines 48 to 51 in 8bcada7

	/// Add a name to ns
	if (NULL == name) {
	return RCUTILS_RET_INVALID_ARGUMENT;
	}

but it seems we need rmw_validate_namespace to check if a name is a valid namespace by using rmw dependency for rcl_yaml_param_parser.

or use one of the following ways:

• Add a new function at rcl_yaml_param_parser (It seems not good.)
• Move https://github.com/ros2/rcl/blob/8bcada7b884693644d7ba2e3bd2c22fd9541eba7/rcl/include/rcl/lexer.h into rcutils

What do you think about that?

[iuhilnehc-ynos]

Add a new function at rcl_yaml_param_parser (It seems not good.)

Which seems not good may be the best choice.

NOTE: I found that using rmw_validate_namespace has some limitations because there are some extra rules, such as "/**" (maybe more in the future), need to be parsed.

[fujitatomoya]

alternative to failing to parse the yaml file would be to parse it and remove the duplicated separators so they are assigned to node lidar_ns/lidar_w

http://design.ros2.org/articles/topic_and_service_names.html#name-tokens tells us Topic and service name tokens: must not be empty, e.g. the name //bar is not allowed. So I believe that it should be failed with //bar. (besides, addressing redundancy is going to be a problem how redundancy we take care, this would be more complicated.)

[jacobperron]

I think the issue is a bit more general. While I agree a namespace containing // in a parameters file should cause an error, I think any invalid namespace should cause an error. This includes namespaces starting with (or containing) invalid characters, e.g. /123. I think we can argue the same for node names. Currently, any string for node names seems acceptable (or silently ignored) in the parameters yaml file.