Merge pull request #9 from webjars/cache-improvements

Cache improvements
webjars · Apr 23, 2024 · 2185a23 · 2185a23
2 parents d61eaf1 + eb83957
commit 2185a23
Show file tree

Hide file tree

Showing 5 changed files with 76 additions and 60 deletions.
diff --git a/pom.xml b/pom.xml
@@ -11,7 +11,7 @@
     <packaging>jar</packaging>
     <groupId>org.webjars</groupId>
     <artifactId>webjars-locator-lite</artifactId>
-    <version>1.0.0-SNAPSHOT</version>
+    <version>0.0.5-SNAPSHOT</version>
     <name>webjars-locator-lite</name>
     <description>WebJars Locator Lite</description>
     <inceptionYear>2012</inceptionYear>

diff --git a/src/main/java/org/webjars/WebJarCache.java b/src/main/java/org/webjars/WebJarCache.java
@@ -1,7 +1,8 @@
 package org.webjars;
 
 
-import org.jspecify.annotations.Nullable;
+import java.util.Optional;
+import java.util.function.Function;
 
 /**
  * WebJar Locator Cache Interface
@@ -10,8 +11,7 @@
  */
 public interface WebJarCache {
 
-    @Nullable String get(final String key);
-
-    void put(final String key, final String value);
+    // todo: null can't be cached but if the locator can't find something, it never will, so consider having the compute function return Optional<String> so that we can cache the non-existence
+    Optional<String> computeIfAbsent(String key, Function<String, Optional<String>> function);
 
 }
diff --git a/src/main/java/org/webjars/WebJarCacheDefault.java b/src/main/java/org/webjars/WebJarCacheDefault.java
@@ -1,25 +1,20 @@
 package org.webjars;
 
-import org.jspecify.annotations.Nullable;
-
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.Optional;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Function;
 
 public class WebJarCacheDefault implements WebJarCache {
 
-    final ConcurrentHashMap<String, String> cache;
+    final ConcurrentMap<String, Optional<String>> cache;
 
-    public WebJarCacheDefault(ConcurrentHashMap<String, String> cache) {
+    public WebJarCacheDefault(ConcurrentMap<String, Optional<String>> cache) {
         this.cache = cache;
     }
 
     @Override
-    public @Nullable String get(String key) {
-        return cache.get(key);
-    }
-
-    @Override
-    public void put(String key, String value) {
-        cache.put(key, value);
+    public Optional<String> computeIfAbsent(String key, Function<String, Optional<String>> function) {
+        return cache.computeIfAbsent(key, function);
     }
 
 }
diff --git a/src/main/java/org/webjars/WebJarVersionLocator.java b/src/main/java/org/webjars/WebJarVersionLocator.java
@@ -5,6 +5,7 @@
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.util.Optional;
 import java.util.Properties;
 import java.util.concurrent.ConcurrentHashMap;
 
@@ -44,27 +45,19 @@ public WebJarVersionLocator(WebJarCache cache) {
      */
     @Nullable
     public String fullPath(final String webJarName, final String exactPath) {
-        final String cacheKey = "fullpath-" + webJarName + "-" + exactPath;
-        final String maybeCached = cache.get(cacheKey);
-        if (maybeCached == null) {
-            final String version = version(webJarName);
-            String fullPath = String.format("%s/%s/%s", WEBJARS_PATH_PREFIX, webJarName, exactPath);
-            if (!isEmpty(version)) {
-                if (!exactPath.startsWith(version)) {
-                    fullPath = String.format("%s/%s/%s/%s", WEBJARS_PATH_PREFIX, webJarName, version, exactPath);
-                }
-            }
+        final String version = version(webJarName);
 
-            if (LOADER.getResource(fullPath) != null) {
-                cache.put(cacheKey, fullPath);
-                return fullPath;
+        if (!isEmpty(version)) {
+            // todo: not sure why we check this
+            if (!exactPath.startsWith(version)) {
+                 return String.format("%s/%s/%s/%s", WEBJARS_PATH_PREFIX, webJarName, version, exactPath);
+            }
+            else {
+                return String.format("%s/%s/%s", WEBJARS_PATH_PREFIX, webJarName, exactPath);
             }
-
-            return null;
-        }
-        else {
-            return maybeCached;
         }
+
+        return null;
     }
 
     /**
@@ -75,13 +68,13 @@ public String fullPath(final String webJarName, final String exactPath) {
      */
     @Nullable
     public String path(final String webJarName, final String exactPath) {
-        String maybeFullPath = fullPath(webJarName, exactPath);
-        if (maybeFullPath != null) {
-            return maybeFullPath.replace(WEBJARS_PATH_PREFIX + "/", "");
-        }
-        else {
-            return null;
+        final String version = version(webJarName);
+
+        if (!isEmpty(version)) {
+            return String.format("%s/%s/%s", webJarName, version, exactPath);
         }
+
+        return null;
     }
 
     /**
@@ -102,8 +95,7 @@ public String webJarVersion(final String webJarName) {
     @Nullable
     public String version(final String webJarName) {
         final String cacheKey = "version-" + webJarName;
-        final String maybeCached = cache.get(cacheKey);
-        if (maybeCached == null) {
+        final Optional<String> optionalVersion = cache.computeIfAbsent(cacheKey, (key) -> {
             InputStream resource = LOADER.getResourceAsStream(PROPERTIES_ROOT + NPM + webJarName + POM_PROPERTIES);
             if (resource == null) {
                 resource = LOADER.getResourceAsStream(PROPERTIES_ROOT + PLAIN + webJarName + POM_PROPERTIES);
@@ -128,24 +120,21 @@ public String version(final String webJarName) {
                 // Sometimes a webjar version is not the same as the Maven artifact version
                 if (version != null) {
                     if (hasResourcePath(webJarName, version)) {
-                        cache.put(cacheKey, version);
-                        return version;
+                        return Optional.of(version);
                     }
                     if (version.contains("-")) {
                         version = version.substring(0, version.indexOf("-"));
                         if (hasResourcePath(webJarName, version)) {
-                            cache.put(cacheKey, version);
-                            return version;
+                            return Optional.of(version);
                         }
                     }
                 }
             }
 
-            return null;
-        }
-        else {
-            return maybeCached;
-        }
+            return Optional.empty();
+        });
+
+        return optionalVersion.orElse(null);
     }
 
     private boolean hasResourcePath(final String webJarName, final String path) {

diff --git a/src/test/java/org/webjars/WebJarVersionLocatorTest.java b/src/test/java/org/webjars/WebJarVersionLocatorTest.java
@@ -5,7 +5,10 @@
 
 import org.junit.Test;
 
+import java.util.Optional;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Function;
 
 public class WebJarVersionLocatorTest {
 
@@ -41,19 +44,48 @@ public void full_path_exists_version_supplied() {
 
     @Test
     public void cache_is_populated_on_lookup() {
-        final ConcurrentHashMap<String, String> cache = new ConcurrentHashMap<>();
-        final WebJarVersionLocator webJarVersionLocator = new WebJarVersionLocator(new WebJarCacheDefault(cache));
+        AtomicInteger numLookups = new AtomicInteger(0);
+
+        class InspectableCache implements WebJarCache {
+            final ConcurrentHashMap<String, Optional<String>> cache = new ConcurrentHashMap<>();
+
+            @Override
+            public Optional<String> computeIfAbsent(String key, Function<String, Optional<String>> function) {
+                Function<String, Optional<String>> inspectableFunction = function.andThen((value) -> {
+                    numLookups.incrementAndGet();
+                    return value;
+                });
+                return cache.computeIfAbsent(key, inspectableFunction);
+            }
+        }
+
+        final WebJarVersionLocator webJarVersionLocator = new WebJarVersionLocator(new InspectableCache());
 
         assertEquals("3.1.1", webJarVersionLocator.version("bootstrap"));
-        assertEquals(1, cache.size());
+        assertEquals(1, numLookups.get());
         // should hit the cache and produce the same value
-        // todo: test that it was actually a cache hit
         assertEquals("3.1.1", webJarVersionLocator.version("bootstrap"));
+        assertEquals(1, numLookups.get());
 
+        // version is already cached so we shouldn't hit it again
         assertEquals(WebJarVersionLocator.WEBJARS_PATH_PREFIX + "/bootstrap/3.1.1/js/bootstrap.js", webJarVersionLocator.fullPath("bootstrap", "js/bootstrap.js"));
-        assertEquals(2, cache.size());
-        // should hit the cache and produce the same value
-        // todo: test that it was actually a cache hit
-        assertEquals(WebJarVersionLocator.WEBJARS_PATH_PREFIX + "/bootstrap/3.1.1/js/bootstrap.js", webJarVersionLocator.fullPath("bootstrap", "js/bootstrap.js"));
+        assertEquals(1, numLookups.get());
+
+        // make sure we don't hit the cache for another file in the already resolved WebJar
+        assertEquals(WebJarVersionLocator.WEBJARS_PATH_PREFIX + "/bootstrap/3.1.1/css/bootstrap.css", webJarVersionLocator.fullPath("bootstrap", "css/bootstrap.css"));
+        assertEquals(1, numLookups.get());
+
+        // another WebJar should hit the cache but only once
+        assertEquals("3.1.1", webJarVersionLocator.version("bootswatch-yeti"));
+        assertEquals(2, numLookups.get());
+
+        assertEquals("3.1.1", webJarVersionLocator.version("bootswatch-yeti"));
+        assertEquals(2, numLookups.get());
+
+        assertNull(webJarVersionLocator.version("asdf"));
+        assertEquals(3, numLookups.get());
+
+        assertNull(webJarVersionLocator.version("asdf"));
+        assertEquals(3, numLookups.get());
     }
 }