Merge pull request #923 from modelix/fix/do-not-cache-null-values-in-…

…PrefetchCache fix(model-datastructure): do not cache `null` values in `PrefetchCache`
modelix · Jul 22, 2024 · 8b6dfef · 8b6dfef
2 parents 784de9c + af34016
commit 8b6dfef
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 9 deletions.
diff --git a/model-datastructure/src/commonMain/kotlin/org/modelix/model/lazy/PrefetchCache.kt b/model-datastructure/src/commonMain/kotlin/org/modelix/model/lazy/PrefetchCache.kt
@@ -43,7 +43,9 @@ class PrefetchCache(private val store: IDeserializingKeyValueStore) : IDeseriali
             entries[hash] as T?
         } else {
             val value = if (ifCached) store.getIfCached(hash, deserializer, isPrefetch) else store.get(hash, deserializer)
-            entries[hash] = value
+            if (value != null) {
+                entries[hash] = value
+            }
             value
         }
     }
@@ -64,7 +66,11 @@ class PrefetchCache(private val store: IDeserializingKeyValueStore) : IDeseriali
         val missingRegular = regular.filterNot { entries.containsKey(it.getHash()) }
         val missingPrefetch = prefetch.filterNot { entries.containsKey(it.getHash()) }
         val missingEntries = store.getAll(missingRegular, missingPrefetch)
-        entries.putAll(missingEntries)
+        for ((key, value) in missingEntries) {
+            if (value != null) {
+                entries[key] = value
+            }
+        }
         val regularAndPrefetch = regular.asSequence() + prefetch.asSequence()
         return regularAndPrefetch.associate { it.getHash() to entries[it.getHash()] as T? }
     }

diff --git a/model-datastructure/src/commonTest/kotlin/org/modelix/model/lazy/PrefetchCacheTest.kt b/model-datastructure/src/commonTest/kotlin/org/modelix/model/lazy/PrefetchCacheTest.kt
@@ -1,5 +1,6 @@
 import org.modelix.model.lazy.AccessTrackingStore
 import org.modelix.model.lazy.NonCachingObjectStore
+import org.modelix.model.lazy.ObjectStoreCache
 import org.modelix.model.lazy.PrefetchCache
 import org.modelix.model.lazy.WrittenEntry
 import org.modelix.model.persistent.CPNode
@@ -11,24 +12,39 @@ import kotlin.test.assertTrue
 class PrefetchCacheTest {
 
     private val keyValueStore = MapBasedStore()
-    private val accessTrackingKeyValueStore = AccessTrackingStore(keyValueStore)
-    private val deserializingKeyValueStore = NonCachingObjectStore(accessTrackingKeyValueStore)
-    private val prefetchCache = PrefetchCache(deserializingKeyValueStore)
 
     @Test
-    fun entriesAreCachedAfterGettingMultipleEntriesAsIterable() {
+    fun nullValueIsNotCached() {
+        val deserializingKeyValueStore = NonCachingObjectStore(keyValueStore)
+        val prefetchCache = PrefetchCache(deserializingKeyValueStore)
+        prefetchCache["key", { value -> value }]
+        keyValueStore.put("key", "value")
+
+        val result = prefetchCache["key", { value -> value }]
+
+        assertEquals("value", result)
+    }
+
+    @Test
+    fun valuesAreCachedWhenGettingMultipleEntriesAsIterable() {
+        val accessTrackingKeyValueStore = AccessTrackingStore(keyValueStore)
+        val deserializingKeyValueStore = NonCachingObjectStore(accessTrackingKeyValueStore)
+        val prefetchCache = PrefetchCache(deserializingKeyValueStore)
         keyValueStore.put("key", "value")
         prefetchCache.getAll(listOf("key")) { _, value -> value }
         accessTrackingKeyValueStore.accessedEntries.clear()
 
         val result = prefetchCache.getAll(listOf("key")) { _, value -> value }
 
-        assertEquals(result, listOf("value"))
+        assertEquals(listOf("value"), result)
         assertTrue(accessTrackingKeyValueStore.accessedEntries.isEmpty())
     }
 
     @Test
-    fun entriesAreCachedAfterGettingMultipleEntriesAsMap() {
+    fun valuesAreCachedWhenGettingMultipleEntriesAsMap() {
+        val accessTrackingKeyValueStore = AccessTrackingStore(keyValueStore)
+        val deserializingKeyValueStore = NonCachingObjectStore(accessTrackingKeyValueStore)
+        val prefetchCache = PrefetchCache(deserializingKeyValueStore)
         val regularKey = "regularKey"
         val prefetchKey = "prefetchKey"
         val nodeForRegularKey = CPNode.create(
@@ -47,7 +63,31 @@ class PrefetchCacheTest {
 
         val result = prefetchCache.getAll(listOf(regularKeyReference), listOf(prefetchKeyReference))
 
-        assertEquals(result, mapOf(regularKey to nodeForRegularKey, prefetchKey to nodeForPrefetchKey))
+        assertEquals(mapOf(regularKey to nodeForRegularKey, prefetchKey to nodeForPrefetchKey), result)
         assertTrue(accessTrackingKeyValueStore.accessedEntries.isEmpty())
     }
+
+    @Test
+    fun nullValuesAreNotCachedWhenGettingMultipleEntriesAsMap() {
+        val deserializingKeyValueStore = ObjectStoreCache(keyValueStore)
+        val prefetchCache = PrefetchCache(deserializingKeyValueStore)
+        val regularKey = "regularKey"
+        val prefetchKey = "prefetchKey"
+        val nodeForRegularKey = CPNode.create(
+            2, null, 1, null, LongArray(0),
+            emptyArray(), emptyArray(), emptyArray(), emptyArray(),
+        )
+        val nodeForPrefetchKey = CPNode.create(
+            3, null, 1, null, LongArray(0),
+            emptyArray(), emptyArray(), emptyArray(), emptyArray(),
+        )
+        val regularKeyReference = WrittenEntry(regularKey) { nodeForRegularKey }
+        val prefetchKeyReference = WrittenEntry(prefetchKey) { nodeForPrefetchKey }
+        prefetchCache.getAll(listOf(regularKeyReference), listOf(prefetchKeyReference))
+        keyValueStore.putAll(mapOf(regularKey to nodeForRegularKey.serialize(), prefetchKey to nodeForPrefetchKey.serialize()))
+
+        val result = prefetchCache.getAll(listOf(regularKeyReference), listOf(prefetchKeyReference))
+
+        assertEquals(mapOf(regularKey to nodeForRegularKey, prefetchKey to nodeForPrefetchKey), result)
+    }
 }