From d8a2bbd541cf8ce77f69e32d50eae2b89e69307a Mon Sep 17 00:00:00 2001 From: Johannes Eriksson Date: Tue, 25 Jan 2022 01:10:28 +0200 Subject: [PATCH 1/8] fix: reliable deletion of node_modules Set writable permission before deletion to handle symlinks properly. --- .../flow/plugin/maven/CleanFrontendMojo.java | 17 +++++---- .../flow/server/frontend/FrontendUtils.java | 36 +++++++++++++++++++ .../server/frontend/TaskRunNpmInstall.java | 2 +- 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/flow-plugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/CleanFrontendMojo.java b/flow-plugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/CleanFrontendMojo.java index 91501f70f8e..99f999004be 100644 --- a/flow-plugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/CleanFrontendMojo.java +++ b/flow-plugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/CleanFrontendMojo.java @@ -26,6 +26,8 @@ import org.apache.maven.plugins.annotations.LifecyclePhase; import org.apache.maven.plugins.annotations.Mojo; +import com.vaadin.flow.server.frontend.FrontendUtils; + import elemental.json.Json; import elemental.json.JsonObject; import elemental.json.impl.JsonUtil; @@ -103,15 +105,12 @@ public void execute() throws MojoFailureException { private void removeNodeModules() { // Remove node_modules folder File nodeModules = new File(npmFolder(), "node_modules"); - if (nodeModules.exists()) { - try { - FileUtils.deleteDirectory(nodeModules); - } catch (IOException exception) { - getLog().debug("Exception removing node_modules", exception); - getLog().error( - "Failed to remove '" + nodeModules.getAbsolutePath() - + "'. Please remove it manually."); - } + try { + FrontendUtils.deleteNodeModules(nodeModules); + } catch (IOException exception) { + getLog().debug("Exception removing node_modules", exception); + getLog().error("Failed to remove '" + nodeModules.getAbsolutePath() + + "'. Please remove it manually."); } } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java b/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java index 4459c68cefc..275e6d94a4b 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java @@ -25,16 +25,19 @@ import java.net.HttpURLConnection; import java.net.URL; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.nio.file.Path; import java.time.LocalDateTime; import java.time.ZonedDateTime; import java.time.format.DateTimeFormatter; import java.util.ArrayList; import java.util.Arrays; +import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Scanner; +import java.util.stream.Collectors; import java.util.stream.Stream; import org.apache.commons.io.FileUtils; @@ -1215,4 +1218,37 @@ public static void console(String format, Object message) { System.out.print(format(format, message)); } + /** + * Try to remove the {@code node_modules} directory, if it exists inside the + * given base directory. Note that pnpm uses symlinks internally, so delete + * utilities that follow symlinks when deleting and/or modifying permissions + * may not work as intended. + * + * @param nodeModules + * the {@code node_modules} directory + * @throws IOException + * on failure to delete any one file, or if the directory name + * is not {@code node_modules} + */ + public static void deleteNodeModules(File nodeModules) throws IOException { + if (!nodeModules.isDirectory() + || !nodeModules.getName().equals("node_modules")) { + throw new IOException(nodeModules.getAbsolutePath() + + " does not look like a node_modules directory"); + } + + Path nodeModulesPath = nodeModules.toPath(); + + Files.walk(nodeModulesPath).map(Path::toFile) + .forEach(file -> file.setWritable(true, true)); + + String undeletable = Files.walk(nodeModulesPath) + .sorted(Comparator.reverseOrder()).map(Path::toFile) + .filter(file -> !file.delete()).map(File::getAbsolutePath) + .collect(Collectors.joining(", ")); + + if (!undeletable.isEmpty()) { + throw new IOException("Unable to delete files: " + undeletable); + } + } } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java b/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java index 72e7da59b3d..af9e4f26d48 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java @@ -531,7 +531,7 @@ private void cleanUp() throws ExecutionFailedException { private void deleteNodeModules(File nodeModulesFolder) throws ExecutionFailedException { try { - FileUtils.forceDelete(nodeModulesFolder); + FrontendUtils.deleteNodeModules(nodeModulesFolder); } catch (IOException exception) { Logger log = packageUpdater.log(); log.debug("Exception removing node_modules", exception); From ac39abff973c895303522cfbcfd636f93f1b70aa Mon Sep 17 00:00:00 2001 From: Johannes Eriksson Date: Tue, 25 Jan 2022 10:09:01 +0200 Subject: [PATCH 2/8] try-with-resources --- .../flow/server/frontend/FrontendUtils.java | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java b/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java index 275e6d94a4b..3bc09d84f9e 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java @@ -1239,16 +1239,20 @@ public static void deleteNodeModules(File nodeModules) throws IOException { Path nodeModulesPath = nodeModules.toPath(); - Files.walk(nodeModulesPath).map(Path::toFile) - .forEach(file -> file.setWritable(true, true)); + try (Stream walk = Files.walk(nodeModulesPath)) { + walk.map(Path::toFile) + .forEach(file -> file.setWritable(true, true)); + } - String undeletable = Files.walk(nodeModulesPath) - .sorted(Comparator.reverseOrder()).map(Path::toFile) - .filter(file -> !file.delete()).map(File::getAbsolutePath) - .collect(Collectors.joining(", ")); + try (Stream walk = Files.walk(nodeModulesPath)) { + String undeletable = walk.sorted(Comparator.reverseOrder()) + .map(Path::toFile).filter(file -> !file.delete()) + .map(File::getAbsolutePath) + .collect(Collectors.joining(", ")); - if (!undeletable.isEmpty()) { - throw new IOException("Unable to delete files: " + undeletable); + if (!undeletable.isEmpty()) { + throw new IOException("Unable to delete files: " + undeletable); + } } } } From 8ac58db112238f9ad641a2b01903141fdf366f47 Mon Sep 17 00:00:00 2001 From: Johannes Eriksson Date: Tue, 25 Jan 2022 14:51:32 +0200 Subject: [PATCH 3/8] Do nothing if directory doesn't exist (as JavaDoc says) to not throw spurious failures --- .../java/com/vaadin/flow/server/frontend/FrontendUtils.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java b/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java index 3bc09d84f9e..8e0f7f44297 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java @@ -1231,6 +1231,10 @@ public static void console(String format, Object message) { * is not {@code node_modules} */ public static void deleteNodeModules(File nodeModules) throws IOException { + if (!nodeModules.exists()) { + return; + } + if (!nodeModules.isDirectory() || !nodeModules.getName().equals("node_modules")) { throw new IOException(nodeModules.getAbsolutePath() From 7b5ae6f5adcf5dfcbc0fbc5cf693f307abb6ac14 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Tue, 25 Jan 2022 15:31:35 +0200 Subject: [PATCH 4/8] Format --- .../java/com/vaadin/flow/server/frontend/FrontendUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java b/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java index 8e0f7f44297..c479142ed57 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java @@ -1234,7 +1234,7 @@ public static void deleteNodeModules(File nodeModules) throws IOException { if (!nodeModules.exists()) { return; } - + if (!nodeModules.isDirectory() || !nodeModules.getName().equals("node_modules")) { throw new IOException(nodeModules.getAbsolutePath() From 29ff8eaafa1fb90b270d6f7b4f12d726ea38d7c0 Mon Sep 17 00:00:00 2001 From: Johannes Eriksson Date: Tue, 25 Jan 2022 15:31:44 +0200 Subject: [PATCH 5/8] Unit test --- .../server/frontend/FrontendUtilsTest.java | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java b/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java index f9f7b165aa6..f71a4ec5f5e 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.io.UnsupportedEncodingException; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.nio.file.Path; import java.util.Arrays; import java.util.List; @@ -316,6 +317,54 @@ public void clearCachedStatsContent_clearsCache() service.getContext().getAttribute(CACHE_KEY)); } + @Test + public void deleteNodeModules_nopIfNotExists() throws IOException { + File nodeModules = new File(tmpDir.getRoot(), "node_modules"); + FrontendUtils.deleteNodeModules(nodeModules); + } + + @Test(expected = IOException.class) + public void deleteNodeModules_throwsIfNotNamedNodeModules() + throws IOException { + File myModules = new File(tmpDir.getRoot(), "my_modules"); + myModules.mkdirs(); + FrontendUtils.deleteNodeModules(myModules); + } + + @Test + public void deleteNodeModules_canDeleteSymlinksAndNotFollowThem() + throws IOException { + File externalDir = new File(tmpDir.getRoot(), "external"); + File externalLicense = new File(externalDir, "LICENSE"); + externalLicense.mkdirs(); + externalLicense.createNewFile(); + + File nodeModules = new File(tmpDir.getRoot(), "node_modules"); + File containing = new File(nodeModules, ".pnpm/a/node_modules/dep"); + containing.mkdirs(); + File license = new File(containing, "LICENSE"); + license.createNewFile(); + + File linking = new File(nodeModules, ".pnpm/b/node_modules/dep"); + linking.getParentFile().mkdirs(); + Files.createSymbolicLink(linking.toPath(), + Path.of("../../a/node_modules/dep")); + + File linkingExternal = new File(nodeModules, ".pnpm/b/node_modules/external"); + Files.createSymbolicLink(linkingExternal.toPath(), + Path.of("../../../../external")); + + Assert.assertTrue(nodeModules.exists()); + Assert.assertTrue(linking.exists()); + Assert.assertTrue(new File(linking, "LICENSE").exists()); + Assert.assertTrue(new File(linkingExternal, "LICENSE").exists()); + + FrontendUtils.deleteNodeModules(nodeModules); + + Assert.assertFalse(nodeModules.exists()); + Assert.assertTrue(externalLicense.exists()); + } + private ResourceProvider mockResourceProvider(VaadinService service) { DeploymentConfiguration config = Mockito .mock(DeploymentConfiguration.class); From f85249a733388df12d28c4304ee98821c4404a09 Mon Sep 17 00:00:00 2001 From: Johannes Eriksson Date: Tue, 25 Jan 2022 15:32:11 +0200 Subject: [PATCH 6/8] Format --- .../com/vaadin/flow/server/frontend/FrontendUtilsTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java b/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java index f71a4ec5f5e..660cebc2e18 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java @@ -350,7 +350,8 @@ public void deleteNodeModules_canDeleteSymlinksAndNotFollowThem() Files.createSymbolicLink(linking.toPath(), Path.of("../../a/node_modules/dep")); - File linkingExternal = new File(nodeModules, ".pnpm/b/node_modules/external"); + File linkingExternal = new File(nodeModules, + ".pnpm/b/node_modules/external"); Files.createSymbolicLink(linkingExternal.toPath(), Path.of("../../../../external")); From 6faedb10b8d082bf603a216bff96ccbcc1425f3c Mon Sep 17 00:00:00 2001 From: Johannes Eriksson Date: Tue, 25 Jan 2022 15:46:43 +0200 Subject: [PATCH 7/8] Do not use Path.of, small test fix, also check that linked external files keep permission --- .../vaadin/flow/server/frontend/FrontendUtilsTest.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java b/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java index 660cebc2e18..9e8a7acb348 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java @@ -336,8 +336,10 @@ public void deleteNodeModules_canDeleteSymlinksAndNotFollowThem() throws IOException { File externalDir = new File(tmpDir.getRoot(), "external"); File externalLicense = new File(externalDir, "LICENSE"); - externalLicense.mkdirs(); + + externalLicense.getParentFile().mkdirs(); externalLicense.createNewFile(); + externalLicense.setWritable(false); File nodeModules = new File(tmpDir.getRoot(), "node_modules"); File containing = new File(nodeModules, ".pnpm/a/node_modules/dep"); @@ -348,12 +350,12 @@ public void deleteNodeModules_canDeleteSymlinksAndNotFollowThem() File linking = new File(nodeModules, ".pnpm/b/node_modules/dep"); linking.getParentFile().mkdirs(); Files.createSymbolicLink(linking.toPath(), - Path.of("../../a/node_modules/dep")); + new File("../../a/node_modules/dep").toPath()); File linkingExternal = new File(nodeModules, ".pnpm/b/node_modules/external"); Files.createSymbolicLink(linkingExternal.toPath(), - Path.of("../../../../external")); + new File("../../../../external").toPath()); Assert.assertTrue(nodeModules.exists()); Assert.assertTrue(linking.exists()); @@ -364,6 +366,7 @@ public void deleteNodeModules_canDeleteSymlinksAndNotFollowThem() Assert.assertFalse(nodeModules.exists()); Assert.assertTrue(externalLicense.exists()); + Assert.assertFalse(externalLicense.canWrite()); } private ResourceProvider mockResourceProvider(VaadinService service) { From 6fae721f76eb62ee7d9d2c95cda72e54bb41ac9d Mon Sep 17 00:00:00 2001 From: Johannes Eriksson Date: Tue, 25 Jan 2022 16:38:00 +0200 Subject: [PATCH 8/8] Do not try to modify permission of files-to-be-deleted --- .../java/com/vaadin/flow/server/frontend/FrontendUtils.java | 6 ------ .../com/vaadin/flow/server/frontend/FrontendUtilsTest.java | 2 -- 2 files changed, 8 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java b/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java index c479142ed57..0eb7677a6b0 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java @@ -1242,12 +1242,6 @@ public static void deleteNodeModules(File nodeModules) throws IOException { } Path nodeModulesPath = nodeModules.toPath(); - - try (Stream walk = Files.walk(nodeModulesPath)) { - walk.map(Path::toFile) - .forEach(file -> file.setWritable(true, true)); - } - try (Stream walk = Files.walk(nodeModulesPath)) { String undeletable = walk.sorted(Comparator.reverseOrder()) .map(Path::toFile).filter(file -> !file.delete()) diff --git a/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java b/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java index 9e8a7acb348..a360d3c34f0 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java @@ -339,7 +339,6 @@ public void deleteNodeModules_canDeleteSymlinksAndNotFollowThem() externalLicense.getParentFile().mkdirs(); externalLicense.createNewFile(); - externalLicense.setWritable(false); File nodeModules = new File(tmpDir.getRoot(), "node_modules"); File containing = new File(nodeModules, ".pnpm/a/node_modules/dep"); @@ -366,7 +365,6 @@ public void deleteNodeModules_canDeleteSymlinksAndNotFollowThem() Assert.assertFalse(nodeModules.exists()); Assert.assertTrue(externalLicense.exists()); - Assert.assertFalse(externalLicense.canWrite()); } private ResourceProvider mockResourceProvider(VaadinService service) {