Ignore broken symlinks for LocalFileSystem object store #2195
Conversation
|
Thanks @jccampagne ❤️ |
There was a problem hiding this comment.
Thanks @jccampagne -- I found the code easy to follow and it made sense to me
I think some might say it is not "idomatic" Rust in that it doesn't use the more functional map / ok type style. I spent some time rewriting to use that style in order to provide some more useful feedback. What I came up with is below but actually think it is harder to understand what is happening than your version 🤷
Thanks again
/// Convert walkdir results and converts not-found errors into `None`.
/// Convert broken symlinks to `None`.
fn convert_walkdir_result(
res: std::result::Result<walkdir::DirEntry, walkdir::Error>,
) -> Result<Option<walkdir::DirEntry>> {
match res {
Ok(entry) => Ok(map_broken_symlink_to_none(entry)),
Err(walkdir_err) => match walkdir_err.io_error() {
Some(io_err) => match io_err.kind() {
io::ErrorKind::NotFound => Ok(None),
_ => Err(Error::UnableToWalkDir {
source: walkdir_err,
}
.into()),
},
None => Err(Error::UnableToWalkDir {
source: walkdir_err,
}
.into()),
},
}
}
fn map_broken_symlink_to_none(entry: walkdir::DirEntry) -> Option<walkdir::DirEntry> {
// To check for broken symlink: call symlink_metadata() - it does not traverse symlinks);
// if ok: check if entry is symlink; and try to read it by calling metadata().
symlink_metadata(entry.path())
.map(|attr| {
if attr.is_symlink() {
metadata(entry.path())
// keep entry if metadata returned successfully
.map(|_| entry)
// discard error
.ok()
}
else {
Some(entry)
}
})
// discard the error from symlink etadata
.ok()
.flatten()
}cc @tustvold
| @@ -990,8 +1015,6 @@ mod tests { | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| #[cfg(target_os = "linux")] | |||
| // macos has some magic in its root '/.VolumeIcon.icns"' which causes this test to fail | |||
There was a problem hiding this comment.
This was actually a red-herring, mac os doesn't always have this file. In particular in CI. I think it would be great if we could have an explicit test of a broken link so that we have some test coverage of this feature
There was a problem hiding this comment.
I don't understand this comment -- the test fails with with this error on (macos) CI. I disabled the check on mac so the CI would pass....
There was a problem hiding this comment.
Fantastic, I had understood that this couldn't be relied on, that's good that we can see the issue and get coverage of it in CI.
However, I still think an explicit symlink test would be worthwhile, especially as I would not be surprised if Windows does something exciting, etc...
| // symlink is valid | ||
| Ok(Some(entry)) | ||
| } | ||
| Err(_) => { |
There was a problem hiding this comment.
It might be nice to check the ErrorKind is NotFound
| Ok(entry) => { | ||
| // To check for broken symlink: call symlink_metadata() - it does not traverse symlinks); | ||
| // if ok: check if entry is symlink; and try to read it by calling metadata(). | ||
| match symlink_metadata(entry.path()) { |
There was a problem hiding this comment.
I wonder if we should invert the order here, to optimise for the common case of non-broken symlinks.
i.e. call std::fs::metadata if this fails with ErrorKind::NotFound, then call symlink_metadata to check if it is a broken symlink we should just ignore?
I think using structured matches as this PR does is idiomatic Rust and I'm happy with it, nice work 👍. FWIW I don't think code needs to be written in a functional style to be considered idiomatic, it is frequently harder to understand, and very often the borrow checker prevents it anyway. |
|
Benchmark runs are scheduled for baseline = cc96687 and contender = b8e034c. b8e034c is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
Thanks for the review and comments. |
|
@jccampagne so I started writing some tests for this behaviour, and actually symlinks behave in very unexpected ways in general - as they get canonicalised as part of path resolution. I'm currently coding something up which will simply ignore symlinks, as I don't think we can support them in a meaningful way without significant engineering effort that I think would be hard to justify |
|
Filed #2206 to track writing tests |
|
Created #2207, please let me know what you both think 😄 |
Which issue does this PR close?
Closes #2174.
Rationale for this change
convert_walkdir_resultnow ignores broken symlinks.Resolution follows suggestions in:
What changes are included in this PR?
convert_walkdir_resultnow tests if aDirEntryis a symlink and if it is broken or not, function ignores broken symlinks (returnOk(None).Are there any user-facing changes?
No.