eugenegujing commented on code in PR #8647:
URL: https://github.com/apache/texera/pull/8647#discussion_r4343925099
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/hub/HubResourceSpec.scala:
##########
@@ -1137,20 +1149,73 @@ 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)
fetchDashboardWorkflowsByWids(Seq(id),
Integer.valueOf(likerUid)).head.isOwner shouldBe false
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.
Review Comment:
Yes. Fixed in bc028cbc72fdecf9c7a72ae43f8562815613a1b3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
