DanielLeens commented on PR #12130: URL: https://github.com/apache/seatunnel/pull/12130#issuecomment-5633101241
@SEZ9 Since @awsomesud347's F2-F8 reply looks like it may have been cut off for you the same way mine was earlier in this thread, here's what I can confirm directly against the current head (`2dd45e26f`) while you wait for a full repost — this overlaps with what I already verified in https://github.com/apache/seatunnel/pull/12130#issuecomment-5601184777, mapped to your numbering: - **F1/F5** (rows validation + overflow guard) — confirmed, as you expected: `PageBaseServlet.java:112-114` runs `rows` through the same `positiveIntParam()` helper as `page`, and `:118-122` computes the offset as `long` and rejects it before narrowing to `int` if it exceeds `Integer.MAX_VALUE`. - **F2** (`start > total` parity with the old `writeJsonWithPagination` check) — confirmed, just re-read it myself: `checkPageInRange()` at `PageBaseServlet.java:150-153` throws exactly when `pageParams.getStart() > total`, and the method's own doc comment (`:142-149`) records that `start == total` is deliberately allowed (empty page), matching the legacy behavior. So this is parity, not a tightening or loosening. - **F3** (docs / incompatible-changes location) — `docs/en/engines/zeta/rest-api-v2.md:786-795` (and the `zh` equivalent) documents the `page`/`rows` constraints and the three 400 conditions; `docs/en/introduction/concepts/incompatible-changes.md:6-12` (and `zh`) has a `## dev` entry naming all three affected endpoints. - **F4** (other paged servlets sharing the helpers) — yes, not left as a follow-up: `RunningJobsServlet` (`/running-jobs`, `/running-jobs/summary`) already calls the same `writeJsonWithPagination()` in `PageBaseServlet`, so both endpoints go through the identical validation path as `/finished-jobs/:state`. - **F6** (preconditions on `getJobsByStateJson`) — `JobInfoService.java:135-146` has explicit `state == null` / `start < 0` / `rows < 1` checks, each naming the offending argument, before `matchingJobStates(state)` runs. - **F7** (servlet-level tests) — yes: `FinishedJobsServletTest.java` (new), enabled by the package-private `FinishedJobsServlet(NodeEngineImpl, JobInfoService)` constructor at `FinishedJobsServlet.java:41-44` that makes the servlet testable without a running node engine. - **F8** (follow-up tracking) — confirmed as a follow-up in the PR description ("One cost this PR does not remove, for the record...") rather than fixed in this PR; not something this PR needs to resolve. Nothing here has moved since my last pass, and Build is still green on `2dd45e26f`. My approval stands unconditionally from a source-review standpoint — happy to have @awsomesud347 repost F2-F8 if there's anything beyond what's listed above, but these pointers should be enough to close out your confirmation pass either way. -- 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]
