This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new 4d7f636683 [Cherry-pick to branch-1.3] [#12598] fix(common): Reject
out-of-range numeric statistic values (#12599) (#12874)
4d7f636683 is described below
commit 4d7f636683d6d56980597135a60040e135006347
Author: roryqi <[email protected]>
AuthorDate: Thu Sep 3 17:32:12 2026 +0800
[Cherry-pick to branch-1.3] [#12598] fix(common): Reject out-of-range
numeric statistic values (#12599) (#12874)
### What changes were proposed in this pull request?
Cherry-pick commit `09e68277b2d8df18b397c2373a2ada2aef8ce04f` from
#12599 to `branch-1.3`.
This change:
- Rejects integral statistic values that cannot be represented as a
signed 64-bit integer.
- Rejects floating-point statistic values that parse to a non-finite
double.
- Adds tests for boundaries, nested values, and statistics update
request deserialization.
### Why are the changes needed?
Out-of-range numeric statistic values are silently converted and stored
as different values in 1.3. For example, an integer larger than
`Long.MAX_VALUE` wraps to a negative value, while an oversized
floating-point value becomes `Infinity`.
This backport prevents silent statistic corruption by rejecting these
values.
Backport: #12599
### Does this PR introduce _any_ user-facing change?
Yes. Out-of-range numeric statistic values now result in a `400 Bad
Request` instead of returning success and storing an altered value.
No API signatures, properties, or stored-data formats are changed.
### How was this patch tested?
```shell
./gradlew :common:test --tests org.apache.gravitino.json.TestJsonUtils
:common:javadoc :common:spotlessCheck -PskipITs
```
Co-authored-by: YangJie <[email protected]>
---
.../java/org/apache/gravitino/json/JsonUtils.java | 38 ++++-----
.../org/apache/gravitino/json/TestJsonUtils.java | 93 ++++++++++++++++++++++
2 files changed, 112 insertions(+), 19 deletions(-)
diff --git a/common/src/main/java/org/apache/gravitino/json/JsonUtils.java
b/common/src/main/java/org/apache/gravitino/json/JsonUtils.java
index 2b664acc36..59a4b5f7ea 100644
--- a/common/src/main/java/org/apache/gravitino/json/JsonUtils.java
+++ b/common/src/main/java/org/apache/gravitino/json/JsonUtils.java
@@ -49,7 +49,6 @@ import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import java.io.IOException;
-import java.io.UnsupportedEncodingException;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
@@ -1367,13 +1366,27 @@ public class JsonUtils {
}
}
- private static StatisticValue<?> getStatisticValue(JsonNode node) throws
IOException {
+ private static StatisticValue<?> getStatisticValue(JsonNode node) {
Preconditions.checkArgument(
node != null && !node.isNull(), "Cannot parse statistic value from
invalid JSON: %s", node);
if (node.isIntegralNumber()) {
+ // BigInteger nodes are integral as well, and asLong() would wrap them
around silently.
+ Preconditions.checkArgument(
+ node.canConvertToLong(),
+ "Statistic value is out of the range of a 64-bit signed integer: %s",
+ node);
return StatisticValues.longValue(node.asLong());
} else if (node.isFloatingPointNumber()) {
- return StatisticValues.doubleValue(node.asDouble());
+ // Jackson parses a literal past the double range into a DoubleNode that
already holds
+ // infinity (USE_BIG_DECIMAL_FOR_FLOATS is off), and the serializer
would write that back
+ // out as the JSON string "Infinity", so the value would come back as a
string.
+ double doubleValue = node.asDouble();
+ Preconditions.checkArgument(
+ Double.isFinite(doubleValue),
+ "Statistic value is out of the range of a 64-bit floating point
number, the literal"
+ + " parsed to %s",
+ doubleValue);
+ return StatisticValues.doubleValue(doubleValue);
} else if (node.isTextual()) {
return StatisticValues.stringValue(node.asText());
} else if (node.isBoolean()) {
@@ -1382,10 +1395,7 @@ public class JsonUtils {
ArrayNode arrayNode = (ArrayNode) node;
List<StatisticValue<Object>> values =
Lists.newArrayListWithCapacity(arrayNode.size());
for (JsonNode arrayElement : arrayNode) {
- StatisticValue<?> value = getStatisticValue(arrayElement);
- if (value != null) {
- values.add((StatisticValue<Object>) value);
- }
+ values.add((StatisticValue<Object>) getStatisticValue(arrayElement));
}
return StatisticValues.listValue(values);
} else if (node.isObject()) {
@@ -1393,20 +1403,10 @@ public class JsonUtils {
Map<String, StatisticValue<?>> map = Maps.newHashMap();
objectNode
.fields()
- .forEachRemaining(
- entry -> {
- try {
- StatisticValue<?> value =
getStatisticValue(entry.getValue());
- if (value != null) {
- map.put(entry.getKey(), value);
- }
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- });
+ .forEachRemaining(entry -> map.put(entry.getKey(),
getStatisticValue(entry.getValue())));
return StatisticValues.objectValue(map);
} else {
- throw new UnsupportedEncodingException(
+ throw new IllegalArgumentException(
String.format("Don't support json node type %s",
node.getNodeType()));
}
}
diff --git a/common/src/test/java/org/apache/gravitino/json/TestJsonUtils.java
b/common/src/test/java/org/apache/gravitino/json/TestJsonUtils.java
index c99af454e3..5d29dd33be 100644
--- a/common/src/test/java/org/apache/gravitino/json/TestJsonUtils.java
+++ b/common/src/test/java/org/apache/gravitino/json/TestJsonUtils.java
@@ -23,6 +23,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.module.SimpleModule;
@@ -36,6 +37,7 @@ import
org.apache.gravitino.dto.rel.partitions.IdentityPartitionDTO;
import org.apache.gravitino.dto.rel.partitions.ListPartitionDTO;
import org.apache.gravitino.dto.rel.partitions.PartitionDTO;
import org.apache.gravitino.dto.rel.partitions.RangePartitionDTO;
+import org.apache.gravitino.dto.requests.StatisticsUpdateRequest;
import org.apache.gravitino.rel.indexes.Index;
import org.apache.gravitino.rel.types.Type;
import org.apache.gravitino.rel.types.Types;
@@ -47,6 +49,11 @@ import org.junit.jupiter.api.Test;
public class TestJsonUtils {
+ private static final String INTEGRAL_RANGE_MESSAGE =
+ "out of the range of a 64-bit signed integer";
+ private static final String FLOATING_RANGE_MESSAGE =
+ "out of the range of a 64-bit floating point number";
+
private static ObjectMapper objectMapper;
@BeforeAll
@@ -520,4 +527,90 @@ public class TestJsonUtils {
objectMapper.readValue(expectJson, StatisticValue.class),
objectMapper.readValue(objectValue, StatisticValue.class));
}
+
+ @Test
+ void testStatisticValueRejectsOutOfRangeIntegral() throws
JsonProcessingException {
+ // The 64-bit boundaries themselves must still be accepted.
+ Assertions.assertEquals(
+ StatisticValues.longValue(Long.MAX_VALUE),
+ objectMapper.readValue("9223372036854775807", StatisticValue.class));
+ Assertions.assertEquals(
+ StatisticValues.longValue(Long.MIN_VALUE),
+ objectMapper.readValue("-9223372036854775808", StatisticValue.class));
+
+ // Anything past a boundary has no lossless long representation and must
be rejected rather
+ // than wrapped around.
+ assertRejected("9223372036854775808", INTEGRAL_RANGE_MESSAGE);
+ assertRejected("-9223372036854775809", INTEGRAL_RANGE_MESSAGE);
+ assertRejected("123456789012345678901234567890", INTEGRAL_RANGE_MESSAGE);
+ assertRejected("-123456789012345678901234567890", INTEGRAL_RANGE_MESSAGE);
+
+ // The deserializer reads the whole tree and recurses in plain Java, so an
element nested in a
+ // list or an object is rejected at the same level as a scalar. This
covers those two branches,
+ // not Jackson's own nesting - see
testStatisticsUpdateRequestRejectsOutOfRangeValue for that.
+ assertRejected("[1,123456789012345678901234567890]",
INTEGRAL_RANGE_MESSAGE);
+ assertRejected("{\"key\":123456789012345678901234567890}",
INTEGRAL_RANGE_MESSAGE);
+ }
+
+ @Test
+ void testStatisticValueRejectsNonFiniteFloatingPoint() {
+ // A magnitude beyond the double range is only representable as an
infinity, which the
+ // serializer writes back out as the JSON string "Infinity" - the value
would come back as a
+ // string on the next round trip. The message reports the parsed double
rather than echoing the
+ // literal, because the node Jackson hands us already holds the infinity;
assert it in full so
+ // that stays visible.
+ assertRejected(
+ "1.5E400", FLOATING_RANGE_MESSAGE + ", the literal parsed to " +
Double.POSITIVE_INFINITY);
+ assertRejected(
+ "-1.5E400", FLOATING_RANGE_MESSAGE + ", the literal parsed to " +
Double.NEGATIVE_INFINITY);
+ }
+
+ @Test
+ void testStatisticsUpdateRequestRejectsOutOfRangeValue() {
+ // The shape the REST layer actually deserializes: the value is Map
content, so Jackson wraps
+ // the rejection into a JsonMappingException, which the server maps to
400. Use a bare mapper
+ // rather than the shared one, because setUp registers a StatisticValue
deserializer onto that
+ // singleton and the assertion would then hold even without the DTO's
@JsonDeserialize
+ // annotation. The server's ObjectMapperProvider registers no such module,
so the annotation is
+ // what has to carry the deserializer here.
+ ObjectMapper mapper = new ObjectMapper();
+ JsonMappingException e =
+ Assertions.assertThrows(
+ JsonMappingException.class,
+ () ->
+ mapper.readValue(
+ "{\"updates\":{\"rowCount\":9223372036854775808}}",
+ StatisticsUpdateRequest.class));
+
+ Assertions.assertInstanceOf(IllegalArgumentException.class, e.getCause());
+
Assertions.assertTrue(e.getCause().getMessage().contains(INTEGRAL_RANGE_MESSAGE));
+ }
+
+ @Test
+ void testStatisticValueRejectsUnsupportedNodeType() {
+ // A BINARY node cannot come from JSON text, but convertValue reaches the
terminal branch
+ // through an embedded-object token. Note that convertValue itself wraps
any IOException the
+ // deserializer throws into an IllegalArgumentException carrying the same
message, so the cause
+ // is what distinguishes our own rejection from a laundered checked
exception.
+ IllegalArgumentException e =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> objectMapper.convertValue(new byte[] {1, 2},
StatisticValue.class));
+
+ Assertions.assertTrue(
+ e.getMessage().contains("Don't support json node type BINARY"),
+ () -> "Unexpected rejection reason: " + e.getMessage());
+ Assertions.assertNull(e.getCause());
+ }
+
+ private static void assertRejected(String json, String expectedMessagePart) {
+ IllegalArgumentException e =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> objectMapper.readValue(json, StatisticValue.class));
+
+ Assertions.assertTrue(
+ e.getMessage().contains(expectedMessagePart),
+ () -> "Unexpected rejection reason: " + e.getMessage());
+ }
}
