From 0fb33942403c8cdb0d0834d795dee7ab3bbe69a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Pf=C3=B6rtner?= Date: Sun, 5 Apr 2026 21:14:55 +0200 Subject: [PATCH] Refine documentation across modules for improved clarity on limitations, security, thread safety, and performance considerations. --- .../aether/datafixers/api/codec/Codecs.java | 5 +++++ .../datafixers/api/dynamic/DynamicOps.java | 12 ++++++++++++ .../datafixers/api/result/DataResult.java | 2 +- .../datafixers/api/schema/SchemaRegistry.java | 4 +++- .../datafixers/cli/command/MigrateCommand.java | 6 +++++- .../codec/xml/jackson/JacksonXmlOps.java | 6 ++++++ .../codec/yaml/snakeyaml/SnakeYamlOps.java | 10 ++++++++++ .../core/schema/SimpleSchemaRegistry.java | 5 +++-- .../schematools/analysis/MigrationAnalyzer.java | 10 +++++++--- .../introspection/TypeIntrospector.java | 6 ++++++ .../validation/ValidationResult.java | 17 ++++++++++++++++- 11 files changed, 74 insertions(+), 9 deletions(-) diff --git a/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/codec/Codecs.java b/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/codec/Codecs.java index d8061cb..45d75eb 100644 --- a/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/codec/Codecs.java +++ b/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/codec/Codecs.java @@ -567,6 +567,11 @@ public DataResult, T>> decode(@NotNull final DynamicOps ops, * GsonOps.INSTANCE, jsonElement); * } * + *

Null handling: The inner codec must never decode to {@code null}. + * If the inner codec successfully decodes but produces a null value, this codec + * returns a {@link DataResult} error. To handle absent values, the inner codec + * should fail (returning an error), which this codec maps to {@code Optional.empty()}.

+ * * @param codec the base codec for the optional value, must not be {@code null} * @param the type of the optional value * @return a new codec for {@code Optional}, never {@code null} diff --git a/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/dynamic/DynamicOps.java b/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/dynamic/DynamicOps.java index a57b69e..b95e95b 100644 --- a/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/dynamic/DynamicOps.java +++ b/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/dynamic/DynamicOps.java @@ -391,6 +391,18 @@ default T createMap(@NotNull final Map map) { /** * Converts a value from another DynamicOps representation. * + *

Cross-format edge cases: Converting between different format representations + * may lose information or fail for format-specific features:

+ *
    + *
  • Null values: TOML has no null representation; nulls may be lost or cause errors
    • + *
  • Array types: TOML requires homogeneous arrays; heterogeneous arrays will fail
    • + *
  • XML attributes: Attribute/element distinction is lost through Dynamic round-trips
    • + *
  • Numeric precision: BigDecimal handling varies between implementations
    • + *
  • Key types: SnakeYAML uses raw strings, Jackson uses TextNode for map keys
    • + *
+ *

For maximum compatibility, use data shapes that are valid across all target formats: + * string/number/boolean primitives, homogeneous arrays, and non-null values.

+ * * @param ops the source ops * @param input the input value * @param the source value type diff --git a/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/result/DataResult.java b/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/result/DataResult.java index 1aedbe6..7d2f5c1 100644 --- a/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/result/DataResult.java +++ b/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/result/DataResult.java @@ -960,7 +960,7 @@ public Optional partialResult() { if (result.isSuccess()) { return new Error<>(this.message, result.result().orElse(null)); } - return new Error<>(this.message + "; " + result.error().orElse(""), result.partialResult().orElse(null)); + return new Error<>(this.message + "\n caused by: " + result.error().orElse(""), result.partialResult().orElse(null)); } return (DataResult) this; } diff --git a/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/schema/SchemaRegistry.java b/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/schema/SchemaRegistry.java index da47be2..442b5a8 100644 --- a/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/schema/SchemaRegistry.java +++ b/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/schema/SchemaRegistry.java @@ -67,7 +67,9 @@ * } * *

Thread Safety

- *

Implementations should be thread-safe for concurrent access.

+ *

Implementations must be thread-safe for concurrent reads after + * {@code freeze()} has been called. Registration methods are not required to be + * thread-safe and should only be called during single-threaded initialization.

* * @author Erik Pförtner * @see Schema diff --git a/aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/command/MigrateCommand.java b/aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/command/MigrateCommand.java index e91ebd2..aea006b 100644 --- a/aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/command/MigrateCommand.java +++ b/aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/command/MigrateCommand.java @@ -378,13 +378,17 @@ public class MigrateCommand implements Callable { *
  • Full stack traces for errors
    • * * + *

    Security note: Full stack traces may expose internal implementation + * details, file paths, and class names. Do not expose verbose output to untrusted + * users if the CLI is wrapped in a service or web interface.

    + * *

    Default value: {@code false}

    * *

    CLI usage: {@code -v} or {@code --verbose}

    */ @Option( names = {"-v", "--verbose"}, - description = "Enable verbose output." + description = "Enable verbose output (includes stack traces; see security note in docs)." ) private boolean verbose; diff --git a/aether-datafixers-codec/src/main/java/de/splatgames/aether/datafixers/codec/xml/jackson/JacksonXmlOps.java b/aether-datafixers-codec/src/main/java/de/splatgames/aether/datafixers/codec/xml/jackson/JacksonXmlOps.java index 19e1bd5..6b7a920 100644 --- a/aether-datafixers-codec/src/main/java/de/splatgames/aether/datafixers/codec/xml/jackson/JacksonXmlOps.java +++ b/aether-datafixers-codec/src/main/java/de/splatgames/aether/datafixers/codec/xml/jackson/JacksonXmlOps.java @@ -156,6 +156,12 @@ * become child elements, but this can be customized with Jackson annotations or mapper * configuration.

    * + *

    Important: The {@code DynamicOps} model treats all properties uniformly + * as map entries. XML attribute information is not preserved through Dynamic round-trips. + * Attributes become nested map entries indistinguishable from child elements, and re-serialized + * XML may differ structurally from the original. For use cases requiring attribute fidelity, + * process XML directly with Jackson's XML annotations rather than through the Dynamic API.

    + * *

    Arrays and Repeated Elements

    *

    XML has no native array type. Arrays are typically represented as repeated elements * with the same name or wrapped in a container element. The {@link XmlMapper}'s diff --git a/aether-datafixers-codec/src/main/java/de/splatgames/aether/datafixers/codec/yaml/snakeyaml/SnakeYamlOps.java b/aether-datafixers-codec/src/main/java/de/splatgames/aether/datafixers/codec/yaml/snakeyaml/SnakeYamlOps.java index 422a511..72c97d4 100644 --- a/aether-datafixers-codec/src/main/java/de/splatgames/aether/datafixers/codec/yaml/snakeyaml/SnakeYamlOps.java +++ b/aether-datafixers-codec/src/main/java/de/splatgames/aether/datafixers/codec/yaml/snakeyaml/SnakeYamlOps.java @@ -1244,6 +1244,16 @@ public Object convertTo(@NotNull final DynamicOps sourceOps, * are immutable in Java) * * + *

    Performance Note: Unlike Jackson-based implementations which delegate to + * the optimized {@code JsonNode.deepCopy()}, this method performs a manual recursive + * copy using Java reflection-free iteration. For very large data structures, this may + * be measurably slower. Consider using Jackson-based implementations + * ({@link de.splatgames.aether.datafixers.codec.yaml.jackson.JacksonYamlOps}) for + * performance-sensitive use cases.

    + * + *
      + *
    + * *

    Performance Note

    *

    Deep copying has O(n) time and space complexity where n is the total number of * elements in the structure. For large data structures, this can be significant. diff --git a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/schema/SimpleSchemaRegistry.java b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/schema/SimpleSchemaRegistry.java index b992d62..5d08e6a 100644 --- a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/schema/SimpleSchemaRegistry.java +++ b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/schema/SimpleSchemaRegistry.java @@ -61,8 +61,9 @@ * } * *

    Thread Safety

    - *

    This implementation is not thread-safe. For concurrent access, external - * synchronization is required.

    + *

    Thread-safe for concurrent reads after {@link #freeze()} is called. + * Registration methods ({@link #register}) are not thread-safe and should + * only be called during single-threaded initialization.

    * * @author Erik Pförtner * @see SchemaRegistry diff --git a/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/analysis/MigrationAnalyzer.java b/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/analysis/MigrationAnalyzer.java index 1b1ba0d..16601c8 100644 --- a/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/analysis/MigrationAnalyzer.java +++ b/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/analysis/MigrationAnalyzer.java @@ -451,9 +451,13 @@ private void analyzeStepCoverage( * but no DataFix is registered to handle the type at this version, * a coverage gap is recorded.

    * - *

    Note: This implementation checks for the presence of any fix - * for the type. A more sophisticated implementation could verify that - * the fix actually handles all specific field changes.

    + *

    Known Limitation: This implementation only checks whether any fix + * exists for the type at this version. It does not verify that the fix handles + * all specific field changes (e.g., a fix may handle field 'armor' but not 'health'). + * This can produce false negatives where a gap exists at the field level but is not + * reported because a type-level fix is present. Verifying field-level coverage would + * require metadata in {@link DataFix} about which fields it modifies, which is not + * currently part of the API.

    * * @param type the type being analyzed, must not be {@code null} * @param sourceSchema the source schema, must not be {@code null} diff --git a/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/introspection/TypeIntrospector.java b/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/introspection/TypeIntrospector.java index 8a44481..e3cbed4 100644 --- a/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/introspection/TypeIntrospector.java +++ b/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/introspection/TypeIntrospector.java @@ -128,6 +128,12 @@ public static TypeStructure introspect(@NotNull final Type type) { *
  • Primitive: Returns an empty list
    • * * + *

    Recursive behavior: Fields are extracted recursively from nested types. + * The result includes both parent fields and their nested children using dot-notation + * paths. For example, a type with field "player" containing nested field "position" + * with sub-field "x" produces entries: {@code ["player", "player.position", + * "player.position.x"]}.

    + * * @param type the type to extract fields from, must not be {@code null} * @return a list of fields found in the type, never {@code null} * @throws NullPointerException if {@code type} is {@code null} diff --git a/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/validation/ValidationResult.java b/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/validation/ValidationResult.java index f6609c5..1396726 100644 --- a/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/validation/ValidationResult.java +++ b/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/validation/ValidationResult.java @@ -121,6 +121,18 @@ private ValidationResult(@NotNull final List issues) { this.infoCount = infos; } + private ValidationResult( + @NotNull final List issues, + final int errorCount, + final int warningCount, + final int infoCount + ) { + this.issues = List.copyOf(Preconditions.checkNotNull(issues, "issues must not be null")); + this.errorCount = errorCount; + this.warningCount = warningCount; + this.infoCount = infoCount; + } + /** * Returns an empty validation result (no issues). * @@ -321,7 +333,10 @@ public ValidationResult merge(@NotNull final ValidationResult other) { final List merged = new ArrayList<>(this.issues.size() + other.issues.size()); merged.addAll(this.issues); merged.addAll(other.issues); - return new ValidationResult(merged); + return new ValidationResult(merged, + this.errorCount + other.errorCount, + this.warningCount + other.warningCount, + this.infoCount + other.infoCount); } @Override