Yicong-Huang commented on code in PR #8134:
URL: https://github.com/apache/texera/pull/8134#discussion_r3939048190
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/hub/HubResourceSpec.scala:
##########
@@ -683,7 +683,8 @@ class HubResourceSpec
}
it should "reject a missing entity type" in {
- intercept[BadRequestException](hub.getCount(null))
+ val thrown = intercept[BadRequestException](hub.getCount(null))
+ thrown.getMessage shouldBe "entityType is required"
Review Comment:
This pins `"entityType is required"`, while `EntityTablesSpec:152` pins
`"Missing entityType"` for the same rule. Both suites are green, so the repo
now records two texts as correct for one behavior.
No misbehavior here — it is the duplication above showing up in the test
tier. Worth flagging because it has to move with the guard: dropping the guard
turns this assertion red, and the dispatcher's message is the one worth
asserting.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/hub/HubResource.scala:
##########
@@ -320,9 +320,12 @@ class HubResource {
.getInstance()
.createDSLContext()
+ /** @throws javax.ws.rs.BadRequestException if entityType is missing */
@GET
@Path("/count")
def getCount(@QueryParam("entityType") entityType: EntityType): Integer = {
+ if (entityType == null)
Review Comment:
Your own #8155 merged on Sep 4 as `2a21b6610`, and it is an ancestor of this
head — you rebased onto it. It put the same rejection at the dispatcher:
```scala
// EntityTables.scala:85
case null => throw new BadRequestException("Missing entityType")
```
So on this PR's parent, `GET /api/hub/count` with no `entityType` already
returns 400. This guard covers 1 of the 6 `EntityTables(...)` call sites here
and shadows what the dispatcher would do anyway.
What it does change is the message: `/hub/count` answers `entityType is
required` where every sibling hub endpoint answers `Missing entityType`.
Nothing at either site points at the other, so the next edit to the canonical
message will silently leave this one behind.
I'd close this PR (and #8129, which duplicates the already-closed #8154). If
you prefer this wording, change it once at `EntityTables.scala:85` so all six
endpoints move together — and carry the `@throws` line above over with it. That
line is the part worth keeping.
--
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]
