fix(model-server): remove repository data after deletion

Author: mhuster23

model-server/src/main/kotlin/org/modelix/model/server/handlers/RepositoriesManager.kt

@@ -226,6 +226,8 @@ class RepositoriesManager(val client: LocalModelClient) : IRepositoriesManager {
     }
 
     override suspend fun removeRepository(repository: RepositoryId): Boolean {
+        val genericStore = store.getGenericStore()
+
         return store.runTransactionSuspendable {
             if (!repositoryExists(repository)) {
                 return@runTransactionSuspendable false
@@ -234,12 +236,12 @@ class RepositoriesManager(val client: LocalModelClient) : IRepositoriesManager {
             for (branchName in getBranchNames(repository)) {
                 putVersionHash(repository.getBranchReference(branchName), null)
             }
-            store.getGenericStore().put(branchListKey(repository), null)
+            genericStore.put(branchListKey(repository), null)
             val isolated = checkNotNull(isIsolated(repository)) { "Repository not found: $repository" }
             val existingRepositories = getRepositories(isolated)
             val remainingRepositories = existingRepositories - repository
             store.put(repositoriesListKey(isolated), remainingRepositories.joinToString("\n") { it.id })
-
+            genericStore.removeRepositoryObjects(repository)
             true
         }
     }

model-server/src/main/kotlin/org/modelix/model/server/store/IGenericStoreClient.kt

@@ -17,8 +17,27 @@
 package org.modelix.model.server.store
 
 import org.modelix.model.IGenericKeyListener
+import org.modelix.model.lazy.RepositoryId
 
-typealias IsolatingStore = IGenericStoreClient<ObjectInRepository>
+/**
+ * A store that saves data on a per-repository basis.
+ * The primary key is of type [ObjectInRepository].
+ */
+interface IsolatingStore : IGenericStoreClient<ObjectInRepository> {
+    /**
+     * Default implementation for removing repository objects.
+     * May be overridden by more efficient, store-specific implementations.
+     *
+     * Callers need to ensure that the repository is not usable anymore before calling this method.
+     */
+    fun removeRepositoryObjects(repositoryId: RepositoryId) {
+        val keysToDelete = getAll().asSequence()
+            .map { it.key }
+            .filter { it.getRepositoryId() == repositoryId.id }
+            .toSet()
+        putAll(keysToDelete.associateWith { null })
+    }
+}
 
 interface IGenericStoreClient<KeyT> : AutoCloseable {
     operator fun get(key: KeyT): String? = getAll(listOf(key)).first()

model-server/src/main/kotlin/org/modelix/model/server/store/InMemoryStoreClient.kt

@@ -103,7 +103,14 @@ class InMemoryStoreClient : IsolatingStore {
                 try {
                     transactionValues = HashMap()
                     val result = body()
-                    values.putAll(transactionValues!!)
+                    val tValues = requireNotNull(transactionValues) { "Passed lambda set 'transactionValues' to null unexpectedly." }
+
+                    val puts = tValues.filterValues { it != null }
+                    val deletes = tValues.filterValues { it == null }.keys
+                    values.putAll(puts)
+                    for (keyToDelete in deletes) {
+                        values.remove(keyToDelete)
+                    }
                     result
                 } finally {
                     transactionValues = null

model-server/src/test/kotlin/org/modelix/model/server/handlers/RepositoriesManagerTest.kt

@@ -0,0 +1,59 @@
+package org.modelix.model.server.handlers
+
+import kotlinx.coroutines.test.runTest
+import org.modelix.model.lazy.RepositoryId
+import org.modelix.model.server.store.IgniteStoreClient
+import org.modelix.model.server.store.InMemoryStoreClient
+import org.modelix.model.server.store.IsolatingStore
+import org.modelix.model.server.store.LocalModelClient
+import kotlin.test.AfterTest
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertTrue
+
+class RepositoriesManagerWithInMemoryStoreClientTest : RepositoriesManagerTest(InMemoryStoreClient())
+class RepositoriesManagerWithIgniteStoreClientTest : RepositoriesManagerTest(IgniteStoreClient(inmemory = true))
+
+abstract class RepositoriesManagerTest(val store: IsolatingStore) {
+    protected val repoManager = RepositoriesManager(LocalModelClient(store))
+    protected suspend fun initRepository(repoId: RepositoryId) {
+        repoManager.createRepository(repoId, "testUser", useRoleIds = true, legacyGlobalStorage = false)
+    }
+
+    @AfterTest
+    fun cleanup() {
+        store.close()
+    }
+
+    @Test
+    fun `repository data is removed when removing repository`() = runTest {
+        val repoId = RepositoryId("abc")
+        initRepository(repoId)
+
+        fun getRepositoryData() = store.getAll().filterKeys { it.getRepositoryId() == repoId.id }
+
+        val dataBeforeDeletion = getRepositoryData()
+        repoManager.removeRepository(repoId)
+        val dataAfterDeletion = getRepositoryData()
+
+        assertTrue(dataBeforeDeletion.isNotEmpty(), "Expected data to be present before deletion.")
+        assertTrue(dataAfterDeletion.isEmpty(), "Leftover repository data was found after deletion. entries=${dataAfterDeletion.size}")
+    }
+
+    @Test
+    fun `data of other repositories remains intact when removing a repository`() = runTest {
+        val existingRepo = RepositoryId("existing")
+        val toBeDeletedRepo = RepositoryId("tobedeleted")
+        initRepository(existingRepo)
+        initRepository(toBeDeletedRepo)
+
+        fun getExistingRepositoryData() = store.getAll().filterKeys { it.getRepositoryId() == existingRepo.id }
+
+        val dataBeforeDeletion = getExistingRepositoryData()
+        repoManager.removeRepository(toBeDeletedRepo)
+        val dataAfterDeletion = getExistingRepositoryData()
+
+        assertTrue(dataBeforeDeletion.isNotEmpty(), "Expected repository data was not found.")
+        assertEquals(dataBeforeDeletion, dataAfterDeletion, "Unexpected change in repository data.")
+    }
+}