RFR: 8294193: Files.createDirectories throws FileAlreadyExistsException for a symbolic link whose target is an existing directory [v2]

[Alan Bateman]

On Thu, 22 Sep 2022 09:20:26 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this change which proposes to fix the issue noted in https://bugs.openjdk.org/browse/JDK-8294193?
>> 
>> As discussed in a related PR https://github.com/openjdk/jdk/pull/10266#issuecomment-1246800456, this is a genuine bug in the `java.nio.file.Files.createDirectories` implementation. The commit in this PR fixes that by making sure that links are followed when checking if a `Path` is a directory during the creation of directories in the `createDirectories` implementation. The `private` method which is changed in this PR is only used in the `Files.createDirectories` and no other place, so this change should not impact any other code. Additionally a new test has been added to reproduce the issue and verify the fix.
>> tier1, tier2 and tier3 tests passed with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   move Files.createDirectories testing from Misc.java to CreateDirectories.java

test/jdk/java/nio/file/Files/CreateDirectories.java line 44:

> 42:     /**
> 43:      * Creates a symlink which points to a directory that exists. Then calls Files.createDirectories
> 44:      * method passing it the path to the symlink and verifies that no exception gets thrown

I think you can simplify the wording with something like "Test Files.createDirectories symbolic file with an existing directory."

test/jdk/java/nio/file/Files/CreateDirectories.java line 49:

> 47:     public void testSymlinkDir() throws Exception {
> 48:         // create a temp dir as the "root" in which we will run our tests.
> 49:         final Path startingDir = Files.createTempDirectory(Path.of("."), "8293792");

The tests in this area use TestUtil.createTemporaryDirectory() so it's probably best to keep it consistent if you can. 

Also can you rename "startingDir" to "top" as that will make the test much easier to read.

test/jdk/java/nio/file/Files/CreateDirectories.java line 56:

> 54:         }
> 55:         System.out.println("Running tests under directory " + startingDir.toAbsolutePath());
> 56:         final Path fooDir = Files.createDirectory(Path.of(startingDir.toString(), "foo"));

You can simplify this to `Files.createDirectory(top.resolve("foo"))`, same for barDir, and symlink.

test/jdk/java/nio/file/Files/CreateDirectories.java line 100:

> 98:         Path subdir = tmpdir.resolve("a");
> 99:         Files.createDirectories(subdir);
> 100:         assertTrue(Files.exists(subdir), subdir + " was expected to exist, but didn't");

I realise this is moved from Misc.java but if you replace the "exists" with "isDirectory" then it improve the testing.

-------------

PR: https://git.openjdk.org/jdk/pull/10383