Copilot commented on code in PR #3035:
URL: https://github.com/apache/hugegraph/pull/3035#discussion_r3336145960
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/schema/PropertyKey.java:
##########
@@ -121,7 +121,23 @@ public void defineDefaultValue(Object value) {
public Object defaultValue() {
// TODO add a field default_value
- return this.userdata().get(Userdata.DEFAULT_VALUE);
+ Object value = this.userdata().get(Userdata.DEFAULT_VALUE);
+ if (value == null) {
+ return null;
+ }
+
+ // Userdata is reloaded from JSON as a raw Map, so a typed default
+ // value (e.g. Date) comes back as a String. Normalize it to the
+ // runtime type expected by this property key's data type. Idempotent
+ // for values already of the expected type.
+ Object normalized = this.validValue(value);
Review Comment:
`defaultValue()` currently calls `validValue(value)`, but `validValue()` can
return `null` for incompatible defaults (e.g. a TEXT key with a non-String
default) without throwing. That would silently drop the default (and
`HugeElement.updateToDefaultValueIfNone()` will skip setting it). Consider
using `validValueOrThrow(value)` here so invalid schema defaults fail fast and
are easier to diagnose.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/schema/PropertyKey.java:
##########
@@ -121,7 +121,23 @@ public void defineDefaultValue(Object value) {
public Object defaultValue() {
// TODO add a field default_value
- return this.userdata().get(Userdata.DEFAULT_VALUE);
+ Object value = this.userdata().get(Userdata.DEFAULT_VALUE);
+ if (value == null) {
+ return null;
+ }
+
+ // Userdata is reloaded from JSON as a raw Map, so a typed default
+ // value (e.g. Date) comes back as a String. Normalize it to the
+ // runtime type expected by this property key's data type. Idempotent
+ // for values already of the expected type.
+ Object normalized = this.validValue(value);
+
+ // For SET cardinality, ensure we return a Set container and collapse
duplicates
+ if (this.cardinality == Cardinality.SET && normalized instanceof
Collection) {
+ return new LinkedHashSet<>((Collection<?>) normalized);
+ }
Review Comment:
The SET-handling added here makes `defaultValue()` return a `Set`, but other
callers of `validValue()` for `Cardinality.SET` can still end up with a `List`
because `convValue()` currently chooses the output container based on the
*input* collection type (`value instanceof Set`) rather than
`this.cardinality`. This means JSON-deserialized arrays (usually `ArrayList`)
may still violate SET cardinality outside `defaultValue()`. Consider fixing
`convValue()` to allocate `LinkedHashSet` whenever `this.cardinality ==
Cardinality.SET`, regardless of the input container.
##########
hugegraph-struct/src/main/java/org/apache/hugegraph/struct/schema/PropertyKey.java:
##########
@@ -126,7 +126,23 @@ public void defineDefaultValue(Object value) {
public Object defaultValue() {
// TODO add a field default_value
- return this.userdata().get(Userdata.DEFAULT_VALUE);
+ Object value = this.userdata().get(Userdata.DEFAULT_VALUE);
+ if (value == null) {
+ return null;
+ }
+
+ // Userdata is reloaded from JSON as a raw Map, so a typed default
+ // value (e.g. Date) comes back as a String. Normalize it to the
+ // runtime type expected by this property key's data type. Idempotent
+ // for values already of the expected type.
+ Object normalized = this.validValue(value);
Review Comment:
`defaultValue()` currently calls `validValue(value)`, but `validValue()` can
return `null` for incompatible defaults without throwing, which would silently
drop the default. Using `validValueOrThrow(value)` here would fail fast for
misconfigured schema defaults and avoid hard-to-debug missing defaults at
runtime.
##########
hugegraph-struct/src/main/java/org/apache/hugegraph/struct/schema/PropertyKey.java:
##########
@@ -126,7 +126,23 @@ public void defineDefaultValue(Object value) {
public Object defaultValue() {
// TODO add a field default_value
- return this.userdata().get(Userdata.DEFAULT_VALUE);
+ Object value = this.userdata().get(Userdata.DEFAULT_VALUE);
+ if (value == null) {
+ return null;
+ }
+
+ // Userdata is reloaded from JSON as a raw Map, so a typed default
+ // value (e.g. Date) comes back as a String. Normalize it to the
+ // runtime type expected by this property key's data type. Idempotent
+ // for values already of the expected type.
+ Object normalized = this.validValue(value);
+
+ // For SET cardinality, ensure we return a Set container and collapse
duplicates
+ if (this.cardinality == Cardinality.SET && normalized instanceof
Collection) {
+ return new LinkedHashSet<>((Collection<?>) normalized);
+ }
Review Comment:
This SET-specific wrapper ensures `defaultValue()` returns a `Set`, but it
doesn’t address the underlying conversion behavior: `convValue()` currently
picks `LinkedHashSet` vs `ArrayList` based on the *input* collection type
(`value instanceof Set`). For `Cardinality.SET`, JSON-deserialized
defaults/properties typically come in as `ArrayList`, so conversions in other
code paths can still yield a `List` and keep duplicates. Consider updating
`convValue()` to choose the output container based on `this.cardinality`
instead of the input container type.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/schema/PropertyKey.java:
##########
@@ -121,7 +121,23 @@ public void defineDefaultValue(Object value) {
public Object defaultValue() {
// TODO add a field default_value
- return this.userdata().get(Userdata.DEFAULT_VALUE);
+ Object value = this.userdata().get(Userdata.DEFAULT_VALUE);
+ if (value == null) {
+ return null;
+ }
+
+ // Userdata is reloaded from JSON as a raw Map, so a typed default
+ // value (e.g. Date) comes back as a String. Normalize it to the
+ // runtime type expected by this property key's data type. Idempotent
+ // for values already of the expected type.
+ Object normalized = this.validValue(value);
Review Comment:
`defaultValue()` currently calls `validValue(value)`, but `validValue()` can
return `null` for incompatible defaults (e.g. a TEXT key with a non-String
default) without throwing. That would silently drop the default (and
`HugeElement.updateToDefaultValueIfNone()` will skip setting it). Consider
using `validValueOrThrow(value)` here so invalid schema defaults fail fast and
are easier to diagnose.
##########
hugegraph-struct/src/main/java/org/apache/hugegraph/struct/schema/PropertyKey.java:
##########
@@ -126,7 +126,23 @@ public void defineDefaultValue(Object value) {
public Object defaultValue() {
// TODO add a field default_value
- return this.userdata().get(Userdata.DEFAULT_VALUE);
+ Object value = this.userdata().get(Userdata.DEFAULT_VALUE);
+ if (value == null) {
+ return null;
+ }
+
+ // Userdata is reloaded from JSON as a raw Map, so a typed default
+ // value (e.g. Date) comes back as a String. Normalize it to the
+ // runtime type expected by this property key's data type. Idempotent
+ // for values already of the expected type.
+ Object normalized = this.validValue(value);
+
+ // For SET cardinality, ensure we return a Set container and collapse
duplicates
+ if (this.cardinality == Cardinality.SET && normalized instanceof
Collection) {
+ return new LinkedHashSet<>((Collection<?>) normalized);
+ }
Review Comment:
This SET-specific wrapper ensures `defaultValue()` returns a `Set`, but it
doesn’t address the underlying conversion behavior: `convValue()` currently
picks `LinkedHashSet` vs `ArrayList` based on the *input* collection type
(`value instanceof Set`). For `Cardinality.SET`, JSON-deserialized
defaults/properties typically come in as `ArrayList`, so conversions in other
code paths can still yield a `List` and keep duplicates. Consider updating
`convValue()` to choose the output container based on `this.cardinality`
instead of the input container type.
##########
hugegraph-struct/src/main/java/org/apache/hugegraph/struct/schema/PropertyKey.java:
##########
@@ -126,7 +126,23 @@ public void defineDefaultValue(Object value) {
public Object defaultValue() {
// TODO add a field default_value
- return this.userdata().get(Userdata.DEFAULT_VALUE);
+ Object value = this.userdata().get(Userdata.DEFAULT_VALUE);
+ if (value == null) {
+ return null;
+ }
+
+ // Userdata is reloaded from JSON as a raw Map, so a typed default
+ // value (e.g. Date) comes back as a String. Normalize it to the
+ // runtime type expected by this property key's data type. Idempotent
+ // for values already of the expected type.
+ Object normalized = this.validValue(value);
Review Comment:
`defaultValue()` currently calls `validValue(value)`, but `validValue()` can
return `null` for incompatible defaults without throwing, which would silently
drop the default. Using `validValueOrThrow(value)` here would fail fast for
misconfigured schema defaults and avoid hard-to-debug missing defaults at
runtime.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/schema/PropertyKey.java:
##########
@@ -121,7 +121,23 @@ public void defineDefaultValue(Object value) {
public Object defaultValue() {
// TODO add a field default_value
- return this.userdata().get(Userdata.DEFAULT_VALUE);
+ Object value = this.userdata().get(Userdata.DEFAULT_VALUE);
+ if (value == null) {
+ return null;
+ }
+
+ // Userdata is reloaded from JSON as a raw Map, so a typed default
+ // value (e.g. Date) comes back as a String. Normalize it to the
+ // runtime type expected by this property key's data type. Idempotent
+ // for values already of the expected type.
+ Object normalized = this.validValue(value);
+
+ // For SET cardinality, ensure we return a Set container and collapse
duplicates
+ if (this.cardinality == Cardinality.SET && normalized instanceof
Collection) {
+ return new LinkedHashSet<>((Collection<?>) normalized);
+ }
Review Comment:
The SET-handling added here makes `defaultValue()` return a `Set`, but other
callers of `validValue()` for `Cardinality.SET` can still end up with a `List`
because `convValue()` currently chooses the output container based on the
*input* collection type (`value instanceof Set`) rather than
`this.cardinality`. This means JSON-deserialized arrays (usually `ArrayList`)
may still violate SET cardinality outside `defaultValue()`. Consider fixing
`convValue()` to allocate `LinkedHashSet` whenever `this.cardinality ==
Cardinality.SET`, regardless of the input container.
--
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]
