LuciferYang opened a new pull request, #12599:
URL: https://github.com/apache/gravitino/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
