This is an automated email from the ASF dual-hosted git repository.
roryqi 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 4456552430 [#12657] fix(common): Validate statistic entries in
PartitionStatisticsUpdateDTO (#12658)
4456552430 is described below
commit 4456552430b07c394fb9d1582ccb303bce55bda5
Author: YangJie <[email protected]>
AuthorDate: Thu Aug 27 02:59:09 2026 -0400
[#12657] fix(common): Validate statistic entries in
PartitionStatisticsUpdateDTO (#12658)
### What changes were proposed in this pull request?
`PartitionStatisticsUpdateDTO.validate()` now walks the `statistics` map
and applies the same two per-entry checks, with the same messages, as
`StatisticsUpdateRequest.validate()`: the statistic name must not be
blank, and the statistic value must not be null.
### Why are the changes needed?
The two statistics update endpoints disagreed on the same request body.
`PUT .../statistics/partitions` with `{"custom-k": null}` returned 200
and the null reached the dispatcher, while `PUT .../statistics` with the
equivalent body returned 400.
The deserializer cannot catch this: Jackson's `MapDeserializer` does not
invoke a `contentUsing` deserializer for a `VALUE_NULL` content token,
so the null guard in `JsonUtils.getStatisticValue` never sees a
top-level null and `validate()` is the only place that can reject one.
The blank-name check matters on a second path too. The server rejects a
non-`custom-` prefixed name in `StatisticOperations`, but
`PartitionStatisticsUpdateDTO.of()` is what `clients/client-java` calls
when building a request, and there the check runs before anything is
sent.
Fix: #12657
### Does this PR introduce _any_ user-facing change?
Yes. `PUT
/metalakes/{metalake}/objects/{type}/{fullName}/statistics/partitions`
now returns 400 for a null statistic value or a blank statistic name,
matching the object-level endpoint. It previously accepted both. Java
callers going through `PartitionStatisticsUpdateDTO.of()` get an
`IllegalArgumentException` for the same inputs.
### How was this patch tested?
`TestStatisticOperations.testUpdatePartitionStatisticsWithNullStatisticValue`
drives the endpoint with a raw JSON body and asserts 400,
`ILLEGAL_ARGUMENTS_CODE`, and an error message naming the offending
statistic. Against the pre-fix code it fails with `expected: <400> but
was: <200>`. The body is sent as a raw string rather than a serialized
DTO on purpose: `of()` now rejects it, and serializing a map would risk
the client mapper dropping the null entry, which would let the test pass
on a different validation error.
New `TestPartitionStatisticsUpdateDTO` covers the DTO, which had no
tests: a null value is rejected with the statistic named, a blank name
is rejected, a valid map is accepted, and the real request body
deserializes into a map holding a null value before `validate()` rejects
it. That last assertion pins the Jackson behaviour the fix depends on;
its first half passes with or without the fix. All three rejection cases
were confirmed to fail against the pre-fix code.
```
./gradlew :common:test :server:test :core:test :clients:client-java:test
:common:javadoc :common:spotlessCheck :server:spotlessCheck -PskipITs
```
passes.
---
.../dto/stats/PartitionStatisticsUpdateDTO.java | 7 ++
.../stats/TestPartitionStatisticsUpdateDTO.java | 81 ++++++++++++++++++++++
.../server/web/rest/TestStatisticOperations.java | 41 +++++++++++
3 files changed, 129 insertions(+)
diff --git
a/common/src/main/java/org/apache/gravitino/dto/stats/PartitionStatisticsUpdateDTO.java
b/common/src/main/java/org/apache/gravitino/dto/stats/PartitionStatisticsUpdateDTO.java
index edaf035e28..3d33c5a306 100644
---
a/common/src/main/java/org/apache/gravitino/dto/stats/PartitionStatisticsUpdateDTO.java
+++
b/common/src/main/java/org/apache/gravitino/dto/stats/PartitionStatisticsUpdateDTO.java
@@ -73,6 +73,13 @@ public class PartitionStatisticsUpdateDTO implements
PartitionStatisticsUpdate {
StringUtils.isNotBlank(partitionName), "\"partitionName\" must not be
null or empty");
Preconditions.checkArgument(
statistics != null && !statistics.isEmpty(), "\"statistics\" must not
be null or empty");
+ statistics.forEach(
+ (name, value) -> {
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(name), "statistic \"name\" must not be
null or empty");
+ Preconditions.checkArgument(
+ value != null, "statistic \"value\" for '%s' must not be null",
name);
+ });
}
/**
diff --git
a/common/src/test/java/org/apache/gravitino/dto/stats/TestPartitionStatisticsUpdateDTO.java
b/common/src/test/java/org/apache/gravitino/dto/stats/TestPartitionStatisticsUpdateDTO.java
new file mode 100644
index 0000000000..b1486f5d3b
--- /dev/null
+++
b/common/src/test/java/org/apache/gravitino/dto/stats/TestPartitionStatisticsUpdateDTO.java
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.dto.stats;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.google.common.collect.ImmutableMap;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.gravitino.dto.requests.PartitionStatisticsUpdateRequest;
+import org.apache.gravitino.json.JsonUtils;
+import org.apache.gravitino.stats.StatisticValue;
+import org.apache.gravitino.stats.StatisticValues;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestPartitionStatisticsUpdateDTO {
+
+ @Test
+ public void testValidateAcceptsStatistics() {
+ PartitionStatisticsUpdateDTO dto =
+ PartitionStatisticsUpdateDTO.of(
+ "p1", ImmutableMap.of("custom-k", StatisticValues.longValue(1L)));
+
+ Assertions.assertEquals("p1", dto.partitionName());
+ Assertions.assertEquals(1, dto.statistics().size());
+ }
+
+ @Test
+ public void testValidateRejectsNullStatisticValue() {
+ Map<String, StatisticValue<?>> statistics = new HashMap<>();
+ statistics.put("custom-k", null);
+
+ IllegalArgumentException e =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> PartitionStatisticsUpdateDTO.of("p1", statistics));
+
+ Assertions.assertTrue(
+ e.getMessage().contains("custom-k"), () -> "Unexpected message: " +
e.getMessage());
+ }
+
+ @Test
+ public void testValidateRejectsBlankStatisticName() {
+ Map<String, StatisticValue<?>> statistics = new HashMap<>();
+ statistics.put(" ", StatisticValues.longValue(1L));
+
+ Assertions.assertThrows(
+ IllegalArgumentException.class, () ->
PartitionStatisticsUpdateDTO.of("p1", statistics));
+ }
+
+ @Test
+ public void testRequestWithNullStatisticValueIsRejected() throws
JsonProcessingException {
+ // Jackson's MapDeserializer does not invoke the contentUsing deserializer
for a VALUE_NULL
+ // content token, it uses getNullValue(), so a top-level JSON null lands
in the map and only
+ // validate() can catch it. Any mapper reproduces that; no registered
module is involved.
+ String json =
"{\"updates\":[{\"partitionName\":\"p1\",\"statistics\":{\"custom-k\":null}}]}";
+ PartitionStatisticsUpdateRequest request =
+ JsonUtils.objectMapper().readValue(json,
PartitionStatisticsUpdateRequest.class);
+
+ Assertions.assertNull(
+ request.getUpdates().get(0).statistics().get("custom-k"),
+ "the JSON null is expected to survive deserialization as a null map
value");
+ Assertions.assertThrows(IllegalArgumentException.class, request::validate);
+ }
+}
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 992dd73ebe..a970af8214 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
@@ -783,6 +783,47 @@ public class TestStatisticOperations extends JerseyTest {
Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
resp.getStatus());
}
+ @Test
+ public void testUpdatePartitionStatisticsWithNullStatisticValue() {
+ when(tableDispatcher.tableExists(any())).thenReturn(true);
+ MetadataObject tableObject =
+ MetadataObjects.parse(
+ String.format("%s.%s.%s", catalog, schema, table),
MetadataObject.Type.TABLE);
+
+ // Sent as raw JSON because PartitionStatisticsUpdateDTO.of rejects this
body. Jackson's
+ // MapDeserializer puts the JSON null straight into the map without
consulting the
+ // StatisticValue deserializer, so only validate() can reject it.
+ String body =
+ "{\"updates\":[{\"partitionName\":\"partition1\",\"statistics\":{\""
+ + Statistic.CUSTOM_PREFIX
+ + "test1\":null}}]}";
+
+ 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(body, MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+
+ ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
errorResp.getCode());
+ // Pin the reason: a body that lost the entry entirely would fail on
"statistics must not be
+ // null or empty" instead, which would let this test pass for the wrong
reason.
+ Assertions.assertTrue(
+ errorResp.getMessage().contains(Statistic.CUSTOM_PREFIX + "test1")
+ && errorResp.getMessage().contains("must not be null"),
+ () -> "Unexpected rejection reason: " + errorResp.getMessage());
+ }
+
@Test
public void testDropPartitionStatistics() {
List<PartitionStatisticsDropDTO> partitionStatistics =
Lists.newArrayList();
