bitflicker64 commented on code in PR #3209:
URL: https://github.com/apache/hugegraph/pull/3209#discussion_r4068325665
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/type/define/DataType.java:
##########
@@ -143,6 +156,65 @@ public <V> Number valueToNumber(V value) {
return number;
}
+ /**
+ * Convert a value to BigDecimal: BigDecimal as is, any other Number and a
+ * decimal string through their exact decimal representation. Float and
+ * Double go through Number.toString(), i.e. the shortest string that
+ * round-trips the binary value, so a client that already holds a lossy
+ * double gets that double, exactly.
+ *
+ * @return the BigDecimal, or null if the value is not a Number or String
+ * @throws IllegalArgumentException if the string is not a decimal number
+ */
+ /*
Review Comment:
🧹 The `valueToDecimal()` Javadoc (lines 159-168) now sits above the bounds
comment and the `DECIMAL_MAX_PRECISION` / `DECIMAL_MAX_SCALE` constants, so the
Javadoc tool attaches it to `DECIMAL_MAX_PRECISION`, and `valueToDecimal()` at
line 179 has no documentation. The struct copy of `DataType` has the constants
before the Javadoc and does not have this problem.
Requested change: move the two constants and their `/* Bounds ... */`
comment above the `/** Convert a value to BigDecimal ... */` block, so the
Javadoc directly precedes `valueToDecimal()`.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/server/ObjectMapperResolver.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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.hugegraph.server;
+
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+import jakarta.ws.rs.ext.ContextResolver;
+import jakarta.ws.rs.ext.Provider;
+
+/**
+ * The Jackson mapper Jersey uses for REST request bodies.
+ *
+ * JSON fraction literals are read as BigDecimal instead of double, so a
+ * value such as {@code 12345678901234567890.10} reaches a DECIMAL property
+ * key exactly. Numeric keys are unaffected: DataType.valueToNumber accepts
+ * any Number and narrows it to the key's type as before.
+ */
+@Provider
+public class ObjectMapperResolver implements ContextResolver<ObjectMapper> {
+
+ private final ObjectMapper mapper;
+
+ public ObjectMapperResolver() {
+ this.mapper = new ObjectMapper();
+ this.mapper.enable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS);
Review Comment:
⚠️ Enabling `USE_BIG_DECIMAL_FOR_FLOATS` on the shared Jersey mapper changes
every untyped `Object` in every REST body, not only vertex and edge properties.
Combined with the `BigDecimalSerializer` this PR adds to
`registerCommonSerializers()` (which `JsonUtil` uses), any fraction that the
server re-encodes with `JsonUtil.toJson()` now comes back as a JSON string, and
existing code that expects a `Number` rejects it.
Two concrete paths:
1. Algorithm and computer jobs. `AlgorithmAPI.post()` and
`ComputerAPI.post()` take `Map<String, Object> parameters` and store
`JsonUtil.toJson(input)` as the task input. `AlgorithmJob.execute()` /
`ComputerJob` read it back with `JsonUtil.fromJson(input, Map.class)`, and
`ParameterUtil.parameterDouble()` checks `value instanceof Number`. So `POST
/graphs/{graph}/jobs/algorithm/page_rank {"alpha": 0.85}` passes the
synchronous `AlgorithmJob.check()` (the value is still a `BigDecimal` there)
and then the task fails with `Expect double value for parameter 'alpha':
'0.85'`. The same applies to `precision` in
`AbstractCommAlgorithm`/`AbstractComputer` and `alpha` in `PageRankComputer`.
2. Schema userdata. `JsonPropertyKey.userdata` (and the vertex/edge/index
label equivalents) is a `Map<String, Object>`, persisted by
`BinarySerializer`/`TextSerializer` through
`JsonUtil.toJson(schema.userdata())`. A DOUBLE key created with `"userdata":
{"~default_value": 1.5}` is stored as `"1.5"`. After a reload from the backend
(restart, schema cache miss, another server node), `PropertyKey.defaultValue()`
calls `validValueOrThrow("1.5")`, `DataType.valueToNumber()` returns null for a
`String`, and `HugeElement` line 102 throws `Invalid property value '1.5'` when
filling defaults. Before this commit the value was a `Double` and round-tripped
as a number.
I reproduced the round trip with plain Jackson 2.9: a mapper with
`USE_BIG_DECIMAL_FOR_FLOATS` reads `{"alpha":0.85}` as `BigDecimal`; a mapper
with the same `BigDecimal` to `writeString(toPlainString())` serializer writes
the task input as `{"parameters":{"alpha":"0.85",...}}`; reading it back gives
`java.lang.String`, `instanceof Number == false`. CI is green because no API
test sends a fractional algorithm parameter or a fractional default value.
Requested change: scope exact-decimal parsing to property values instead of
flipping the global mapper, for example a content deserializer on
`JsonElement.properties` (or converting `BigDecimal` back to `Double` for
DOUBLE/FLOAT keys only where the key is known), and drop the global
`ObjectMapperResolver`. If the global switch is kept, the job and userdata
paths need explicit normalisation plus API tests: an algorithm job with a
fractional parameter, and a DOUBLE property key with a fractional
`~default_value` read after the schema is reloaded.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
