From e5c2ed8b85beed9451390f0aca87a210430c9737 Mon Sep 17 00:00:00 2001 From: Eugene Date: Tue, 22 Sep 2026 21:40:10 -0700 Subject: [PATCH 1/2] fix(amber): compare dashboard workflow uids by value Replace the `AnyRef.eq` identity check in `WorkflowResource.mapWorkflowEntries` with value equality, and extend `HubResourceSpec` with `isOwner` cases on both sides of the `java.lang.Integer` cache boundary and with the two null-uid negatives. --- .../user/workflow/WorkflowResource.scala | 2 +- .../dashboard/hub/HubResourceSpec.scala | 79 +++++++++++++++++-- 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala index 46fad49580b..1109f3c02b7 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala @@ -269,7 +269,7 @@ object WorkflowResource { .map(workflowRecord => DashboardWorkflow( if (uid != null) - workflowRecord.into(WORKFLOW_OF_USER).getUid.eq(uid) + workflowRecord.into(WORKFLOW_OF_USER).getUid == uid else false, workflowRecord .into(WORKFLOW_USER_ACCESS) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/hub/HubResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/hub/HubResourceSpec.scala index 88b910107e4..6a823e31066 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/hub/HubResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/hub/HubResourceSpec.scala @@ -129,6 +129,15 @@ class HubResourceSpec // --------------------------------------------------------------------------- private val thirdUid = 9003 + // An owner whose uid falls inside the java.lang.Integer cache (-128..127), where two + // boxes of the same value are the same object. The isOwner cases below use it next to + // the 9001-9003 uids so both sides of that cache boundary are covered. + private val cachedUid = 42 + // The cache boundary itself. 127 is the highest value java.lang.Integer caches and 128 the + // lowest it does not, so these two are the pair that actually pins where identity stops + // agreeing with value. 42 and 9001 only sit comfortably on either side of them. + private val lastCachedUid = 127 + private val firstUncachedUid = 128 private val Ds: EntityType = EntityType.Dataset private val Md: EntityType = EntityType.Model @@ -377,6 +386,9 @@ class HubResourceSpec // A third liker, needed by the ranking cases below to make "most liked" a // strict order (3 > 2 > 1) rather than a tie between two users' likes. userDao.insert(makeUser(thirdUid, "hub_third")) + userDao.insert(makeUser(cachedUid, "hub_cached_uid")) + userDao.insert(makeUser(lastCachedUid, "hub_uid_127")) + userDao.insert(makeUser(firstUncachedUid, "hub_uid_128")) val wf = new Workflow wf.setWid(Integer.valueOf(wid)) @@ -1137,13 +1149,43 @@ class HubResourceSpec entry.coverImage shouldBe empty } - // NOTE: `isOwner == true` is deliberately not asserted anywhere. WorkflowResource's - // mapWorkflowEntries compares the two uids with `AnyRef.eq` (reference equality on - // boxed Integers), so it is true only for uids that land in the java.lang.Integer - // cache (-128..127); this suite's uids (9001-9003) are outside it, so the owner's own - // request reports false today. Asserting it either way would either cement that bug or - // force this suite onto unrealistically small uids. The two negatives below hold for - // the current implementation and for a value-equality fix alike. + // WorkflowResource's mapWorkflowEntries compares the record's uid with the caller's, + // both boxed Integers. Only value equality gets this right: reference equality holds + // for two boxes of the same value solely inside the java.lang.Integer cache + // (-128..127), so the pair below pins both sides of that boundary. + it should "set isOwner for the owner's own request when the uid is outside the Integer cache" in { + val id = seedWorkflow(811003, "wf_mine_uncached", owner = ownerUid) + + fetchDashboardWorkflowsByWids(Seq(id), Integer.valueOf(ownerUid)).head.isOwner shouldBe true + } + + it should "set isOwner for the owner's own request when the uid is inside the Integer cache" in { + val id = seedWorkflow(811004, "wf_mine_cached", owner = cachedUid) + + fetchDashboardWorkflowsByWids(Seq(id), Integer.valueOf(cachedUid)).head.isOwner shouldBe true + } + + // 127 and 128 are the boundary proper. Under the old reference comparison 127 passed and + // 128 failed, so this pair localises the break to one increment rather than to the gap + // between 42 and 9001. + it should "set isOwner for the highest uid inside the Integer cache (127)" in { + val id = seedWorkflow(811006, "wf_mine_uid_127", owner = lastCachedUid) + + fetchDashboardWorkflowsByWids( + Seq(id), + Integer.valueOf(lastCachedUid) + ).head.isOwner shouldBe true + } + + it should "set isOwner for the lowest uid outside the Integer cache (128)" in { + val id = seedWorkflow(811007, "wf_mine_uid_128", owner = firstUncachedUid) + + fetchDashboardWorkflowsByWids( + Seq(id), + Integer.valueOf(firstUncachedUid) + ).head.isOwner shouldBe true + } + it should "clear isOwner for a different uid and for an anonymous (null) uid" in { val id = seedWorkflow(811002, "wf_not_mine", owner = ownerUid) @@ -1151,6 +1193,29 @@ class HubResourceSpec fetchDashboardWorkflowsByWids(Seq(id), null).head.isOwner shouldBe false } + // workflow_of_user is LEFT JOINed, so a workflow with no ownership row yields a null + // uid. The access row is still granted because that is the only way the entry reaches + // mapWorkflowEntries' privilege dereference with something non-null. + it should "clear isOwner for a workflow with no ownership row" in { + val id = seedWorkflow(811005, "wf_unowned", withOwnerRows = false) + grantWorkflowAccess(811005, ownerUid, PrivilegeEnum.READ) + + val entry = fetchDashboardWorkflowsByWids(Seq(id), Integer.valueOf(ownerUid)).head + entry.ownerId shouldBe null + entry.isOwner shouldBe false + } + + // Both sides null at once: an anonymous caller looking at a workflow with no ownership + // row. Scala's `==` reports two nulls as equal, so the `uid != null` guard in + // mapWorkflowEntries is the only thing keeping this false. Without this case, deleting + // that guard as redundant would leave the whole suite green. + it should "clear isOwner when an anonymous caller sees a workflow with no ownership row" in { + val id = seedWorkflow(811008, "wf_unowned_anon", withOwnerRows = false) + grantWorkflowAccess(811008, ownerUid, PrivilegeEnum.READ) + + fetchDashboardWorkflowsByWids(Seq(id), null).head.isOwner shouldBe false + } + // Every resolved id hits LakeFS, so an empty list is the only input testable without one. "fetchDashboardVersionedResourcesByIds" should "return an empty list for an empty id list" in { fetchDashboardVersionedResourcesByIds( From bc028cbc72fdecf9c7a72ae43f8562815613a1b3 Mon Sep 17 00:00:00 2001 From: Eugene Date: Wed, 21 Oct 2026 18:59:27 -0700 Subject: [PATCH 2/2] fix(amber): read the owner uid once in mapWorkflowEntries and cover /workflow/list isOwner Collapse the isOwner expression in WorkflowResource.mapWorkflowEntries to `uid != null && ownerUid == uid` and read the owner uid once per row for both isOwner and ownerId. Add a WorkflowResourceSpec case that lists workflows through retrieveWorkflowsBySessionUser for an owner with uid 1001 and asserts isOwner for the owner and for a read-only user. In HubResourceSpec, merge the two no-ownership-row cases into one, reuse the id returned by seedWorkflow, and shorten the isOwner comments to describe the default Integer autoboxing cache. --- .../user/workflow/WorkflowResource.scala | 11 +++-- .../dashboard/file/WorkflowResourceSpec.scala | 29 ++++++++++++++ .../dashboard/hub/HubResourceSpec.scala | 40 ++++++------------- 3 files changed, 46 insertions(+), 34 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala index 1109f3c02b7..32c0f4ab5cf 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala @@ -266,11 +266,10 @@ object WorkflowResource { uid: Integer ): List[DashboardWorkflow] = { workflowEntries - .map(workflowRecord => + .map { workflowRecord => + val ownerUid = workflowRecord.into(WORKFLOW_OF_USER).getUid DashboardWorkflow( - if (uid != null) - workflowRecord.into(WORKFLOW_OF_USER).getUid == uid - else false, + uid != null && ownerUid == uid, workflowRecord .into(WORKFLOW_USER_ACCESS) .into(classOf[WorkflowUserAccess]) @@ -278,10 +277,10 @@ object WorkflowResource { .toString, workflowRecord.into(USER).getName, workflowRecord.into(WORKFLOW).into(classOf[Workflow]), - workflowRecord.into(WORKFLOW_OF_USER).getUid, + ownerUid, Option(workflowRecord.get("cover_image", classOf[String])) ) - ) + } .asScala .toList } 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 d197808d00e..8a3fa7f8ac8 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 @@ -1510,4 +1510,33 @@ class WorkflowResourceSpec ) } + // The listing's isOwner compares two boxed Integers. testUser and testUser2 have uids 1 + // and 2, inside the default Integer autoboxing cache where reference equality happens to + // agree with value equality, so this case needs an owner whose uid is outside it. + "WorkflowResource.retrieveWorkflowsBySessionUser" should "set isOwner for an owner whose uid is outside the Integer cache" in { + val owner = new User + owner.setUid(Integer.valueOf(1001)) + owner.setName("large_uid_owner") + owner.setRole(UserRoleEnum.REGULAR) + new UserDao(getDSLContext.configuration()).insert(owner) + val ownerSession = new SessionUser(owner) + + val wid = seedWorkflow(ownerSession, "wf_of_large_uid_user").workflow.getWid + try { + val own = workflowResource.retrieveWorkflowsBySessionUser(ownerSession) + assert(own.map(_.workflow.getWid) == List(wid)) + assert(own.head.ownerId == owner.getUid) + assert(own.head.isOwner, "the owner's own listing must report isOwner") + + // A reader with the same workflow in their listing is not its owner. + grantAccess(wid, testUser, PrivilegeEnum.READ) + val shared = workflowResource.retrieveWorkflowsBySessionUser(sessionUser1) + assert(shared.exists(_.workflow.getWid == wid)) + assert(!shared.find(_.workflow.getWid == wid).get.isOwner) + } finally { + // afterEach only clears testUser's and testUser2's own workflows. + workflowResource.deleteWorkflow(WorkflowIDs(List(wid)), ownerSession) + } + } + } diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/hub/HubResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/hub/HubResourceSpec.scala index 6a823e31066..695cf9bf469 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/hub/HubResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/hub/HubResourceSpec.scala @@ -129,13 +129,10 @@ class HubResourceSpec // --------------------------------------------------------------------------- private val thirdUid = 9003 - // An owner whose uid falls inside the java.lang.Integer cache (-128..127), where two - // boxes of the same value are the same object. The isOwner cases below use it next to - // the 9001-9003 uids so both sides of that cache boundary are covered. + // Owners inside the default java.lang.Integer autoboxing cache (-128..127), where + // reference equality on boxed values happens to agree with value equality. The + // 9001-9003 uids above sit outside it. private val cachedUid = 42 - // The cache boundary itself. 127 is the highest value java.lang.Integer caches and 128 the - // lowest it does not, so these two are the pair that actually pins where identity stops - // agreeing with value. 42 and 9001 only sit comfortably on either side of them. private val lastCachedUid = 127 private val firstUncachedUid = 128 private val Ds: EntityType = EntityType.Dataset @@ -1149,10 +1146,8 @@ class HubResourceSpec entry.coverImage shouldBe empty } - // WorkflowResource's mapWorkflowEntries compares the record's uid with the caller's, - // both boxed Integers. Only value equality gets this right: reference equality holds - // for two boxes of the same value solely inside the java.lang.Integer cache - // (-128..127), so the pair below pins both sides of that boundary. + // mapWorkflowEntries compares two boxed Integers, so the owner's own request must be + // recognised on both sides of the default Integer cache (-128..127). it should "set isOwner for the owner's own request when the uid is outside the Integer cache" in { val id = seedWorkflow(811003, "wf_mine_uncached", owner = ownerUid) @@ -1165,9 +1160,7 @@ class HubResourceSpec fetchDashboardWorkflowsByWids(Seq(id), Integer.valueOf(cachedUid)).head.isOwner shouldBe true } - // 127 and 128 are the boundary proper. Under the old reference comparison 127 passed and - // 128 failed, so this pair localises the break to one increment rather than to the gap - // between 42 and 9001. + // The boundary itself: 127 is the last cached value and 128 the first uncached one. it should "set isOwner for the highest uid inside the Integer cache (127)" in { val id = seedWorkflow(811006, "wf_mine_uid_127", owner = lastCachedUid) @@ -1193,26 +1186,17 @@ class HubResourceSpec fetchDashboardWorkflowsByWids(Seq(id), null).head.isOwner shouldBe false } - // workflow_of_user is LEFT JOINed, so a workflow with no ownership row yields a null - // uid. The access row is still granted because that is the only way the entry reaches - // mapWorkflowEntries' privilege dereference with something non-null. - it should "clear isOwner for a workflow with no ownership row" in { + // workflow_of_user is LEFT JOINed, so a workflow with no ownership row has a null owner + // uid; the access row keeps the privilege non-null. For the anonymous caller both uids + // are null, and Scala's `==` treats two nulls as equal, so the `uid != null` guard in + // mapWorkflowEntries is what keeps that one false. + it should "clear isOwner for a workflow with no ownership row, for an authenticated and an anonymous caller" in { val id = seedWorkflow(811005, "wf_unowned", withOwnerRows = false) - grantWorkflowAccess(811005, ownerUid, PrivilegeEnum.READ) + grantWorkflowAccess(id, ownerUid, PrivilegeEnum.READ) val entry = fetchDashboardWorkflowsByWids(Seq(id), Integer.valueOf(ownerUid)).head entry.ownerId shouldBe null entry.isOwner shouldBe false - } - - // Both sides null at once: an anonymous caller looking at a workflow with no ownership - // row. Scala's `==` reports two nulls as equal, so the `uid != null` guard in - // mapWorkflowEntries is the only thing keeping this false. Without this case, deleting - // that guard as redundant would leave the whole suite green. - it should "clear isOwner when an anonymous caller sees a workflow with no ownership row" in { - val id = seedWorkflow(811008, "wf_unowned_anon", withOwnerRows = false) - grantWorkflowAccess(811008, ownerUid, PrivilegeEnum.READ) - fetchDashboardWorkflowsByWids(Seq(id), null).head.isOwner shouldBe false }