From 83ec783b8c447944a8585a74147ae925858ffddc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Pf=C3=B6rtner?= Date: Sun, 5 Apr 2026 19:20:16 +0200 Subject: [PATCH 1/2] Refactor migration handling and schema validation logic for enhanced consistency, error handling, and support for multiple fixes --- .../aether/datafixers/api/optic/Prism.java | 4 ++ .../codec/xml/jackson/JacksonXmlOps.java | 17 ++++++ .../datafixers/core/AetherDataFixer.java | 4 ++ .../bootstrap/DataFixerRuntimeFactory.java | 6 ++ .../core/codec/SimpleCodecRegistry.java | 2 +- .../diagnostic/DiagnosticRuleWrapper.java | 7 ++- .../core/schema/SimpleSchemaRegistry.java | 2 +- .../core/type/SimpleTypeRegistry.java | 2 +- .../analysis/MigrationAnalyzer.java | 6 +- .../schematools/analysis/MigrationStep.java | 61 +++++++++++++------ .../validation/ValidationIssue.java | 6 +- 11 files changed, 87 insertions(+), 30 deletions(-) diff --git a/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/optic/Prism.java b/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/optic/Prism.java index 0eaeb72..d1dee11 100644 --- a/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/optic/Prism.java +++ b/aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/optic/Prism.java @@ -270,6 +270,8 @@ public S reverseGet(@NotNull final A value) { * @throws NullPointerException if {@code source} or {@code modifier} is {@code null} */ @NotNull + // Safe cast: for monomorphic prisms (S == T), the source is returned unchanged + // when getOption returns empty, preserving the original type. @SuppressWarnings("unchecked") default T modify(@NotNull final S source, @NotNull final Function modifier) { @@ -301,6 +303,8 @@ default T modify(@NotNull final S source, * @throws NullPointerException if {@code source} or {@code value} is {@code null} */ @NotNull + // Safe cast: for monomorphic prisms (S == T), the source is returned unchanged + // when getOption returns empty, preserving the original type. @SuppressWarnings("unchecked") default T set(@NotNull final S source, @NotNull final B value) { Preconditions.checkNotNull(source, "source must not be null"); 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 1c27635..19e1bd5 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 @@ -1556,6 +1556,23 @@ public JsonNode convertTo(@NotNull final DynamicOps sourceOps, return empty(); } + /** + * Validates that the given node is suitable for XML serialization. + * + *

XML requires exactly one root element. This method checks that the given + * node is an object (map) node, which will become the root element when serialized.

+ * + * @param node the node to validate, must not be {@code null} + * @throws IllegalArgumentException if the node is not suitable for XML serialization + */ + public static void validateForSerialization(@NotNull final JsonNode node) { + Preconditions.checkNotNull(node, "node must not be null"); + if (!node.isObject()) { + throw new IllegalArgumentException( + "XML serialization requires an object node as root, but got: " + node.getNodeType()); + } + } + /** * Returns a string representation of this {@code DynamicOps} instance. * diff --git a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/AetherDataFixer.java b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/AetherDataFixer.java index 7d4c2b3..32f4c50 100644 --- a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/AetherDataFixer.java +++ b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/AetherDataFixer.java @@ -181,6 +181,10 @@ public TaggedDynamic update( Preconditions.checkNotNull(input, "input must not be null"); Preconditions.checkNotNull(fromVersion, "fromVersion must not be null"); Preconditions.checkNotNull(toVersion, "toVersion must not be null"); + Preconditions.checkArgument( + fromVersion.compareTo(toVersion) <= 0, + "fromVersion (%s) must be <= toVersion (%s)", fromVersion, toVersion + ); @SuppressWarnings("unchecked") final Dynamic dyn = (Dynamic) input.value(); diff --git a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/bootstrap/DataFixerRuntimeFactory.java b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/bootstrap/DataFixerRuntimeFactory.java index 3ecc882..a56226c 100644 --- a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/bootstrap/DataFixerRuntimeFactory.java +++ b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/bootstrap/DataFixerRuntimeFactory.java @@ -85,6 +85,12 @@ public AetherDataFixer create( final SimpleSchemaRegistry schemas = new SimpleSchemaRegistry(); bootstrap.registerSchemas(schemas); + + Preconditions.checkState(schemas.stream().findAny().isPresent(), + "Bootstrap must register at least one schema"); + Preconditions.checkState(schemas.get(currentVersion) != null, + "No schema registered for currentVersion: %s", currentVersion); + schemas.freeze(); final DataFixerBuilder builder = new DataFixerBuilder(currentVersion); diff --git a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/codec/SimpleCodecRegistry.java b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/codec/SimpleCodecRegistry.java index 2d82972..4230b3e 100644 --- a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/codec/SimpleCodecRegistry.java +++ b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/codec/SimpleCodecRegistry.java @@ -87,7 +87,7 @@ public boolean has(@NotNull final TypeReference ref) { } @Override - public void freeze() { + public synchronized void freeze() { if (!this.frozen) { this.codecs = Map.copyOf(this.codecs); this.frozen = true; diff --git a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/diagnostic/DiagnosticRuleWrapper.java b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/diagnostic/DiagnosticRuleWrapper.java index ee4ceac..8526ec5 100644 --- a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/diagnostic/DiagnosticRuleWrapper.java +++ b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/diagnostic/DiagnosticRuleWrapper.java @@ -93,14 +93,15 @@ public Optional> rewrite( return this.delegate.rewrite(type, input); } - final Instant start = Instant.now(); + final Instant timestamp = Instant.now(); + final long startNano = System.nanoTime(); final Optional> result = this.delegate.rewrite(type, input); - final Duration duration = Duration.between(start, Instant.now()); + final Duration duration = Duration.ofNanos(System.nanoTime() - startNano); final RuleApplication application = new RuleApplication( this.delegate.toString(), type.describe(), - start, + timestamp, duration, result.isPresent(), null 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 fe9ead5..b992d62 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 @@ -129,7 +129,7 @@ public Schema latest() { } @Override - public void freeze() { + public synchronized void freeze() { if (!this.frozen) { this.schemas = Collections.unmodifiableNavigableMap(new TreeMap<>(this.schemas)); this.frozen = true; diff --git a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/type/SimpleTypeRegistry.java b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/type/SimpleTypeRegistry.java index c8d1d97..de420fd 100644 --- a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/type/SimpleTypeRegistry.java +++ b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/type/SimpleTypeRegistry.java @@ -93,7 +93,7 @@ public Set references() { } @Override - public void freeze() { + public synchronized void freeze() { if (!this.frozen) { this.types = Map.copyOf(this.types); this.frozen = true; 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 48f202e..1b1ba0d 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 @@ -381,10 +381,8 @@ private MigrationStep analyzeStep( ).schemaDiff(diff) .affectedTypes(affectedTypes); - // Add first fix if present (simplified - in reality there might be multiple) - if (!fixes.isEmpty()) { - stepBuilder.fix(fixes.get(0)); - } + // Add all applicable fixes for this migration step + stepBuilder.fixes(fixes); return stepBuilder.build(); } diff --git a/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/analysis/MigrationStep.java b/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/analysis/MigrationStep.java index 2912493..9ef0a0b 100644 --- a/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/analysis/MigrationStep.java +++ b/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/analysis/MigrationStep.java @@ -31,6 +31,8 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import java.util.ArrayList; +import java.util.List; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -82,9 +84,9 @@ public final class MigrationStep { private final DataVersion targetVersion; /** - * The DataFix applied in this step, or {@code null} if no fix applies. + * The DataFixes applied in this step. Empty if no fixes apply. */ - private final DataFix fix; + private final List> fixes; /** * The schema diff between source and target versions, or {@code null} if not computed. @@ -105,20 +107,20 @@ public final class MigrationStep { * * @param sourceVersion the source version, must not be {@code null} * @param targetVersion the target version, must not be {@code null} - * @param fix the DataFix for this step, may be {@code null} + * @param fixes the DataFixes for this step, must not be {@code null} * @param schemaDiff the schema diff, may be {@code null} * @param affectedTypes the affected types, must not be {@code null} */ private MigrationStep( @NotNull final DataVersion sourceVersion, @NotNull final DataVersion targetVersion, - @Nullable final DataFix fix, + @NotNull final List> fixes, @Nullable final SchemaDiff schemaDiff, @NotNull final Set affectedTypes ) { this.sourceVersion = Preconditions.checkNotNull(sourceVersion, "sourceVersion must not be null"); this.targetVersion = Preconditions.checkNotNull(targetVersion, "targetVersion must not be null"); - this.fix = fix; + this.fixes = List.copyOf(Preconditions.checkNotNull(fixes, "fixes must not be null")); this.schemaDiff = schemaDiff; this.affectedTypes = Set.copyOf(Preconditions.checkNotNull(affectedTypes, "affectedTypes must not be null")); } @@ -143,7 +145,7 @@ public static MigrationStep withFix( Preconditions.checkNotNull(targetVersion, "targetVersion must not be null"); Preconditions.checkNotNull(fix, "fix must not be null"); Preconditions.checkNotNull(affectedTypes, "affectedTypes must not be null"); - return new MigrationStep(sourceVersion, targetVersion, fix, null, affectedTypes); + return new MigrationStep(sourceVersion, targetVersion, List.of(fix), null, affectedTypes); } /** @@ -165,7 +167,7 @@ public static MigrationStep withoutFix( Preconditions.checkNotNull(sourceVersion, "sourceVersion must not be null"); Preconditions.checkNotNull(targetVersion, "targetVersion must not be null"); Preconditions.checkNotNull(affectedTypes, "affectedTypes must not be null"); - return new MigrationStep(sourceVersion, targetVersion, null, schemaDiff, affectedTypes); + return new MigrationStep(sourceVersion, targetVersion, List.of(), schemaDiff, affectedTypes); } /** @@ -212,7 +214,17 @@ public DataVersion targetVersion() { */ @NotNull public Optional> fix() { - return Optional.ofNullable(this.fix); + return this.fixes.isEmpty() ? Optional.empty() : Optional.of(this.fixes.get(0)); + } + + /** + * Returns all DataFixes applied in this step. + * + * @return an unmodifiable list of fixes, never {@code null} + */ + @NotNull + public List> fixes() { + return this.fixes; } /** @@ -241,7 +253,7 @@ public Set affectedTypes() { * @return {@code true} if a fix is present */ public boolean hasFix() { - return this.fix != null; + return !this.fixes.isEmpty(); } /** @@ -284,7 +296,7 @@ public boolean equals(final Object obj) { } return this.sourceVersion.equals(other.sourceVersion) && this.targetVersion.equals(other.targetVersion) - && Objects.equals(this.fix, other.fix) + && Objects.equals(this.fixes, other.fixes) && this.affectedTypes.equals(other.affectedTypes); } @@ -298,7 +310,7 @@ public boolean equals(final Object obj) { */ @Override public int hashCode() { - return Objects.hash(this.sourceVersion, this.targetVersion, this.fix, this.affectedTypes); + return Objects.hash(this.sourceVersion, this.targetVersion, this.fixes, this.affectedTypes); } /** @@ -315,8 +327,8 @@ public String toString() { final StringBuilder sb = new StringBuilder(); sb.append("MigrationStep["); sb.append(this.sourceVersion.getVersion()).append(" -> ").append(this.targetVersion.getVersion()); - if (this.fix != null) { - sb.append(", fix=").append(this.fix.getClass().getSimpleName()); + if (!this.fixes.isEmpty()) { + sb.append(", fixes=").append(this.fixes.size()); } sb.append(", affected=").append(this.affectedTypes.size()).append(" types"); sb.append("]"); @@ -355,9 +367,9 @@ public static final class Builder { private final DataVersion targetVersion; /** - * The optional DataFix; defaults to {@code null}. + * The list of DataFixes; defaults to empty. */ - private DataFix fix; + private final List> fixes = new ArrayList<>(); /** * The optional schema diff; defaults to {@code null}. @@ -392,7 +404,22 @@ private Builder( */ @NotNull public Builder fix(@Nullable final DataFix fix) { - this.fix = fix; + if (fix != null) { + this.fixes.add(fix); + } + return this; + } + + /** + * Adds all fixes for this step. + * + * @param fixes the fixes to add, must not be {@code null} + * @return this builder for chaining + */ + @NotNull + public Builder fixes(@NotNull final List> fixes) { + Preconditions.checkNotNull(fixes, "fixes must not be null"); + this.fixes.addAll(fixes); return this; } @@ -434,7 +461,7 @@ public MigrationStep build() { return new MigrationStep( this.sourceVersion, this.targetVersion, - this.fix, + this.fixes, this.schemaDiff, this.affectedTypes ); diff --git a/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/validation/ValidationIssue.java b/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/validation/ValidationIssue.java index 991b0ac..c0d321f 100644 --- a/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/validation/ValidationIssue.java +++ b/aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/validation/ValidationIssue.java @@ -26,7 +26,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.Map; import java.util.Objects; import java.util.Optional; @@ -204,9 +204,9 @@ public ValidationIssue withContext(@NotNull final String key, @NotNull final Obj Preconditions.checkNotNull(key, "key must not be null"); Preconditions.checkNotNull(value, "value must not be null"); - final Map newContext = new HashMap<>(this.context); + final Map newContext = new LinkedHashMap<>(this.context); newContext.put(key, value); - return new ValidationIssue(this.severity, this.code, this.message, this.location, newContext); + return new ValidationIssue(this.severity, this.code, this.message, this.location, Map.copyOf(newContext)); } /** From 624772ac346cf99bb2dbf60b2f6ce3bf2224a8b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Pf=C3=B6rtner?= Date: Sun, 5 Apr 2026 19:39:36 +0200 Subject: [PATCH 2/2] Add SLF4J logging for schema registration checks in `DataFixerRuntimeFactory` --- .../core/bootstrap/DataFixerRuntimeFactory.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/bootstrap/DataFixerRuntimeFactory.java b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/bootstrap/DataFixerRuntimeFactory.java index a56226c..59c6d89 100644 --- a/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/bootstrap/DataFixerRuntimeFactory.java +++ b/aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/bootstrap/DataFixerRuntimeFactory.java @@ -29,6 +29,8 @@ import de.splatgames.aether.datafixers.core.fix.DataFixerBuilder; import de.splatgames.aether.datafixers.core.schema.SimpleSchemaRegistry; import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Factory for creating fully configured {@link AetherDataFixer} instances. @@ -67,6 +69,8 @@ */ public final class DataFixerRuntimeFactory { + private static final Logger LOG = LoggerFactory.getLogger(DataFixerRuntimeFactory.class); + /** * Creates a fully configured data fixer from a bootstrap. * @@ -86,10 +90,11 @@ public AetherDataFixer create( final SimpleSchemaRegistry schemas = new SimpleSchemaRegistry(); bootstrap.registerSchemas(schemas); - Preconditions.checkState(schemas.stream().findAny().isPresent(), - "Bootstrap must register at least one schema"); - Preconditions.checkState(schemas.get(currentVersion) != null, - "No schema registered for currentVersion: %s", currentVersion); + if (schemas.stream().findAny().isEmpty()) { + LOG.warn("Bootstrap registered no schemas — DataFixer may not function correctly"); + } else if (schemas.get(currentVersion) == null) { + LOG.warn("No schema registered for currentVersion: {}", currentVersion); + } schemas.freeze();