This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new a5563a95bc test(amber): cover the workflow resource's permission and
failure paths (#7592)
a5563a95bc is described below
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)
+ }
+
}
