Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize relative paths from project $path entries #340

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ termcolor = "1.0.5"
thiserror = "1.0.11"
tokio = "0.1.22"
uuid = { version = "0.8.1", features = ["v4", "serde"] }
relative-path = "1.2.1"
path-slash = "0.1.3"

[target.'cfg(windows)'.dependencies]
winreg = "0.6.2"
Expand Down
81 changes: 79 additions & 2 deletions src/snapshot_middleware/project.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use std::{borrow::Cow, collections::HashMap, path::Path};
use path_slash::PathExt;
use relative_path::RelativePath;
use std::{borrow::Cow, collections::HashMap, path::Path, path::PathBuf};
Comment on lines +1 to +3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.


use memofs::Vfs;
use rbx_reflection::{get_class_descriptor, try_resolve_value};
Expand Down Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@amoss-roblox amoss-roblox Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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
  1. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Some(p) => p,
None => return Ok(None),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be obviated by rewrite to custom function

};
let path_with_slash_separator = match path.to_slash() {
Some(p) => p,
None => return Ok(None),
};

// Join and resolve relative paths
let normalized_path = RelativePath::new(&project_folder_with_slash_separator)
.join_normalized(&path_with_slash_separator);

let resolved_path = if PathBuf::from(normalized_path.as_str()).is_relative() {
// Resolve path relative to original root if project_folder was absolute
normalized_path.to_path(
project_folder
.components()
.next()
.expect("Could not get root of project folder"),
)
} else {
PathBuf::from(normalized_path.as_str())
};

log::trace!(
"original {}, project_folder {}, path {}, normalized {}, resolved {}",
project_folder.join(path).display(),
project_folder_with_slash_separator,
path_with_slash_separator,
normalized_path,
resolved_path.display()
);

Cow::Owned(resolved_path)
} else {
Cow::Borrowed(path)
};
Expand Down Expand Up @@ -512,6 +549,46 @@ mod test {
insta::assert_yaml_snapshot!(instance_snapshot);
}

#[test]
fn project_with_relative_path() {
let _ = env_logger::try_init();

let mut imfs = InMemoryFs::new();
imfs.load_snapshot(
"/baz",
VfsSnapshot::dir(hashmap! {
"other.txt" => VfsSnapshot::file("Hello, world!"),
}),
)
.unwrap();
imfs.load_snapshot(
"/foo/bar",
VfsSnapshot::dir(hashmap! {
"default.project.json" => VfsSnapshot::file(r#"
{
"name": "path-project",
"tree": {
"$path": "../../baz/other.txt"
}
}
"#),
}),
)
.unwrap();

let mut vfs = Vfs::new(imfs);

let instance_snapshot = snapshot_project(
&InstanceContext::default(),
&mut vfs,
Path::new("/foo/bar/default.project.json"),
)
.expect("snapshot error")
.expect("snapshot returned no instances");

insta::assert_yaml_snapshot!(instance_snapshot);
}

/// Ensures that if a property is defined both in the resulting instance
/// from $path and also in $properties, that the $properties value takes
/// precedence.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
---
source: src/snapshot_middleware/project.rs
expression: instance_snapshot
---
snapshot_id: ~
metadata:
ignore_unknown_instances: false
instigating_source:
Path: /foo/bar/default.project.json
relevant_paths:
- /baz/other.txt
- /baz/other.meta.json
- /foo/bar/default.project.json
context: {}
name: path-project
class_name: StringValue
properties:
Value:
Type: String
Value: "Hello, world!"
children: []