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-7592-c35bdb134cab4126b25c39974be3e29a0a17a672 in repository https://gitbox.apache.org/repos/asf/texera.git
commit a5563a95bc659bb37d16fd236480bb4aa8e745ff Author: Xinyuan Lin <[email protected]> AuthorDate: Thu Aug 13 03:24:15 2026 +0000 test(amber): cover the workflow resource's permission and failure paths (#7592) ### What changes were proposed in this PR? `WorkflowResource` sat at **82.6% of lines**, and its residue was not scattered: permission-guard arms, exception paths, and one whole endpoint that had never been called — `cloneWorkflow`. Permission guards are exactly where a silent regression matters, which is why this is worth doing despite the modest line count. Adds 16 tests to the existing spec (no third spec file), taking the file to **100% of lines**. Everything it reaches is database-only, so `MockTexeraDB` suffices — `WorkflowVersionResource.insertVersion` is jOOQ plus Jackson `JsonDiff`, `HubResource.recordClone` is jOOQ, and nothing needs LakeFS, Docker or an engine. ### Verification 22 mutations applied and reverted, production diff confirmed empty each time — the clone's `isPublic` argument, the version-insert ordering, the access-level comparisons, and the exception-wrapping arms among them. **Reviewing my own tests then found three that claimed more than they pinned.** All three are now stated in the spec rather than left to be discovered: | Claim | Reality | What changed | |---|---|---| | "wrap a failure raised inside the transaction" | `assignNewOperatorIds` fails *before* `createWorkflow` inserts, so "no copy was created" holds with or without a transaction — replacing `context.transaction` with a plain block leaves the suite green | renamed to what it pins (the exception wrapping), with the gap recorded | | the delete test covers the cleanup tail | it does not — emptying the collected execution ids leaves the suite green. `LargeBinaryManager` is an S3-backed `object` with no seam, and document cleanup needs Iceberg fixtures this spec lacks | recorded as entered-not-verified. What the test *does* pin was confirmed by mutation: removing the `case NonFatal` arm of the outer catch turns it red, so an undecodable URI really is tolerated rather than aborting the delete | | two assertions in the write-access test | both already hold before `persistWorkflow` is called; its write branch touches only `WORKFLOW` and `WORKFLOW_VERSION` | relabelled as guards; the content and version-count assertions carry the pin | ### A note on the numbers jacoco reports a wide line-versus-branch split here — branch coverage stays low even at 100% lines — because of the synthetic branches the Scala compiler generates for this style of code. The line figure is the meaningful one on this file. No production file is touched. ### Any related issues, documentation, discussions? Closes #7591 ### How was this PR tested? ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.dashboard.file.WorkflowResourceSpec" ``` ``` [info] Total number of tests run: 68 [info] Tests: succeeded 68, failed 0, canceled 0, ignored 0, pending 0 ``` 16 new on top of the existing 52. `Test/scalafmtCheck` and `Test/scalafix --check` both pass. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --- .../dashboard/file/WorkflowResourceSpec.scala | 259 ++++++++++++++++++++- 1 file changed, 256 insertions(+), 3 deletions(-) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala index d2e0f2bef5..089b5234cd 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala @@ -21,7 +21,14 @@ package org.apache.texera.web.resource.dashboard.file import org.apache.texera.auth.SessionUser import org.apache.texera.dao.MockTexeraDB -import org.apache.texera.dao.jooq.generated.Tables.{USER, WORKFLOW, WORKFLOW_OF_PROJECT} +import org.apache.texera.dao.jooq.generated.Tables.{ + USER, + WORKFLOW, + WORKFLOW_EXECUTIONS, + WORKFLOW_OF_PROJECT, + WORKFLOW_USER_CLONES, + WORKFLOW_VERSION +} import org.apache.texera.dao.jooq.generated.enums.{PrivilegeEnum, UserRoleEnum} import org.apache.texera.dao.jooq.generated.tables.daos.{UserDao, WorkflowUserAccessDao} import org.apache.texera.dao.jooq.generated.tables.pojos.{ @@ -42,6 +49,7 @@ import org.apache.texera.web.resource.dashboard.user.workflow.WorkflowResource.{ import org.apache.texera.web.resource.dashboard.{DashboardResource, FulltextSearchQueryUtils} import org.jooq.Condition import org.jooq.impl.DSL.noCondition +import org.scalamock.scalatest.MockFactory import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach} @@ -51,14 +59,25 @@ import java.time.{Duration, OffsetDateTime, ZoneOffset} import java.util import java.util.Collections import java.util.concurrent.TimeUnit -import javax.ws.rs.ForbiddenException +import javax.servlet.http.HttpServletRequest +import javax.ws.rs.{ + BadRequestException, + ForbiddenException, + NotFoundException, + WebApplicationException +} import scala.jdk.CollectionConverters._ -import org.apache.texera.web.resource.dashboard.user.workflow.WorkflowAccessResource +import org.apache.texera.web.auth.GuestAuthFilter +import org.apache.texera.web.resource.dashboard.user.workflow.{ + WorkflowAccessResource, + WorkflowVersionResource +} class WorkflowResourceSpec extends AnyFlatSpec with BeforeAndAfterAll with BeforeAndAfterEach + with MockFactory with MockTexeraDB { // An example creation time to test Account Creation Time attribute @@ -1041,4 +1060,238 @@ class WorkflowResourceSpec assert(names.contains("dup-src") && names.contains("dup-src_copy")) } + // ─── shared-access and failure paths (issue #7591) ────────────────────────── + + // Content with an operators array: duplicate/clone reassign operator ids and fail + // on content that has none. + private val contentWithOperator = + "{\"operators\":[{\"operatorID\":\"op1\",\"operatorType\":\"CSVFileScan\"}]}" + + private def grantAccess(wid: Integer, user: User, privilege: PrivilegeEnum): Unit = + new WorkflowUserAccessDao(getDSLContext.configuration()) + .merge(new WorkflowUserAccess(user.getUid, wid, privilege)) + + // A request whose remote address is a valid IPv4 so recordCloneAction stores it. + private def cloneRequest: HttpServletRequest = { + val r = stub[HttpServletRequest] + (r.getRemoteAddr _).when().returns("127.0.0.1") + r + } + + private def workflowNamesOf(user: SessionUser): List[String] = + workflowResource.retrieveWorkflowsBySessionUser(user).map(_.workflow.getName) + + private def versionCount(wid: Integer): Int = + getDSLContext.fetchCount(WORKFLOW_VERSION, WORKFLOW_VERSION.WID.eq(wid)) + + "WorkflowResource.getWorkflowName (companion)" should "return the stored name" in { + val wid = seedWorkflow(sessionUser1, "companion-name-wf").workflow.getWid + assert(WorkflowResource.getWorkflowName(wid) == "companion-name-wf") + } + + it should "throw NotFoundException for a wid that does not exist" in { + val wid = seedWorkflow(sessionUser1, "companion-missing-wf").workflow.getWid + assertThrows[NotFoundException](WorkflowResource.getWorkflowName(wid + 100000)) + } + + "WorkflowResource.persistWorkflow" should "reject the guest user" in { + val workflow = seedWorkflow(sessionUser1, "guest-wf", "d", "{\"a\":1}").workflow + workflow.setContent("{\"a\":2}") + + // the message distinguishes the guest rejection from the access-privilege one + val thrown = intercept[ForbiddenException]( + workflowResource.persistWorkflow(workflow, new SessionUser(GuestAuthFilter.GUEST)) + ) + assert(thrown.getMessage.contains("Guest user")) + assert(workflowResource.retrieveWorkflow(workflow.getWid, sessionUser1).content == "{\"a\":1}") + } + + it should "update the workflow in place for its owner and record a version" in { + val workflow = seedWorkflow(sessionUser1, "persist-owner", "d", "{\"a\":1}").workflow + val versionsBefore = versionCount(workflow.getWid) + workflow.setContent("{\"a\":2}") + + val persisted = workflowResource.persistWorkflow(workflow, sessionUser1) + + assert(persisted.getContent == "{\"a\":2}") + assert(workflowResource.retrieveWorkflow(workflow.getWid, sessionUser1).content == "{\"a\":2}") + assert(versionCount(workflow.getWid) == versionsBefore + 1) + // updating must not create a second workflow + assert(workflowNamesOf(sessionUser1) == List("persist-owner")) + } + + it should "let a non-owner with write access update the workflow and record a version" in { + val workflow = seedWorkflow(sessionUser1, "persist-writer", "d", "{\"a\":1}").workflow + grantAccess(workflow.getWid, testUser2, PrivilegeEnum.WRITE) + val versionsBefore = versionCount(workflow.getWid) + workflow.setContent("{\"a\":3}") + + workflowResource.persistWorkflow(workflow, sessionUser2) + + assert(workflowResource.retrieveWorkflow(workflow.getWid, sessionUser1).content == "{\"a\":3}") + assert(versionCount(workflow.getWid) == versionsBefore + 1) + // Guards rather than pins: both already hold before persistWorkflow is called, since + // seedWorkflow and grantAccess establish them and the write-access branch touches only + // WORKFLOW and WORKFLOW_VERSION. They document the intent -- the writer updates the owner's + // workflow rather than getting a copy of its own -- while the two assertions above carry the + // actual pin. + assert(workflowResource.getOwnerName(workflow.getWid) == testUser.getName) + assert(workflowNamesOf(sessionUser2) == List("persist-writer")) + } + + it should "reject a non-owner with only read access" in { + val workflow = seedWorkflow(sessionUser1, "persist-reader", "d", "{\"a\":1}").workflow + grantAccess(workflow.getWid, testUser2, PrivilegeEnum.READ) + workflow.setContent("{\"a\":9}") + + assertThrows[ForbiddenException](workflowResource.persistWorkflow(workflow, sessionUser2)) + assert(workflowResource.retrieveWorkflow(workflow.getWid, sessionUser1).content == "{\"a\":1}") + } + + it should "reject a user with no access to an existing workflow" in { + val workflow = seedWorkflow(sessionUser1, "persist-no-access", "d", "{\"a\":1}").workflow + workflow.setContent("{\"a\":9}") + + assertThrows[ForbiddenException](workflowResource.persistWorkflow(workflow, sessionUser2)) + assert(workflowResource.retrieveWorkflow(workflow.getWid, sessionUser1).content == "{\"a\":1}") + // the rejected persist must not silently create a copy owned by the caller + assert(workflowNamesOf(sessionUser2).isEmpty) + } + + "WorkflowResource.createWorkflow" should "reject a workflow that already carries an id" in { + val existing = seedWorkflow(sessionUser1, "already-has-id").workflow + + assertThrows[BadRequestException](workflowResource.createWorkflow(existing, sessionUser1)) + assert(workflowNamesOf(sessionUser1) == List("already-has-id")) + } + + "WorkflowResource.updateWorkflowName" should "accept a non-owner with write access" in { + val wid = seedWorkflow(sessionUser1, "writer-rename").workflow.getWid + grantAccess(wid, testUser2, PrivilegeEnum.WRITE) + val update = new Workflow() + update.setWid(wid) + update.setName("renamed-by-writer") + + workflowResource.updateWorkflowName(update, sessionUser2) + + assert(workflowResource.getWorkflowName(wid) == "renamed-by-writer") + } + + it should "reject a user with neither ownership nor write access" in { + val wid = seedWorkflow(sessionUser1, "no-access-rename").workflow.getWid + val update = new Workflow() + update.setWid(wid) + update.setName("should-not-apply") + + assertThrows[ForbiddenException](workflowResource.updateWorkflowName(update, sessionUser2)) + assert(workflowResource.getWorkflowName(wid) == "no-access-rename") + } + + "WorkflowResource.makePrivate" should "reject a user without write access" in { + val wid = seedWorkflow(sessionUser1, "private-forbidden").workflow.getWid + workflowResource.makePublic(wid, sessionUser1) + + assertThrows[ForbiddenException](workflowResource.makePrivate(wid, sessionUser2)) + assert(workflowResource.getWorkflowType(wid) == "Public") + } + + "WorkflowResource.cloneWorkflow" should "copy the workflow to the caller and record the clone" in { + // The source is made public because that is the flow this endpoint serves: the hub's clone + // button, on someone else's published workflow. Cloning a *private* workflow the caller has + // no access to also succeeds today -- cloneWorkflow fetches by wid with no `hasReadAccess` + // guard, unlike retrieveWorkflow and duplicateWorkflow -- but that is a gap to fix in the + // resource, not a contract to pin here, so this test does not assert it either way. + val wid = seedWorkflow(sessionUser1, "clone-src", "d", contentWithOperator).workflow.getWid + workflowResource.makePublic(wid, sessionUser1) + + val newWid = workflowResource.cloneWorkflow(wid, sessionUser2, cloneRequest) + + assert(newWid != wid) + val clone = workflowResource.retrieveWorkflow(newWid, sessionUser2) + assert(clone.name == "clone-src_clone") + assert(clone.description == "d") + assert(!clone.content.contains("\"op1\"")) // operator ids are reassigned + assert(clone.content.contains("\"CSVFileScan\"")) + assert(!clone.isPublished) // the clone starts private; the source's publicness is not inherited + // the clone belongs to the caller, not to the original owner + assert(workflowResource.getOwnerName(newWid) == testUser2.getName) + assert( + getDSLContext.fetchCount( + WORKFLOW_USER_CLONES, + WORKFLOW_USER_CLONES.WID.eq(wid).and(WORKFLOW_USER_CLONES.UID.eq(testUser2.getUid)) + ) == 1 + ) + } + + "WorkflowResource.duplicateWorkflow" should "add the copy, not the original, to the project" in { + val wid = + seedWorkflow(sessionUser1, "dup-into-project", "d", contentWithOperator).workflow.getWid + val pid = projectResource.createProject(sessionUser1, "dup-target-project").getPid + + val copies = workflowResource.duplicateWorkflow(WorkflowIDs(List(wid), Some(pid)), sessionUser1) + + assert(copies.size == 1) + val widsInProject = getDSLContext + .select(WORKFLOW_OF_PROJECT.WID) + .from(WORKFLOW_OF_PROJECT) + .where(WORKFLOW_OF_PROJECT.PID.eq(pid)) + .fetchInto(classOf[Integer]) + .asScala + .toList + assert(widsInProject == List(copies.head.workflow.getWid)) + } + + it should "wrap a failure raised while copying the workflow in a WebApplicationException" in { + // "{}" has no operators array, so assignNewOperatorIds throws. + // + // Note what this does NOT pin: transactionality. assignNewOperatorIds fails before + // createWorkflow inserts anything, so "no copy was created" holds whether or not the body runs + // in a transaction -- replacing `context.transaction { ... }` with a plain block leaves this + // test green. Pinning the rollback would need a failure raised after the insert, and there is + // no seam for one. + val wid = seedWorkflow(sessionUser1, "dup-no-operators").workflow.getWid + + val thrown = intercept[WebApplicationException]( + workflowResource.duplicateWorkflow(WorkflowIDs(List(wid), None), sessionUser1) + ) + + // not a ForbiddenException/BadRequestException, which the same catch swallows + assert(thrown.getClass == classOf[WebApplicationException]) + assert(thrown.getCause.isInstanceOf[NoSuchElementException]) + assert(workflowNamesOf(sessionUser1) == List("dup-no-operators")) + } + + "WorkflowResource.deleteWorkflow" should "wrap an unexpected failure in a WebApplicationException" in { + // A request body without a "wids" field deserializes to a null list. + val thrown = intercept[WebApplicationException]( + workflowResource.deleteWorkflow(WorkflowIDs(null, None), sessionUser1) + ) + + assert(thrown.getClass == classOf[WebApplicationException]) + assert(thrown.getCause.isInstanceOf[NullPointerException]) + } + + // What this pins, and what it does not. It DOES pin that a URI which cannot be decoded is + // tolerated rather than aborting the delete: removing the `case NonFatal(exception) =>` arm of + // deleteWorkflow's outer catch turns this test red. + // + // It does NOT pin the post-transaction cleanup tail it happens to execute. `LargeBinaryManager` + // is an object talking to S3 with no injectable seam, and the document cleanup needs real + // Iceberg-backed documents this spec has no fixture for -- emptying the collected execution ids + // leaves the suite green. Those lines are entered, not verified. + it should "still delete the workflow when a stored execution URI cannot be decoded" in { + val wid = seedWorkflow(sessionUser1, "undecodable-uri-wf").workflow.getWid + getDSLContext + .insertInto(WORKFLOW_EXECUTIONS) + .set(WORKFLOW_EXECUTIONS.VID, WorkflowVersionResource.getLatestVersion(wid)) + .set(WORKFLOW_EXECUTIONS.UID, testUser.getUid) + .set(WORKFLOW_EXECUTIONS.ENVIRONMENT_VERSION, "test-env") + .set(WORKFLOW_EXECUTIONS.RUNTIME_STATS_URI, "bogus://not-a-vfs-uri") + .execute() + + workflowResource.deleteWorkflow(WorkflowIDs(List(wid), None), sessionUser1) + + assert(workflowNamesOf(sessionUser1).isEmpty) + } + }
