awsomesud347 commented on PR #12130: URL: https://github.com/apache/seatunnel/pull/12130#issuecomment-5612564374
@SEZ9 All eight are already addressed on this commit. They were answered per item in https://github.com/apache/seatunnel/pull/12130#issuecomment-5578063614, with locations in https://github.com/apache/seatunnel/pull/12130#issuecomment-5578311990, and @DanielLeens independently verified them against the file contents in https://github.com/apache/seatunnel/pull/12130#issuecomment-5601184777. Here they are once more against your F numbering, with line numbers on `2dd45e26f` so you can check each one directly. **F1 and F5**, `PageBaseServlet.java:107-124`. `rows` goes through the same `positiveIntParam()` helper as `page` at `:112-114`, so both are rejected when non-integer or below 1. The offset is computed as `long start = (long) (page - 1) * rows` at `:119` and rejected at `:120-122` if it exceeds `Integer.MAX_VALUE`, before it is narrowed to `int`. `PageParams.getStart()` at `:91-93` returns a value validated at construction, so it cannot throw and cannot wrap. `positiveIntParam()` itself is at `:126-139` and names the offending parameter rather than surfacing the JDK message. **F2**, `PageBaseServlet.java:148-152`. It uses `start > total`, which is exactly the legacy `writeJsonWithPagination` condition (`start > total || page < 1`), so a page starting precisely at `total` returns an empty page just as it always has. The javadoc at `:141-147` records that this is deliberate. **F4**, `PageBaseServlet.java:48-70`. `writeJsonWithPagination()` no longer parses or validates anything itself. It calls `pageParams(req)` at `:53`, `checkPageInRange(...)` at `:58` and `writeJsonPage(...)` at `:69`. Since `RunningJobsServlet` uses that method for both `/running-jobs` and `/running-jobs/summary`, all three paginated endpoints now share one validation path. **F6**, `JobInfoService.java:135-146`. Explicit checks for a null `state`, a negative `start` and a `rows` below 1, each with a message naming the argument, before any stream code runs. **F3**, `docs/en/engines/zeta/rest-api-v2.md:786-795` states the constraints, the 400 conditions and the empty-page carve-out, with the same content in Chinese at `docs/zh/engines/zeta/rest-api-v2.md:759-765`. There is also a `## dev` entry in `docs/en/introduction/concepts/incompatible-changes.md:8` and its Chinese equivalent, naming all three affected endpoints. **F7**, new file `FinishedJobsServletTest`, nine tests covering zero and negative `rows`, non-integer input, a page below 1, an offset that overflows an int, a page beyond the end, a page exactly at `total`, and the routing between the paged and unpaged service calls. It uses a package-private `FinishedJobsServlet(NodeEngineImpl, JobInfoService)` constructor added for that purpose. **F8**, agreed as a follow-up, and recorded in the PR description in the paragraph beginning "One cost this PR does not remove, for the record." Two of the F items describe the code before `fad034342` rather than the current head, which may be why they read as outstanding. F1's concern about an overflow wrapping past `checkPageInRange` is what `:119-122` now prevents, and the duplication in F4 was removed when `writeJsonWithPagination` was routed through the shared helpers. -- 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]
