This is an automated email from the ASF dual-hosted git repository.
jmclean pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 8b25e4b1df [#10124] refactor: Make StatisticOperations error handling
null-safe (#10200)
8b25e4b1df is described below
commit 8b25e4b1df1b9c415134e19190953b91ce82c80b
Author: dayoung <[email protected]>
AuthorDate: Thu Mar 5 08:12:23 2026 +0900
[#10124] refactor: Make StatisticOperations error handling null-safe
(#10200)
### What changes were proposed in this pull request?
This PR improves the error handling in `StatisticOperations` to avoid a
possible
`NullPointerException` in the catch path.
Previously, in both table and partition statistics update methods,
the catch block dereferenced `request.getUpdates()` to build an error
context string. If the request payload or its `updates` field was null,
this could trigger a second NPE during error handling, masking the
original
validation failure and resulting in an incorrect `500 Internal Server
Error`.
This PR makes the catch-path error formatting null-safe by:
- Introducing helper methods (`getStatisticNames`, `getPartitionNames`)
that defensively handle null `request` or null `updates`.
- Preserving the original exception and passing it to
`ExceptionHandlers`
without re-triggering another NPE.
### Why are the changes needed?
The previous implementation was fragile when request payloads were null
or invalid. A secondary NPE in the catch block could mask the original
validation error and produce incorrect server-side error responses.
This change ensures:
- The original validation exception is preserved.
- Error formatting in the catch path is null-safe.
- The API consistently returns client-facing validation errors (400)
instead of incorrectly returning 500.
Fix: #10124
### Does this PR introduce _any_ user-facing change?
No.
This change only improves internal error handling logic.
There are no API changes, configuration changes, or behavioral changes
other than preventing incorrect 500 responses in invalid request
scenarios.
### How was this patch tested?
- Added a regression test in `TestStatisticOperations`.
- The test explicitly sends a payload with `"updates": null`
and verifies that the API returns `400 Bad Request`.
- Confirmed that no secondary `NullPointerException`
is thrown during error handling.
I aligned the regression test with the existing Jersey `target(...)`
style in this test class to match its convention.
Please let me know if a white-box unit test style would be preferred
instead.
---
.../server/web/rest/StatisticOperations.java | 29 ++++++++++++++++------
.../server/web/rest/TestStatisticOperations.java | 22 ++++++++++++++++
2 files changed, 44 insertions(+), 7 deletions(-)
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/StatisticOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/StatisticOperations.java
index ce35d859a2..0c87a9f03f 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/StatisticOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/StatisticOperations.java
@@ -188,7 +188,7 @@ public class StatisticOperations {
});
} catch (Exception e) {
return ExceptionHandlers.handleStatisticException(
- OperationType.UPDATE,
StringUtils.join(request.getUpdates().keySet(), ","), fullName, e);
+ OperationType.UPDATE, getStatisticNames(request), fullName, e);
}
}
@@ -398,12 +398,7 @@ public class StatisticOperations {
fullName,
metalake,
e);
- String partitions =
- StringUtils.joinWith(
- ",",
- request.getUpdates().stream()
- .map(PartitionStatisticsUpdateDTO::partitionName)
- .collect(Collectors.toList()));
+ String partitions = getPartitionNames(request);
return ExceptionHandlers.handlePartitionStatsException(
OperationType.UPDATE, partitions, fullName, e);
}
@@ -500,4 +495,24 @@ public class StatisticOperations {
}
}
}
+
+ private static String getStatisticNames(StatisticsUpdateRequest request) {
+ if (request == null || request.getUpdates() == null) {
+ return "";
+ }
+
+ return StringUtils.join(request.getUpdates().keySet(), ",");
+ }
+
+ private static String getPartitionNames(PartitionStatisticsUpdateRequest
request) {
+ if (request == null || request.getUpdates() == null) {
+ return "";
+ }
+
+ return StringUtils.joinWith(
+ ",",
+ request.getUpdates().stream()
+ .map(PartitionStatisticsUpdateDTO::partitionName)
+ .collect(Collectors.toList()));
+ }
}
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestStatisticOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestStatisticOperations.java
index b6f6eac26a..547d79ad74 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestStatisticOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestStatisticOperations.java
@@ -732,6 +732,28 @@ public class TestStatisticOperations extends JerseyTest {
UnmodifiableStatisticException.class.getSimpleName(),
errorResp4.getType());
}
+ @Test
+ public void testUpdatePartitionStatisticsWithNullUpdates() {
+ when(tableDispatcher.tableExists(any())).thenReturn(true);
+ MetadataObject tableObject =
+ MetadataObjects.parse(
+ String.format("%s.%s.%s", catalog, schema, table),
MetadataObject.Type.TABLE);
+
+ Response resp =
+ target(
+ "/metalakes/"
+ + metalake
+ + "/objects/"
+ + tableObject.type()
+ + "/"
+ + tableObject.fullName()
+ + "/statistics/partitions")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .put(entity("{\"updates\":null}",
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
resp.getStatus());
+ }
+
@Test
public void testDropPartitionStatistics() {
List<PartitionStatisticsDropDTO> partitionStatistics =
Lists.newArrayList();
