Honour cache versioning Caches are versioned and their values should only be considered valid if they were stored by a cache having the same version of the one retrieving the value. Bug: Issue 13457 Change-Id: I7ee65fa21719dca143512be528387bdeed2c1ea3

diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
index f3f59cf..071bada 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java

@@ -42,6 +42,7 @@
   private final Duration expireAfterWrite;
   private final Duration refreshAfterWrite;
   private final int maxBloatFactor;
+  private final int version;
 
   public interface Factory {
     ChronicleMapCacheConfig create(
@@ -49,7 +50,8 @@
         @Assisted("ConfigKey") String configKey,
         @Assisted("DiskLimit") long diskLimit,
         @Nullable @Assisted("ExpireAfterWrite") Duration expireAfterWrite,
-        @Nullable @Assisted("RefreshAfterWrite") Duration refreshAfterWrite);
+        @Nullable @Assisted("RefreshAfterWrite") Duration refreshAfterWrite,
+        int version);
   }
 
   @AssistedInject
@@ -60,11 +62,15 @@
       @Assisted("ConfigKey") String configKey,
       @Assisted("DiskLimit") long diskLimit,
       @Nullable @Assisted("ExpireAfterWrite") Duration expireAfterWrite,
-      @Nullable @Assisted("RefreshAfterWrite") Duration refreshAfterWrite)
+      @Nullable @Assisted("RefreshAfterWrite") Duration refreshAfterWrite,
+      @Assisted int version)
       throws IOException {
+    this.version = version;
     final Path cacheDir = getCacheDir(site, cfg.getString("cache", null, "directory"));
     this.persistedFile =
-        cacheDir != null ? cacheDir.resolve(String.format("%s.dat", name)).toFile() : null;
+        cacheDir != null
+            ? cacheDir.resolve(String.format("%s_%s.dat", name, version)).toFile()
+            : null;
     this.diskLimit = cfg.getLong("cache", configKey, "diskLimit", diskLimit);
 
     this.maxEntries =
@@ -91,6 +97,10 @@
         cfg.getInt("cache", configKey, "maxBloatFactor", Defaults.maxBloatFactorFor(configKey));
   }
 
+  public int getVersion() {
+    return version;
+  }
+
   public Duration getExpireAfterWrite() {
     return expireAfterWrite;
   }

diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheFactory.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheFactory.java
index 929c1c6..bf5ca56 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheFactory.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheFactory.java

@@ -70,7 +70,8 @@
             in.configKey(),
             in.diskLimit(),
             in.expireAfterWrite(),
-            in.refreshAfterWrite());
+            in.refreshAfterWrite(),
+            in.version());
     ChronicleMapCacheImpl<K, V> cache = null;
     try {
       cache = new ChronicleMapCacheImpl<>(in, config, null);
@@ -93,7 +94,8 @@
             in.configKey(),
             in.diskLimit(),
             in.expireAfterWrite(),
-            in.refreshAfterWrite());
+            in.refreshAfterWrite(),
+            in.version());
     ChronicleMapCacheImpl<K, V> cache = null;
     try {
       cache = new ChronicleMapCacheImpl<>(in, config, loader);

diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheImpl.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheImpl.java
index 59f9f5a..c03b1ac 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheImpl.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheImpl.java

@@ -85,10 +85,11 @@
     }
 
     logger.atInfo().log(
-        "Initialized '%s'|avgKeySize: %s bytes|avgValueSize: %s bytes|"
-            + "entries: %s|maxBloatFactor: %s|remainingAutoResizes: %s|"
-            + "percentageFreeSpace: %s",
+        "Initialized '%s'|version: %s|avgKeySize: %s bytes|avgValueSize:"
+            + " %s bytes|entries: %s|maxBloatFactor: %s|remainingAutoResizes:"
+            + " %s|percentageFreeSpace: %s",
         def.name(),
+        config.getVersion(),
         mapBuilder.constantlySizedKeys() ? "CONSTANT" : config.getAverageKeySize(),
         config.getAverageValueSize(),
         config.getMaxEntries(),

diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java
index 735d23b..febff2f 100644
--- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java

@@ -38,6 +38,7 @@
   private final String cacheName = "foobar-cache";
   private final String cacheKey = "foobar-cache-key";
   private final long definitionDiskLimit = 100;
+  private final int version = 1;
   private final Duration expireAfterWrite = Duration.ofSeconds(10_000);
   private final Duration refreshAfterWrite = Duration.ofSeconds(20_000);
 
@@ -204,6 +205,7 @@
         cacheKey,
         definitionDiskLimit,
         expireAfterWrite,
-        refreshAfterWrite);
+        refreshAfterWrite,
+        version);
   }
 }

diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java
index 6abf49d..bf0317d 100644
--- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java

@@ -60,6 +60,19 @@
   }
 
   @Test
+  public void getIfPresentShouldReturnNullWhenThereCacheHasADifferentVersion() throws Exception {
+    gerritConfig.setString("cache", null, "directory", "cache");
+    gerritConfig.save();
+    final ChronicleMapCacheImpl<String, String> cacheV1 = newCacheVersion(1);
+
+    cacheV1.put("foo", "value version 1");
+    cacheV1.close();
+
+    final ChronicleMapCacheImpl<String, String> cacheV2 = newCacheVersion(2);
+    assertThat(cacheV2.getIfPresent("foo")).isNull();
+  }
+
+  @Test
   public void getWithLoaderShouldPopulateTheCache() throws Exception {
     String cachedValue = UUID.randomUUID().toString();
     final ChronicleMapCacheImpl<String, String> cache = newCacheWithLoader();
@@ -77,6 +90,38 @@
   }
 
   @Test
+  public void getShouldRetrieveANewValueWhenCacheHasADifferentVersion() throws Exception {
+    gerritConfig.setString("cache", null, "directory", "cache");
+    gerritConfig.save();
+    final ChronicleMapCacheImpl<String, String> cacheV1 = newCacheVersion(1);
+
+    cacheV1.put("foo", "value version 1");
+    cacheV1.close();
+
+    final ChronicleMapCacheImpl<String, String> cacheV2 = newCacheVersion(2);
+
+    final String v2Value = "value version 2";
+    assertThat(cacheV2.get("foo", () -> v2Value)).isEqualTo(v2Value);
+  }
+
+  @Test
+  public void getShouldRetrieveCachedValueWhenCacheHasSameVersion() throws Exception {
+    int cacheVersion = 2;
+    gerritConfig.setString("cache", null, "directory", "cache");
+    gerritConfig.save();
+    final ChronicleMapCacheImpl<String, String> cache = newCacheVersion(cacheVersion);
+
+    final String originalValue = "value 1";
+    cache.put("foo", originalValue);
+    cache.close();
+
+    final ChronicleMapCacheImpl<String, String> newCache = newCacheVersion(cacheVersion);
+
+    final String newValue = "value 2";
+    assertThat(newCache.get("foo", () -> newValue)).isEqualTo(originalValue);
+  }
+
+  @Test
   public void getShoudThrowWhenNoLoaderHasBeenProvided() throws Exception {
     final ChronicleMapCacheImpl<String, String> cache = newCacheWithoutLoader();
 
@@ -164,7 +209,8 @@
 
   @Test
   public void getIfPresentShouldReturnNullWhenValueIsExpired() throws Exception {
-    ChronicleMapCacheImpl<String, String> cache = newCache(true, null, Duration.ofSeconds(1), null);
+    ChronicleMapCacheImpl<String, String> cache =
+        newCache(true, null, Duration.ofSeconds(1), null, 1);
     cache.put("foo", "some-stale-value");
     Thread.sleep(1010); // Allow cache entry to expire
     assertThat(cache.getIfPresent("foo")).isNull();
@@ -174,7 +220,7 @@
   public void getShouldRefreshValueWhenExpired() throws Exception {
     String newCachedValue = UUID.randomUUID().toString();
     ChronicleMapCacheImpl<String, String> cache =
-        newCache(true, newCachedValue, null, Duration.ofSeconds(1));
+        newCache(true, newCachedValue, null, Duration.ofSeconds(1), 1);
     cache.put("foo", "some-stale-value");
     Thread.sleep(1010); // Allow cache to be flagged as needing refresh
     assertThat(cache.get("foo")).isEqualTo(newCachedValue);
@@ -182,7 +228,8 @@
 
   @Test
   public void shouldPruneExpiredValues() throws Exception {
-    ChronicleMapCacheImpl<String, String> cache = newCache(true, null, Duration.ofSeconds(1), null);
+    ChronicleMapCacheImpl<String, String> cache =
+        newCache(true, null, Duration.ofSeconds(1), null, 1);
     cache.put("foo1", "some-stale-value1");
     cache.put("foo2", "some-stale-value1");
     Thread.sleep(1010); // Allow cache entries to expire
@@ -219,7 +266,8 @@
       Boolean withLoader,
       @Nullable String cachedValue,
       @Nullable Duration expireAfterWrite,
-      @Nullable Duration refreshAfterWrite)
+      @Nullable Duration refreshAfterWrite,
+      Integer version)
       throws IOException {
     TestPersistentCacheDef cacheDef = new TestPersistentCacheDef(cachedValue);
 
@@ -231,22 +279,27 @@
             cacheDef.configKey(),
             cacheDef.diskLimit(),
             expireAfterWrite != null ? expireAfterWrite : Duration.ZERO,
-            refreshAfterWrite != null ? refreshAfterWrite : Duration.ZERO);
+            refreshAfterWrite != null ? refreshAfterWrite : Duration.ZERO,
+            version);
 
     return new ChronicleMapCacheImpl<>(cacheDef, config, withLoader ? cacheDef.loader() : null);
   }
 
   private ChronicleMapCacheImpl<String, String> newCacheWithLoader(@Nullable String cachedValue)
       throws IOException {
-    return newCache(true, cachedValue, null, null);
+    return newCache(true, cachedValue, null, null, 1);
   }
 
   private ChronicleMapCacheImpl<String, String> newCacheWithLoader() throws IOException {
-    return newCache(true, null, null, null);
+    return newCache(true, null, null, null, 1);
+  }
+
+  private ChronicleMapCacheImpl<String, String> newCacheVersion(int version) throws IOException {
+    return newCache(true, null, null, null, version);
   }
 
   private ChronicleMapCacheImpl<String, String> newCacheWithoutLoader() throws IOException {
-    return newCache(false, null, null, null);
+    return newCache(false, null, null, null, 1);
   }
 
   public static class TestPersistentCacheDef implements PersistentCacheDef<String, String> {