-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
walkFiles consistently relative or absolute #3773
walkFiles consistently relative or absolute #3773
Conversation
The expected behavior from Files.walkFileTree is that if the starting path is absolute, then all the visited paths are absolute. If the starting path is relative, then all the visited paths are relative. This PR adds a test for this behavior, and shows that our current code fails for an absolute path: it returns a mix of relative and absolute paths. This PR also adds a code change to fix the problem, so the actual behavior matches what is expected. This should fix issue googleapis#3772
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hzyi-google Do you also want to take a look?
...contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java
Outdated
Show resolved
Hide resolved
...contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java
Show resolved
Hide resolved
Hi. I would think that these modifications, along with the ones in #3775, would allow the following unit test to pass: @Test
public void testWalkFileTree() throws IOException {
Path baseDir = Paths.get(URI.create("gs://bucket/liverpool"));
//Base directory with an ending forwards slash is not passing either
//Path baseDir = Paths.get(URI.create("gs://bucket/liverpool/"));
Path file1 = baseDir.resolve("firmino.player");
Path file2 = baseDir.resolve("salah.player");
Files.createFile(file1);
Files.createFile(file2);
Set<Path> expectedDirs = Sets.newHashSet(baseDir);
Set<Path> expectedFiles = Sets.newHashSet(file1,file2);
final Set<Path> foundDirs = Sets.newHashSet();
final Set<Path> foundFiles = Sets.newHashSet();
FileVisitor<Path> testVisitor = new SimpleFileVisitor<Path>() {
@Override
public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) {
assertThat(foundDirs).doesNotContain(dir);
foundDirs.add(dir);
return FileVisitResult.CONTINUE;
}
@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
foundFiles.add(file);
return FileVisitResult.CONTINUE;
}
@Override
public FileVisitResult postVisitDirectory(Path dir, IOException exc) {
assertThat(foundDirs).contains(dir);
return FileVisitResult.CONTINUE;
}
};
Files.walkFileTree(baseDir,Sets.newHashSet(FileVisitOption.FOLLOW_LINKS),Integer.MAX_VALUE,testVisitor);
assertThat(foundFiles).containsExactlyElementsIn(expectedFiles);
assertThat(foundDirs).containsExactlyElementsIn(expectedDirs);
} Files.walkFileTree visits the Basedir twice, first without an ending slash, and then with an ending slash. Best regards, |
@olifly I see there is a bug in this code snippet: Once I fixed the code snippet, it passed with just the code in this PR. |
Hi. Maybe it's the combination of these two pull requests, but changing the test to insert the expected Paths one at a time into the Sets is not making it pass for me. Files.walkFileTree is still calling preVisitDirectory twice for the base dir, once without a forward slash and once with it. Here's my modified code @Test
public void testWalkFileTree() throws IOException {
Path baseDir = Paths.get(URI.create("gs://bucket/liverpool"));
Path file1 = baseDir.resolve("firmino.player");
Path file2 = baseDir.resolve("salah.player");
Files.createFile(file1);
Files.createFile(file2);
Set<Path> expectedDirs = Sets.newHashSet();
expectedDirs.add(baseDir);
Set<Path> expectedFiles = Sets.newHashSet();
expectedFiles.add(file1);
expectedFiles.add(file2);
final Set<Path> foundDirs = Sets.newHashSet();
final Set<Path> foundFiles = Sets.newHashSet();
FileVisitor<Path> testVisitor = new SimpleFileVisitor<Path>() {
@Override
public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) {
assertThat(foundDirs).doesNotContain(dir);
foundDirs.add(dir);
return FileVisitResult.CONTINUE;
}
@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
foundFiles.add(file);
return FileVisitResult.CONTINUE;
}
@Override
public FileVisitResult postVisitDirectory(Path dir, IOException exc) {
assertThat(foundDirs).contains(dir);
return FileVisitResult.CONTINUE;
}
};
Files.walkFileTree(baseDir,Sets.newHashSet(FileVisitOption.FOLLOW_LINKS),Integer.MAX_VALUE,testVisitor);
assertThat(foundFiles).containsExactlyElementsIn(expectedFiles);
assertThat(foundDirs).containsExactlyElementsIn(expectedDirs);
} The test passes if the baseDir has a forward slash, gs://bucket/liverpool/, but fails on: gs://bucket/liverpool I'm pretty sure Liverpool is not to blame though :) JDK Used: 1.8, u 181. Best Regards, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks straight-forward - just a question about the clarity of the test.
...contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java
Outdated
Show resolved
Hide resolved
...contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java
Show resolved
Hide resolved
@chingor13 the functions returns the folders and files traversed, so the 5 paths are: I added them as comment to the code in case another reader wonders too. (sorry, for some reason GitHub isn't letting me reply to your comment directly). |
@jean-philippe-martin - The changes you did to pull request #3775 did indeed solve the issue. Thanks a bunch. Can't wait for these changes to be merged upstream :) Best Regards |
@olifly thanks for checking, that's great! I'm glad this works for you now. I see the reviewers have already merged in this PR so we're halfway there! |
Thank you @chingor13 ! |
The expected behavior from Files.walkFileTree is that if the starting path is absolute, then all the visited paths are absolute. If the starting path is relative, then all the visited paths are relative.
This PR adds a test for this behavior, and shows that our current code
fails for an absolute path: it returns a mix of relative and absolute
paths.
This PR also adds a code change to fix the problem, so the actual
behavior matches what is expected.
This should fix issue #3772