make npmInstallCache more robust (#2096)

nedtwigg · web-flow · commit 5968076df566 · 2024-05-22T23:54:37.000-07:00
diff --git a/CHANGES.md b/CHANGES.md
@@ -26,6 +26,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 ### Fixed
 * Ignore system git config when running tests ([#1990](https://github.com/diffplug/spotless/issues/1990))
 * Correctly provide EditorConfig property types for Ktlint ([#2052](https://github.com/diffplug/spotless/issues/2052))
+* Made ShadowCopy (`npmInstallCache`) more robust by re-creating the cache dir if it goes missing ([#1984](https://github.com/diffplug/spotless/issues/1984),[2096](https://github.com/diffplug/spotless/pull/2096))
 
 ## [2.45.0] - 2024-01-23
 ### Added
diff --git a/lib/src/main/java/com/diffplug/spotless/npm/NodeModulesCachingNpmProcessFactory.java b/lib/src/main/java/com/diffplug/spotless/npm/NodeModulesCachingNpmProcessFactory.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2023 DiffPlug
+ * Copyright 2023-2024 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -38,11 +38,11 @@ public class NodeModulesCachingNpmProcessFactory implements NpmProcessFactory {
 
 	private NodeModulesCachingNpmProcessFactory(@Nonnull File cacheDir) {
 		this.cacheDir = Objects.requireNonNull(cacheDir);
-		assertDir(cacheDir);
-		this.shadowCopy = new ShadowCopy(cacheDir);
+		assertDir(); // throws if cacheDir is not a directory
+		this.shadowCopy = new ShadowCopy(this::assertDir);
 	}
 
-	private void assertDir(File cacheDir) {
+	private synchronized File assertDir() {
 		if (cacheDir.exists() && !cacheDir.isDirectory()) {
 			throw new IllegalArgumentException("Cache dir must be a directory");
 		}
@@ -51,6 +51,7 @@ private void assertDir(File cacheDir) {
 				throw new IllegalArgumentException("Cache dir could not be created.");
 			}
 		}
+		return cacheDir;
 	}
 
 	public static NodeModulesCachingNpmProcessFactory create(@Nonnull File cacheDir) {
diff --git a/lib/src/main/java/com/diffplug/spotless/npm/ShadowCopy.java b/lib/src/main/java/com/diffplug/spotless/npm/ShadowCopy.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2023 DiffPlug
+ * Copyright 2023-2024 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@
 import java.nio.file.attribute.BasicFileAttributes;
 import java.time.Duration;
 import java.util.concurrent.TimeoutException;
+import java.util.function.Supplier;
 
 import javax.annotation.Nonnull;
 
@@ -39,13 +40,18 @@ class ShadowCopy {
 
 	private static final Logger logger = LoggerFactory.getLogger(ShadowCopy.class);
 
-	private final File shadowCopyRoot;
+	private final Supplier<File> shadowCopyRootSupplier;
 
-	public ShadowCopy(@Nonnull File shadowCopyRoot) {
-		this.shadowCopyRoot = shadowCopyRoot;
+	public ShadowCopy(@Nonnull Supplier<File> shadowCopyRootSupplier) {
+		this.shadowCopyRootSupplier = shadowCopyRootSupplier;
+	}
+
+	private File shadowCopyRoot() {
+		File shadowCopyRoot = shadowCopyRootSupplier.get();
 		if (!shadowCopyRoot.isDirectory()) {
-			throw new IllegalArgumentException("Shadow copy root must be a directory: " + shadowCopyRoot);
+			throw new IllegalStateException("Shadow copy root must be a directory: " + shadowCopyRoot);
 		}
+		return shadowCopyRoot;
 	}
 
 	public void addEntry(String key, File orig) {
@@ -86,17 +92,17 @@ private void cleanupReservation(String key) {
 	}
 
 	private Path markerFilePath(String key) {
-		return Paths.get(shadowCopyRoot.getAbsolutePath(), key + ".marker");
+		return Paths.get(shadowCopyRoot().getAbsolutePath(), key + ".marker");
 	}
 
 	private File entry(String key, String origName) {
-		return Paths.get(shadowCopyRoot.getAbsolutePath(), key, origName).toFile();
+		return Paths.get(shadowCopyRoot().getAbsolutePath(), key, origName).toFile();
 	}
 
 	private boolean reserveSubFolder(String key) {
 		// put a marker file named "key".marker in "shadowCopyRoot" to make sure no other process is using it or return false if it already exists
 		try {
-			Files.createFile(Paths.get(shadowCopyRoot.getAbsolutePath(), key + ".marker"));
+			Files.createFile(Paths.get(shadowCopyRoot().getAbsolutePath(), key + ".marker"));
 			return true;
 		} catch (FileAlreadyExistsException e) {
 			return false;
diff --git a/plugin-gradle/CHANGES.md b/plugin-gradle/CHANGES.md
@@ -8,6 +8,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 * Ignore system git config when running tests ([#1990](https://github.com/diffplug/spotless/issues/1990))
 * Correctly provide EditorConfig property types for Ktlint ([#2052](https://github.com/diffplug/spotless/issues/2052))
 * Fixed memory leak introduced in 6.21.0 ([#2067](https://github.com/diffplug/spotless/issues/2067))
+* Made ShadowCopy (`npmInstallCache`) more robust by re-creating the cache dir if it goes missing ([#1984](https://github.com/diffplug/spotless/issues/1984),[2096](https://github.com/diffplug/spotless/pull/2096))
 ### Changes
 * Bump default `ktfmt` version to latest `0.46` -> `0.47`. ([#2045](https://github.com/diffplug/spotless/pull/2045))
 * Bump default `sortpom` version to latest `3.2.1` -> `3.4.0`. ([#2049](https://github.com/diffplug/spotless/pull/2049))
diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/PrettierIntegrationTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/PrettierIntegrationTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2023 DiffPlug
+ * Copyright 2016-2024 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -358,6 +358,54 @@ void autodetectNpmrcFileConfig(String prettierVersion) throws IOException {
 		Assertions.assertThat(spotlessApply.getOutput()).containsPattern("Running npm command.*npm install.* failed with exit code: 1");
 	}
 
+	@ParameterizedTest(name = "{index}: verifyCleanAndSpotlessWorks with prettier {0}")
+	@ValueSource(strings = {PRETTIER_VERSION_2, PRETTIER_VERSION_3})
+	void verifyCleanAndSpotlessWorks(String prettierVersion) throws IOException {
+		setFile("build.gradle").toLines(
+				"plugins {",
+				"    id 'com.diffplug.spotless'",
+				"}",
+				"repositories { mavenCentral() }",
+				"def prettierConfig = [:]",
+				"prettierConfig['printWidth'] = 20",
+				"prettierConfig['parser'] = 'typescript'",
+				"spotless {",
+				"    format 'mytypescript', {",
+				"        target 'test.ts'",
+				"        prettier('" + prettierVersion + "').config(prettierConfig)",
+				"    }",
+				"}");
+		setFile("test.ts").toResource("npm/prettier/config/typescript.dirty");
+		final BuildResult spotlessApply = gradleRunner().withArguments("--stacktrace", "clean", "spotlessApply").build();
+		Assertions.assertThat(spotlessApply.getOutput()).contains("BUILD SUCCESSFUL");
+		final BuildResult spotlessApply2 = gradleRunner().withArguments("--stacktrace", "clean", "spotlessApply").build();
+		Assertions.assertThat(spotlessApply2.getOutput()).contains("BUILD SUCCESSFUL");
+	}
+
+	@ParameterizedTest(name = "{index}: verifyCleanAndSpotlessWithNpmInstallCacheWorks with prettier {0}")
+	@ValueSource(strings = {PRETTIER_VERSION_2, PRETTIER_VERSION_3})
+	void verifyCleanAndSpotlessWithNpmInstallCacheWorks(String prettierVersion) throws IOException {
+		setFile("build.gradle").toLines(
+				"plugins {",
+				"    id 'com.diffplug.spotless'",
+				"}",
+				"repositories { mavenCentral() }",
+				"def prettierConfig = [:]",
+				"prettierConfig['printWidth'] = 20",
+				"prettierConfig['parser'] = 'typescript'",
+				"spotless {",
+				"    format 'mytypescript', {",
+				"        target 'test.ts'",
+				"        prettier('" + prettierVersion + "').npmInstallCache().config(prettierConfig)",
+				"    }",
+				"}");
+		setFile("test.ts").toResource("npm/prettier/config/typescript.dirty");
+		final BuildResult spotlessApply = gradleRunner().withArguments("--stacktrace", "clean", "spotlessApply").build();
+		Assertions.assertThat(spotlessApply.getOutput()).contains("BUILD SUCCESSFUL");
+		final BuildResult spotlessApply2 = gradleRunner().withArguments("--stacktrace", "clean", "spotlessApply").build();
+		Assertions.assertThat(spotlessApply2.getOutput()).contains("BUILD SUCCESSFUL");
+	}
+
 	@ParameterizedTest(name = "{index}: autodetectNpmrcFileConfig with prettier {0}")
 	@ValueSource(strings = {PRETTIER_VERSION_2, PRETTIER_VERSION_3})
 	void pickupNpmrcFileConfig(String prettierVersion) throws IOException {
diff --git a/plugin-maven/CHANGES.md b/plugin-maven/CHANGES.md
@@ -6,6 +6,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 ### Fixed
 * Ignore system git config when running tests ([#1990](https://github.com/diffplug/spotless/issues/1990))
 * Correctly provide EditorConfig property types for Ktlint ([#2052](https://github.com/diffplug/spotless/issues/2052))
+* Made ShadowCopy (`npmInstallCache`) more robust by re-creating the cache dir if it goes missing ([#1984](https://github.com/diffplug/spotless/issues/1984),[2096](https://github.com/diffplug/spotless/pull/2096))
 ### Changes
 * Bump default `ktfmt` version to latest `0.46` -> `0.47`. ([#2045](https://github.com/diffplug/spotless/pull/2045))
 * Bump default `sortpom` version to latest `3.2.1` -> `3.4.0`. ([#2049](https://github.com/diffplug/spotless/pull/2049))
diff --git a/testlib/src/test/java/com/diffplug/spotless/npm/ShadowCopyTest.java b/testlib/src/test/java/com/diffplug/spotless/npm/ShadowCopyTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2023 DiffPlug
+ * Copyright 2023-2024 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -29,6 +29,7 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import com.diffplug.common.base.Suppliers;
 import com.diffplug.spotless.ResourceHarness;
 
 class ShadowCopyTest extends ResourceHarness {
@@ -43,7 +44,7 @@ class ShadowCopyTest extends ResourceHarness {
 	@BeforeEach
 	void setUp() throws IOException {
 		shadowCopyRoot = newFolder("shadowCopyRoot");
-		shadowCopy = new ShadowCopy(shadowCopyRoot);
+		shadowCopy = new ShadowCopy(Suppliers.ofInstance(shadowCopyRoot));
 	}
 
 	@Test

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`/*`
`2`		`- * Copyright 2023 DiffPlug`
	`2`	`+ * Copyright 2023-2024 DiffPlug`
`3`	`3`	`*`
`4`	`4`	`* Licensed under the Apache License, Version 2.0 (the "License");`
`5`	`5`	`* you may not use this file except in compliance with the License.`
`@@ -38,11 +38,11 @@ public class NodeModulesCachingNpmProcessFactory implements NpmProcessFactory {`
`38`	`38`
`39`	`39`	`private NodeModulesCachingNpmProcessFactory(@Nonnull File cacheDir) {`
`40`	`40`	`this.cacheDir = Objects.requireNonNull(cacheDir);`
`41`		`- assertDir(cacheDir);`
`42`		`- this.shadowCopy = new ShadowCopy(cacheDir);`
	`41`	`+ assertDir(); // throws if cacheDir is not a directory`
	`42`	`+ this.shadowCopy = new ShadowCopy(this::assertDir);`
`43`	`43`	`}`
`44`	`44`
`45`		`- private void assertDir(File cacheDir) {`
	`45`	`+ private synchronized File assertDir() {`
`46`	`46`	`if (cacheDir.exists() && !cacheDir.isDirectory()) {`
`47`	`47`	`throw new IllegalArgumentException("Cache dir must be a directory");`
`48`	`48`	`}`
`@@ -51,6 +51,7 @@ private void assertDir(File cacheDir) {`
`51`	`51`	`throw new IllegalArgumentException("Cache dir could not be created.");`
`52`	`52`	`}`
`53`	`53`	`}`
	`54`	`+ return cacheDir;`
`54`	`55`	`}`
`55`	`56`
`56`	`57`	`public static NodeModulesCachingNpmProcessFactory create(@Nonnull File cacheDir) {`