imbajin commented on code in PR #3026:
URL: https://github.com/apache/hugegraph/pull/3026#discussion_r3246761733
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/schema/SchemaElement.java:
##########
@@ -99,11 +100,37 @@ public Map<String, Object> userdata() {
public void userdata(String key, Object value) {
Review Comment:
⚠️ The two public `userdata(...)` setters now silently coerce `String →
Date` for `~create_time` and throw `IllegalArgumentException` on a malformed
string. The detailed Javadoc lives on the private helper, where IDE tooltips /
public-API readers won't see it.
A one-line note on each public setter (or on the class header) calling out
the implicit conversion would prevent surprise for callers reading the public
API.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/core/SchemaElementTest.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.unit.core;
+
+import java.util.Date;
+
+import org.apache.hugegraph.backend.id.IdGenerator;
+import org.apache.hugegraph.schema.PropertyKey;
+import org.apache.hugegraph.schema.SchemaElement;
+import org.apache.hugegraph.schema.Userdata;
+import org.apache.hugegraph.testutil.Assert;
+import org.apache.hugegraph.util.DateUtil;
+import org.junit.Test;
+
+public class SchemaElementTest {
+
+ private static final String DATE_FORMAT = "yyyy-MM-dd HH:mm:ss.SSS";
+
+ private static SchemaElement newSchema() {
+ return new PropertyKey(null, IdGenerator.of(1L), "test");
+ }
+
+ @Test
+ public void testSingleSetterNormalizesCreateTimeStringToDate() {
+ SchemaElement schema = newSchema();
+ String formatted = "2026-05-14 10:11:12.345";
+
+ schema.userdata(Userdata.CREATE_TIME, formatted);
+
+ Object value = schema.userdata().get(Userdata.CREATE_TIME);
+ Assert.assertTrue("CREATE_TIME should be a Date, was " +
+ (value == null ? "null" : value.getClass()),
+ value instanceof Date);
+ Assert.assertEquals(DateUtil.parse(formatted, DATE_FORMAT), value);
+ }
+
+ @Test
+ public void testSingleSetterKeepsCreateTimeDateUnchanged() {
+ SchemaElement schema = newSchema();
+ Date now = DateUtil.now();
+
+ schema.userdata(Userdata.CREATE_TIME, now);
+
+ Assert.assertSame(now, schema.userdata().get(Userdata.CREATE_TIME));
+ }
+
+ @Test
+ public void testSingleSetterRejectsInvalidCreateTimeString() {
+ SchemaElement schema = newSchema();
+
+ Assert.assertThrows(IllegalArgumentException.class, () -> {
+ schema.userdata(Userdata.CREATE_TIME, "not-a-date");
+ }, e -> {
+ Assert.assertContains(Userdata.CREATE_TIME, e.getMessage());
+ Assert.assertContains("not-a-date", e.getMessage());
+ });
+ }
+
+ @Test
+ public void testSingleSetterLeavesOtherStringKeysUntouched() {
+ SchemaElement schema = newSchema();
Review Comment:
⚠️ The PR specifically wraps `DateUtil.parse(...)`'s `RuntimeException` as
the cause of the thrown `IllegalArgumentException`. Worth locking that contract
in:
```suggestion
}, e -> {
Assert.assertContains(Userdata.CREATE_TIME, e.getMessage());
Assert.assertContains("not-a-date", e.getMessage());
Assert.assertNotNull(e.getCause());
});
```
Protects against a future refactor that drops the cause chain (which would
silently degrade error diagnostics).
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/schema/SchemaElement.java:
##########
@@ -99,11 +100,37 @@ public Map<String, Object> userdata() {
public void userdata(String key, Object value) {
E.checkArgumentNotNull(key, "userdata key");
E.checkArgumentNotNull(value, "userdata value");
- this.userdata.put(key, value);
+ this.userdata.put(key, normalizeUserdataValue(key, value));
}
public void userdata(Userdata userdata) {
- this.userdata.putAll(userdata);
+ E.checkArgumentNotNull(userdata, "userdata");
+ for (Map.Entry<String, Object> e : userdata.entrySet()) {
+ this.userdata.put(e.getKey(),
+ normalizeUserdataValue(e.getKey(),
e.getValue()));
+ }
+ }
+
+ /**
+ * Normalize internal userdata values whose runtime type can diverge from
+ * their serialized form. The only such key today is
+ * {@link Userdata#CREATE_TIME}: it is written as a {@link java.util.Date}
+ * but persisted as a formatted JSON string by the backend serializers,
+ * and Jackson cannot re-type a value to {@code Date} when the target is
+ * a raw {@code Map}. This method restores the original type after
+ * deserialization. Idempotent for values already of the expected type.
+ */
+ private static Object normalizeUserdataValue(String key, Object value) {
Review Comment:
⚠️ Consider moving `normalizeUserdataValue` onto `Userdata` itself (next to
`CREATE_TIME` / `DEFAULT_VALUE` constants and the existing `check()`
validator). Three benefits:
1. **Cohesion** — the rule is *about* userdata semantics, not about
`SchemaElement`.
2. **Reuse for the announced follow-up** — the PR notes that
`hugegraph-struct` has a mirror `Userdata` with the same shape; if
normalization lives on `Userdata`, the struct fix becomes a one-line delegation.
3. **Future userdata readers** — anything that hydrates a `Userdata` from
raw JSON gets the fix for free, not just the two `SchemaElement` setters.
Not blocking — the current placement works. But shifting it would prevent
re-implementing this for every userdata-bearing class.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/core/SchemaElementTest.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.unit.core;
+
+import java.util.Date;
+
+import org.apache.hugegraph.backend.id.IdGenerator;
+import org.apache.hugegraph.schema.PropertyKey;
+import org.apache.hugegraph.schema.SchemaElement;
+import org.apache.hugegraph.schema.Userdata;
+import org.apache.hugegraph.testutil.Assert;
+import org.apache.hugegraph.util.DateUtil;
+import org.junit.Test;
+
+public class SchemaElementTest {
+
+ private static final String DATE_FORMAT = "yyyy-MM-dd HH:mm:ss.SSS";
+
+ private static SchemaElement newSchema() {
+ return new PropertyKey(null, IdGenerator.of(1L), "test");
+ }
+
+ @Test
+ public void testSingleSetterNormalizesCreateTimeStringToDate() {
+ SchemaElement schema = newSchema();
+ String formatted = "2026-05-14 10:11:12.345";
+
+ schema.userdata(Userdata.CREATE_TIME, formatted);
+
+ Object value = schema.userdata().get(Userdata.CREATE_TIME);
+ Assert.assertTrue("CREATE_TIME should be a Date, was " +
+ (value == null ? "null" : value.getClass()),
+ value instanceof Date);
+ Assert.assertEquals(DateUtil.parse(formatted, DATE_FORMAT), value);
+ }
+
+ @Test
+ public void testSingleSetterKeepsCreateTimeDateUnchanged() {
+ SchemaElement schema = newSchema();
+ Date now = DateUtil.now();
+
+ schema.userdata(Userdata.CREATE_TIME, now);
+
+ Assert.assertSame(now, schema.userdata().get(Userdata.CREATE_TIME));
+ }
+
+ @Test
+ public void testSingleSetterRejectsInvalidCreateTimeString() {
+ SchemaElement schema = newSchema();
+
+ Assert.assertThrows(IllegalArgumentException.class, () -> {
+ schema.userdata(Userdata.CREATE_TIME, "not-a-date");
+ }, e -> {
+ Assert.assertContains(Userdata.CREATE_TIME, e.getMessage());
+ Assert.assertContains("not-a-date", e.getMessage());
+ });
+ }
+
+ @Test
+ public void testSingleSetterLeavesOtherStringKeysUntouched() {
+ SchemaElement schema = newSchema();
+
+ schema.userdata("note", "2026-05-14 10:11:12.345");
+
+ Object value = schema.userdata().get("note");
+ Assert.assertTrue(value instanceof String);
+ Assert.assertEquals("2026-05-14 10:11:12.345", value);
+ }
+
+ @Test
+ public void testBulkSetterNormalizesCreateTimeAndKeepsOtherEntries() {
+ SchemaElement schema = newSchema();
+ Userdata bulk = new Userdata();
+ String formatted = "2026-05-14 10:11:12.345";
+ bulk.put(Userdata.CREATE_TIME, formatted);
+ bulk.put("note", "hello");
+ bulk.put("count", 42);
+
+ schema.userdata(bulk);
+
+ Object createTime = schema.userdata().get(Userdata.CREATE_TIME);
+ Assert.assertTrue(createTime instanceof Date);
+ Assert.assertEquals(DateUtil.parse(formatted, DATE_FORMAT),
createTime);
+ Assert.assertEquals("hello", schema.userdata().get("note"));
+ Assert.assertEquals(42, schema.userdata().get("count"));
+ }
+
+ @Test
+ public void testBulkSetterKeepsCreateTimeDateUnchanged() {
+ SchemaElement schema = newSchema();
+ Userdata bulk = new Userdata();
+ Date now = DateUtil.now();
+ bulk.put(Userdata.CREATE_TIME, now);
+
+ schema.userdata(bulk);
+
+ Assert.assertSame(now, schema.userdata().get(Userdata.CREATE_TIME));
+ }
+
+ @Test
+ public void testBulkSetterRejectsNullUserdata() {
+ SchemaElement schema = newSchema();
+
+ Assert.assertThrows(IllegalArgumentException.class, () -> {
+ schema.userdata(null);
+ }, e -> {
+ Assert.assertContains("userdata", e.getMessage());
+ });
+ }
Review Comment:
⚠️ Consider one more test that asserts
`schema.userdata(Userdata.CREATE_TIME, null)` still throws
`IllegalArgumentException` from the existing `E.checkArgumentNotNull(value,
...)`.
Reason: the new helper sits between the null check and the `put`. A future
refactor that reorders these (e.g., parse first, null-check second) would
silently break the null contract — currently nothing pins it down for the
`CREATE_TIME` key specifically.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/serializer/TextSerializerTest.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.unit.serializer;
+
+import java.util.Date;
+
+import org.apache.hugegraph.backend.id.IdGenerator;
+import org.apache.hugegraph.backend.serializer.TextSerializer;
+import org.apache.hugegraph.backend.store.BackendEntry;
+import org.apache.hugegraph.config.HugeConfig;
+import org.apache.hugegraph.schema.PropertyKey;
+import org.apache.hugegraph.schema.Userdata;
+import org.apache.hugegraph.testutil.Assert;
+import org.apache.hugegraph.unit.BaseUnitTest;
+import org.apache.hugegraph.unit.FakeObjects;
+import org.apache.hugegraph.util.DateUtil;
+import org.junit.Test;
+
+public class TextSerializerTest extends BaseUnitTest {
+
+ private static final String DATE_FORMAT = "yyyy-MM-dd HH:mm:ss.SSS";
+
+ @Test
+ public void testPropertyKeyUserdataCreateTimeRoundTripsAsDate() {
+ HugeConfig config = FakeObjects.newConfig();
+ TextSerializer ser = new TextSerializer(config);
+
+ FakeObjects objects = new FakeObjects();
+ PropertyKey original = objects.newPropertyKey(IdGenerator.of(1L),
+ "name");
+ Date created = DateUtil.parse("2026-05-14 10:11:12.345", DATE_FORMAT);
+ original.userdata(Userdata.CREATE_TIME, created);
+
+ BackendEntry entry = ser.writePropertyKey(original);
+ PropertyKey reloaded = ser.readPropertyKey(objects.graph(), entry);
+
+ Object value = reloaded.userdata().get(Userdata.CREATE_TIME);
+ Assert.assertTrue("CREATE_TIME should be a Date after round-trip, " +
+ "was " + (value == null ? "null" : value.getClass()),
+ value instanceof Date);
+ Assert.assertEquals(created, value);
Review Comment:
⚠️ Two coverage gaps worth one small change each:
1. **Only `PropertyKey` is round-tripped.** `VertexLabel`, `EdgeLabel`, and
`IndexLabel` reach the helper through the *bulk* setter (`fromMap` →
`userdata(new Userdata(...))`), which is a different control path than
`readPropertyKey`'s per-entry single setter. Adding even one assertion for any
of those three would harden coverage cheaply.
2. **Near-duplicate test classes.** This file and the new
`BinarySerializerTest#testPropertyKeyUserdataCreateTimeRoundTripsAsDate` differ
only in serializer type. A `@Parameterized` test or a shared helper in
`BaseUnitTest` taking `Function<HugeConfig, AbstractSerializer>` would remove
~50 lines and stop the two from drifting.
Neither blocks merge; (1) is the more valuable of the two.
--
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]
