ss666 opened a new pull request, #12866:
URL: https://github.com/apache/gravitino/pull/12866
### What changes were proposed in this pull request?
- Reject a null request body in the 20 operations listed in #12834, using
operation-level guards in 9 resource classes.
- Move the pre-existing `updateStatistics` guard out of the `Utils.doAs`
lambda to the front of the method and drop its private message constant, so all
four operations in `StatisticOperations` report the same message.
- Return the 400 for `testConnection` directly through
`Utils.illegalArguments(...)` instead of `handleTestConnectionException`, which
by design reports connection-test outcomes inside an HTTP 200 body.
- Follow the existing pattern of `MetalakeOperations.createMetalake`.
- Simplify the null-safe field extractions used for logging and are now
unreachable, in `testConnection`, `PermissionOperations` and
`StatisticOperations`.
- Add regression coverage for every fixed operation, plus a malformed-JSON
test and a normal-path `setMetalake` test.
### Why are the changes needed?
An omitted body, an empty body, or a JSON literal `null` is a client error,
but these endpoints answer it with HTTP 500.
Fix: #12834
### Does this PR introduce _any_ user-facing change?
Yes. These endpoints now return HTTP 400 with error code `1001`, error type
`IllegalArgumentException`, and a message stating that the request body cannot
be null, instead of HTTP 500.
One existing message also changes: a null body on `PUT
/api/metalakes/{metalake}/objects/{type}/{fullName}/statistics` previously
returned `Statistics update request body cannot be null` and now returns the
canonical `Request body cannot be null`. Its status code and error code are
unchanged.
There are no API schema or configuration changes, valid requests behave
exactly as before, and malformed-JSON handling is unchanged.
### How was this patch tested?
Added 21 `testXxxWithNullRequest()` tests, one per fixed operation plus one
for the deprecated tag route, using the shared `assertNullRequestBodyRejected`
helper introduced by #12770. `TestStatisticOperations` and
`TestMetadataObjectPolicyOperations` now extend `BaseOperationsTest` to use it,
and the pre-existing `testUpdateTableStatisticsWithNullRequestBody` was
converged onto it. Also added `testSetMetalakeWithMalformedJson`, which sends
`{` and asserts the response still comes from the JSON exception mappers, and
`testSetMetalake` for the normal enable/disable path, which had no coverage
before.
With the production changes reverted and only the tests applied, all 21
null-body tests fail with:
```text
expected: <400> but was: <500>
```
With the fix applied:
```shell
./gradlew :server:test -PskipITs --no-daemon # 396 tests, 0 failures,
0 errors
./gradlew :server:spotlessCheck --no-daemon
git diff --check
```
### Note for reviewers
With `gravitino.authorization.enable=true` the two `associate*` endpoints
never reach the new guard: `AssociateTagAuthorizationExecutor` /
`AssociatePolicyAuthorizationExecutor` return `false` for a null request, so
`GravitinoInterceptionService` answers 403 before the resource method runs. The
guard is still correct for the default configuration, where that service is not
registered at all. I left the executors alone as changing an authorization
decision felt out of scope; a follow-up would have to `return true` there, so
that a null body reaches the guard in the resource method and is rejected with
the 400. Happy to file that issue if you would like it handled.
--
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]
