This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7580-befcf3f0813371c8dd351bc962b70c1bdc94f4b1 in repository https://gitbox.apache.org/repos/asf/texera.git
commit c7e7362a4022d41238227f47ec474dadd225150e Author: Eugene Gu <[email protected]> AuthorDate: Thu Aug 13 05:01:38 2026 +0000 test(computing-unit): extend ComputingUnitManagingResourceSpec to cover the create, rename and configuration endpoints (#7580) ### What changes were proposed in this PR? This PR extends `ComputingUnitManagingResourceSpec` (added in #6853, extended in #7337) to the endpoints of `ComputingUnitManagingResource` that had no coverage. The existing spec only covered `getComputingUnitInfo`, `getComputingUnitMetricsEndpoint`, `listComputingUnits` and `terminateComputingUnit`; the create, rename and configuration endpoints were untested. All new tests keep the existing spec's approach: local-type units driven against the embedded Postgres (`MockTexeraDB`), so no Kubernetes calls are made. New coverage, by endpoint: - **createWorkflowComputingUnit** — local happy path (persisted with a generated `cuid`, the user URI landing in both the `uri` column and the resource JSON's `nodeAddresses`, response reporting owner/WRITE/Running with NaN metrics); whitespace-only name rejected with `ForbiddenException` and nothing stored; unknown type (`quantum`) rejected; `kubernetes` type rejected while disabled; missing and blank URI rejected; the per-user running-unit quota not applying to local units. - **renameComputingUnit** — owner success; non-owner without access 403 (name kept); READ-only grantee 403; WRITE grantee success; blank name 400; nonexistent unit `NotFoundException`; database failure (name overflowing the VARCHAR(128) column) rolling back and keeping the name; and an admin who neither owns nor was granted access getting 403 (rename, unlike terminate, has no ADMIN bypass). - **terminateComputingUnit** — a WRITE grantee rejected with 400 and the unit not terminated (terminate requires strict ownership or the ADMIN role). - **getComputingUnitInfo** — a READ grantee sees the unit with `isOwner = false` and `accessPrivilege = READ`; a nonexistent unit yields `NotFoundException`; the owner response also reports the non-empty owner avatar. - **getComputingUnitTypes** — lists exactly `local` while Kubernetes is disabled. - **getComputingUnitLimitOptions** — returns the configured cpu/memory/gpu option lists. - **getComputingUnitResourceLimit** — the local branch returns NaN limits for the owner; a non-owner gets `BadRequestException`; a nonexistent unit yields `NotFoundException`. - **getComputingUnitMetricsEndpoint** — adds the missing negative direction: a non-owner gets `BadRequestException`. Note: the Kubernetes-only validation in `createWorkflowComputingUnit` sits behind the supported-type gate and is unreachable while `kubernetes.enabled` is false; that flag is a load-time val the test JVM does not override, so those branches cannot be exercised in this suite. A spec comment records this. No production code is changed. ### Any related issues, documentation, discussions? Closes #7576 ### How was this PR tested? This PR is itself test-only. The new specs were run with: ``` sbt "ComputingUnitManagingService/testOnly org.apache.texera.service.resource.ComputingUnitManagingResourceSpec" ``` All 31 tests pass (7 pre-existing + 24 new) against the embedded database; no external services are needed. The suite was mutation-checked: targeted mutations of the resource (removing the blank-name and missing-URI checks, inverting the rename ownership gate, skipping the rename blank-name 400, making `getComputingUnitTypes` also return `kubernetes`, and removing the non-owner check in `getComputingUnitResourceLimit`) each caused at least one new test to fail, and the source was restored afterwards. `ComputingUnitManagingService/Test/scalafmtCheck` passes. ### Was this PR authored or co-authored using generative AI tooling? Co-authored by: Claude Code (Claude Fable 5) --- .../ComputingUnitManagingResourceSpec.scala | 303 ++++++++++++++++++++- 1 file changed, 299 insertions(+), 4 deletions(-) diff --git a/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitManagingResourceSpec.scala b/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitManagingResourceSpec.scala index e1142a6346..5b7c64104a 100644 --- a/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitManagingResourceSpec.scala +++ b/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitManagingResourceSpec.scala @@ -19,21 +19,36 @@ package org.apache.texera.service.resource -import jakarta.ws.rs.NotFoundException +import jakarta.ws.rs.{BadRequestException, ForbiddenException, NotFoundException} import org.apache.texera.auth.SessionUser +import org.apache.texera.common.config.KubernetesConfig.maxNumOfRunningComputingUnitsPerUser import org.apache.texera.dao.MockTexeraDB import org.apache.texera.dao.jooq.generated.enums.{ PrivilegeEnum, UserRoleEnum, WorkflowComputingUnitTypeEnum } -import org.apache.texera.dao.jooq.generated.tables.daos.{UserDao, WorkflowComputingUnitDao} -import org.apache.texera.dao.jooq.generated.tables.pojos.{User, WorkflowComputingUnit} -import org.apache.texera.service.resource.ComputingUnitManagingResource.WorkflowComputingUnitMetrics +import org.apache.texera.dao.jooq.generated.tables.daos.{ + ComputingUnitUserAccessDao, + UserDao, + WorkflowComputingUnitDao +} +import org.apache.texera.dao.jooq.generated.tables.pojos.{ + ComputingUnitUserAccess, + User, + WorkflowComputingUnit +} +import org.apache.texera.service.resource.ComputingUnitManagingResource.{ + WorkflowComputingUnitCreationParams, + WorkflowComputingUnitMetrics, + WorkflowComputingUnitResourceLimit +} import org.scalatest.BeforeAndAfterAll import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers +import scala.jdk.CollectionConverters.CollectionHasAsScala + // Drives the per-user computing-unit endpoints against the embedded database using // local units (so no Kubernetes calls are made). class ComputingUnitManagingResourceSpec @@ -69,6 +84,10 @@ class ComputingUnitManagingResourceSpec new SessionUser(makeUser(900, "admin", UserRoleEnum.ADMIN)) private lazy val strangerUser: SessionUser = new SessionUser(makeUser(902, "stranger", UserRoleEnum.REGULAR)) + // Dedicated owner for the create tests, so the units they create never show up in + // listComputingUnits(user)'s exact-set assertion. + private lazy val creatorUser: SessionUser = + new SessionUser(makeUser(903, "creator", UserRoleEnum.REGULAR)) private def localUnit(cuid: Int, name: String): WorkflowComputingUnit = localUnitOwnedBy(cuid, uid, name) @@ -91,6 +110,43 @@ class ComputingUnitManagingResourceSpec .fetchOneByCuid(cuid) .getTerminateTime != null + private def unitName(cuid: Int): String = + new WorkflowComputingUnitDao(getDSLContext.configuration()) + .fetchOneByCuid(cuid) + .getName + + private def runningUnitCountOwnedBy(ownerUid: Int): Int = + new WorkflowComputingUnitDao(getDSLContext.configuration()) + .fetchByUid(ownerUid) + .asScala + .count(_.getTerminateTime == null) + + /** Inserts an access row directly, bypassing the grant endpoint, to set up the rename tests. */ + private def grantAccess(cuid: Int, granteeUid: Int, privilege: PrivilegeEnum): Unit = { + val access = new ComputingUnitUserAccess + access.setCuid(cuid) + access.setUid(granteeUid) + access.setPrivilege(privilege) + new ComputingUnitUserAccessDao(getDSLContext.configuration()).insert(access) + } + + // The cpu/memory/gpu/jvm/shm quantities are ignored for local units, so the create tests only + // vary the fields the local branch actually reads: name, unitType and uri. + private def localCreationParams( + name: String, + uri: Option[String] + ): WorkflowComputingUnitCreationParams = + WorkflowComputingUnitCreationParams( + name = name, + unitType = "local", + cpuLimit = "NaN", + memoryLimit = "NaN", + gpuLimit = "NaN", + jvmMemorySize = "NaN", + shmSize = "NaN", + uri = uri + ) + override protected def beforeAll(): Unit = { super.beforeAll() initializeDBAndReplaceDSLContext() @@ -99,6 +155,7 @@ class ComputingUnitManagingResourceSpec userDao.insert(adminUser.getUser) userDao.insert(makeUser(901, "victim", UserRoleEnum.REGULAR)) userDao.insert(strangerUser.getUser) + userDao.insert(creatorUser.getUser) val unitDao = new WorkflowComputingUnitDao(getDSLContext.configuration()) unitDao.insert(localUnit(800, "cu-a")) unitDao.insert(localUnit(801, "cu-b")) @@ -119,6 +176,23 @@ class ComputingUnitManagingResourceSpec info.isOwner shouldBe true info.accessPrivilege shouldBe PrivilegeEnum.WRITE info.ownerName shouldBe "owner" + info.ownerGoogleAvatar shouldBe "owner-avatar" + } + + it should "report READ access for a grantee holding READ access" in { + insertLocalUnit(cuid = 950, ownerUid = 901, name = "shared-info-cu") + grantAccess(950, granteeUid = 902, PrivilegeEnum.READ) + + val info = resource.getComputingUnitInfo(950, strangerUser) + + info.computingUnit.getCuid.intValue() shouldBe 950 + info.isOwner shouldBe false + info.accessPrivilege shouldBe PrivilegeEnum.READ + info.ownerName shouldBe "victim" + } + + it should "throw NotFoundException for a nonexistent unit" in { + a[NotFoundException] should be thrownBy resource.getComputingUnitInfo(99999, user) } "getComputingUnitMetricsEndpoint" should "return NaN metrics for an owned local unit" in { @@ -126,6 +200,11 @@ class ComputingUnitManagingResourceSpec WorkflowComputingUnitMetrics("NaN", "NaN") } + it should "reject a non-owner with BadRequestException" in { + a[BadRequestException] should be thrownBy + resource.getComputingUnitMetricsEndpoint("800", strangerUser) + } + "listComputingUnits" should "return the caller's owned, non-terminated units" in { val result = resource.listComputingUnits(user) @@ -162,7 +241,223 @@ class ComputingUnitManagingResourceSpec isTerminated(912) shouldBe true } + it should "reject a WRITE grantee with 400 and not terminate the unit" in { + // Unlike rename, terminate requires strict ownership (or the ADMIN role), so + // even a WRITE grantee is rejected. + insertLocalUnit(cuid = 913, ownerUid = 901, name = "grantee-cannot-terminate") + grantAccess(913, granteeUid = 902, PrivilegeEnum.WRITE) + + val response = resource.terminateComputingUnit(913, strangerUser) + + response.getStatus shouldBe 400 + isTerminated(913) shouldBe false + } + it should "return 404 when an admin terminates a nonexistent unit" in { a[NotFoundException] should be thrownBy resource.terminateComputingUnit(99999, adminUser) } + + // The kubernetes type is disabled in the test JVM (kubernetes.conf's enabled flag defaults to + // false and is a load-time val), so createWorkflowComputingUnit rejects it at the supported-type + // gate. The kubernetes-only validation behind that gate (cpu/memory/gpu limit options, shm + // parsing and bounds, JVM-heap bound, and the running-unit quota throw) is unreachable here. + + "createWorkflowComputingUnit" should "create a local unit and report it as owned, Running and writable" in { + val uri = "http://localhost:8085"; + + val created = + resource.createWorkflowComputingUnit(localCreationParams("local-cu", Some(uri)), creatorUser) + + val cuid = created.computingUnit.getCuid + cuid.intValue() should be > 0 + created.computingUnit.getUid.intValue() shouldBe 903 + created.computingUnit.getName shouldBe "local-cu" + created.computingUnit.getType shouldBe WorkflowComputingUnitTypeEnum.local + created.computingUnit.getUri shouldBe uri + created.computingUnit.getResource should include(s""""nodeAddresses":["$uri"]""") + created.computingUnit.getResource should include(""""cpuLimit":"NaN"""") + created.status shouldBe "Running" + created.metrics shouldBe WorkflowComputingUnitMetrics("NaN", "NaN") + created.isOwner shouldBe true + created.accessPrivilege shouldBe PrivilegeEnum.WRITE + created.ownerName shouldBe "creator" + // makeUser sets no avatar, so the owner-avatar lookup resolves to null + created.ownerGoogleAvatar shouldBe null + + // The unit is persisted, not just echoed back + unitName(cuid) shouldBe "local-cu" + } + + it should "reject a whitespace-only name with ForbiddenException and store nothing" in { + val before = runningUnitCountOwnedBy(903) + + val ex = the[ForbiddenException] thrownBy + resource.createWorkflowComputingUnit( + localCreationParams(" ", Some("http://localhost:8085";)), + creatorUser + ) + + ex.getMessage should include("name cannot be empty") + runningUnitCountOwnedBy(903) shouldBe before + } + + it should "reject an unknown unit type with ForbiddenException" in { + val params = localCreationParams("unknown-type-cu", Some("http://localhost:8085";)) + .copy(unitType = "quantum") + + val ex = the[ForbiddenException] thrownBy + resource.createWorkflowComputingUnit(params, creatorUser) + + ex.getMessage should include("Unit type 'quantum' is not allowed") + } + + it should "reject the kubernetes type while it is disabled in the configuration" in { + val params = localCreationParams("k8s-cu", None).copy(unitType = "kubernetes") + + val ex = the[ForbiddenException] thrownBy + resource.createWorkflowComputingUnit(params, creatorUser) + + ex.getMessage should include("Unit type 'kubernetes' is not allowed") + } + + it should "reject a local unit without a URI" in { + val ex = the[ForbiddenException] thrownBy + resource.createWorkflowComputingUnit(localCreationParams("no-uri-cu", None), creatorUser) + + ex.getMessage should include("URI is required") + } + + it should "reject a local unit whose URI is blank" in { + val ex = the[ForbiddenException] thrownBy + resource.createWorkflowComputingUnit( + localCreationParams("blank-uri-cu", Some(" ")), + creatorUser + ) + + ex.getMessage should include("URI is required") + } + + it should "not apply the kubernetes running-unit quota to local units" in { + // Fill the creator's quota with directly inserted local units (cuids in the 930s) + val base = runningUnitCountOwnedBy(903) + ((base + 1) to maxNumOfRunningComputingUnitsPerUser).foreach { i => + insertLocalUnit(cuid = 930 + i, ownerUid = 903, name = s"quota-filler-$i") + } + runningUnitCountOwnedBy(903) should be >= maxNumOfRunningComputingUnitsPerUser + + val created = resource.createWorkflowComputingUnit( + localCreationParams("over-quota-local", Some("http://localhost:8085";)), + creatorUser + ) + + created.computingUnit.getName shouldBe "over-quota-local" + unitName(created.computingUnit.getCuid) shouldBe "over-quota-local" + } + + // Fixtures for the rename tests live in the 920s and are owned by "victim" (901) or + // "stranger" (902), so they never show up in listComputingUnits(user)'s exact-set assertion. + + "renameComputingUnit" should "let the owner rename its unit" in { + insertLocalUnit(cuid = 920, ownerUid = 902, name = "before-rename") + + val response = resource.renameComputingUnit(920, "after-rename", strangerUser) + + response.getStatus shouldBe 200 + unitName(920) shouldBe "after-rename" + } + + it should "reject a non-owner without granted access with 403 and keep the name" in { + insertLocalUnit(cuid = 921, ownerUid = 901, name = "victim-name") + + val response = resource.renameComputingUnit(921, "hijacked", strangerUser) + + response.getStatus shouldBe 403 + unitName(921) shouldBe "victim-name" + } + + it should "reject a grantee holding only READ access with 403 and keep the name" in { + insertLocalUnit(cuid = 922, ownerUid = 901, name = "read-only-name") + grantAccess(922, granteeUid = 902, PrivilegeEnum.READ) + + val response = resource.renameComputingUnit(922, "hijacked", strangerUser) + + response.getStatus shouldBe 403 + unitName(922) shouldBe "read-only-name" + } + + it should "let a grantee holding WRITE access rename the unit" in { + insertLocalUnit(cuid = 923, ownerUid = 901, name = "shared-name") + grantAccess(923, granteeUid = 902, PrivilegeEnum.WRITE) + + val response = resource.renameComputingUnit(923, "renamed-by-grantee", strangerUser) + + response.getStatus shouldBe 200 + unitName(923) shouldBe "renamed-by-grantee" + } + + it should "reject a blank name with 400 and keep the name" in { + insertLocalUnit(cuid = 924, ownerUid = 902, name = "keep-me") + + val response = resource.renameComputingUnit(924, " ", strangerUser) + + response.getStatus shouldBe 400 + unitName(924) shouldBe "keep-me" + } + + it should "reject an admin who neither owns nor was granted access with 403" in { + // Unlike terminate, rename has no ADMIN bypass: the role is never consulted. + insertLocalUnit(cuid = 951, ownerUid = 901, name = "admin-cannot-rename") + + val response = resource.renameComputingUnit(951, "hijacked", adminUser) + + response.getStatus shouldBe 403 + unitName(951) shouldBe "admin-cannot-rename" + } + + it should "return 404 for a nonexistent unit" in { + a[NotFoundException] should be thrownBy + resource.renameComputingUnit(99999, "new-name", strangerUser) + } + + it should "propagate a database failure and keep the name" in { + insertLocalUnit(cuid = 925, ownerUid = 902, name = "short-name") + val oversized = "x" * 200 // the name column is VARCHAR(128) + + // The endpoint's `return Response(500)` inside the withTransaction lambda is a Scala + // non-local return (a ControlThrowable, not a RuntimeException), so jOOQ's transaction + // wraps it in DataAccessException("Rollback caused") and the intended 500 response is + // never produced; this pins the actual behavior. + a[org.jooq.exception.DataAccessException] should be thrownBy + resource.renameComputingUnit(925, oversized, strangerUser) + + unitName(925) shouldBe "short-name" + } + + "getComputingUnitTypes" should "list exactly the local type while kubernetes is disabled" in { + resource.getComputingUnitTypes(user).typeOptions shouldBe List("local") + } + + "getComputingUnitLimitOptions" should "return the configured cpu, memory and gpu options" in { + // The values are kubernetes.conf's defaults; the test JVM does not override them. + val options = resource.getComputingUnitLimitOptions(user) + + options.cpuLimitOptions shouldBe List("1", "2", "4") + options.memoryLimitOptions shouldBe List("1Gi", "2Gi", "4Gi") + options.gpuLimitOptions shouldBe List("0", "1", "2") + } + + "getComputingUnitResourceLimit" should "return NaN limits for an owned local unit" in { + resource.getComputingUnitResourceLimit("800", user) shouldBe + WorkflowComputingUnitResourceLimit("NaN", "NaN", "NaN") + } + + it should "reject a non-owner with BadRequestException" in { + a[BadRequestException] should be thrownBy + resource.getComputingUnitResourceLimit("800", strangerUser) + } + + it should "throw NotFoundException for a nonexistent unit" in { + a[NotFoundException] should be thrownBy + resource.getComputingUnitResourceLimit("99999", user) + } }
