bitflicker64 commented on code in PR #3209:
URL: https://github.com/apache/hugegraph/pull/3209#discussion_r4154105008
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/schema/PropertyKey.java:
##########
@@ -116,7 +116,43 @@ public PropertyKey properties(Id... properties) {
public void defineDefaultValue(Object value) {
// TODO add a field default_value
- this.userdata().put(Userdata.DEFAULT_VALUE, value);
+ this.userdata().put(Userdata.DEFAULT_VALUE,
this.normalizeDefaultValue(value));
+ }
+
+ /**
+ * The default value is kept in userdata in the runtime type of this
+ * key's data type (a JSON fraction arrives as BigDecimal: a DOUBLE key
+ * keeps a Double, a DECIMAL key the exact BigDecimal), both when the key
+ * is defined through the API and when it is read back from the backend,
+ * so the value serializes the same way on every path.
+ */
+ @Override
+ public void userdata(String key, Object value) {
+ if (Userdata.DEFAULT_VALUE.equals(key)) {
+ value = this.normalizeDefaultValue(value);
Review Comment:
Important: This override converts `~default_value` for every data type, not
only DECIMAL, and it also runs when a schema is read back:
`BinarySerializer.readPropertyKey()` sets `data_type` and `cardinality` and
then calls `readUserdata()`, which calls `userdata(key, value)` for each entry.
On master the default stays as the user sent it, and `defaultValue()` converts
it lazily.
Two things change for existing graphs without anyone opting in:
1. The stored and returned default changes form. I built hugegraph-core at
this head and created a memory-backend property key with
`schema.propertyKey("day").asDate().userdata("~default_value",
"2020-01-01").create()`. `JsonUtil.toJson(propertyKey)` now returns
`"~default_value":"2020-01-01 00:00:00.000"`, where master returns
`"2020-01-01"`. Clients that read `GET /schema/propertykeys/{name}` see a
different value, and the next write of the key persists the new form.
2. A stored default that does not convert now throws during the schema read.
On master nothing validates `~default_value` at creation (`Userdata.check()`
only rejects null), so a graph can hold a DATE key whose default is not in one
of the accepted date formats or a LIST key with a scalar default. With this
change `validValue()` throws `IllegalArgumentException` from `userdata()`
(probe: `"not-a-date"` on a DATE key, `1` on an INT LIST key), so after the
upgrade every schema lookup of that key fails. On master the same key loads,
and the error appears only when the default is actually applied.
Requested change: restrict the eager conversion to `DataType.DECIMAL`, which
is the case this PR needs, and keep the raw value for the other types as
before. Alternatively, convert only on the create/append path and never on the
backend read path, and fall back to the raw value instead of throwing when an
already stored default does not convert. Please add a test that a DATE key with
`~default_value` `"2020-01-01"` returns the same string from GET as on master.
--
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]
