Yicong-Huang commented on code in PR #8136:
URL: https://github.com/apache/texera/pull/8136#discussion_r3890819154
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/hub/HubResource.scala:
##########
@@ -418,7 +418,9 @@ class HubResource {
@QueryParam("uid") uid: Integer,
@QueryParam("limit") limit: Integer
): java.util.Map[String, java.util.List[DashboardClickableFileEntry]] = {
- val tableSet = EntityTables(entityType)
+ val tableSet = EntityTables(
+ Option(entityType).getOrElse(throw new BadRequestException("entityType
is required"))
Review Comment:
`getCount` (:324-325) takes the identical `@QueryParam("entityType")
entityType: EntityType` and passes it unguarded into `EntityTables(...).base`,
so `GET /api/hub/count` with no query string still reaches the same partial
match and still returns 500 — the same symptom and one-request reproduction
#8135 describes for the top list, one method above the line this diff changes.
That twin is fixed by your own #8134, opened seventeen minutes before this
one, so the two are complementary and the class closes when both land. Advisory
rather than a blocker for that reason; merging either alone ships it
half-closed.
The design half is worth stating separately: because the check lives at the
call site rather than at the parameter, closing one endpoint does not close its
neighbour, and every future `EntityTables(...)` caller inherits the same
obligation. `recordLikeAction` (:186) and `postView` (:379) take the same shape
from a JSON body — I did not verify what Jackson does with an omitted field, so
those are worth a look rather than a claim.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/hub/HubResource.scala:
##########
@@ -418,7 +418,9 @@ class HubResource {
@QueryParam("uid") uid: Integer,
@QueryParam("limit") limit: Integer
): java.util.Map[String, java.util.List[DashboardClickableFileEntry]] = {
- val tableSet = EntityTables(entityType)
+ val tableSet = EntityTables(
+ Option(entityType).getOrElse(throw new BadRequestException("entityType
is required"))
Review Comment:
The guard is embedded in the argument expression handed to
`EntityTables(...)` rather than stated as a precondition. `getCounts` in this
same class already establishes the shape for exactly this concern — `if (...)
throw new BadRequestException(...)` as its first statement (:525-531).
Nothing a caller observes differs, so this is consistency rather than a
defect. It is worth a line because a second shape for one concern in one class
is drift — and because the precondition form is what would have made the twin
at `getCount` obvious.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/hub/HubResource.scala:
##########
@@ -418,7 +418,9 @@ class HubResource {
@QueryParam("uid") uid: Integer,
@QueryParam("limit") limit: Integer
): java.util.Map[String, java.util.List[DashboardClickableFileEntry]] = {
- val tableSet = EntityTables(entityType)
+ val tableSet = EntityTables(
+ Option(entityType).getOrElse(throw new BadRequestException("entityType
is required"))
Review Comment:
`getTops` gains a required-parameter contract without gaining its
documentation. The Scaladoc (:397-411) still describes `entityType` as "The
EntityType enum value (Workflow, Dataset) to query" and marks the other params
"Optional", with no `@throws` — while `getCounts` documents precisely this at
:514-515.
One `@throws javax.ws.rs.BadRequestException if entityType is missing` line
closes it.
--
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]
