From 12e6fe78e0e82e7141481c02cb342da69b0665bd Mon Sep 17 00:00:00 2001 From: James Ward Date: Tue, 23 Apr 2024 06:21:05 -0600 Subject: [PATCH 1/4] use computeIfAbsent and only cache version - for #8 --- src/main/java/org/webjars/WebJarCache.java | 7 +-- .../java/org/webjars/WebJarCacheDefault.java | 10 ++-- .../org/webjars/WebJarVersionLocator.java | 50 +++++++------------ .../org/webjars/WebJarVersionLocatorTest.java | 44 +++++++++++++--- 4 files changed, 61 insertions(+), 50 deletions(-) diff --git a/src/main/java/org/webjars/WebJarCache.java b/src/main/java/org/webjars/WebJarCache.java index 9c15d25..c9b44fc 100644 --- a/src/main/java/org/webjars/WebJarCache.java +++ b/src/main/java/org/webjars/WebJarCache.java @@ -3,6 +3,8 @@ import org.jspecify.annotations.Nullable; +import java.util.function.Function; + /** * WebJar Locator Cache Interface * Since classpath resources are essentially immutable, the WebJarsCache does not have the concept of expiry. @@ -10,8 +12,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 so that we can cache the non-existence + @Nullable String computeIfAbsent(String key, Function function); } diff --git a/src/main/java/org/webjars/WebJarCacheDefault.java b/src/main/java/org/webjars/WebJarCacheDefault.java index 3029782..56d0c8c 100644 --- a/src/main/java/org/webjars/WebJarCacheDefault.java +++ b/src/main/java/org/webjars/WebJarCacheDefault.java @@ -3,6 +3,7 @@ import org.jspecify.annotations.Nullable; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; public class WebJarCacheDefault implements WebJarCache { @@ -13,13 +14,8 @@ public WebJarCacheDefault(ConcurrentHashMap 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 @Nullable String computeIfAbsent(String key, Function function) { + return cache.computeIfAbsent(key, function); } } diff --git a/src/main/java/org/webjars/WebJarVersionLocator.java b/src/main/java/org/webjars/WebJarVersionLocator.java index fd29012..d31cbb0 100644 --- a/src/main/java/org/webjars/WebJarVersionLocator.java +++ b/src/main/java/org/webjars/WebJarVersionLocator.java @@ -44,27 +44,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 +67,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 +94,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) { + return 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,13 +119,11 @@ 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; } if (version.contains("-")) { version = version.substring(0, version.indexOf("-")); if (hasResourcePath(webJarName, version)) { - cache.put(cacheKey, version); return version; } } @@ -142,10 +131,7 @@ public String version(final String webJarName) { } return null; - } - else { - return maybeCached; - } + }); } 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 index 498441b..204a178 100644 --- a/src/test/java/org/webjars/WebJarVersionLocatorTest.java +++ b/src/test/java/org/webjars/WebJarVersionLocatorTest.java @@ -3,9 +3,12 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import org.jspecify.annotations.Nullable; import org.junit.Test; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Function; public class WebJarVersionLocatorTest { @@ -41,19 +44,44 @@ public void full_path_exists_version_supplied() { @Test public void cache_is_populated_on_lookup() { - final ConcurrentHashMap cache = new ConcurrentHashMap<>(); - final WebJarVersionLocator webJarVersionLocator = new WebJarVersionLocator(new WebJarCacheDefault(cache)); + AtomicInteger numLookups = new AtomicInteger(0); + + class InspectableCache implements WebJarCache { + final ConcurrentHashMap cache = new ConcurrentHashMap<>(); + + @Override + public @Nullable String computeIfAbsent(String key, Function function) { + Function 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()); + + // todo: test that null lookups are also cached } } From f5d5f76dc5ec5345a8c6c6d9319be7361620266a Mon Sep 17 00:00:00 2001 From: James Ward Date: Tue, 23 Apr 2024 06:27:11 -0600 Subject: [PATCH 2/4] reduce interface to ConcurrentMap --- src/main/java/org/webjars/WebJarCacheDefault.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/webjars/WebJarCacheDefault.java b/src/main/java/org/webjars/WebJarCacheDefault.java index 56d0c8c..7e4ffff 100644 --- a/src/main/java/org/webjars/WebJarCacheDefault.java +++ b/src/main/java/org/webjars/WebJarCacheDefault.java @@ -2,14 +2,14 @@ import org.jspecify.annotations.Nullable; -import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.function.Function; public class WebJarCacheDefault implements WebJarCache { - final ConcurrentHashMap cache; + final ConcurrentMap cache; - public WebJarCacheDefault(ConcurrentHashMap cache) { + public WebJarCacheDefault(ConcurrentMap cache) { this.cache = cache; } From 735a6a70b3c8c373cdf616de39ebfc7ca45e1a2d Mon Sep 17 00:00:00 2001 From: James Ward Date: Tue, 23 Apr 2024 06:35:57 -0600 Subject: [PATCH 3/4] also cache missing lookups --- src/main/java/org/webjars/WebJarCache.java | 5 ++--- src/main/java/org/webjars/WebJarCacheDefault.java | 9 ++++----- .../java/org/webjars/WebJarVersionLocator.java | 11 +++++++---- .../java/org/webjars/WebJarVersionLocatorTest.java | 14 +++++++++----- 4 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/webjars/WebJarCache.java b/src/main/java/org/webjars/WebJarCache.java index c9b44fc..b3d8a71 100644 --- a/src/main/java/org/webjars/WebJarCache.java +++ b/src/main/java/org/webjars/WebJarCache.java @@ -1,8 +1,7 @@ package org.webjars; -import org.jspecify.annotations.Nullable; - +import java.util.Optional; import java.util.function.Function; /** @@ -13,6 +12,6 @@ public interface WebJarCache { // 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 so that we can cache the non-existence - @Nullable String computeIfAbsent(String key, Function function); + Optional computeIfAbsent(String key, Function> function); } diff --git a/src/main/java/org/webjars/WebJarCacheDefault.java b/src/main/java/org/webjars/WebJarCacheDefault.java index 7e4ffff..b5e09e9 100644 --- a/src/main/java/org/webjars/WebJarCacheDefault.java +++ b/src/main/java/org/webjars/WebJarCacheDefault.java @@ -1,20 +1,19 @@ package org.webjars; -import org.jspecify.annotations.Nullable; - +import java.util.Optional; import java.util.concurrent.ConcurrentMap; import java.util.function.Function; public class WebJarCacheDefault implements WebJarCache { - final ConcurrentMap cache; + final ConcurrentMap> cache; - public WebJarCacheDefault(ConcurrentMap cache) { + public WebJarCacheDefault(ConcurrentMap> cache) { this.cache = cache; } @Override - public @Nullable String computeIfAbsent(String key, Function function) { + public Optional computeIfAbsent(String key, Function> function) { return cache.computeIfAbsent(key, function); } diff --git a/src/main/java/org/webjars/WebJarVersionLocator.java b/src/main/java/org/webjars/WebJarVersionLocator.java index d31cbb0..29b245a 100644 --- 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; @@ -94,7 +95,7 @@ public String webJarVersion(final String webJarName) { @Nullable public String version(final String webJarName) { final String cacheKey = "version-" + webJarName; - return cache.computeIfAbsent(cacheKey, (key) -> { + final Optional 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); @@ -119,19 +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)) { - return version; + return Optional.of(version); } if (version.contains("-")) { version = version.substring(0, version.indexOf("-")); if (hasResourcePath(webJarName, version)) { - return version; + return Optional.of(version); } } } } - return null; + 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 index 204a178..718ab7f 100644 --- a/src/test/java/org/webjars/WebJarVersionLocatorTest.java +++ b/src/test/java/org/webjars/WebJarVersionLocatorTest.java @@ -3,9 +3,9 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; -import org.jspecify.annotations.Nullable; import org.junit.Test; +import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; @@ -47,11 +47,11 @@ public void cache_is_populated_on_lookup() { AtomicInteger numLookups = new AtomicInteger(0); class InspectableCache implements WebJarCache { - final ConcurrentHashMap cache = new ConcurrentHashMap<>(); + final ConcurrentHashMap> cache = new ConcurrentHashMap<>(); @Override - public @Nullable String computeIfAbsent(String key, Function function) { - Function inspectableFunction = function.andThen((value) -> { + public Optional computeIfAbsent(String key, Function> function) { + Function> inspectableFunction = function.andThen((value) -> { numLookups.incrementAndGet(); return value; }); @@ -82,6 +82,10 @@ class InspectableCache implements WebJarCache { assertEquals("3.1.1", webJarVersionLocator.version("bootswatch-yeti")); assertEquals(2, numLookups.get()); - // todo: test that null lookups are also cached + assertNull(webJarVersionLocator.version("asdf")); + assertEquals(3, numLookups.get()); + + assertNull(webJarVersionLocator.version("asdf")); + assertEquals(3, numLookups.get()); } } From eb8395766a5b7b302010be7f93771e0c0bb46d32 Mon Sep 17 00:00:00 2001 From: James Ward Date: Tue, 23 Apr 2024 09:51:23 -0600 Subject: [PATCH 4/4] ready 0.0.5 for testing --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 4afc5fd..13badf8 100644 --- a/pom.xml +++ b/pom.xml @@ -11,7 +11,7 @@ jar org.webjars webjars-locator-lite - 1.0.0-SNAPSHOT + 0.0.5-SNAPSHOT webjars-locator-lite WebJars Locator Lite 2012