Yicong-Huang commented on code in PR #8146:
URL: https://github.com/apache/texera/pull/8146#discussion_r3890869970
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/DashboardResource.scala:
##########
@@ -91,6 +91,8 @@ object DashboardResource {
@BeanParam params: SearchQueryParams,
includePublic: Boolean = false
): DashboardSearchResult = {
+ if (params.offset < 0 || params.count < 0)
+ throw new BadRequestException("start and count must be non-negative")
Review Comment:
This closes the negative-*literal* path completely, but not the arithmetic
one a line below. `:113` computes `.limit(params.count + 1)`, and `count =
Int.MaxValue` wraps to `Int.MinValue` — a negative row-count that sails past a
guard which only proved `count >= 0`.
If Postgres rejects a negative LIMIT the way it rejects the negative OFFSET
behind #8145's original 500, then `?count=2147483647` still answers invalid
pagination with a 500 rather than the 400 this establishes one line above.
Asking rather than asserting: I verified the overflow but not the end-to-end
response, which needs a build and a live database. It is also pre-existing on
main — this diff does not touch that line — which is why it is advisory. If
confirmed, the fix is a clause in the guard you already added.
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/DashboardResourceSpec.scala:
##########
@@ -123,7 +132,15 @@ class DashboardResourceSpec extends AnyFlatSpec with
Matchers {
// -- searchAllResources: the dispatch guard --------------------------------
- "searchAllResources" should "reject an unknown resourceType before it builds
any query" in {
+ "searchAllResources" should "reject a negative start" in {
Review Comment:
Both negative cases are pinned, but not the boundary the description
explicitly promises — "zero remains valid". Nothing in the suite fails if `< 0`
is later tightened to `<= 0`, which would break every default-free caller while
CI stays green.
A connection-free case costs three lines: `SearchQueryParams(offset = 0,
count = 0, resourceType = "notAResourceType")` must still throw
`IllegalArgumentException`, not `BadRequestException` — proving the guard let
zero through without needing a database.
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/DashboardResourceSpec.scala:
##########
@@ -123,7 +132,15 @@ class DashboardResourceSpec extends AnyFlatSpec with
Matchers {
// -- searchAllResources: the dispatch guard --------------------------------
- "searchAllResources" should "reject an unknown resourceType before it builds
any query" in {
+ "searchAllResources" should "reject a negative start" in {
Review Comment:
The spec's own index of what it guards no longer matches its contents. The
class Scaladoc at :33-34 still enumerates exactly two things — "the `orderBy`
translation and the resource-type dispatch guard" — and its "Breakage this
catches" list has no pagination entry, while these new cases sit under the
section header at :133 reading "searchAllResources: the dispatch guard".
Both under-describe the file now.
--
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]
