fix: Deno.remove() to properly remove dangling symlinks (#3860)

For some reason, the unit tests for Deno.remove() were not being imported to unit_tests.ts and, consequently, not being executed. Thus, I imported them, refactored some existent ones and wrote new ones for the symlink removal case. Since the creation of a symlink is not implemented for Windows yet, assertions that consider this state were added when the tests are executed in this OS.
2025-03-03 09:31:22 -05:00 · 2020-02-03 10:20:15 -03:00 · 2020-02-03 10:20:15 -03:00 · 55063dd8e8
commit 55063dd8e8
parent f168597b7a
6 changed files with 277 additions and 126 deletions
--- a/cli/js/remove.ts
+++ b/cli/js/remove.ts
@ -6,7 +6,7 @@ export interface RemoveOption {
  recursive?: boolean;
 }

-/** Removes the named file or directory synchronously. Would throw
+/** Removes the named file, directory or symlink synchronously. Would throw
 * error if permission denied, not found, or directory not empty if `recursive`
 * set to false.
 * `recursive` is set to false by default.
@ -17,7 +17,7 @@ export function removeSync(path: string, options: RemoveOption = {}): void {
  sendSync(dispatch.OP_REMOVE, { path, recursive: !!options.recursive });
 }

-/** Removes the named file or directory. Would throw error if
+/** Removes the named file, directory or symlink. Would throw error if
 * permission denied, not found, or directory not empty if `recursive` set
 * to false.
 * `recursive` is set to false by default.
--- a/cli/js/remove_test.ts
+++ b/cli/js/remove_test.ts
@ -3,9 +3,9 @@ import { testPerm, assert, assertEquals } from "./test_util.ts";

 // SYNC

-testPerm({ write: true }, function removeSyncDirSuccess(): void {
+testPerm({ write: true, read: true }, function removeSyncDirSuccess(): void {
  // REMOVE EMPTY DIRECTORY
-  const path = Deno.makeTempDirSync() + "/dir/subdir";
+  const path = Deno.makeTempDirSync() + "/subdir";
  Deno.mkdirSync(path);
  const pathInfo = Deno.statSync(path);
  assert(pathInfo.isDirectory()); // check exist first
@ -22,7 +22,7 @@ testPerm({ write: true }, function removeSyncDirSuccess(): void {
  assertEquals(err.name, "NotFound");
 });

-testPerm({ write: true }, function removeSyncFileSuccess(): void {
+testPerm({ write: true, read: true }, function removeSyncFileSuccess(): void {
  // REMOVE FILE
  const enc = new TextEncoder();
  const data = enc.encode("Hello");
@ -43,11 +43,11 @@ testPerm({ write: true }, function removeSyncFileSuccess(): void {
  assertEquals(err.name, "NotFound");
 });

-testPerm({ write: true }, function removeSyncFail(): void {
+testPerm({ write: true, read: true }, function removeSyncFail(): void {
  // NON-EMPTY DIRECTORY
  const path = Deno.makeTempDirSync() + "/dir/subdir";
  const subPath = path + "/subsubdir";
-  Deno.mkdirSync(path);
+  Deno.mkdirSync(path, { recursive: true });
  Deno.mkdirSync(subPath);
  const pathInfo = Deno.statSync(path);
  assert(pathInfo.isDirectory()); // check exist first
@ -74,6 +74,72 @@ testPerm({ write: true }, function removeSyncFail(): void {
  assertEquals(err.name, "NotFound");
 });

+testPerm(
+  { write: true, read: true },
+  function removeSyncDanglingSymlinkSuccess(): void {
+    const danglingSymlinkPath = Deno.makeTempDirSync() + "/dangling_symlink";
+    // TODO(#3832): Remove "Not Implemented" error checking when symlink creation is implemented for Windows
+    let errOnWindows;
+    try {
+      Deno.symlinkSync("unexistent_file", danglingSymlinkPath);
+    } catch (err) {
+      errOnWindows = err;
+    }
+    if (Deno.build.os === "win") {
+      assertEquals(errOnWindows.kind, Deno.ErrorKind.Other);
+      assertEquals(errOnWindows.message, "Not implemented");
+    } else {
+      const pathInfo = Deno.lstatSync(danglingSymlinkPath);
+      assert(pathInfo.isSymlink());
+      Deno.removeSync(danglingSymlinkPath);
+      let err;
+      try {
+        Deno.lstatSync(danglingSymlinkPath);
+      } catch (e) {
+        err = e;
+      }
+      assertEquals(err.kind, Deno.ErrorKind.NotFound);
+      assertEquals(err.name, "NotFound");
+    }
+  }
+);
+
+testPerm(
+  { write: true, read: true },
+  function removeSyncValidSymlinkSuccess(): void {
+    const encoder = new TextEncoder();
+    const data = encoder.encode("Test");
+    const tempDir = Deno.makeTempDirSync();
+    const filePath = tempDir + "/test.txt";
+    const validSymlinkPath = tempDir + "/valid_symlink";
+    Deno.writeFileSync(filePath, data, { perm: 0o666 });
+    // TODO(#3832): Remove "Not Implemented" error checking when symlink creation is implemented for Windows
+    let errOnWindows;
+    try {
+      Deno.symlinkSync(filePath, validSymlinkPath);
+    } catch (err) {
+      errOnWindows = err;
+    }
+    if (Deno.build.os === "win") {
+      assertEquals(errOnWindows.kind, Deno.ErrorKind.Other);
+      assertEquals(errOnWindows.message, "Not implemented");
+    } else {
+      const symlinkPathInfo = Deno.statSync(validSymlinkPath);
+      assert(symlinkPathInfo.isFile());
+      Deno.removeSync(validSymlinkPath);
+      let err;
+      try {
+        Deno.statSync(validSymlinkPath);
+      } catch (e) {
+        err = e;
+      }
+      Deno.removeSync(filePath);
+      assertEquals(err.kind, Deno.ErrorKind.NotFound);
+      assertEquals(err.name, "NotFound");
+    }
+  }
+);
+
 testPerm({ write: false }, function removeSyncPerm(): void {
  let err;
  try {
@ -85,10 +151,10 @@ testPerm({ write: false }, function removeSyncPerm(): void {
  assertEquals(err.name, "PermissionDenied");
 });

-testPerm({ write: true }, function removeAllSyncDirSuccess(): void {
+testPerm({ write: true, read: true }, function removeAllSyncDirSuccess(): void {
  // REMOVE EMPTY DIRECTORY
  let path = Deno.makeTempDirSync() + "/dir/subdir";
-  Deno.mkdirSync(path);
+  Deno.mkdirSync(path, { recursive: true });
  let pathInfo = Deno.statSync(path);
  assert(pathInfo.isDirectory()); // check exist first
  Deno.removeSync(path, { recursive: true }); // remove
@ -105,7 +171,7 @@ testPerm({ write: true }, function removeAllSyncDirSuccess(): void {
  // REMOVE NON-EMPTY DIRECTORY
  path = Deno.makeTempDirSync() + "/dir/subdir";
  const subPath = path + "/subsubdir";
-  Deno.mkdirSync(path);
+  Deno.mkdirSync(path, { recursive: true });
  Deno.mkdirSync(subPath);
  pathInfo = Deno.statSync(path);
  assert(pathInfo.isDirectory()); // check exist first
@ -123,26 +189,29 @@ testPerm({ write: true }, function removeAllSyncDirSuccess(): void {
  assertEquals(err.name, "NotFound");
 });

-testPerm({ write: true }, function removeAllSyncFileSuccess(): void {
-  // REMOVE FILE
-  const enc = new TextEncoder();
-  const data = enc.encode("Hello");
-  const filename = Deno.makeTempDirSync() + "/test.txt";
-  Deno.writeFileSync(filename, data, { perm: 0o666 });
-  const fileInfo = Deno.statSync(filename);
-  assert(fileInfo.isFile()); // check exist first
-  Deno.removeSync(filename, { recursive: true }); // remove
-  // We then check again after remove
-  let err;
-  try {
-    Deno.statSync(filename);
-  } catch (e) {
-    err = e;
+testPerm(
+  { write: true, read: true },
+  function removeAllSyncFileSuccess(): void {
+    // REMOVE FILE
+    const enc = new TextEncoder();
+    const data = enc.encode("Hello");
+    const filename = Deno.makeTempDirSync() + "/test.txt";
+    Deno.writeFileSync(filename, data, { perm: 0o666 });
+    const fileInfo = Deno.statSync(filename);
+    assert(fileInfo.isFile()); // check exist first
+    Deno.removeSync(filename, { recursive: true }); // remove
+    // We then check again after remove
+    let err;
+    try {
+      Deno.statSync(filename);
+    } catch (e) {
+      err = e;
+    }
+    // File is gone
+    assertEquals(err.kind, Deno.ErrorKind.NotFound);
+    assertEquals(err.name, "NotFound");
  }
-  // File is gone
-  assertEquals(err.kind, Deno.ErrorKind.NotFound);
-  assertEquals(err.name, "NotFound");
-});
+);

 testPerm({ write: true }, function removeAllSyncFail(): void {
  // NON-EXISTENT DIRECTORY/FILE
@ -170,51 +239,59 @@ testPerm({ write: false }, function removeAllSyncPerm(): void {

 // ASYNC

-testPerm({ write: true }, async function removeDirSuccess(): Promise<void> {
-  // REMOVE EMPTY DIRECTORY
-  const path = Deno.makeTempDirSync() + "/dir/subdir";
-  Deno.mkdirSync(path);
-  const pathInfo = Deno.statSync(path);
-  assert(pathInfo.isDirectory()); // check exist first
-  await Deno.remove(path); // remove
-  // We then check again after remove
-  let err;
-  try {
-    Deno.statSync(path);
-  } catch (e) {
-    err = e;
+testPerm(
+  { write: true, read: true },
+  async function removeDirSuccess(): Promise<void> {
+    // REMOVE EMPTY DIRECTORY
+    const path = Deno.makeTempDirSync() + "/dir/subdir";
+    Deno.mkdirSync(path, { recursive: true });
+    const pathInfo = Deno.statSync(path);
+    assert(pathInfo.isDirectory()); // check exist first
+    await Deno.remove(path); // remove
+    // We then check again after remove
+    let err;
+    try {
+      Deno.statSync(path);
+    } catch (e) {
+      err = e;
+    }
+    // Directory is gone
+    assertEquals(err.kind, Deno.ErrorKind.NotFound);
+    assertEquals(err.name, "NotFound");
  }
-  // Directory is gone
-  assertEquals(err.kind, Deno.ErrorKind.NotFound);
-  assertEquals(err.name, "NotFound");
-});
+);

-testPerm({ write: true }, async function removeFileSuccess(): Promise<void> {
-  // REMOVE FILE
-  const enc = new TextEncoder();
-  const data = enc.encode("Hello");
-  const filename = Deno.makeTempDirSync() + "/test.txt";
-  Deno.writeFileSync(filename, data, { perm: 0o666 });
-  const fileInfo = Deno.statSync(filename);
-  assert(fileInfo.isFile()); // check exist first
-  await Deno.remove(filename); // remove
-  // We then check again after remove
-  let err;
-  try {
-    Deno.statSync(filename);
-  } catch (e) {
-    err = e;
+testPerm(
+  { write: true, read: true },
+  async function removeFileSuccess(): Promise<void> {
+    // REMOVE FILE
+    const enc = new TextEncoder();
+    const data = enc.encode("Hello");
+    const filename = Deno.makeTempDirSync() + "/test.txt";
+    Deno.writeFileSync(filename, data, { perm: 0o666 });
+    const fileInfo = Deno.statSync(filename);
+    assert(fileInfo.isFile()); // check exist first
+    await Deno.remove(filename); // remove
+    // We then check again after remove
+    let err;
+    try {
+      Deno.statSync(filename);
+    } catch (e) {
+      err = e;
+    }
+    // File is gone
+    assertEquals(err.kind, Deno.ErrorKind.NotFound);
+    assertEquals(err.name, "NotFound");
  }
-  // File is gone
-  assertEquals(err.kind, Deno.ErrorKind.NotFound);
-  assertEquals(err.name, "NotFound");
-});
+);

-testPerm({ write: true }, async function removeFail(): Promise<void> {
+testPerm({ write: true, read: true }, async function removeFail(): Promise<
+  void
+> {
  // NON-EMPTY DIRECTORY
  const path = Deno.makeTempDirSync() + "/dir/subdir";
  const subPath = path + "/subsubdir";
-  Deno.mkdirSync(path);
+  Deno.mkdirSync(path, { recursive: true });
  Deno.mkdirSync(subPath);
  const pathInfo = Deno.statSync(path);
  assert(pathInfo.isDirectory()); // check exist first
@ -240,6 +317,72 @@ testPerm({ write: true }, async function removeFail(): Promise<void> {
  assertEquals(err.name, "NotFound");
 });

+testPerm(
+  { write: true, read: true },
+  async function removeDanglingSymlinkSuccess(): Promise<void> {
+    const danglingSymlinkPath = Deno.makeTempDirSync() + "/dangling_symlink";
+    // TODO(#3832): Remove "Not Implemented" error checking when symlink creation is implemented for Windows
+    let errOnWindows;
+    try {
+      Deno.symlinkSync("unexistent_file", danglingSymlinkPath);
+    } catch (e) {
+      errOnWindows = e;
+    }
+    if (Deno.build.os === "win") {
+      assertEquals(errOnWindows.kind, Deno.ErrorKind.Other);
+      assertEquals(errOnWindows.message, "Not implemented");
+    } else {
+      const pathInfo = Deno.lstatSync(danglingSymlinkPath);
+      assert(pathInfo.isSymlink());
+      await Deno.remove(danglingSymlinkPath);
+      let err;
+      try {
+        Deno.lstatSync(danglingSymlinkPath);
+      } catch (e) {
+        err = e;
+      }
+      assertEquals(err.kind, Deno.ErrorKind.NotFound);
+      assertEquals(err.name, "NotFound");
+    }
+  }
+);
+
+testPerm(
+  { write: true, read: true },
+  async function removeValidSymlinkSuccess(): Promise<void> {
+    const encoder = new TextEncoder();
+    const data = encoder.encode("Test");
+    const tempDir = Deno.makeTempDirSync();
+    const filePath = tempDir + "/test.txt";
+    const validSymlinkPath = tempDir + "/valid_symlink";
+    Deno.writeFileSync(filePath, data, { perm: 0o666 });
+    // TODO(#3832): Remove "Not Implemented" error checking when symlink creation is implemented for Windows
+    let errOnWindows;
+    try {
+      Deno.symlinkSync(filePath, validSymlinkPath);
+    } catch (e) {
+      errOnWindows = e;
+    }
+    if (Deno.build.os === "win") {
+      assertEquals(errOnWindows.kind, Deno.ErrorKind.Other);
+      assertEquals(errOnWindows.message, "Not implemented");
+    } else {
+      const symlinkPathInfo = Deno.statSync(validSymlinkPath);
+      assert(symlinkPathInfo.isFile());
+      await Deno.remove(validSymlinkPath);
+      let err;
+      try {
+        Deno.statSync(validSymlinkPath);
+      } catch (e) {
+        err = e;
+      }
+      Deno.removeSync(filePath);
+      assertEquals(err.kind, Deno.ErrorKind.NotFound);
+      assertEquals(err.name, "NotFound");
+    }
+  }
+);
+
 testPerm({ write: false }, async function removePerm(): Promise<void> {
  let err;
  try {
@ -251,64 +394,70 @@ testPerm({ write: false }, async function removePerm(): Promise<void> {
  assertEquals(err.name, "PermissionDenied");
 });

-testPerm({ write: true }, async function removeAllDirSuccess(): Promise<void> {
-  // REMOVE EMPTY DIRECTORY
-  let path = Deno.makeTempDirSync() + "/dir/subdir";
-  Deno.mkdirSync(path);
-  let pathInfo = Deno.statSync(path);
-  assert(pathInfo.isDirectory()); // check exist first
-  await Deno.remove(path, { recursive: true }); // remove
-  // We then check again after remove
-  let err;
-  try {
-    Deno.statSync(path);
-  } catch (e) {
-    err = e;
+testPerm(
+  { write: true, read: true },
+  async function removeAllDirSuccess(): Promise<void> {
+    // REMOVE EMPTY DIRECTORY
+    let path = Deno.makeTempDirSync() + "/dir/subdir";
+    Deno.mkdirSync(path, { recursive: true });
+    let pathInfo = Deno.statSync(path);
+    assert(pathInfo.isDirectory()); // check exist first
+    await Deno.remove(path, { recursive: true }); // remove
+    // We then check again after remove
+    let err;
+    try {
+      Deno.statSync(path);
+    } catch (e) {
+      err = e;
+    }
+    // Directory is gone
+    assertEquals(err.kind, Deno.ErrorKind.NotFound);
+    assertEquals(err.name, "NotFound");
+    // REMOVE NON-EMPTY DIRECTORY
+    path = Deno.makeTempDirSync() + "/dir/subdir";
+    const subPath = path + "/subsubdir";
+    Deno.mkdirSync(path, { recursive: true });
+    Deno.mkdirSync(subPath);
+    pathInfo = Deno.statSync(path);
+    assert(pathInfo.isDirectory()); // check exist first
+    const subPathInfo = Deno.statSync(subPath);
+    assert(subPathInfo.isDirectory()); // check exist first
+    await Deno.remove(path, { recursive: true }); // remove
+    // We then check parent directory again after remove
+    try {
+      Deno.statSync(path);
+    } catch (e) {
+      err = e;
+    }
+    // Directory is gone
+    assertEquals(err.kind, Deno.ErrorKind.NotFound);
+    assertEquals(err.name, "NotFound");
  }
-  // Directory is gone
-  assertEquals(err.kind, Deno.ErrorKind.NotFound);
-  assertEquals(err.name, "NotFound");
-  // REMOVE NON-EMPTY DIRECTORY
-  path = Deno.makeTempDirSync() + "/dir/subdir";
-  const subPath = path + "/subsubdir";
-  Deno.mkdirSync(path);
-  Deno.mkdirSync(subPath);
-  pathInfo = Deno.statSync(path);
-  assert(pathInfo.isDirectory()); // check exist first
-  const subPathInfo = Deno.statSync(subPath);
-  assert(subPathInfo.isDirectory()); // check exist first
-  await Deno.remove(path, { recursive: true }); // remove
-  // We then check parent directory again after remove
-  try {
-    Deno.statSync(path);
-  } catch (e) {
-    err = e;
-  }
-  // Directory is gone
-  assertEquals(err.kind, Deno.ErrorKind.NotFound);
-  assertEquals(err.name, "NotFound");
-});
+);

-testPerm({ write: true }, async function removeAllFileSuccess(): Promise<void> {
-  // REMOVE FILE
-  const enc = new TextEncoder();
-  const data = enc.encode("Hello");
-  const filename = Deno.makeTempDirSync() + "/test.txt";
-  Deno.writeFileSync(filename, data, { perm: 0o666 });
-  const fileInfo = Deno.statSync(filename);
-  assert(fileInfo.isFile()); // check exist first
-  await Deno.remove(filename, { recursive: true }); // remove
-  // We then check again after remove
-  let err;
-  try {
-    Deno.statSync(filename);
-  } catch (e) {
-    err = e;
+testPerm(
+  { write: true, read: true },
+  async function removeAllFileSuccess(): Promise<void> {
+    // REMOVE FILE
+    const enc = new TextEncoder();
+    const data = enc.encode("Hello");
+    const filename = Deno.makeTempDirSync() + "/test.txt";
+    Deno.writeFileSync(filename, data, { perm: 0o666 });
+    const fileInfo = Deno.statSync(filename);
+    assert(fileInfo.isFile()); // check exist first
+    await Deno.remove(filename, { recursive: true }); // remove
+    // We then check again after remove
+    let err;
+    try {
+      Deno.statSync(filename);
+    } catch (e) {
+      err = e;
+    }
+    // File is gone
+    assertEquals(err.kind, Deno.ErrorKind.NotFound);
+    assertEquals(err.name, "NotFound");
  }
-  // File is gone
-  assertEquals(err.kind, Deno.ErrorKind.NotFound);
-  assertEquals(err.name, "NotFound");
-});
+);

 testPerm({ write: true }, async function removeAllFail(): Promise<void> {
  // NON-EXISTENT DIRECTORY/FILE
--- a/cli/js/unit_tests.ts
+++ b/cli/js/unit_tests.ts
@ -39,6 +39,7 @@ import "./realpath_test.ts";
 import "./read_dir_test.ts";
 import "./read_file_test.ts";
 import "./read_link_test.ts";
+import "./remove_test.ts";
 import "./rename_test.ts";
 import "./request_test.ts";
 import "./resources_test.ts";
--- a/cli/ops/fs.rs
+++ b/cli/ops/fs.rs
@ -168,8 +168,9 @@ fn op_remove(
  let is_sync = args.promise_id.is_none();
  blocking_json(is_sync, move || {
    debug!("op_remove {}", path.display());
-    let metadata = fs::metadata(&path)?;
-    if metadata.is_file() {
+    let metadata = fs::symlink_metadata(&path)?;
+    let file_type = metadata.file_type();
+    if file_type.is_file() || file_type.is_symlink() {
      fs::remove_file(&path)?;
    } else if recursive {
      remove_dir_all(&path)?;
--- a/std/encoding/testdata/cargo.toml
+++ b/std/encoding/testdata/cargo.toml
@ -38,7 +38,7 @@ libc = "0.2.49"
 log = "0.4.6"
 rand = "0.6.5"
 regex = "1.1.0"
-remove_dir_all = "0.5.1"
+remove_dir_all = "0.5.2"
 ring = "0.14.6"
 rustyline = "3.0.0"
 serde_json = "1.0.38"
--- a/std/encoding/toml_test.ts
+++ b/std/encoding/toml_test.ts
@ -276,7 +276,7 @@ test({
        log: "0.4.6",
        rand: "0.6.5",
        regex: "1.1.0",
-        remove_dir_all: "0.5.1",
+        remove_dir_all: "0.5.2",
        ring: "0.14.6",
        rustyline: "3.0.0",
        serde_json: "1.0.38",