DanielLeens commented on PR #12130:
URL: https://github.com/apache/seatunnel/pull/12130#issuecomment-5645047893
@SEZ9 Nothing further needed from me on F2/F3/F4/F6/F7/F8 — your plan to
verify those directly against the diff rather than the summary is the right
call, and I'd rather you check them yourself too.
On the one gap you flagged, F1/F5, I went back and re-pulled
`2dd45e26f8613e0ee682d48aed9e963fc1d004f7` to confirm the exact lines and the
test coverage, since that's the one part of your question nobody had answered
with specifics yet:
- `PageBaseServlet.java:112-114` — `rows` goes through the same
`positiveIntParam()` helper as `page`, so both reject non-integer and
non-positive input with a message naming the parameter.
- `PageBaseServlet.java:119-122` — the offset is computed as `long start =
(long) (page - 1) * rows;` and rejected with `IllegalArgumentException("Page
number exceeds total pages")` if it exceeds `Integer.MAX_VALUE`, before
narrowing to `int`. The comment right above it (`:116-118`) spells out why: an
`int` overflow here could otherwise wrap to a small positive offset that slips
past `checkPageInRange`.
- `PageBaseServlet.java:91-93` — `PageParams.getStart()` just returns the
value validated at construction, so it can't throw or wrap on its own.
For test coverage specifically, `FinishedJobsServletTest.java` does cover
all three of the inputs you asked about:
- `shouldRejectZeroRows` (`:127-133`) and `shouldRejectNegativeRows`
(`:135-141`) both assert `"Parameter 'rows' must be greater than 0"`.
- `shouldRejectOffsetThatOverflowsAnInt` (`:163-169`) sends
`page=Integer.MAX_VALUE, rows=10`, asserts `"Page number exceeds total pages"`,
and additionally verifies `jobInfoService.getJobsByStateJson(...)` is never
called — so the overflow is caught before it reaches the service layer, not
just before it reaches `checkPageInRange`.
So F1/F5 are covered both in source and in tests on the current head.
Nothing here changes my `APPROVED` — just closing the specific gap you called
out.
--
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]
