Merge pull request #714 from modelix/feature/delete-branches

languitar · web-flow · commit 48aa04bf3860 · 2024-04-25T14:51:37.000+02:00
feat(model-server): implement deleting branches via REST API
diff --git a/model-client/src/commonMain/kotlin/org/modelix/model/client2/IModelClientV2.kt b/model-client/src/commonMain/kotlin/org/modelix/model/client2/IModelClientV2.kt
@@ -44,6 +44,14 @@ interface IModelClientV2 {
     suspend fun deleteRepository(repository: RepositoryId): Boolean
     suspend fun listBranches(repository: RepositoryId): List<BranchReference>
 
+    /**
+     * Deletes a branch from a repository if it exists.
+     *
+     * @param branch the branch to delete
+     * @return true if the branch existed and could be deleted, else false.
+     */
+    suspend fun deleteBranch(branch: BranchReference): Boolean
+
     @Deprecated("repository ID is required for permission checks")
     @DeprecationInfo("3.7.0", "May be removed with the next major release. Also remove the endpoint from the model-server.")
     suspend fun loadVersion(versionHash: String, baseVersion: IVersion?): IVersion
diff --git a/model-client/src/commonMain/kotlin/org/modelix/model/client2/ModelClientV2.kt b/model-client/src/commonMain/kotlin/org/modelix/model/client2/ModelClientV2.kt
@@ -22,6 +22,7 @@ import io.ktor.client.plugins.ResponseException
 import io.ktor.client.plugins.contentnegotiation.ContentNegotiation
 import io.ktor.client.plugins.expectSuccess
 import io.ktor.client.request.HttpRequestBuilder
+import io.ktor.client.request.delete
 import io.ktor.client.request.get
 import io.ktor.client.request.post
 import io.ktor.client.request.prepareGet
@@ -168,6 +169,25 @@ class ModelClientV2(
         }.bodyAsText().lines().filter { it.isNotEmpty() }.map { repository.getBranchReference(it) }
     }
 
+    override suspend fun deleteBranch(branch: BranchReference): Boolean {
+        try {
+            return httpClient.delete {
+                url {
+                    takeFrom(baseUrl)
+                    appendPathSegmentsEncodingSlash(
+                        "repositories",
+                        branch.repositoryId.id,
+                        "branches",
+                        branch.branchName,
+                    )
+                }
+            }.status == HttpStatusCode.NoContent
+        } catch (ex: Exception) {
+            LOG.error(ex) { ex.message }
+            return false
+        }
+    }
+
     @Deprecated("repository ID is required for permission checks")
     @DeprecationInfo("3.7.0", "May be removed with the next major release. Also remove the endpoint from the model-server.")
     override suspend fun loadVersion(versionHash: String, baseVersion: IVersion?): IVersion {
diff --git a/model-server-openapi/specifications/model-server.yaml b/model-server-openapi/specifications/model-server.yaml
@@ -85,6 +85,24 @@ paths:
         "200":
           $ref: '#/components/responses/200'
   /v2/repositories/{repository}/branches/{branch}:
+    delete:
+      operationId: deleteRepositoryBranch
+      parameters:
+        - name: repository
+          in: "path"
+          required: true
+          schema:
+            type: string
+        - name: branch
+          in: "path"
+          required: true
+          schema:
+            type: string
+      responses:
+        "404":
+          $ref: '#/components/responses/404'
+        "204":
+          description: "Branch successfully deleted"
     get:
       operationId: getRepositoryBranch
       parameters:
diff --git a/model-server/src/main/kotlin/org/modelix/model/server/handlers/ModelReplicationServer.kt b/model-server/src/main/kotlin/org/modelix/model/server/handlers/ModelReplicationServer.kt
@@ -23,6 +23,7 @@ import io.ktor.server.plugins.origin
 import io.ktor.server.request.acceptItems
 import io.ktor.server.request.receive
 import io.ktor.server.request.receiveStream
+import io.ktor.server.resources.delete
 import io.ktor.server.resources.get
 import io.ktor.server.resources.post
 import io.ktor.server.resources.put
@@ -148,6 +149,37 @@ class ModelReplicationServer(
             call.respondDelta(versionHash, baseVersionHash)
         }
 
+        delete<Paths.deleteRepositoryBranch> {
+            val repositoryName = call.parameters["repository"]
+            if (repositoryName == null) {
+                call.respondText("Request lacks repository name", status = HttpStatusCode.BadRequest)
+                return@delete
+            }
+            val repositoryId = try {
+                RepositoryId(repositoryName)
+            } catch (e: IllegalArgumentException) {
+                call.respondText("Invalid repository name '$repositoryName'", status = HttpStatusCode.BadRequest)
+                return@delete
+            }
+            val branch = call.parameters["branch"]
+            if (branch == null) {
+                call.respondText("Request lacks branch name", status = HttpStatusCode.BadRequest)
+                return@delete
+            }
+
+            if (!repositoriesManager.getBranchNames(repositoryId).contains(branch)) {
+                call.respondText(
+                    "Repository does not exist or branch '$branch' does not exist in repository '$repositoryId'",
+                    status = HttpStatusCode.NotFound,
+                )
+                return@delete
+            }
+
+            repositoriesManager.removeBranches(repositoryId, setOf(branch))
+
+            call.respond(HttpStatusCode.NoContent)
+        }
+
         get<Paths.getRepositoryBranchHash> {
             fun ApplicationCall.repositoryId() = RepositoryId(parameters["repository"]!!)
             fun PipelineContext<Unit, ApplicationCall>.repositoryId() = call.repositoryId()
diff --git a/model-server/src/test/kotlin/org/modelix/model/server/ModelClientV2Test.kt b/model-server/src/test/kotlin/org/modelix/model/server/ModelClientV2Test.kt
@@ -24,6 +24,7 @@ import org.modelix.model.api.PBranch
 import org.modelix.model.client2.ModelClientV2
 import org.modelix.model.client2.runWrite
 import org.modelix.model.client2.runWriteOnBranch
+import org.modelix.model.lazy.BranchReference
 import org.modelix.model.lazy.CLTree
 import org.modelix.model.lazy.CLVersion
 import org.modelix.model.lazy.RepositoryId
@@ -165,6 +166,47 @@ class ModelClientV2Test {
         assertFalse(containsRepository)
     }
 
+    @Test
+    fun `branches from non-existing repositories cannot be removed`() = runTest {
+        val client = createModelClient()
+        val repositoryId = RepositoryId(UUID.randomUUID().toString())
+
+        val success = client.deleteBranch(BranchReference(repositoryId, "doesntmatter"))
+
+        assertFalse(success)
+    }
+
+    @Test
+    fun `non-existing branches from existing repositories cannot be removed`() = runTest {
+        val client = createModelClient()
+        val repositoryId = RepositoryId(UUID.randomUUID().toString())
+        client.initRepository(repositoryId)
+
+        val success = client.deleteBranch(BranchReference(repositoryId, "doesnotexist"))
+
+        assertFalse(success)
+    }
+
+    @Test
+    fun `existing branches from existing repositories can be removed`() = runTest {
+        val client = createModelClient()
+        val repositoryId = RepositoryId(UUID.randomUUID().toString())
+        client.initRepository(repositoryId)
+        val branchToDelete = BranchReference(repositoryId, "todelete")
+        client.push(
+            branchToDelete,
+            requireNotNull(
+                client.pullIfExists(BranchReference(repositoryId, "master")),
+            ) { "the master branch must always exist" },
+            null,
+        )
+
+        val success = client.deleteBranch(BranchReference(repositoryId, branchToDelete.branchName))
+
+        assertTrue(success)
+        assertFalse(client.listBranches(repositoryId).contains(branchToDelete))
+    }
+
     @Test
     fun `pulling existing versions pulls all referenced objects`() = runTest {
         // Arrange
diff --git a/model-server/src/test/kotlin/org/modelix/model/server/handlers/ModelReplicationServerTest.kt b/model-server/src/test/kotlin/org/modelix/model/server/handlers/ModelReplicationServerTest.kt
@@ -19,7 +19,9 @@ import io.ktor.client.HttpClient
 import io.ktor.client.engine.cio.CIO
 import io.ktor.client.plugins.HttpTimeout
 import io.ktor.client.plugins.defaultRequest
+import io.ktor.client.request.delete
 import io.ktor.client.request.get
+import io.ktor.client.statement.bodyAsText
 import io.ktor.http.HttpHeaders
 import io.ktor.http.HttpStatusCode
 import io.ktor.http.appendPathSegments
@@ -51,39 +53,52 @@ import org.modelix.model.server.store.InMemoryStoreClient
 import org.modelix.model.server.store.LocalModelClient
 import org.modelix.modelql.core.assertNotEmpty
 import kotlin.test.Test
+import kotlin.test.assertContains
 import kotlin.test.assertEquals
+import kotlin.test.assertFalse
 import kotlin.test.fail
 import kotlin.time.Duration.Companion.seconds
 
 class ModelReplicationServerTest {
 
-    private fun getDefaultModelReplicationServer(): ModelReplicationServer {
+    private data class Fixture(
+        val storeClient: InMemoryStoreClient,
+        val modelClient: LocalModelClient,
+        val repositoriesManager: IRepositoriesManager,
+        val modelReplicationServer: ModelReplicationServer,
+    )
+
+    private fun getDefaultModelReplicationServerFixture(): Fixture {
         val storeClient = InMemoryStoreClient()
         val modelClient = LocalModelClient(storeClient)
         val repositoriesManager = RepositoriesManager(modelClient)
-        return ModelReplicationServer(repositoriesManager, modelClient, InMemoryModels())
+        return Fixture(
+            storeClient,
+            modelClient,
+            repositoriesManager,
+            ModelReplicationServer(repositoriesManager, modelClient, InMemoryModels()),
+        )
     }
 
     private fun runWithTestModelServer(
-        modelReplicationServer: ModelReplicationServer = getDefaultModelReplicationServer(),
-        block: suspend ApplicationTestBuilder.(scope: CoroutineScope) -> Unit,
+        fixture: Fixture = getDefaultModelReplicationServerFixture(),
+        block: suspend ApplicationTestBuilder.(scope: CoroutineScope, fixture: Fixture) -> Unit,
     ) = testApplication {
         application {
             installAuthentication(unitTestMode = true)
             installDefaultServerPlugins()
-            modelReplicationServer.init(this)
+            fixture.modelReplicationServer.init(this)
         }
 
         coroutineScope {
-            block(this)
+            block(this, fixture)
         }
     }
 
     @Test
-    fun `pulling delta does not return objects twice`() = runWithTestModelServer {
+    fun `pulling delta does not return objects twice`() = runWithTestModelServer { _, _ ->
         // Arrange
-        val url = "http://localhost/v2"
-        val modelClient = ModelClientV2.builder().url(url).client(client).build().also { it.init() }
+        val modelClient = ModelClientV2.builder().url("v2").client(client).build().also { it.init() }
         val repositoryId = RepositoryId("repo1")
         val branchId = repositoryId.getBranchReference("my-branch")
         // By calling modelClient.runWrite twice, we create to versions.
@@ -98,8 +113,7 @@ class ModelReplicationServerTest {
         // Act
         val response = client.get {
             url {
-                takeFrom(url)
-                appendPathSegments("repositories", repositoryId.id, "branches", branchId.branchName)
+                appendPathSegments("v2", "repositories", repositoryId.id, "branches", branchId.branchName)
             }
             useVersionStreamFormat()
         }
@@ -117,6 +131,75 @@ class ModelReplicationServerTest {
         }
     }
 
+    @Test
+    fun `responds with 404 when deleting a branch from a non-existent repository`() {
+        runWithTestModelServer { _, _ ->
+            val response = client.delete {
+                url {
+                    appendPathSegments("v2", "repositories", "doesnotexist", "branches", "does not exist")
+                }
+            }
+
+            assertEquals(HttpStatusCode.NotFound, response.status)
+            assertContains(response.bodyAsText(), "does not exist in repository")
+        }
+    }
+
+    @Test
+    fun `responds with 404 when deleting a non-existent branch`() {
+        val repositoryId = RepositoryId("repo1")
+
+        runWithTestModelServer { _, fixture ->
+            fixture.repositoriesManager.createRepository(repositoryId, null)
+
+            val response = client.delete {
+                url {
+                    appendPathSegments("v2", "repositories", repositoryId.id, "branches", "does not exist")
+                }
+            }
+
+            assertEquals(HttpStatusCode.NotFound, response.status)
+            assertContains(response.bodyAsText(), "does not exist in repository")
+        }
+    }
+
+    @Test
+    fun `responds with 400 when deleting from an invalid repository ID`() {
+        runWithTestModelServer { _, fixture ->
+            val response = client.delete {
+                url {
+                    appendPathSegments("v2", "repositories", "invalid with spaces", "branches", "master")
+                }
+            }
+            assertEquals(HttpStatusCode.BadRequest, response.status)
+            assertContains(response.bodyAsText(), "Invalid repository name")
+        }
+    }
+
+    @Test
+    fun `successfully deletes existing branches`() {
+        val repositoryId = RepositoryId("repo1")
+        val branch = "testbranch"
+        val defaultBranchRef = repositoryId.getBranchReference("master")
+
+        runWithTestModelServer { _, fixture ->
+            fixture.repositoriesManager.createRepository(repositoryId, null)
+            fixture.repositoriesManager.mergeChanges(
+                repositoryId.getBranchReference(branch),
+                checkNotNull(fixture.repositoriesManager.getVersionHash(defaultBranchRef)) { "Default branch must exist" },
+            )
+
+            val response = client.delete {
+                url {
+                    appendPathSegments("v2", "repositories", repositoryId.id, "branches", branch)
+                }
+            }
+
+            assertEquals(HttpStatusCode.NoContent, response.status)
+            assertFalse(fixture.repositoriesManager.getBranchNames(repositoryId).contains(branch))
+        }
+    }
+
     @Test
     fun `server responds with error when failing to compute delta before starting to respond`() {
         // Arrange
@@ -130,18 +213,16 @@ class ModelReplicationServerTest {
             }
         }
         val modelReplicationServer = ModelReplicationServer(faultyRepositoriesManager, modelClient, InMemoryModels())
-        val url = "http://localhost/v2"
         val repositoryId = RepositoryId("repo1")
         val branchRef = repositoryId.getBranchReference()
 
-        runWithTestModelServer(modelReplicationServer) {
+        runWithTestModelServer(Fixture(storeClient, modelClient, faultyRepositoriesManager, modelReplicationServer)) { _, _ ->
             repositoriesManager.createRepository(repositoryId, null)
 
             // Act
             val response = client.get {
                 url {
-                    takeFrom(url)
-                    appendPathSegments("repositories", repositoryId.id, "branches", branchRef.branchName)
+                    appendPathSegments("v2", "repositories", repositoryId.id, "branches", branchRef.branchName)
                 }
                 useVersionStreamFormat()
             }
@@ -210,10 +291,10 @@ class ModelReplicationServerTest {
         runWithNettyServer(setupBlock, testBlock)
     }
 
-    fun `client can pull versions in legacy version delta format`() = runWithTestModelServer {
+    @Test
+    fun `client can pull versions in legacy version delta format`() = runWithTestModelServer { _, _ ->
         // Arrange
-        val url = "http://localhost/v2"
-        val modelClient = ModelClientV2.builder().url(url).client(client).build().also { it.init() }
+        val modelClient = ModelClientV2.builder().url("v2").client(client).build().also { it.init() }
         val repositoryId = RepositoryId("repo1")
         val branchId = repositoryId.getBranchReference("my-branch")
         modelClient.runWrite(branchId) { root ->
@@ -224,8 +305,7 @@ class ModelReplicationServerTest {
         val response = client.get {
             headers[HttpHeaders.Accept] = VersionDeltaStream.CONTENT_TYPE.toString()
             url {
-                takeFrom(url)
-                appendPathSegments("repositories", repositoryId.id, "branches", branchId.branchName)
+                appendPathSegments("v2", "repositories", repositoryId.id, "branches", branchId.branchName)
             }
         }
         val versionDelta = response.readVersionDelta()
