SEZ9 commented on PR #12130: URL: https://github.com/apache/seatunnel/pull/12130#issuecomment-5564484946
Thanks @DanielLeens for the careful re-review, and especially for re-pulling the head into a clean worktree and confirming that `git diff --stat 96805dee1934da973893060cd7b676fc96e1e001 441455e5569482a084734cc365493a0d17f5bda2` produces no output. That settles that `441455e556` is a pure trigger-CI commit and that the code under review is identical to what was approved at `96805dee19`. Because the diff between those two commits is empty, though, it also means none of the points from my earlier pass have moved in the meantime. Before I'm comfortable merging I'd like to close the loop on each of them explicitly (either a code change or a short "won't fix, because…" is fine): 1. **`pageParams()` / `getStart()` input validation (PageBaseServlet)** – `page` is validated but `rows` is not, and `getStart()` multiplies two caller-controlled ints. Overflow can wrap to a small positive value that slips past `checkPageInRange` and silently serves the wrong page, and other bad input surfaces as raw JDK exceptions rather than the endpoint's own error message. Please validate `rows` alongside `page` and compute the start offset in a way that rejects overflow with the servlet's error response. 2. **`checkPageInRange` boundary (PageBaseServlet)** – it uses `start > total`, so a page starting exactly at `total` returns an empty page instead of being rejected. Could you confirm whether this is intentional and matches what the legacy `writeJsonWithPagination` did? If they differ, please align them (or call out the change). 3. **Duplication with `writeJsonWithPagination()` (PageBaseServlet)** – parsing/validation now lives in two places used by different servlets. Ideally the pre-existing path is routed through the new `pageParams()`/`checkPageInRange()` helpers so there is a single source of truth for pagination semantics. 4. **`getJobsByStateJson(state, start, rows)` preconditions (JobInfoService)** – the new service-level overload has no argument checks and relies on Stream internals to fail. A couple of explicit precondition checks with clear messages would make it safe for any future caller, not just `FinishedJobsServlet`. 5. **Docs (FinishedJobsServlet)** – the pagination behaviour of `GET /finished-jobs/:state` changes (source-side slicing, new out-of-range semantics), but there is no REST API documentation or release-note update. Please add a short note covering the new behaviour. 6. **Test coverage** – the added tests in `JobInfoServiceNullSafetyTest` exercise the service overload directly, but the servlet-side surface (`pageParams`, `checkPageInRange`, `writeJsonPage`, and the `FinishedJobsServlet` wiring) is untested. A servlet-level test that covers the invalid-`rows`, overflow and boundary cases from points 1–2 would be very welcome. 7. **`matchingJobStates()` cost (JobInfoService)** – it still materializes, deserializes and sorts every retained `JobState` per request; only the per-row lookups became lazy. I'm fine with that as a follow-up given the scope of this PR, but please mention it in the description so the remaining cost is documented. If you'd prefer to split items 3 and 7 into follow-up PRs, just say so and I'll track them separately; the rest I'd like addressed here. Once a new commit lands I'll do another pass promptly. <!-- streview-comment:861 --> -- 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]
