[#12769] fix(server): handle null request bodies in createView - #12770
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes POST .../views so that an empty/null request body no longer triggers a NullPointerException (HTTP 500 with empty body) and instead returns the existing structured client error response (HTTP 400) consistent with other REST endpoints (e.g., MetalakeOperations.createMetalake).
Changes:
- Add a null guard in
ViewOperations.createViewand route the error throughExceptionHandlers.handleViewExceptionto produce a structured HTTP 400 response. - Cache the view name after the null guard and reuse it for logging, identifier construction, and catch-path error handling.
- Add REST tests covering a null request entity and malformed JSON, registering the JSON exception mappers to validate expected HTTP 400 behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| server/src/main/java/org/apache/gravitino/server/web/rest/ViewOperations.java | Prevents null dereference on create-view requests and returns structured HTTP 400 via existing exception handling. |
| server/src/test/java/org/apache/gravitino/server/web/rest/TestViewOperations.java | Adds regression tests for null request bodies and malformed JSON, ensuring HTTP 400 + structured ErrorResponse. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code Coverage Report
Files
|
jerryshao
reviewed
Sep 1, 2026
| new IllegalArgumentException("Request body cannot be null")); | ||
| } | ||
|
|
||
| String viewName = request.getName(); |
Contributor
There was a problem hiding this comment.
Name can also be null before validation. You'd better also check nullable before logging.
Contributor
Author
There was a problem hiding this comment.
SLF4J safely logs a null name as null; returning before the log would lose that request trace. request.validate() then rejects it before use.
yuqi1129
reviewed
Sep 1, 2026
| ViewCreateRequest request) { | ||
| LOG.info( | ||
| "Received create view request: {}.{}.{}.{}", metalake, catalog, schema, request.getName()); | ||
| if (request == null) { |
Contributor
There was a problem hiding this comment.
Why do we only check null and ignore the same problem in L181?
Contributor
Author
There was a problem hiding this comment.
The original change only covered the reported createView path. alterView and the equivalent alter operations now reject null request bodies before validate(), with regression tests.
yuqi1129
approved these changes
Sep 1, 2026
jerryshao
pushed a commit
that referenced
this pull request
Sep 2, 2026
…reate/register/add operations (#12791) ### What changes were proposed in this pull request? Several `create`/`register`/`add` REST operations called `request.validate()` without first checking for a null request body, so an empty or JSON `null` body dereferenced the request and returned HTTP 500 instead of a proper 400. Applied the existing `createMetalake` pattern (null-check first, then route an `IllegalArgumentException` through the corresponding exception handler) to: - Catalog, Schema, Table, Fileset, Topic, Policy, and Tag creation - Function, Model, and JobTemplate registration - User, Group, and Role creation - Bulk user and group creation Also updated a pre-existing test in `TestGroupOperations` that had asserted the old 500 status as expected behavior, and added a `WithNullRequest` test for each fixed operation, matching the `assertNullRequestBodyRejected` pattern from #12770. ### Why are the changes needed? A null request body currently returns HTTP 500 with an internal error response for these operations, instead of a structured 400. This is inconsistent with the already-fixed `alter`/`createView` operations (#12769, #12770) and with `createMetalake`, and it leaks an internal error to the caller for what is really a bad request. Fix: #12788 ### Does this PR introduce _any_ user-facing change? Yes. A `create`/`register`/`add` request with a null or empty body now returns HTTP 400 with error code `1001`, error type `IllegalArgumentException`, and a message stating the request body cannot be null, instead of HTTP 500. ### How was this patch tested? Added a `testXxxWithNullRequest()` test for each of the 14 fixed operations, verifying HTTP 400, error code `1001`, error type `IllegalArgumentException`, and the error message, using the shared `assertNullRequestBodyRejected` helper. Ran `:server:test` scoped to the 14 affected test classes: 176 tests, all passing.
yuqi1129
pushed a commit
that referenced
this pull request
Sep 3, 2026
… body before validate() in create/register/add operations (#12791) (#12845) **Cherry-pick Information:** - Original commit: 29a566b - Target branch: `branch-1.3` - Status: ✅ Conflicts resolved, ready for review **Conflict resolution notes:** The automated cherry-pick left unresolved git conflict markers in 13 REST operation classes and `TestGroupOperations`, and introduced `BulkOperations.java`/`TestBulkOperations.java` as brand-new files whose dependencies (`org.apache.gravitino.bulk.*`, `Bulk*Request`/`Response` DTOs) do not exist on `branch-1.3`. The following manual resolution was applied: - Resolved conflict markers by keeping the incoming null-check fix in Catalog/Schema/Table/Fileset/Topic/Function/Model/Tag/Policy/Job/Role/ Group/User operations. - Fixed unused local variable warnings (errorprone `-Werror`) left by the resolution by wiring the new name variables into the existing catch-block exception handler calls, matching upstream `main`. - Removed `BulkOperations.java` and `TestBulkOperations.java`: the Bulk user/group REST feature they depend on has not been ported to `branch-1.3`. - Removed `testAddGroupWithNullRequestBodyDoesNotExposeNpe` and `testAddGroupWithExternalId` from `TestGroupOperations`: they originate from unrelated upstream commits (#12590, #11921) not present on `branch-1.3`, and leaked in via cherry-pick diff context. Verified `:server:compileJava`, `:server:compileTestJava`, and `:server:spotlessCheck` pass, and the targeted unit tests for all 13 modified operation test classes pass with 0 failures. --- ### Original PR description (#12791) ### What changes were proposed in this pull request? Several `create`/`register`/`add` REST operations called `request.validate()` without first checking for a null request body, so an empty or JSON `null` body dereferenced the request and returned HTTP 500 instead of a proper 400. Applied the existing `createMetalake` pattern (null-check first, then route an `IllegalArgumentException` through the corresponding exception handler) to: - Catalog, Schema, Table, Fileset, Topic, Policy, and Tag creation - Function, Model, and JobTemplate registration - User, Group, and Role creation ### Why are the changes needed? A null request body currently returns HTTP 500 with an internal error response for these operations, instead of a structured 400. This is inconsistent with the already-fixed `alter`/`createView` operations (#12769, #12770) and with `createMetalake`, and it leaks an internal error to the caller for what is really a bad request. Fix: #12788 ### Does this PR introduce _any_ user-facing change? Yes. A `create`/`register`/`add` request with a null or empty body now returns HTTP 400 with error code `1001`, error type `IllegalArgumentException`, and a message stating the request body cannot be null, instead of HTTP 500. ### How was this patch tested? Ran the targeted unit tests for the 13 affected test classes on `branch-1.3`: all passing with 0 failures. --------- Co-authored-by: Fayupable <90789180+Fayupable@users.noreply.github.com> Co-authored-by: Jerry Shao <jshao@apache.org> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mchades
pushed a commit
that referenced
this pull request
Sep 3, 2026
…erations (#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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
ViewOperations.createViewagainst a null request before accessing anyrequest fields or invoking
request.validate().the endpoint returns a structured HTTP 400 response.
construction, and catch-path error handling.
TestViewOperationscoverage for an empty/null request entity.mappers and returns HTTP 400.
Why are the changes needed?
An empty create-view request currently causes
ViewOperations.createViewtodereference
requestbefore null validation. This produces an unhandled HTTP500 response with an empty body instead of the structured client-error response
used by other REST endpoints.
The change follows the existing null-request handling pattern in
MetalakeOperations.createMetalake.Fix: #12769
Does this PR introduce any user-facing change?
Yes. A create-view request with an empty or null body now returns a structured
HTTP 400 response instead of an empty HTTP 500 response.
There are no API schema or configuration changes. Valid requests and malformed
JSON handling are unchanged.
How was this patch tested?
Before the fix, the null-request regression test failed with:
The malformed-JSON regression test passed independently, confirming that the
existing mapper behavior was not the cause.
After the fix:
Result: 9 tests passed, 0 failures, 0 errors.
Additional checks:
Both checks passed.