[7.1.0] Canonicalize the parent path in RemoteActionFileSystem#rename…

…To. (#21285)

FileSystem#renameTo is documented as not following symlinks, but that
refers to the last component only; we are still required to canonicalize
the parent path, possibly taking into account symlinks that straddle
underlying sources.

PiperOrigin-RevId: 605606357
Change-Id: I6508c07413ceccc9947e0a03cce93d769b31f2b9

src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java

@@ -734,20 +734,23 @@ private boolean isOutput(PathFragment path) {
   }
 
   @Override
-  public void renameTo(PathFragment sourcePath, PathFragment targetPath) throws IOException {
-    checkArgument(isOutput(sourcePath), "sourcePath must be an output path");
-    checkArgument(isOutput(targetPath), "targetPath must be an output path");
+  public void renameTo(PathFragment srcPath, PathFragment dstPath) throws IOException {
+    srcPath = resolveSymbolicLinksForParent(srcPath);
+    dstPath = resolveSymbolicLinksForParent(dstPath);
+
+    checkArgument(isOutput(srcPath), "srcPath must be an output path");
+    checkArgument(isOutput(dstPath), "dstPath must be an output path");
 
     FileNotFoundException remoteException = null;
     try {
-      remoteOutputTree.renameTo(sourcePath, targetPath);
+      remoteOutputTree.renameTo(srcPath, dstPath);
     } catch (FileNotFoundException e) {
       remoteException = e;
     }
 
     FileNotFoundException localException = null;
     try {
-      localFs.renameTo(sourcePath, targetPath);
+      localFs.renameTo(srcPath, dstPath);
     } catch (FileNotFoundException e) {
       localException = e;
     }

src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java

@@ -1167,6 +1167,65 @@ public void renameTo_onlyLocalFile_renameLocalFile() throws Exception {
     assertThat(getLocalFileSystem(actionFs).exists(newPath)).isTrue();
   }
 
+  @Test
+  public void renameTo_moveSymlink() throws Exception {
+    FileSystem actionFs = createActionFileSystem();
+    PathFragment oldLinkPath = getOutputPath("oldLink");
+    PathFragment newLinkPath = getOutputPath("newLink");
+    PathFragment targetPath = getOutputPath("target");
+    actionFs.getPath(oldLinkPath).createSymbolicLink(execRoot.getRelative(targetPath).asFragment());
+    writeLocalFile(actionFs, targetPath, "content");
+
+    actionFs.renameTo(oldLinkPath, newLinkPath);
+
+    assertThat(actionFs.getPath(oldLinkPath).exists(Symlinks.NOFOLLOW)).isFalse();
+    assertThat(actionFs.getPath(newLinkPath).exists(Symlinks.NOFOLLOW)).isTrue();
+    assertThat(actionFs.getPath(newLinkPath).readSymbolicLink()).isEqualTo(targetPath);
+    assertThat(actionFs.getPath(targetPath).exists()).isTrue();
+  }
+
+  @Test
+  public void renameTo_followSymlinks(
+      @TestParameter FilesystemTestParam from, @TestParameter FilesystemTestParam to)
+      throws Exception {
+    RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+    FileSystem fromFs = from.getFilesystem(actionFs);
+    FileSystem toFs = to.getFilesystem(actionFs);
+
+    PathFragment srcDirLinkPath = getOutputPath("srcDirLink");
+    PathFragment srcDirTargetPath = getOutputPath("srcDirTarget");
+    PathFragment naiveSrcPath = srcDirLinkPath.getChild("oldFile");
+    PathFragment canonicalSrcPath = srcDirTargetPath.getChild("oldFile");
+
+    PathFragment dstDirLinkPath = getOutputPath("dstDirLink");
+    PathFragment dstDirTargetPath = getOutputPath("dstDirTarget");
+    PathFragment naiveDstPath = dstDirLinkPath.getChild("newFile");
+    PathFragment canonicalDstPath = dstDirTargetPath.getChild("newFile");
+
+    actionFs.getPath(srcDirTargetPath).createDirectoryAndParents();
+    actionFs.getPath(dstDirTargetPath).createDirectoryAndParents();
+
+    fromFs
+        .getPath(srcDirLinkPath)
+        .createSymbolicLink(execRoot.getRelative(srcDirTargetPath).asFragment());
+    fromFs
+        .getPath(dstDirLinkPath)
+        .createSymbolicLink(execRoot.getRelative(dstDirTargetPath).asFragment());
+
+    if (toFs.equals(actionFs.getLocalFileSystem())) {
+      writeLocalFile(actionFs, canonicalSrcPath, "content");
+    } else {
+      injectRemoteFile(actionFs, canonicalSrcPath, "content");
+    }
+
+    actionFs.renameTo(naiveSrcPath, naiveDstPath);
+
+    assertThat(actionFs.exists(naiveSrcPath)).isFalse();
+    assertThat(actionFs.exists(canonicalSrcPath)).isFalse();
+    assertThat(actionFs.exists(naiveDstPath)).isTrue();
+    assertThat(actionFs.exists(canonicalDstPath)).isTrue();
+  }
+
   @Override
   @CanIgnoreReturnValue
   protected FileArtifactValue injectRemoteFile(

src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java

@@ -161,14 +161,24 @@ public void delete_localAndRemoteFile_succeeds() throws Exception {
   }
 
   @Test
-  public void renameTo_fileDoesNotExist_throwError() throws Exception {
+  public void renameTo_sourceFileDoesNotExist_throwError() throws Exception {
     FileSystem actionFs = createActionFileSystem();
     PathFragment path = getOutputPath("file");
     PathFragment newPath = getOutputPath("file-new");
 
     assertThrows(FileNotFoundException.class, () -> actionFs.renameTo(path, newPath));
   }
 
+  @Test
+  public void renameTo_targetDirectoryDoesNotExist_throwError() throws Exception {
+    FileSystem actionFs = createActionFileSystem();
+    PathFragment path = getOutputPath("file");
+    PathFragment newPath = getOutputPath("dir/file-new");
+    writeLocalFile(actionFs, path, "local-content");
+
+    assertThrows(FileNotFoundException.class, () -> actionFs.renameTo(path, newPath));
+  }
+
   @Test
   public void renameTo_onlyRemoteFile_renameRemoteFile() throws Exception {
     FileSystem actionFs = createActionFileSystem();