#4768 Fix import file path issue

stroom-core-client/src/main/java/stroom/dispatch/client/ExportFileCompleteUtil.java

@@ -82,7 +82,8 @@ private static String getMessage(final ResourceGeneration result) {
     private static void download(final LocationManager locationManager, final ResourceGeneration result) {
         // Change the browser location to download the zip
         // file.
-        locationManager.replace(GWT.getHostPageBaseURL() + "resourcestore/" +
-                result.getResourceKey().getName() + "?UUID=" + result.getResourceKey().getKey());
+        locationManager.replace(GWT.getHostPageBaseURL() +
+                "resourcestore/?uuid=" +
+                result.getResourceKey().getKey());
     }
 }

stroom-data/stroom-data-store-impl/src/main/java/stroom/data/store/impl/ImportFileServlet.java

@@ -99,7 +99,8 @@ protected void doPost(final HttpServletRequest request, final HttpServletRespons
             }
 
             propertyMap.setSuccess(true);
-            resourceKey.write(propertyMap);
+            propertyMap.put(ResourceKey.NAME, fileName);
+            propertyMap.put(ResourceKey.KEY, resourceKey.getKey());
             fileItem.delete();
 
         } catch (final RuntimeException e) {

stroom-resource/stroom-resource-api/src/main/java/stroom/resource/api/ResourceStore.java

@@ -27,7 +27,7 @@
 public interface ResourceStore {
 
     /**
-     * Create a temporary file and give it a string key.
+     * Create a temporary file and give it a UUID.
      */
     ResourceKey createTempFile(final String name);
 

stroom-resource/stroom-resource-impl/src/main/java/stroom/resource/impl/ResourceStoreImpl.java

@@ -20,31 +20,34 @@
 import stroom.task.api.TaskContextFactory;
 import stroom.util.io.FileUtil;
 import stroom.util.io.TempDirProvider;
+import stroom.util.logging.LambdaLogger;
+import stroom.util.logging.LambdaLoggerFactory;
 import stroom.util.shared.ResourceKey;
 
 import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.nio.file.Paths;
-import java.util.HashSet;
-import java.util.Set;
+import java.time.Instant;
+import java.util.Map;
+import java.util.UUID;
+import java.util.concurrent.ConcurrentHashMap;
 import javax.inject.Inject;
 import javax.inject.Singleton;
 
 /**
- * Simple Store that gives you 1 hour to use your temp file and then it deletes
- * it.
+ * Simple Store that gives you 1 hour to use your temp file before it deletes it.
  */
 @Singleton
 public class ResourceStoreImpl implements ResourceStore {
 
+    private static final LambdaLogger LOGGER = LambdaLoggerFactory.getLogger(ResourceStoreImpl.class);
+
     private final TempDirProvider tempDirProvider;
     private final TaskContextFactory taskContextFactory;
+    private final Map<String, ResourceItem> currentFiles = new ConcurrentHashMap<>();
 
-    private Set<ResourceKey> currentFiles = new HashSet<>();
-    private Set<ResourceKey> oldFiles = new HashSet<>();
-    private long sequence;
+    private volatile Instant lastCleanupTime;
 
     @Inject
     public ResourceStoreImpl(final TempDirProvider tempDirProvider,
@@ -72,47 +75,108 @@ void shutdown() {
     }
 
     @Override
-    public synchronized ResourceKey createTempFile(final String name) {
-        final String fileName = FileUtil.getCanonicalPath(getTempDir().resolve((sequence++) + name));
-        final ResourceKey resourceKey = new ResourceKey(name, fileName);
-        currentFiles.add(resourceKey);
-
+    public ResourceKey createTempFile(final String name) {
+        final String uuid = UUID.randomUUID().toString();
+        final Path path = getTempDir().resolve(uuid);
+        final ResourceKey resourceKey = new ResourceKey(uuid, name);
+        final ResourceItem resourceItem = new ResourceItem(resourceKey, path, Instant.now());
+        currentFiles.put(uuid, resourceItem);
         return resourceKey;
     }
 
     @Override
-    public synchronized void deleteTempFile(final ResourceKey resourceKey) {
-        currentFiles.remove(resourceKey);
-        oldFiles.remove(resourceKey);
-        final Path file = Paths.get(resourceKey.getKey());
-        try {
-            Files.deleteIfExists(file);
-        } catch (final IOException e) {
-            throw new UncheckedIOException(e);
+    public void deleteTempFile(final ResourceKey resourceKey) {
+        final ResourceItem resourceItem = currentFiles.remove(resourceKey.getKey());
+        if (resourceItem != null) {
+            final Path file = resourceItem.getPath();
+            try {
+                Files.deleteIfExists(file);
+            } catch (final IOException e) {
+                throw new UncheckedIOException(e);
+            }
         }
     }
 
     @Override
-    public synchronized Path getTempFile(final ResourceKey resourceKey) {
+    public Path getTempFile(final ResourceKey resourceKey) {
         // File gone !
-        if (!currentFiles.contains(resourceKey) && !oldFiles.contains(resourceKey)) {
+        final ResourceItem resourceItem = currentFiles.get(resourceKey.getKey());
+        if (resourceItem == null) {
             return null;
         }
-        return Paths.get(resourceKey.getKey());
+        resourceItem.setLastAccessTime(Instant.now());
+        return resourceItem.getPath();
     }
 
     void execute() {
         taskContextFactory.current().info(() -> "Deleting temp files");
-        flipStore();
+        cleanup();
     }
 
     /**
-     * Move the current files to the old files deleting the old ones.
+     * Delete files that haven't been accessed since the last cleanup.
+     * This allows us to choose the cleanup frequency.
      */
-    private synchronized void flipStore() {
-        final Set<ResourceKey> clonedOldFiles = new HashSet<>(oldFiles);
-        clonedOldFiles.forEach(this::deleteTempFile);
-        oldFiles = currentFiles;
-        currentFiles = new HashSet<>();
+    private synchronized void cleanup() {
+        if (lastCleanupTime != null) {
+            // Delete anything that hasn't been accessed since we were last asked to cleanup.
+            currentFiles.values().forEach(resourceItem -> {
+                try {
+                    if (resourceItem.getLastAccessTime().isBefore(lastCleanupTime)) {
+                        deleteTempFile(resourceItem.getResourceKey());
+                    }
+                } catch (final RuntimeException e) {
+                    LOGGER.error(e::getMessage, e);
+                }
+            });
+        }
+        lastCleanupTime = Instant.now();
+    }
+
+    private static class ResourceItem {
+
+        private final ResourceKey resourceKey;
+        private final Path path;
+        private final Instant createTime;
+        private volatile Instant lastAccessTime;
+
+        public ResourceItem(final ResourceKey resourceKey,
+                            final Path path,
+                            final Instant createTime) {
+            this.resourceKey = resourceKey;
+            this.path = path;
+            this.createTime = createTime;
+            this.lastAccessTime = createTime;
+        }
+
+        public ResourceKey getResourceKey() {
+            return resourceKey;
+        }
+
+        public Path getPath() {
+            return path;
+        }
+
+        public Instant getCreateTime() {
+            return createTime;
+        }
+
+        public Instant getLastAccessTime() {
+            return lastAccessTime;
+        }
+
+        public void setLastAccessTime(final Instant lastAccessTime) {
+            this.lastAccessTime = lastAccessTime;
+        }
+
+        @Override
+        public String toString() {
+            return "ResourceItem{" +
+                    "resourceKey=" + resourceKey +
+                    ", path=" + path +
+                    ", createTime=" + createTime +
+                    ", lastAccessTime=" + lastAccessTime +
+                    '}';
+        }
     }
 }

stroom-resource/stroom-resource-impl/src/main/java/stroom/resource/impl/SessionResourceStoreImpl.java

@@ -17,7 +17,6 @@
 package stroom.resource.impl;
 
 import stroom.resource.api.ResourceStore;
-import stroom.util.io.FileUtil;
 import stroom.util.servlet.HttpServletRequestHolder;
 import stroom.util.shared.IsServlet;
 import stroom.util.shared.ResourceKey;
@@ -39,11 +38,10 @@
  */
 public class SessionResourceStoreImpl extends HttpServlet implements ResourceStore, IsServlet {
 
-    private static final long serialVersionUID = -4533441835216235920L;
     private static final Set<String> PATH_SPECS = Set.of("/resourcestore/*");
-    private static final String UUID_ARG = "UUID";
+    private static final String UUID_ARG = "uuid";
 
-    private final ResourceStore resourceStore;
+    private final ResourceStoreImpl resourceStore;
     private final HttpServletRequestHolder httpServletRequestHolder;
 
     @Inject
@@ -56,29 +54,33 @@ public class SessionResourceStoreImpl extends HttpServlet implements ResourceSto
     @Override
     public ResourceKey createTempFile(final String name) {
         final ResourceKey key = resourceStore.createTempFile(name);
-        final ResourceKey sessionKey = new ResourceKey(name, UUID.randomUUID().toString());
+        final ResourceKey sessionKey = new ResourceKey(UUID.randomUUID().toString(), name);
 
         getMap().put(sessionKey, key);
 
         return sessionKey;
     }
 
     @Override
-    public void deleteTempFile(final ResourceKey key) {
-        final ResourceKey realKey = getMap().remove(key);
+    public Path getTempFile(final ResourceKey key) {
+        final ResourceKey realKey = getRealKey(key);
         if (realKey == null) {
-            return;
+            return null;
         }
-        resourceStore.deleteTempFile(realKey);
+        return resourceStore.getTempFile(realKey);
     }
 
     @Override
-    public Path getTempFile(final ResourceKey key) {
-        final ResourceKey realKey = getMap().get(key);
+    public void deleteTempFile(final ResourceKey key) {
+        final ResourceKey realKey = getRealKey(key);
         if (realKey == null) {
-            return null;
+            return;
         }
-        return resourceStore.getTempFile(getMap().get(key));
+        resourceStore.deleteTempFile(realKey);
+    }
+
+    private ResourceKey getRealKey(final ResourceKey key) {
+        return getMap().get(key);
     }
 
     private ResourceMap getMap() {
@@ -113,20 +115,22 @@ protected void doGet(final HttpServletRequest req, final HttpServletResponse res
             final String uuid = req.getParameter(UUID_ARG);
             boolean found = false;
             if (uuid != null) {
-                final ResourceKey resourceKey = new ResourceKey(null, uuid);
-                try {
-                    final Path file = getTempFile(resourceKey);
-                    if (file != null && Files.isRegularFile(file)) {
-                        if (FileUtil.getCanonicalPath(file).toLowerCase().endsWith(".zip")) {
-                            resp.setContentType("application/zip");
-                        } else {
-                            resp.setContentType("application/octet-stream");
+                final ResourceKey resourceKey = getRealKey(new ResourceKey(uuid, null));
+                if (resourceKey != null) {
+                    try {
+                        final Path file = resourceStore.getTempFile(resourceKey);
+                        if (file != null && Files.isRegularFile(file)) {
+                            if (resourceKey.getName().toLowerCase().endsWith(".zip")) {
+                                resp.setContentType("application/zip");
+                            } else {
+                                resp.setContentType("application/octet-stream");
+                            }
+                            resp.getOutputStream().write(Files.readAllBytes(file));
+                            found = true;
                         }
-                        resp.getOutputStream().write(Files.readAllBytes(file));
-                        found = true;
+                    } finally {
+                        deleteTempFile(resourceKey);
                     }
-                } finally {
-                    deleteTempFile(resourceKey);
                 }
             }
 

stroom-resource/stroom-resource-impl/src/test/java/stroom/resource/impl/TestResourceStore.java

@@ -39,25 +39,47 @@ void testSimple() throws IOException {
         resourceStore.execute();
 
         final ResourceKey key1 = resourceStore.createTempFile("TestResourceStore1.dat");
-        assertThat(key1.toString().endsWith("TestResourceStore1.dat")).isTrue();
+        assertThat(key1.getName()).isEqualTo("TestResourceStore1.dat");
 
         final ResourceKey key2 = resourceStore.createTempFile("TestResourceStore2.dat");
-        assertThat(key2.toString().endsWith("TestResourceStore2.dat")).isTrue();
+        assertThat(key2.getName()).isEqualTo("TestResourceStore2.dat");
 
         Files.createFile(resourceStore.getTempFile(key1));
         Files.createFile(resourceStore.getTempFile(key2));
 
         assertThat(Files.isRegularFile(resourceStore.getTempFile(key1))).isTrue();
         assertThat(Files.isRegularFile(resourceStore.getTempFile(key2))).isTrue();
 
-        // Roll to Old
+        // Cleanup
         resourceStore.execute();
-        final Path file1 = resourceStore.getTempFile(key1);
+        Path file1 = resourceStore.getTempFile(key1);
         assertThat(Files.isRegularFile(file1)).isTrue();
-        final Path file2 = resourceStore.getTempFile(key2);
+        Path file2 = resourceStore.getTempFile(key2);
         assertThat(Files.isRegularFile(file2)).isTrue();
 
-        // Roll to Delete
+        // Cleanup
+        resourceStore.execute();
+
+        // Files should still exist as we have accessed them since last roll.
+        file1 = resourceStore.getTempFile(key1);
+        assertThat(Files.isRegularFile(file1)).isTrue();
+        file2 = resourceStore.getTempFile(key2);
+        assertThat(Files.isRegularFile(file2)).isTrue();
+
+        // Cleanup
+        resourceStore.execute();
+        file1 = resourceStore.getTempFile(key1);
+        assertThat(Files.isRegularFile(file1)).isTrue();
+        resourceStore.execute();
+
+        // We should have deleted file2 as we didn't access since last cleanup.
+        file1 = resourceStore.getTempFile(key1);
+        assertThat(Files.isRegularFile(file1)).isTrue();
+        assertThat(resourceStore.getTempFile(key2)).isNull();
+        assertThat(Files.isRegularFile(file2)).isFalse();
+
+        // Now delete everything.
+        resourceStore.execute();
         resourceStore.execute();
         assertThat(resourceStore.getTempFile(key1)).isNull();
         assertThat(Files.isRegularFile(file1)).isFalse();