kunwp1 commented on code in PR #8647:
URL: https://github.com/apache/texera/pull/8647#discussion_r4320980975
##########
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 {
Review Comment:
These cases go through the hub path, but the authenticated endpoint this fix
is mainly about is `/workflow/list` (`retrieveWorkflowsBySessionUser`).
`WorkflowResourceSpec` only uses uids 1 and 2 and never asserts `isOwner`, so
it could not have caught this bug. Could we add one case there with an owner
uid above 127? The end-to-end check in the description already does exactly
that, so this would mostly mean committing it.
##########
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,
Review Comment:
The `if ... else false` could become a single `uid != null && ...`
expression, and the owner uid could be read once per row. It is read again a
few lines below for `ownerId`, so reading it once avoids converting the record
twice.
##########
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 {
Review Comment:
This case and the anonymous one below have the same setup and differ only by
caller, so they could be a single test with two assertions.
##########
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:
A few comments in this diff describe the history of the PR rather than what
the test checks. A one-line reason per test is probably enough, for example
that both uids are null and `==` treats two nulls as equal, so the guard is
what keeps this false.
--
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]
