Merge pull request #1095 from amvanbaren/improve-file-caching

Use generated file name for file caching
eclipse · Jan 31, 2025 · c3b160e · c3b160e
2 parents 8eea18e + 3cd0599
commit c3b160e
Show file tree

Hide file tree

Showing 9 changed files with 104 additions and 27 deletions.
diff --git a/server/src/main/java/org/eclipse/openvsx/adapter/WebResourceService.java b/server/src/main/java/org/eclipse/openvsx/adapter/WebResourceService.java
@@ -11,6 +11,7 @@
 
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.eclipse.openvsx.cache.CacheService;
+import org.eclipse.openvsx.cache.FilesCacheKeyGenerator;
 import org.eclipse.openvsx.entities.FileResource;
 import org.eclipse.openvsx.repositories.RepositoryService;
 import org.eclipse.openvsx.storage.StorageUtilService;
@@ -40,11 +41,18 @@ public class WebResourceService {
     private final StorageUtilService storageUtil;
     private final RepositoryService repositories;
     private final CacheService cache;
+    private final FilesCacheKeyGenerator filesCacheKeyGenerator;
 
-    public WebResourceService(StorageUtilService storageUtil, RepositoryService repositories, CacheService cache) {
+    public WebResourceService(
+            StorageUtilService storageUtil,
+            RepositoryService repositories,
+            CacheService cache,
+            FilesCacheKeyGenerator filesCacheKeyGenerator
+    ) {
         this.storageUtil = storageUtil;
         this.repositories = repositories;
         this.cache = cache;
+        this.filesCacheKeyGenerator = filesCacheKeyGenerator;
     }
 
     @Cacheable(value = CACHE_WEB_RESOURCE_FILES, keyGenerator = GENERATOR_FILES)
@@ -74,9 +82,11 @@ public Path getWebResource(String namespace, String extension, String targetPlat
             if(fileEntry != null) {
                 var fileExtIndex = fileEntry.getName().lastIndexOf('.');
                 var fileExt = fileExtIndex != -1 ? fileEntry.getName().substring(fileExtIndex) : "";
-                var file = Files.createTempFile("webresource_", fileExt);
-                try(var in = zip.getInputStream(fileEntry)) {
-                    Files.copy(in, file, StandardCopyOption.REPLACE_EXISTING);
+                var file = filesCacheKeyGenerator.generateCachedWebResourcePath(namespace, extension, targetPlatform, version, name, fileExt);
+                if(!Files.exists(file)) {
+                    try (var in = zip.getInputStream(fileEntry)) {
+                        Files.copy(in, file);
+                    }
                 }
 
                 return file;
@@ -93,14 +103,17 @@ public Path getWebResource(String namespace, String extension, String targetPlat
                     return null;
                 }
 
-                var file = Files.createTempFile("webresource_", ".unpkg.json");
-                var baseUrl = UrlUtil.createApiUrl(UrlUtil.getBaseUrl(), "vscode", "unpkg", namespace, extension, version);
-                var mapper = new ObjectMapper();
-                var node = mapper.createArrayNode();
-                for(var entry : dirEntries) {
-                    node.add(baseUrl + "/" + entry);
+                var file = filesCacheKeyGenerator.generateCachedWebResourcePath(namespace, extension, targetPlatform, version, name, ".unpkg.json");
+                if(!Files.exists(file)) {
+                    var baseUrl = UrlUtil.createApiUrl(UrlUtil.getBaseUrl(), "vscode", "unpkg", namespace, extension, version);
+                    var mapper = new ObjectMapper();
+                    var node = mapper.createArrayNode();
+                    for (var entry : dirEntries) {
+                        node.add(baseUrl + "/" + entry);
+                    }
+                    mapper.writeValue(file.toFile(), node);
                 }
-                mapper.writeValue(file.toFile(), node);
+
                 return file;
             } else {
                 return null;

diff --git a/server/src/main/java/org/eclipse/openvsx/cache/ExpiredFileListener.java b/server/src/main/java/org/eclipse/openvsx/cache/ExpiredFileListener.java
@@ -11,19 +11,26 @@
 
 import org.ehcache.event.CacheEvent;
 import org.ehcache.event.CacheEventListener;
+import org.ehcache.event.EventType;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 
-public class ExpiredFileListener implements CacheEventListener<String, Path> {
+// V can be Path or NullValue (Spring's way of caching null values), that's why Object is used as V type
+public class ExpiredFileListener implements CacheEventListener<String, Object> {
     protected final Logger logger = LoggerFactory.getLogger(ExpiredFileListener.class);
     @Override
-    public void onEvent(CacheEvent<? extends String, ? extends Path> cacheEvent) {
+    public void onEvent(CacheEvent<? extends String, ?> cacheEvent) {
         logger.info("Expired file cache event: {} | key: {}", cacheEvent.getType(), cacheEvent.getKey());
-        var path = cacheEvent.getOldValue();
+        var oldValue = cacheEvent.getOldValue();
+        var path = oldValue instanceof Path ? (Path) oldValue : null;
+        if(path == null || (cacheEvent.getType() == EventType.UPDATED && path.equals(cacheEvent.getNewValue()))) {
+            return;
+        }
+
         try {
             var deleted = Files.deleteIfExists(path);
             if(deleted) {

diff --git a/server/src/main/java/org/eclipse/openvsx/cache/FilesCacheKeyGenerator.java b/server/src/main/java/org/eclipse/openvsx/cache/FilesCacheKeyGenerator.java
@@ -9,6 +9,7 @@
  * ****************************************************************************** */
 package org.eclipse.openvsx.cache;
 
+import org.apache.commons.codec.digest.DigestUtils;
 import org.eclipse.openvsx.adapter.WebResourceService;
 import org.eclipse.openvsx.entities.FileResource;
 import org.eclipse.openvsx.storage.IStorageService;
@@ -17,6 +18,7 @@
 import org.springframework.stereotype.Component;
 
 import java.lang.reflect.Method;
+import java.nio.file.Path;
 
 @Component
 public class FilesCacheKeyGenerator implements KeyGenerator {
@@ -47,4 +49,20 @@ public String generate(FileResource resource) {
     public String generate(String namespace, String extension, String targetPlatform, String version, String name) {
         return UrlUtil.createApiFileUrl("", namespace, extension, targetPlatform, version, name);
     }
+
+    public Path generateCachedExtensionPath(FileResource resource) {
+        var key = generate(resource);
+        return generateCachedPath(key, "ce_", ".tmp");
+    }
+
+    public Path generateCachedWebResourcePath(String namespace, String extension, String targetPlatform, String version, String name, String fileExtension) {
+        var key = generate(namespace, extension, targetPlatform, version, name);
+        return generateCachedPath(key, "cr_", fileExtension);
+    }
+
+    private Path generateCachedPath(String key, String prefix, String extension) {
+        var hash = DigestUtils.sha256Hex(key);
+        var tmp = System.getProperty("java.io.tmpdir");
+        return Path.of(tmp, prefix + hash + extension);
+    }
 }
diff --git a/server/src/main/java/org/eclipse/openvsx/storage/AwsStorageService.java b/server/src/main/java/org/eclipse/openvsx/storage/AwsStorageService.java
@@ -21,6 +21,7 @@
 
 import org.apache.commons.lang3.ArrayUtils;
 import org.apache.commons.lang3.StringUtils;
+import org.eclipse.openvsx.cache.FilesCacheKeyGenerator;
 import org.eclipse.openvsx.entities.FileResource;
 import org.eclipse.openvsx.entities.Namespace;
 import org.eclipse.openvsx.util.TempFile;
@@ -47,6 +48,7 @@
 public class AwsStorageService implements IStorageService {
 
     private final FileCacheDurationConfig fileCacheDurationConfig;
+    private final FilesCacheKeyGenerator filesCacheKeyGenerator;
 
     @Value("${ovsx.storage.aws.access-key-id:}")
     String accessKeyId;
@@ -68,8 +70,9 @@ public class AwsStorageService implements IStorageService {
 
     private S3Client s3Client;
 
-    public AwsStorageService(FileCacheDurationConfig fileCacheDurationConfig) {
+    public AwsStorageService(FileCacheDurationConfig fileCacheDurationConfig, FilesCacheKeyGenerator filesCacheKeyGenerator) {
         this.fileCacheDurationConfig = fileCacheDurationConfig;
+        this.filesCacheKeyGenerator = filesCacheKeyGenerator;
     }
 
     protected S3Client getS3Client() {
@@ -250,10 +253,13 @@ public Path getCachedFile(FileResource resource) throws IOException {
                 .key(objectKey)
                 .build();
 
-        var path = Files.createTempFile("cached_file", null);
-        try (var stream = getS3Client().getObject(request)) {
-            Files.copy(stream, path, StandardCopyOption.REPLACE_EXISTING);
+        var path = filesCacheKeyGenerator.generateCachedExtensionPath(resource);
+        if(!Files.exists(path)) {
+            try (var stream = getS3Client().getObject(request)) {
+                Files.copy(stream, path);
+            }
         }
+
         return path;
     }
 

diff --git a/server/src/main/java/org/eclipse/openvsx/storage/AzureBlobStorageService.java b/server/src/main/java/org/eclipse/openvsx/storage/AzureBlobStorageService.java
@@ -19,6 +19,7 @@
 import com.azure.storage.blob.models.CopyStatusType;
 import org.apache.commons.lang3.ArrayUtils;
 import org.apache.commons.lang3.StringUtils;
+import org.eclipse.openvsx.cache.FilesCacheKeyGenerator;
 import org.eclipse.openvsx.entities.FileResource;
 import org.eclipse.openvsx.entities.Namespace;
 import org.eclipse.openvsx.util.TempFile;
@@ -46,6 +47,8 @@ public class AzureBlobStorageService implements IStorageService {
 
     public static final String AZURE_USER_AGENT = "OpenVSX";
 
+    private final FilesCacheKeyGenerator filesCacheKeyGenerator;
+
     @Value("${ovsx.storage.azure.service-endpoint:}")
     String serviceEndpoint;
 
@@ -57,6 +60,10 @@ public class AzureBlobStorageService implements IStorageService {
 
     private BlobContainerClient containerClient;
 
+    public AzureBlobStorageService(FilesCacheKeyGenerator filesCacheKeyGenerator) {
+        this.filesCacheKeyGenerator = filesCacheKeyGenerator;
+    }
+
 	@Override
 	public boolean isEnabled() {
 		return !StringUtils.isEmpty(serviceEndpoint);
@@ -236,8 +243,11 @@ public Path getCachedFile(FileResource resource) throws IOException {
             throw new IllegalStateException(missingEndpointMessage(blobName));
         }
 
-        var path = Files.createTempFile("cached_file", null);
-        getContainerClient().getBlobClient(blobName).downloadToFile(path.toAbsolutePath().toString(), true);
+        var path = filesCacheKeyGenerator.generateCachedExtensionPath(resource);
+        if(!Files.exists(path)) {
+            getContainerClient().getBlobClient(blobName).downloadToFile(path.toAbsolutePath().toString());
+        }
+
         return path;
     }
 }
diff --git a/server/src/main/java/org/eclipse/openvsx/storage/GoogleCloudStorageService.java b/server/src/main/java/org/eclipse/openvsx/storage/GoogleCloudStorageService.java
@@ -15,6 +15,7 @@
 import com.google.cloud.storage.StorageOptions;
 import org.apache.commons.lang3.ArrayUtils;
 import org.apache.commons.lang3.StringUtils;
+import org.eclipse.openvsx.cache.FilesCacheKeyGenerator;
 import org.eclipse.openvsx.entities.FileResource;
 import org.eclipse.openvsx.entities.Namespace;
 import org.eclipse.openvsx.util.TempFile;
@@ -39,6 +40,8 @@ public class GoogleCloudStorageService implements IStorageService {
 
     private static final String BASE_URL = "https://storage.googleapis.com/";
 
+    private final FilesCacheKeyGenerator filesCacheKeyGenerator;
+
     @Value("${ovsx.storage.gcp.project-id:}")
     String projectId;
 
@@ -47,6 +50,10 @@ public class GoogleCloudStorageService implements IStorageService {
 
     private Storage storage;
 
+    public GoogleCloudStorageService(FilesCacheKeyGenerator filesCacheKeyGenerator) {
+        this.filesCacheKeyGenerator = filesCacheKeyGenerator;
+    }
+
     @Override
     public boolean isEnabled() {
         return !StringUtils.isEmpty(bucketId);
@@ -212,9 +219,12 @@ public Path getCachedFile(FileResource resource) throws IOException {
             throw new IllegalStateException(missingBucketIdMessage(resource.getName()));
         }
 
-        var path = Files.createTempFile("cached_file", null);
-        var objectId = getObjectId(resource);
-        getStorage().downloadTo(BlobId.of(bucketId, objectId), path);
+        var path = filesCacheKeyGenerator.generateCachedExtensionPath(resource);
+        if(!Files.exists(path)) {
+            var objectId = getObjectId(resource);
+            getStorage().downloadTo(BlobId.of(bucketId, objectId), path);
+        }
+
         return path;
     }
 }
diff --git a/server/src/main/resources/ehcache.xml b/server/src/main/resources/ehcache.xml
@@ -92,6 +92,7 @@
                 <events-to-fire-on>EXPIRED</events-to-fire-on>
                 <events-to-fire-on>EVICTED</events-to-fire-on>
                 <events-to-fire-on>REMOVED</events-to-fire-on>
+                <events-to-fire-on>UPDATED</events-to-fire-on>
             </listener>
         </listeners>
         <resources>
@@ -110,6 +111,7 @@
                 <events-to-fire-on>EXPIRED</events-to-fire-on>
                 <events-to-fire-on>EVICTED</events-to-fire-on>
                 <events-to-fire-on>REMOVED</events-to-fire-on>
+                <events-to-fire-on>UPDATED</events-to-fire-on>
             </listener>
         </listeners>
         <resources>

diff --git a/server/src/test/java/org/eclipse/openvsx/adapter/VSCodeAPITest.java b/server/src/test/java/org/eclipse/openvsx/adapter/VSCodeAPITest.java
@@ -892,8 +892,13 @@ TokenService tokenService(
         }
 
         @Bean
-        WebResourceService webResourceService(StorageUtilService storageUtil, RepositoryService repositories, CacheService cache) {
-            return new WebResourceService(storageUtil, repositories, cache);
+        WebResourceService webResourceService(
+                StorageUtilService storageUtil,
+                RepositoryService repositories,
+                CacheService cache,
+                FilesCacheKeyGenerator filesCacheKeyGenerator
+        ) {
+            return new WebResourceService(storageUtil, repositories, cache, filesCacheKeyGenerator);
         }
 
         @Bean

diff --git a/server/src/test/java/org/eclipse/openvsx/storage/AzureBlobStorageServiceTest.java b/server/src/test/java/org/eclipse/openvsx/storage/AzureBlobStorageServiceTest.java
@@ -13,6 +13,7 @@
 
 import java.net.URI;
 
+import org.eclipse.openvsx.cache.FilesCacheKeyGenerator;
 import org.eclipse.openvsx.entities.Extension;
 import org.eclipse.openvsx.entities.ExtensionVersion;
 import org.eclipse.openvsx.entities.FileResource;
@@ -60,8 +61,13 @@ void testGetLocation() {
     @TestConfiguration
     static class TestConfig {
         @Bean
-        AzureBlobStorageService azureBlobStorageService() {
-            return new AzureBlobStorageService();
+        FilesCacheKeyGenerator filesCacheKeyGenerator() {
+            return new FilesCacheKeyGenerator();
+        }
+
+        @Bean
+        AzureBlobStorageService azureBlobStorageService(FilesCacheKeyGenerator filesCacheKeyGenerator) {
+            return new AzureBlobStorageService(filesCacheKeyGenerator);
         }
     }
 }