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

[7.1.0] Canonicalize the parent path in RemoteActionFileSystem#renameTo. #21285

Merged
merged 1 commit into from
Feb 12, 2024
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading