Yicong-Huang commented on code in PR #8146:
URL: https://github.com/apache/texera/pull/8146#discussion_r3899532131
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/DashboardResource.scala:
##########
@@ -91,6 +91,10 @@ object DashboardResource {
@BeanParam params: SearchQueryParams,
includePublic: Boolean = false
): DashboardSearchResult = {
+ if (params.offset < 0 || params.count < 0 || params.count == Int.MaxValue)
Review Comment:
The bound now lives in three unlinked places: `Int.MaxValue` here, the
literal `2147483646` in the message below, and the `+ 1` at `:115` both are
derived from. Widen that lookahead later — a `+ 2` page-probe — and this clause
silently stops covering it while the message becomes false, CI green either way.
A named constant beside the guard would carry the reason and give the
message something to interpolate. Worth citing from `@param count` at `:67`
too, which documents no constraint today — unlike the peer
`HubResource.getTops` (`HubResource.scala:407`).
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/DashboardResourceSpec.scala:
##########
@@ -121,9 +132,30 @@ class DashboardResourceSpec extends AnyFlatSpec with
Matchers {
// every one of them maps to a non-null Field, so getColumnField never
// returns None.
- // -- searchAllResources: the dispatch guard --------------------------------
+ // -- searchAllResources: pagination and dispatch guards --------------------
+
+ "searchAllResources" should "reject a negative start" in {
+ assertInvalidPagination(SearchQueryParams(offset = -1))
+ }
+
+ it should "reject a negative count" in {
+ assertInvalidPagination(SearchQueryParams(count = -1))
+ }
+
+ it should "reject a count that overflows the query limit" in {
+ assertInvalidPagination(SearchQueryParams(count = Int.MaxValue))
+ }
+
+ it should "allow zero pagination values" in {
Review Comment:
This case proves "allowed" by asserting a *throw*, which reads as a
contradiction of its own name. The exception comes from the dispatch below the
guard, not the guard itself. The case at `:158` explains exactly that trick in
a comment; this one leaves the reader to reconstruct it.
One line above the `intercept` closes it — something like `// Zero passes
the pagination guard, so the failure has to come from the dispatch below it.`
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/DashboardResourceSpec.scala:
##########
@@ -121,9 +132,30 @@ class DashboardResourceSpec extends AnyFlatSpec with
Matchers {
// every one of them maps to a non-null Field, so getColumnField never
// returns None.
- // -- searchAllResources: the dispatch guard --------------------------------
+ // -- searchAllResources: pagination and dispatch guards --------------------
+
+ "searchAllResources" should "reject a negative start" in {
+ assertInvalidPagination(SearchQueryParams(offset = -1))
+ }
+
+ it should "reject a negative count" in {
+ assertInvalidPagination(SearchQueryParams(count = -1))
+ }
+
+ it should "reject a count that overflows the query limit" in {
Review Comment:
The rejected end of the new range is pinned here; the accepted end is not.
Nothing in the suite fails if `== Int.MaxValue` is later tightened to something
smaller — the message's "0 and 2147483646" would just quietly become a lie.
This is the mirror of the zero case four lines down, and costs the same:
```scala
it should "allow the largest valid count" in {
intercept[IllegalArgumentException](
DashboardResource.searchAllResources(
new SessionUser(new User()),
SearchQueryParams(count = Int.MaxValue - 1, resourceType =
"notAResourceType")
)
)
}
```
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/DashboardResourceSpec.scala:
##########
@@ -43,6 +44,7 @@ import org.scalatest.matchers.should.Matchers
* full match), which would start accepting junk like `SortByNameAsc`;
* - an unrecognised `orderBy` no longer degrading to "no ORDER BY", or the
* unknown-resourceType guard no longer throwing before the query is
built.
Review Comment:
Appending the new bullet left this one mid-list, so it still closes the
enumeration with a period while the three above it use `;`.
```suggestion
* unknown-resourceType guard no longer throwing before the query is
built;
```
--
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]
