SCANNERAPI-170 Properly handle case when SONAR_USER_HOME point to a symlink

Author: henryju

api/src/main/java/org/sonarsource/scanner/api/internal/cache/FileCache.java

@@ -21,7 +21,11 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.AtomicMoveNotSupportedException;
+import java.nio.file.FileAlreadyExistsException;
 import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
 import javax.annotation.CheckForNull;
 
 /**
@@ -31,28 +35,25 @@
  */
 public class FileCache {
 
-  /** Maximum loop count when creating temp directories. */
-  private static final int TEMP_DIR_ATTEMPTS = 10000;
-
-  private final File dir;
-  private final File tmpDir;
+  private final Path dir;
+  private final Path tmpDir;
   private final FileHashes hashes;
   private final Logger logger;
 
-  FileCache(File dir, FileHashes fileHashes, Logger logger) {
+  FileCache(Path dir, FileHashes fileHashes, Logger logger) {
     this.hashes = fileHashes;
     this.logger = logger;
     this.dir = createDir(dir, "user cache");
-    logger.info(String.format("User cache: %s", dir.getAbsolutePath()));
-    this.tmpDir = createDir(new File(dir, "_tmp"), "temp dir");
+    logger.info(String.format("User cache: %s", dir.toString()));
+    this.tmpDir = createDir(dir.resolve("_tmp"), "temp dir");
   }
 
-  static FileCache create(File dir, Logger logger) {
+  static FileCache create(Path dir, Logger logger) {
     return new FileCache(dir, new FileHashes(), logger);
   }
 
   public File getDir() {
-    return dir;
+    return dir.toFile();
   }
 
   /**
@@ -61,9 +62,9 @@ public File getDir() {
    */
   @CheckForNull
   public File get(String filename, String hash) {
-    File cachedFile = new File(new File(dir, hash), filename);
-    if (cachedFile.exists()) {
-      return cachedFile;
+    Path cachedFile = dir.resolve(hash).resolve(filename);
+    if (Files.exists(cachedFile)) {
+      return cachedFile.toFile();
     }
     logger.debug(String.format("No file found in the cache with name %s and hash %s", filename, hash));
     return null;
@@ -76,84 +77,74 @@ public interface Downloader {
 
   public File get(String filename, String hash, Downloader downloader) {
     // Does not fail if another process tries to create the directory at the same time.
-    File hashDir = hashDir(hash);
-    File targetFile = new File(hashDir, filename);
-    if (!targetFile.exists()) {
-      File tempFile = newTempFile();
+    Path hashDir = hashDir(hash);
+    Path targetFile = hashDir.resolve(filename);
+    if (!Files.exists(targetFile)) {
+      Path tempFile = newTempFile();
       download(downloader, filename, tempFile);
-      String downloadedHash = hashes.of(tempFile);
+      String downloadedHash = hashes.of(tempFile.toFile());
       if (!hash.equals(downloadedHash)) {
-        throw new IllegalStateException("INVALID HASH: File " + tempFile.getAbsolutePath() + " was expected to have hash " + hash
+        throw new IllegalStateException("INVALID HASH: File " + tempFile.toAbsolutePath() + " was expected to have hash " + hash
           + " but was downloaded with hash " + downloadedHash);
       }
       mkdirQuietly(hashDir);
       renameQuietly(tempFile, targetFile);
     }
-    return targetFile;
+    return targetFile.toFile();
   }
 
-  private void download(Downloader downloader, String filename, File tempFile) {
+  private static void download(Downloader downloader, String filename, Path tempFile) {
     try {
-      downloader.download(filename, tempFile);
+      downloader.download(filename, tempFile.toFile());
     } catch (IOException e) {
       throw new IllegalStateException("Fail to download " + filename + " to " + tempFile, e);
     }
   }
 
-  private void renameQuietly(File sourceFile, File targetFile) {
-    boolean rename = sourceFile.renameTo(targetFile);
-    // Check if the file was cached by another process during download
-    if (!rename && !targetFile.exists()) {
-      logger.warn(String.format("Unable to rename %s to %s", sourceFile.getAbsolutePath(), targetFile.getAbsolutePath()));
+  private void renameQuietly(Path sourceFile, Path targetFile) {
+    try {
+      Files.move(sourceFile, targetFile, StandardCopyOption.ATOMIC_MOVE);
+    } catch (AtomicMoveNotSupportedException ex) {
+      logger.warn(String.format("Unable to rename %s to %s", sourceFile.toAbsolutePath(), targetFile.toAbsolutePath()));
       logger.warn("A copy/delete will be tempted but with no guarantee of atomicity");
       try {
-        Files.move(sourceFile.toPath(), targetFile.toPath());
+        Files.move(sourceFile, targetFile);
       } catch (IOException e) {
-        throw new IllegalStateException("Fail to move " + sourceFile.getAbsolutePath() + " to " + targetFile, e);
+        throw new IllegalStateException("Fail to move " + sourceFile.toAbsolutePath() + " to " + targetFile, e);
       }
+    } catch (FileAlreadyExistsException e) {
+      // File was probably cached by another process in the mean time
+    } catch (IOException e) {
+      throw new IllegalStateException("Fail to move " + sourceFile.toAbsolutePath() + " to " + targetFile, e);
     }
   }
 
-  private File hashDir(String hash) {
-    return new File(dir, hash);
+  private Path hashDir(String hash) {
+    return dir.resolve(hash);
   }
 
-  private static void mkdirQuietly(File hashDir) {
+  private static void mkdirQuietly(Path hashDir) {
     try {
-      Files.createDirectories(hashDir.toPath());
+      Files.createDirectories(hashDir);
     } catch (IOException e) {
       throw new IllegalStateException("Fail to create cache directory: " + hashDir, e);
     }
   }
 
-  private File newTempFile() {
+  private Path newTempFile() {
     try {
-      return File.createTempFile("fileCache", null, tmpDir);
+      return Files.createTempFile(tmpDir, "fileCache", null);
     } catch (IOException e) {
       throw new IllegalStateException("Fail to create temp file in " + tmpDir, e);
     }
   }
 
-  public File createTempDir() {
-    String baseName = System.currentTimeMillis() + "-";
-
-    for (int counter = 0; counter < TEMP_DIR_ATTEMPTS; counter++) {
-      File tempDir = new File(tmpDir, baseName + counter);
-      if (tempDir.mkdir()) {
-        return tempDir;
-      }
-    }
-    throw new IllegalStateException("Failed to create directory in " + tmpDir);
-  }
-
-  private File createDir(File dir, String debugTitle) {
-    if (!dir.isDirectory() || !dir.exists()) {
-      logger.debug("Create : " + dir.getAbsolutePath());
-      try {
-        Files.createDirectories(dir.toPath());
-      } catch (IOException e) {
-        throw new IllegalStateException("Unable to create " + debugTitle + dir.getAbsolutePath(), e);
-      }
+  private Path createDir(Path dir, String debugTitle) {
+    logger.debug("Create: " + dir.toString());
+    try {
+      Files.createDirectories(dir);
+    } catch (IOException e) {
+      throw new IllegalStateException("Unable to create " + debugTitle + dir.toString(), e);
     }
     return dir;
   }

api/src/main/java/org/sonarsource/scanner/api/internal/cache/FileCacheBuilder.java

@@ -20,7 +20,6 @@
 package org.sonarsource.scanner.api.internal.cache;
 
 import java.io.File;
-
 import javax.annotation.Nullable;
 
 public class FileCacheBuilder {
@@ -46,9 +45,9 @@ public FileCache build() {
       userHome = findHome();
     }
     File cacheDir = new File(userHome, "cache");
-    return FileCache.create(cacheDir, logger);
+    return FileCache.create(cacheDir.toPath(), logger);
   }
-  
+
   private static File findHome() {
     String path = System.getenv("SONAR_USER_HOME");
     if (path == null) {

api/src/test/java/org/sonarsource/scanner/api/internal/cache/FileCacheBuilderTest.java

@@ -20,12 +20,11 @@
 package org.sonarsource.scanner.api.internal.cache;
 
 import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
-import org.sonarsource.scanner.api.internal.cache.FileCache;
-import org.sonarsource.scanner.api.internal.cache.FileCacheBuilder;
-import org.sonarsource.scanner.api.internal.cache.Logger;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
@@ -53,6 +52,19 @@ public void user_home_property_can_be_null() {
     assertThat(cache.getDir().getName()).isEqualTo("cache");
   }
 
+  @Test
+  public void user_home_property_can_be_a_symlink() throws IOException {
+    File realSonarHome = temp.newFolder();
+    File symlink = temp.newFolder();
+    symlink.delete();
+    Files.createSymbolicLink(symlink.toPath(), realSonarHome.toPath());
+
+    FileCache cache = new FileCacheBuilder(mock(Logger.class)).setUserHome(symlink).build();
+
+    assertThat(cache.getDir()).isDirectory().exists();
+    assertThat(cache.getDir().getName()).isEqualTo("cache");
+  }
+
   @Test
   public void use_default_path_or_env_variable() {
     FileCache cache = new FileCacheBuilder(mock(Logger.class)).build();

api/src/test/java/org/sonarsource/scanner/api/internal/cache/FileCacheTest.java

@@ -23,33 +23,32 @@
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
-
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Matchers.any;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 public class FileCacheTest {
   @Rule
-  public TemporaryFolder tempFolder = new TemporaryFolder();
+  public TemporaryFolder temp = new TemporaryFolder();
 
   @Rule
   public ExpectedException thrown = ExpectedException.none();
 
   @Test
   public void not_in_cache() throws IOException {
-    FileCache cache = FileCache.create(tempFolder.newFolder(), mock(Logger.class));
+    FileCache cache = FileCache.create(temp.newFolder().toPath(), mock(Logger.class));
     assertThat(cache.get("sonar-foo-plugin-1.5.jar", "ABCDE")).isNull();
   }
 
   @Test
   public void found_in_cache() throws IOException {
-    FileCache cache = FileCache.create(tempFolder.newFolder(), mock(Logger.class));
+    FileCache cache = FileCache.create(temp.newFolder().toPath(), mock(Logger.class));
 
     // populate the cache. Assume that hash is correct.
     File cachedFile = new File(new File(cache.getDir(), "ABCDE"), "sonar-foo-plugin-1.5.jar");
@@ -61,19 +60,31 @@ public void found_in_cache() throws IOException {
   @Test
   public void download_and_add_to_cache() throws IOException {
     FileHashes hashes = mock(FileHashes.class);
-    FileCache cache = new FileCache(tempFolder.newFolder(), hashes, mock(Logger.class));
+    FileCache cache = new FileCache(temp.newFolder().toPath(), hashes, mock(Logger.class));
     when(hashes.of(any(File.class))).thenReturn("ABCDE");
 
     FileCache.Downloader downloader = new FileCache.Downloader() {
+      boolean single = false;
+
       public void download(String filename, File toFile) throws IOException {
+        if (single) {
+          throw new IllegalStateException("Already called");
+        }
         write(toFile, "body");
+        single = true;
       }
     };
     File cachedFile = cache.get("sonar-foo-plugin-1.5.jar", "ABCDE", downloader);
     assertThat(cachedFile).isNotNull().exists().isFile();
     assertThat(cachedFile.getName()).isEqualTo("sonar-foo-plugin-1.5.jar");
     assertThat(cachedFile.getParentFile().getParentFile()).isEqualTo(cache.getDir());
     assertThat(read(cachedFile)).isEqualTo("body");
+
+    File againFromCache = cache.get("sonar-foo-plugin-1.5.jar", "ABCDE", downloader);
+    assertThat(againFromCache).isNotNull().exists().isFile();
+    assertThat(againFromCache.getName()).isEqualTo("sonar-foo-plugin-1.5.jar");
+    assertThat(againFromCache.getParentFile().getParentFile()).isEqualTo(cache.getDir());
+    assertThat(read(againFromCache)).isEqualTo("body");
   }
 
   @Test
@@ -82,7 +93,7 @@ public void download_corrupted_file() throws IOException {
     thrown.expectMessage("INVALID HASH");
 
     FileHashes hashes = mock(FileHashes.class);
-    FileCache cache = new FileCache(tempFolder.newFolder(), hashes, mock(Logger.class));
+    FileCache cache = new FileCache(temp.newFolder().toPath(), hashes, mock(Logger.class));
     when(hashes.of(any(File.class))).thenReturn("VWXYZ");
 
     FileCache.Downloader downloader = new FileCache.Downloader() {
@@ -97,7 +108,7 @@ public void download(String filename, File toFile) throws IOException {
   public void concurrent_download() throws IOException {
     FileHashes hashes = mock(FileHashes.class);
     when(hashes.of(any(File.class))).thenReturn("ABCDE");
-    final FileCache cache = new FileCache(tempFolder.newFolder(), hashes, mock(Logger.class));
+    final FileCache cache = new FileCache(temp.newFolder().toPath(), hashes, mock(Logger.class));
 
     FileCache.Downloader downloader = new FileCache.Downloader() {
       public void download(String filename, File toFile) throws IOException {