Fail lookups on invalid resource names

[rob-clarke]

Depends on ros2/ros2cli#643

---

This was found through ros2launch, where attempting to provide an absolute path to a launch file, along with launch arguments, resulted in failure. This was because ros2launch was passing the absolute path to get_package_prefix() in attempting to see if it was a package.

If a valid absolute path was passed to get_package_prefix() as the package name, a PackageNotFoundError was not raised and instead the first path from get_search_paths() was returned.

This resulted from the behaviour of get_resource(): if a valid absolute path was passed to get_resource() as the resource name, the contents of that absolute path was returned along with the first path from get_search_paths() as the path.

This PR adds an InvalidResourceNameError which is raised when the resource name argument contains a /. This catches the absolute path case as well as cases that may result from relative paths. I couldn't find any hard requirements on resource names other than a suggestion here that they should conform to REP 127 suggestions on naming, which are now captured in REP 144. If stricter restrictions on resource names are defined, these can be added to the name_is_invalid() function added here.

I'm unsure if this is a good fit for here or the problem is better off being addressed in ros2launch, where input to get_package_prefix() could be sanitised. Even if changed here, a fix in ros2launch will be needed but the error message should now be a little clearer.

[audrow]

@rob-clarke, thanks for the PR the code and tests look good to me.

As you pointed out, I'm not sure if this should be fixed in ros2launch. @hidmic, what do you think?

[hidmic]

I couldn't find any hard requirements on resource names other than a suggestion here that they should conform to REP 127 suggestions on naming, which are now captured in REP 144.

@rob-clarke to the best of my knowledge, there aren't any strict requirements. I'd rather enforce valid package names instead of coming up with extra checks for resource names.

[rob-clarke]

The current resource index documentation says:

Implementations should consider that spaces are allowed in marker file names...

So I'm hesitant to enforce package names as it doesn't fit with the warnings there.

Currently the PR only checks for forward slashes in the resource name which fixes the issue that led to this. It could be made stricter to only accept valid package names and additionally spaces to follow the resource index doc.

[hidmic]

@rob-clarke I meant enforcing valid package names where a package name is expected i.e. in get_package_prefix(), not in get_resource. Sorry if I wasn't clear.

[rob-clarke]

Yes, a check for a valid package name can be added to get_package_prefix(). Would this then raise PackageNotFound or be more descriptive? e.g. raise ValueError

I think some form of resource name check is still necessary. Ultimately the cause of this was that if a resource name starting with / (or presumably \ on Windows) is passed to get_resource(), the use of os.path.join() results in that name replacing the entire path. If that name is also a valid file path, the subsequent os.path.isfile() check passes and no exception is raised. As far as I can tell, resources are only ever expected to be marker files, so raising an exception for any resource name containing either / or \ should be safe.

[hidmic]

Would this then raise PackageNotFound or be more descriptive? e.g. raise ValueError

ValueError seems appropriate.

I think some form of resource name check is still necessary. Ultimately the cause of this was that if a resource name starting with / (or presumably \ on Windows) is passed to get_resource(), the use of os.path.join() results in that name replacing the entire path. If that name is also a valid file path, the subsequent os.path.isfile() check passes and no exception is raised. As far as I can tell, resources are only ever expected to be marker files, so raising an exception for any resource name containing either / or \ should be safe.

That's fair. I think it's reasonable to assume absolute paths are no-go for resource names and types, as these break search paths. But a relative path for a resource type or name wouldn't -- and it may be even be handy for someone, though I don't know of an existing use case.

Checking for invalid package names and absolute paths in both resource types and names should address the issue in ros2 launch when a path (absolute or relative) to a launch file is provided. WDYT?

[rob-clarke]

Quoting from the docs again:

Resource types are represented as folders in the resource index and should be shallow, i.e. there should only be marker files within the resource type folders. This means that there is no nesting of resource types, which keeps the file system flat, and makes answering "What resource types are in this ?" as easy as simply listing the directories in the resource index folder. Any folders within the resource type folders, and any folders/files starting with a dot (.), should be ignored while listing the directories in the resource index folder. Instead of nesting resource type folders, the convention of using prefixes and suffixes separated by periods should be used.

That suggests that relative paths should not be expected for resource type names or resource names.

Looking at the bigger picture: is looking up a resource of a type with a relative path name not just equivalent to looking up a different resource type? At which point would the user not be better off just calling get_resource('type_i_actually_want','name') rather than gaming the API?

[hidmic]

That suggests that relative paths should not be expected for resource type names or resource names.

Indeed. I missed that piece of documentation. I'm fine with blocking slashes for resource types and names then.

is looking up a resource of a type with a relative path name not just equivalent to looking up a different resource type?

Yes, it'd equivalent from a user POV. The ambiguity is not necessarily bad, but it appears it wasn't intended.

[hidmic]

Overall LGTM

[hidmic]

@rob-clarke nit:

Suggested change

	if ('/' in resource_name) or ('\\' in resource_name):
	return True
	return False
	return ('/' in resource_name) or ('\\' in resource_name)

[audrow]

Looks good to me with green CI.

[audrow]

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

While this is definitely a bugfix, I'm nervous about putting it into Galactic at this juncture. It's in such a low-level package that any problems here have the potential to cause instabilities all the way up the stack. I'm going to suggest that we hold off on this one for post-Galactic, and then consider backporting it into Galactic in a patch release. If we go that route, I think this also deserves an entry in the Galactic release notes as a Known Issue.

[audrow]

I'm going to suggest that we hold off on this one for post-Galactic, and then consider backporting it into Galactic in a patch release.

Sounds good to me.

If we go that route, I think this also deserves an entry in the Galactic release notes as a Known Issue.

If that is what we decide to do, I can add this to the release notes.

[clalancette]

This one sounds like a good one to add to the (not-yet-created) Galactic Patch Release 1 board. @cottsay please add this one as well when you get a chance.

[rob-clarke]

The main thing to watch out for in the full stack testing is use of get_package_prefix with autogenerated package names that may not conform to REP-144. The check implemented here checks for REP-144 but additionally allows consecutive underscores that may appear in autogenerated names:

• not use multiple _ separators consecutively. This allows generated symbols to use the __ separator to guarenteed the avoidance of collisions with symbols from other packages, for example in the message generators.

[audrow]

@rob-clarke, from you fix the CI errors? It looks like (1) there is a linter warning and (2) that another PR needs to be added to update a test in ros2pkg to expect a ValueError. Let me know if you have any questions.

[cottsay]

This one sounds like a good one to add to the (not-yet-created) Galactic Patch Release 1 board. @cottsay please add this one as well when you get a chance.

Done: https://github.com/orgs/ros2/projects/32

[rob-clarke]

I think that fixes the linter warnings.

I'll need a bit more input on the ros2pkg error. It comes down to deciding if a leading underscore is grounds for raising a ValueError for an invalid package name. By REP-144 it would be an invalid name, but if this is likely to be used with autogenerated package names that may not conform, it could be made less strict here.

[clalancette]

@rob-clarke You've mentioned auto-generated package names that don't conform to REP-144 a couple of times here. Could you explain more on where you expect these to come from?

[rob-clarke]

It's not really that I expect them to exist, just that the standard leaves that option open so I don't want to break things elsewhere in the stack.

[tfoote]

We should not relax the naming rules in REP 144.

First, we should not relax the REP for "generated" packages. To a user a generated package is the same as any other package on their system. You'll find that several packages already have generated content pragmatically. To keep "generated" packages from colliding I recommend using a unique prefix with the name relating to the project or organization. Packages operate in a flat namespace and it's the developer's responsibility not to clobber others in that namespace. Adding the prefix underscore will keep you from colliding with others not using the double underscore. But if you're generating packages without controls or limits it would seem to be easy for _foo to be generated by two different generators which will collide because you'll never know when someone else is also "generating" packages.

And second, the name rules are leveraged in our different language code generators so that generated code cannot collide with user written code that follows our naming conventions. Relaxing the requirement that the underscores are only internal separators would prevent the generated code from using a double underscore to provide a higher level separator. This is particularly important as we need to have this same guarantee even if a new language support is added that the developer didn't know about when writing their code etc.

[audrow]

@rob-clarke, is this ready to be reviewed again by @wjwwood ?

[rob-clarke]

Yes, I think I've addressed all the comments.

[wjwwood]

Lgtm

[audrow]

• Linux
• Linux-aarch64
• macOS
• Windows

[audrow]

It seems that this test is now throwing a ValueError: https://github.com/ros2/ros2cli/blob/fe074a30979c18f148b119c008a4a49c11c346cf/ros2pkg/test/test_api.py#L32

@rob-clarke, if you make a small PR in ros2cli, you can ping me for review.

[audrow]

New CI with ros2/ros2cli#643:

• Linux
• Linux-aarch64
• macOS
• Windows

[audrow]

Thanks @rob-clarke for the PR! 🎉