Yicong-Huang commented on code in PR #8134:
URL: https://github.com/apache/texera/pull/8134#discussion_r3890817648
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/hub/HubResource.scala:
##########
@@ -322,7 +322,10 @@ class HubResource {
@GET
@Path("/count")
def getCount(@QueryParam("entityType") entityType: EntityType): Integer = {
- val entityTables = EntityTables(entityType).base
+ val entityTables =
+ EntityTables(
+ Option(entityType).getOrElse(throw new BadRequestException("entityType
is required"))
Review Comment:
This guard covers "absent", not "unusable".
`EntityType` is converted by `EntityType.fromString`, which throws
`IllegalArgumentException` for any unrecognized string including the empty one
(`EntityType.scala:43-48`). That runs during param conversion, before the
method body, so `?entityType=` and `?entityType=bogus` never reach this line
and do not get the 400 the description advertises.
Asking rather than asserting — I did not exercise what Jersey returns for a
converter failure. It seems worth knowing, since an empty value is the shape a
buggy client sends most often, and a reader of the description would assume it
is now covered.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/hub/HubResource.scala:
##########
@@ -322,7 +322,10 @@ class HubResource {
@GET
@Path("/count")
def getCount(@QueryParam("entityType") entityType: EntityType): Integer = {
- val entityTables = EntityTables(entityType).base
+ val entityTables =
+ EntityTables(
+ Option(entityType).getOrElse(throw new BadRequestException("entityType
is required"))
Review Comment:
The sibling validator states the same check differently: `getCounts` puts it
in a plain guard clause as the method's first statement and documents it with
`@throws javax.ws.rs.BadRequestException …` (:515, :526-532).
Embedding the throw in the `Option(...).getOrElse(...)` argument turns a
one-line `val entityTables = EntityTables(entityType).base` into a four-line
expression and leaves the new failure mode undocumented. A first-line `if
(entityType == null) throw new BadRequestException("entityType is required")`
plus a one-line `@throws` would read the same as its neighbour and keep the
table lookup intact.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/hub/HubResource.scala:
##########
@@ -322,7 +322,10 @@ class HubResource {
@GET
@Path("/count")
def getCount(@QueryParam("entityType") entityType: EntityType): Integer = {
- val entityTables = EntityTables(entityType).base
+ val entityTables =
+ EntityTables(
+ Option(entityType).getOrElse(throw new BadRequestException("entityType
is required"))
Review Comment:
`getTops` (:424) is the other scalar `entityType` entry point into this
dispatcher and calls `EntityTables(entityType)` with no guard, so `GET
/api/hub/getTops` with no query params keeps returning the same `MatchError`
500 this fixes for `/count`.
That half is fixed by your own #8136, opened seventeen minutes after this
one — so the two are complementary and the class closes when both land.
Advisory rather than a blocker for that reason, with one caveat: merging either
alone ships the defect class half-closed, and the suite already has a
regression test for an earlier 500 in `getTops` from this same family
(`HubResourceSpec.scala:767-768`).
The list-valued endpoints are not exposed this way — JAX-RS injects an empty
list for an absent multi-valued param, and `getCounts` guards explicitly on top
of that — so it really is just these two methods.
--
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]
