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 09e68277b2 [#12598] fix(common): Reject out-of-range numeric statistic
values (#12599)
09e68277b2 is described below
commit 09e68277b2d8df18b397c2373a2ada2aef8ce04f
Author: YangJie <[email protected]>
AuthorDate: Thu Aug 27 02:50:39 2026 -0400
[#12598] fix(common): Reject out-of-range numeric statistic values (#12599)
### What changes were proposed in this pull request?
Add two range guards to `JsonUtils.getStatisticValue`:
- integral branch: `JsonNode.canConvertToLong()` before `asLong()`
- floating-point branch: `Double.isFinite()` on the result of
`asDouble()`
Both reject through `Preconditions.checkArgument`, matching how the
other read paths in this file signal bad input (`readFunctionArg` and
the partition reader both throw `IllegalArgumentException`).
The change also drops the checked-exception plumbing around the terminal
branch. It threw `UnsupportedEncodingException`, a character-encoding
error used to report a bad JSON node type, which forced
`getStatisticValue` to declare `throws IOException`, which in turn
forced the object branch to launder that exception out of a lambda
through a bare `RuntimeException`. None of it was ever reachable, before
or after this change: JSON text can only produce node types the method
already handles. The branch is reachable through
`ObjectMapper.convertValue` with an embedded binary node, which is what
the new test for it uses. It now throws `IllegalArgumentException`, and
the `throws` clause and the `try/catch` are gone.
Two `if (value != null)` checks in the recursive branches are removed as
well. `getStatisticValue` never returns null: every branch either
returns a `StatisticValues` instance or throws.
### Why are the changes needed?
A statistic value past the 64-bit range was silently replaced by a
different number, with the sign flipped in some cases.
`9223372036854775808` was stored as `-9223372036854775808`, and
`123456789012345678901234567890` as `-4362896299872285998`. An
out-of-range floating-point literal became `Infinity`, which the
serializer writes back out as the JSON string `"Infinity"`, so the value
returned as a `StringValue` on the next round trip.
`StatisticsUpdateRequest.validate()` cannot catch this: it only checks
for a null value, and it runs after Jackson has built the map, by which
point the truncated `long` is all that is left. The deserializer is the
only place where the information needed to detect the loss still exists.
`StatisticValue` has no BigInteger or BigDecimal type, so there is no
lossless representation to fall back to, and failing the request is
better than storing a wrong number.
Fix: #12598
### Does this PR introduce _any_ user-facing change?
Yes. `PUT /metalakes/{metalake}/objects/{type}/{fullName}/statistics`
and its `/partitions` variant now return 400 for a numeric statistic
value outside the `long` range, or a floating-point value that is not
finite. They previously returned 200 and stored a wrong number.
No stored data becomes unreadable. The serializer can only emit in-range
longs and finite doubles (a non-finite double goes out as the quoted
string `"Infinity"`), so nothing already persisted trips the new guards.
Rows already corrupted by the old behaviour keep their wrong value; this
change does not repair them.
No API signatures, property keys, or configuration change.
### How was this patch tested?
Four test methods in `TestJsonUtils`. All four were confirmed to fail
against the pre-fix code by reverting `JsonUtils.java` and re-running,
not by inspection:
- `testStatisticValueRejectsOutOfRangeIntegral` — both 64-bit boundaries
are still accepted; one past each boundary and two far outside are
rejected, including nested inside a list and inside an object.
- `testStatisticValueRejectsNonFiniteFloatingPoint` — `±1.5E400`,
asserting the full message including the rendered `Infinity` /
`-Infinity`. The message reports the parsed double rather than echoing
the literal, because the node Jackson hands the deserializer already
holds the infinity.
- `testStatisticsUpdateRequestRejectsOutOfRangeValue` — the real
request-body shape, where the value is `Map` content and Jackson wraps
the rejection into `JsonMappingException`, which the server maps to 400.
It uses a bare `ObjectMapper` so the assertion rests on the DTO's
`@JsonDeserialize(contentUsing = ...)` annotation rather than on a
module the test registered; removing that annotation makes the test
fail.
- `testStatisticValueRejectsUnsupportedNodeType` — the terminal branch.
It asserts `assertNull(e.getCause())`, because
`ObjectMapper.convertValue` relaunders a deserializer `IOException` into
an `IllegalArgumentException` carrying the same message, so only the
cause distinguishes our own rejection from the old checked exception.
```
./gradlew :common:test :core:test :server:test :common:javadoc
:common:spotlessCheck -PskipITs
```
passes.
Follow-ups found while working on this, not included here to keep the
change to one concern:
- `StatisticValues.doubleValue(double)` accepts `Infinity` and `NaN`, so
the write side can still produce a value this change now refuses to read
back as a double. The root fix belongs in `api` and carries its own
compatibility discussion.
- `PartitionStatisticsUpdateDTO.validate()` has no per-entry null check,
unlike `StatisticsUpdateRequest.validate()`. Jackson's `MapDeserializer`
does not invoke a `contentUsing` deserializer for a `VALUE_NULL` content
token, so a top-level JSON null reaches storage on that route.
- `JdbcPartitionStatisticStorage.parseResultSet` catches
`JsonProcessingException` to log the partition and statistic name; an
unchecked `IllegalArgumentException` bypasses that handler.
---
.../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 ad71feb557..e98bc7e387 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;
@@ -1388,13 +1387,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()) {
@@ -1403,10 +1416,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()) {
@@ -1414,20 +1424,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 84497a0505..2649667c4b 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
@@ -560,4 +567,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());
+ }
}
