Copilot commented on code in PR #2938:
URL:
https://github.com/apache/incubator-hugegraph/pull/2938#discussion_r2690058205
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/schema/builder/EdgeLabelBuilder.java:
##########
@@ -670,7 +667,33 @@ private void checkStableVars() {
}
}
- private void checkTtl() {
+ /**
+ * Update TTL in two cases:
+ * 1) ttl > 0L: set or change a positive TTL
+ * 2) ttl == 0L and existing ttl > 0L: explicitly clear an existing TTL
+ * This allows removing TTL from a label that previously had TTL
configured.
+ * Note: ttl == null means not set, so we skip the update.
+ */
+ private void updateTTL(EdgeLabel edgeLabel) {
+ if (this.ttl == null) {
+ return;
+ }
+ if (this.ttl > 0L) {
+ edgeLabel.ttl(this.ttl);
+ if (this.ttlStartTime != null) {
+
edgeLabel.ttlStartTime(this.graph().propertyKey(this.ttlStartTime).id());
+ }
+ } else if (this.ttl == 0L && edgeLabel.ttl() > 0L) {
Review Comment:
The condition checks if `edgeLabel.ttl() > 0L` but `edgeLabel.ttl()` might
be 0 on a new edge label created without TTL. In the `build()` method, the
edgeLabel's TTL is only set through `updateTTL()`, which means if `this.ttl` is
null, the edgeLabel's TTL will remain uninitialized/default. This could cause
unexpected behavior when trying to clear TTL on labels that never had it set.
Consider checking if the existing TTL was explicitly configured or verify the
default TTL value for newly created labels.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/schema/builder/VertexLabelBuilder.java:
##########
@@ -546,7 +533,33 @@ private void checkStableVars() {
}
}
- private void checkTtl() {
+ /**
+ * Update TTL in two cases:
+ * 1) ttl > 0L: set or change a positive TTL
+ * 2) ttl == 0L and existing ttl > 0L: explicitly clear an existing TTL
+ * This allows removing TTL from a label that previously had TTL
configured.
+ * Note: ttl == null means not set, so we skip the update.
+ */
+ private void updateTTL(VertexLabel vertexLabel) {
+ if (this.ttl == null) {
+ return;
+ }
+ if (this.ttl > 0L) {
+ vertexLabel.ttl(this.ttl);
+ if (this.ttlStartTime != null) {
+
vertexLabel.ttlStartTime(this.graph().propertyKey(this.ttlStartTime).id());
+ }
+ } else if (this.ttl == 0L && vertexLabel.ttl() > 0L) {
Review Comment:
The condition checks if `vertexLabel.ttl() > 0L` but `vertexLabel.ttl()`
might be 0 on a new vertex label created without TTL. In the `build()` method,
the vertexLabel's TTL is only set through `updateTTL()`, which means if
`this.ttl` is null, the vertexLabel's TTL will remain uninitialized/default.
This could cause unexpected behavior when trying to clear TTL on labels that
never had it set. Consider checking if the existing TTL was explicitly
configured or verify the default TTL value for newly created labels.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/VertexLabelCoreTest.java:
##########
@@ -633,6 +633,84 @@ public void testAddVertexLabelWithTtl() {
assertContainsPk(ImmutableSet.of(student.ttlStartTime()), "born");
}
+ @Test
+ public void testAppendVertexLabelWithTTL() {
+ super.initPropertyKeys();
+
+ SchemaManager schema = graph().schema();
+
+ schema.propertyKey("born").asDate().ifNotExist().create();
+
+ // Create a vertex label without TTL
+ VertexLabel person = schema.vertexLabel("person")
+ .properties("name", "age", "city", "born")
+ .create();
+
+ Assert.assertNotNull(person);
+ Assert.assertEquals("person", person.name());
+ Assert.assertEquals(0L, person.ttl());
+
+ // Update the vertex label with TTL via append
+ person = schema.vertexLabel("person")
+ .ttl(86400L)
+ .append();
+
+ Assert.assertNotNull(person);
+ Assert.assertEquals("person", person.name());
+ Assert.assertEquals(86400L, person.ttl());
+ }
+
+ @Test
+ public void testAppendVertexLabelResetTTL() {
+ super.initPropertyKeys();
+
+ SchemaManager schema = graph().schema();
+
+ // Create a vertex label with TTL
+ VertexLabel person = schema.vertexLabel("person")
+ .properties("name", "age", "city")
+ .ttl(86400L)
+ .create();
+
+ Assert.assertNotNull(person);
+ Assert.assertEquals("person", person.name());
+ Assert.assertEquals(86400L, person.ttl());
+
+ // Reset TTL to 0 via append
+ person = schema.vertexLabel("person")
+ .ttl(0L)
+ .append();
+
+ Assert.assertNotNull(person);
+ Assert.assertEquals("person", person.name());
+ Assert.assertEquals(0L, person.ttl());
+ }
+
+ @Test
+ public void testAppendVertexLabelWithoutTTLShouldNotClearExistingTTL() {
+ super.initPropertyKeys();
+
+ SchemaManager schema = graph().schema();
+
+ // Create label with TTL
+ VertexLabel person = schema.vertexLabel("person")
+ .properties("name", "age", "city")
+ .ttl(86400L)
+ .create();
+
+ Assert.assertNotNull(person);
+ Assert.assertEquals(86400L, person.ttl());
+
+ // Append property WITHOUT specifying ttl
+ person = schema.vertexLabel("person")
+ .nullableKeys("city")
+ .append();
+
+ // TTL should remain unchanged
+ Assert.assertNotNull(person);
+ Assert.assertEquals(86400L, person.ttl());
Review Comment:
The test verifies TTL is preserved during append, but doesn't verify that
`ttlStartTime` is also preserved when the original label was created with a TTL
start time. Consider adding a test case that creates a label with both TTL and
TTL start time, then appends properties without specifying TTL, and verifies
both TTL and TTL start time remain unchanged.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/EdgeLabelCoreTest.java:
##########
@@ -561,6 +561,117 @@ public void testAddEdgeLabelWithTtl() {
assertContainsPk(ImmutableSet.of(write.ttlStartTime()), "date");
}
+ @Test
+ public void testAppendEdgeLabelWithTTL() {
+ super.initPropertyKeys();
+
+ SchemaManager schema = graph().schema();
+
+ schema.propertyKey("date").asDate().ifNotExist().create();
+
+ schema.vertexLabel("person")
+ .properties("name", "age", "city")
+ .primaryKeys("name")
+ .nullableKeys("city")
+ .create();
+
+ schema.vertexLabel("book")
+ .properties("name")
+ .primaryKeys("name")
+ .create();
+
+ // Create an edge label without TTL
+ EdgeLabel read = schema.edgeLabel("read").link("person", "book")
+ .properties("date", "weight")
+ .create();
+
+ Assert.assertNotNull(read);
+ Assert.assertEquals("read", read.name());
+ Assert.assertEquals(0L, read.ttl());
+
+ // Update the edge label with TTL via append
+ read = schema.edgeLabel("read")
+ .ttl(86400L)
+ .append();
+
+ Assert.assertNotNull(read);
+ Assert.assertEquals("read", read.name());
+ Assert.assertEquals(86400L, read.ttl());
+ }
+
+ @Test
+ public void testAppendEdgeLabelResetTTL() {
+ super.initPropertyKeys();
+
+ SchemaManager schema = graph().schema();
+
+ schema.vertexLabel("person")
+ .properties("name", "age", "city")
+ .primaryKeys("name")
+ .nullableKeys("city")
+ .create();
+
+ schema.vertexLabel("book")
+ .properties("name")
+ .primaryKeys("name")
+ .create();
+
+ // Create an edge label with TTL
+ EdgeLabel read = schema.edgeLabel("read").link("person", "book")
+ .properties("time", "weight")
+ .ttl(86400L)
+ .create();
+
+ Assert.assertNotNull(read);
+ Assert.assertEquals("read", read.name());
+ Assert.assertEquals(86400L, read.ttl());
+
+ // Reset TTL to 0 via append
+ read = schema.edgeLabel("read")
+ .ttl(0L)
+ .append();
+
+ Assert.assertNotNull(read);
+ Assert.assertEquals("read", read.name());
+ Assert.assertEquals(0L, read.ttl());
+ }
+
+ @Test
+ public void testAppendEdgeLabelWithoutTTLShouldNotClearExistingTTL() {
+ super.initPropertyKeys();
+
+ SchemaManager schema = graph().schema();
+
+ schema.vertexLabel("person")
+ .properties("name", "age", "city")
+ .primaryKeys("name")
+ .nullableKeys("city")
+ .create();
+
+ schema.vertexLabel("book")
+ .properties("name")
+ .primaryKeys("name")
+ .create();
+
+ // Create edge label with TTL
+ EdgeLabel read = schema.edgeLabel("read").link("person", "book")
+ .properties("time", "weight")
+ .ttl(86400L)
+ .create();
+
+ Assert.assertNotNull(read);
+ Assert.assertEquals(86400L, read.ttl());
+
+ // Append property WITHOUT specifying ttl
+ read = schema.edgeLabel("read")
+ .nullableKeys("weight")
+ .append();
+
+ // TTL should remain unchanged
+ Assert.assertNotNull(read);
+ Assert.assertEquals(86400L, read.ttl());
Review Comment:
The test verifies TTL is preserved during append, but doesn't verify that
`ttlStartTime` is also preserved when the original label was created with a TTL
start time. Consider adding a test case that creates a label with both TTL and
TTL start time, then appends properties without specifying TTL, and verifies
both TTL and TTL start time remain unchanged.
--
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]
