Normalize relative paths from project $path entries #340
Conversation
There was a problem hiding this comment.
I'm not sure we need to introduce two more dependencies to do this, and I don't really like mucking with the path formatting more than is necessary. I made a couple suggestions for how we might roll our own implementation for normalization, it shouldn't be too bad.
| @@ -78,7 +80,42 @@ pub fn snapshot_project_node( | |||
| // If the path specified in the project is relative, we assume it's | |||
| // relative to the folder that the project is in, project_folder. | |||
| let path = if path.is_relative() { | |||
| Cow::Owned(project_folder.join(path)) | |||
| // Convert paths to use forward slashes for compatibility with the relative-path crate | |||
| let project_folder_with_slash_separator = match project_folder.to_slash() { | |||
There was a problem hiding this comment.
I dug into the relative-path crate a bit I'm... not really a fan o.O. I don't like that we have to fuss around with the paths.
Is it possible to create a solution to this that just works in terms of std::path? It seems like you could do this using https://doc.rust-lang.org/std/path/struct.Path.html#method.components or check out solutions here: rust-lang/rfcs#2208
As the description of components mentions, I'm a little concerned about handling symlinks correctly. If someone's entire project structure is under a symlink, it's possible that this logic could trip it up. I can't find any info on whether or not those libraries support this correctly o.O
I think it'd probably be most reasonable to just adapt the implementation from relative_path: https://docs.rs/relative-path/1.3.2/src/relative_path/lib.rs.html#1287-1291
At the end, where they rejoin with "/", maybe we could just rebuild a path via std::path at that point?
There was a problem hiding this comment.
Agreed - the relative-path crate is painful with regards to converting back and forth. It's a shame that rust doesn't have a built-in function for normalising paths.
Neither of these libraries (as used here) will handle symlinks as they are not hitting the FS to do any checks. Thinking about contrived symlink cases:
- Source project is loaded from under a symlink.
/foo/default.project.json: $path: ../world
/bar/baz: symlink to /foo
load(/bar/baz/default.project.json): /bar/baz/../world -> /bar/world
load(/foo/default.project.json): /foo/../world -> /world
- Intermediate directory in $path is a symlink.
/foo/default.project.json: $path: ../bar/../world
/bar: symlink to /biff/baz
/biff/baz
load(/foo/default.project.json): /foo/../bar/../world ->Unclear whether this should resolve to /world or /biff/world
Perhaps we should go with the components stack approach, but when processing the components check if any component is a symlink and abort the normalisation if it is, instead returning the original path. This shouldn't compromise achieving the goal of the solution because symlinks are less common on Windows anyway, which is the only platform where we need this.
There was a problem hiding this comment.
That sounds like a reasonable approach! If we adjust it so we only check for symlinks specifically on the components we want to normalize away, then even if you are within a symlinked directory, it doesn't matter if you don't need to climb that far
There was a problem hiding this comment.
If we go down the road of checking for symlinks, we'll likely need to expand the API surface of memofs. We can't use the stock fs APIs because a project could be built from an in-memory virtual filesystem, like with rojo plugin install.
| // Convert paths to use forward slashes for compatibility with the relative-path crate | ||
| let project_folder_with_slash_separator = match project_folder.to_slash() { | ||
| Some(p) => p, | ||
| None => return Ok(None), |
There was a problem hiding this comment.
I don't think these returns are correct, it looks like we'll end up returning early in the case that we happen to be unable to convert paths. Do we have a good understanding of when this will happen?
There was a problem hiding this comment.
Should be obviated by rewrite to custom function
|
Thought my review would trigger the CLA bot, but it doesn't look like it did. Maybe this comment will do it? |
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
I have read the CLA Document and I hereby sign the CLA |
| use path_slash::PathExt; | ||
| use relative_path::RelativePath; | ||
| use std::{borrow::Cow, collections::HashMap, path::Path, path::PathBuf}; |
There was a problem hiding this comment.
| use path_slash::PathExt; | |
| use relative_path::RelativePath; | |
| use std::{borrow::Cow, collections::HashMap, path::Path, path::PathBuf}; | |
| use std::{borrow::Cow, collections::HashMap, path::Path, path::PathBuf}; | |
| use path_slash::PathExt; | |
| use relative_path::RelativePath; |
In this project, we generally prefer to group std imports away from imports from other crates.
| @@ -78,7 +80,42 @@ pub fn snapshot_project_node( | |||
| // If the path specified in the project is relative, we assume it's | |||
| // relative to the folder that the project is in, project_folder. | |||
| let path = if path.is_relative() { | |||
| Cow::Owned(project_folder.join(path)) | |||
| // Convert paths to use forward slashes for compatibility with the relative-path crate | |||
| let project_folder_with_slash_separator = match project_folder.to_slash() { | |||
There was a problem hiding this comment.
If we go down the road of checking for symlinks, we'll likely need to expand the API surface of memofs. We can't use the stock fs APIs because a project could be built from an in-memory virtual filesystem, like with rojo plugin install.
|
I don't think Roblox employees are supposed to sign the Roblox CLA. Instead, you should be added to the whitelist. I should also probably be removed. 😃 |
|
Closing off as I have a separate solution, thanks for the help & advice! |
This helps to sidestep too-long path crashes on Windows when a project that is already at a deep path uses a relative $path reference. In the test case, the previous behaviour would be to attempt to access
/foo/bar/../../baz/other.txtwhereas this would now be resolved to/baz/other.txtbefore attempting any filesystem operations.