Yicong-Huang commented on code in PR #8140:
URL: https://github.com/apache/texera/pull/8140#discussion_r3890821616
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala:
##########
@@ -840,6 +840,8 @@ class WorkflowResource extends LazyLogging {
.where(WORKFLOW.WID.eq(wid))
.and(WORKFLOW.IS_PUBLIC.isTrue)
.fetchOne()
+ if (workflow == null)
+ throw new NotFoundException(s"Public workflow with id $wid not found")
Review Comment:
The same unguarded dereference survives one method above this one.
`getWorkflowType` (:800-801, `@GET @Path("/type/{wid}")`) does
`workflowDao.fetchOneByWid(wid)` and then reads `workflow.getIsPublic` with no
null check.
Unlike `makePublic`/`makePrivate` — shielded by `hasWriteAccess` at
:709/:721, since `getPrivilege` returns non-WRITE when no access row exists —
it has no access-object call at all. So any REGULAR user hitting
`/api/workflow/type/999999` still gets the 500 that #8139 is about.
Enumerating every single-row fetch in this file (88, 160, 411, 469, 501,
551, 592, 712, 724, 800, 842), this is the only remaining instance.
Advisory rather than a blocker, deliberately: it is in the merge target,
this diff neither touches nor relies on it, and the PR fixes exactly what its
issue scopes. The ask is a scope question — pull it in here, or file the
follow-up — not a reason to hold a correct fix.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala:
##########
@@ -840,6 +840,8 @@ class WorkflowResource extends LazyLogging {
.where(WORKFLOW.WID.eq(wid))
.and(WORKFLOW.IS_PUBLIC.isTrue)
.fetchOne()
+ if (workflow == null)
+ throw new NotFoundException(s"Public workflow with id $wid not found")
Review Comment:
This is the only brace-less `if (…) throw` in the file — the other 13
guard-throws, including the analogue at :89-91, use braces. No behavioural
difference; noting it only so the file stays uniform.
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResourceCoverSpec.scala:
##########
@@ -152,6 +152,21 @@ class WorkflowResourceCoverSpec
.execute()
}
+ "retrievePublicWorkflow" should "return a public workflow" in {
Review Comment:
These landed in the cover-image-scoped spec, while
`WorkflowResourceSpec.scala` (1303 lines) owns `WorkflowResource` — and it
already asserts the positive case at :1019, inside the `makePublic /
makePrivate` test.
So the new "return a public workflow" case duplicates coverage that exists,
and the case that actually guards this fix — the private/missing rejection — is
filed where the next person changing `retrievePublicWorkflow` will not look for
it.
Suggestion: move the rejection test to `WorkflowResourceSpec.scala` beside
the existing `makePublic / makePrivate` block, and drop the positive case as
redundant with :1019.
--
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]
