yuqi1129 commented on PR #13670: URL: https://github.com/apache/gravitino/pull/13670#issuecomment-5953250257
Reviewed head `1759aeba0d50141e3d1354a32630296a9b4cbedc` against base `ba47e2225afe7c27d60a6b0bdee3c7c53d3317be` using the v7 checklist. No blocking code findings. I also scanned all 61 request-DTO endpoints in the main Gravitino REST service, traced the three tag-association delegation paths, checked the lineage entry point, and checked request dereferences in exception handling. I found no additional missing null-body guard of the same kind outside the three operations fixed here. Existing catalog connection testing and policy/tag association intentionally allow an omitted body and handle it safely. The permission endpoints already have guards from #12590 / #12866. This scan does not cover the separate Iceberg/Lance REST services. **[Nit] Please narrow the behavior claims in the PR description:** - `runJob` returns the new HTTP 400 only when the resource method is reached. With authorization enabled, `RunJobAuthorizationExecutor.execute()` returns `false` for a null request (lines 60–61), and `GravitinoInterceptionService` returns HTTP 403 without invoking `runJob`. Tag-association authorization has the same existing behavior, so “diverging from every other REST operation” is also too broad. This is pre-existing behavior; I am not suggesting changing authorization ordering in this PR. Please qualify the 400 guarantee accordingly. - `setCatalog` and `setMetalake` already reject null bodies before dereferencing the request in the base commit. Their immutable boolean DTO fields mean the catch-block reads are not a reachable null-body NPE. Please describe those changes as behavior-preserving operation-type reuse, rather than additional null-body fixes. Validation: JDK 17; complete non-Docker `:server:test -PskipITs` passed (64 classes, 559 tests, zero failures), and `:server:spotlessCheck` passed. All three new HTTP regression tests failed on the base implementations (expected 400, actual 500), then passed after restoring the fixes. A temporary interceptor-level test confirmed that a null run-job request returns 403 and never calls `proceed()`; it was removed after verification. Additional non-Docker `:lineage:test -PskipITs` passed (5 classes, 14 tests). Docker tests and a full authorization-enabled server HTTP test were not run. -- 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]
