Merge pull request #261 from aether-framework/bugfix/124-136-138-152-164-165-187-192-203-211-final-batch

Introduce stricter state validation and refine related features

Author: Splatcrafter

aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/diagnostic/DiagnosticContext.java

@@ -26,6 +26,8 @@
 import de.splatgames.aether.datafixers.api.fix.DataFixerContext;
 import org.jetbrains.annotations.NotNull;
 
+import java.util.ServiceLoader;
+
 /**
  * A diagnostic-aware context for capturing detailed migration information.
  *
@@ -94,9 +96,14 @@ static DiagnosticContext create() {
     @NotNull
     static DiagnosticContext create(@NotNull final DiagnosticOptions options) {
         Preconditions.checkNotNull(options, "options must not be null");
-        // Use ServiceLoader or direct instantiation
-        // For now, we'll use direct instantiation via the core module
-        // This will be resolved at runtime by the core implementation
+
+        // Discover implementation via ServiceLoader (preferred over reflection)
+        for (final DiagnosticContextFactory factory
+                : ServiceLoader.load(DiagnosticContextFactory.class)) {
+            return factory.create(options);
+        }
+
+        // Fallback to reflection for backward compatibility
         try {
             final Class<?> implClass = Class.forName(
                     "de.splatgames.aether.datafixers.core.diagnostic.DiagnosticContextImpl"
@@ -107,7 +114,8 @@ static DiagnosticContext create(@NotNull final DiagnosticOptions options) {
         } catch (final ReflectiveOperationException e) {
             throw new IllegalStateException(
                     "Failed to create DiagnosticContext. " +
-                            "Ensure aether-datafixers-core is on the classpath.",
+                            "Ensure aether-datafixers-core is on the classpath "
+                            + "or register a DiagnosticContextFactory via ServiceLoader.",
                     e
             );
         }

aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/diagnostic/DiagnosticContextFactory.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright (c) 2025 Splatgames.de Software and Contributors
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+package de.splatgames.aether.datafixers.api.diagnostic;
+
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Factory interface for creating {@link DiagnosticContext} instances.
+ *
+ * <p>Implementations are discovered via {@link java.util.ServiceLoader}. Register an
+ * implementation by creating a file at
+ * {@code META-INF/services/de.splatgames.aether.datafixers.api.diagnostic.DiagnosticContextFactory}
+ * containing the fully qualified class name of the factory implementation.</p>
+ *
+ * @author Erik Pförtner
+ * @see DiagnosticContext#create(DiagnosticOptions)
+ * @since 1.0.0
+ */
+public interface DiagnosticContextFactory {
+
+    /**
+     * Creates a new {@link DiagnosticContext} with the specified options.
+     *
+     * @param options the diagnostic options, must not be {@code null}
+     * @return a new diagnostic context, never {@code null}
+     */
+    @NotNull
+    DiagnosticContext create(@NotNull DiagnosticOptions options);
+}

aether-datafixers-api/src/main/java/de/splatgames/aether/datafixers/api/dynamic/DynamicOps.java

@@ -99,12 +99,21 @@ public interface DynamicOps<T> {
     // ==================== Empty/Null ====================
 
     /**
-     * Creates an empty value representation.
+     * Creates an empty/null value representation.
      *
-     * <p>Implementations typically return an empty map/object node, but the exact meaning is
-     * defined by the concrete ops.</p>
+     * <p>The returned value is the canonical "null" representation for this format.
+     * Implementations differ in how they represent null:</p>
+     * <ul>
+     *   <li>Jackson-based (JSON, YAML, XML): {@code NullNode.getInstance()}</li>
+     *   <li>SnakeYAML: {@code YamlNull} sentinel object</li>
+     *   <li>TOML: Note that TOML has no null representation — null values may cause
+     *       serialization errors</li>
+     * </ul>
+     * <p>When converting between formats via {@link #convertTo}, null representations
+     * are normalized through this method. Use {@link #isNull(Object)} to check for null
+     * regardless of the underlying representation.</p>
      *
-     * @return an empty value
+     * @return the canonical empty/null value for this format
      */
     @NotNull T empty();
 

aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/command/MigrateCommand.java

@@ -47,6 +47,9 @@
 import java.nio.file.StandardCopyOption;
 import java.time.Duration;
 import java.time.Instant;
+import java.time.ZoneOffset;
+import java.time.ZonedDateTime;
+import java.time.format.DateTimeFormatter;
 import java.util.List;
 import java.util.concurrent.Callable;
 
@@ -688,8 +691,10 @@ private void writeOutput(@NotNull final File inputFile, @NotNull final String co
             try {
                 Files.writeString(tempPath, content);
                 if (this.backup) {
+                    final String timestamp = ZonedDateTime.now(ZoneOffset.UTC)
+                            .format(DateTimeFormatter.ofPattern("yyyyMMdd'T'HHmmss'Z'"));
                     final Path backupPath = inputFile.toPath().resolveSibling(
-                            inputFile.getName() + ".bak");
+                            inputFile.getName() + ".bak." + timestamp);
                     Files.move(inputFile.toPath(), backupPath, StandardCopyOption.REPLACE_EXISTING);
                 }
                 Files.move(tempPath, inputFile.toPath(), StandardCopyOption.REPLACE_EXISTING);

aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/command/ValidateCommand.java

@@ -41,11 +41,15 @@
 import java.util.concurrent.Callable;
 
 /**
- * CLI command to validate data files and check if migration is needed.
+ * CLI command to check data file versions and determine if migration is needed.
  *
- * <p>The validate command checks data files against a target schema version
- * without performing any modifications. This is useful for batch validation,
- * CI/CD pipelines, and pre-migration checks.</p>
+ * <p>This command validates that data files contain version information and checks
+ * whether the version is at or above the target version. It does <b>not</b> validate
+ * schema compliance — only version numbers are checked. For full schema validation,
+ * use the programmatic {@code SchemaValidator} API from the schema-tools module.</p>
+ *
+ * <p>The command is useful for batch pre-migration checks in CI/CD pipelines
+ * to identify files that need migration without modifying them.</p>
  *
  * <h2>Usage Examples</h2>
  * <pre>{@code
@@ -81,7 +85,7 @@
  */
 @Command(
         name = "validate",
-        description = "Validate data files and check if migration is needed.",
+        description = "Check data file versions and report which files need migration (does not validate schema compliance).",
         mixinStandardHelpOptions = true
 )
 public class ValidateCommand implements Callable<Integer> {

aether-datafixers-cli/src/test/java/de/splatgames/aether/datafixers/cli/command/MigrateCommandTest.java

@@ -214,8 +214,13 @@ void createsBackupFiles() throws IOException {
                     "--from", "1");
 
             assertThat(exitCode).isEqualTo(0);
-            assertThat(Files.exists(file1.resolveSibling("player1.json.bak"))).isTrue();
-            assertThat(Files.exists(file2.resolveSibling("player2.json.bak"))).isTrue();
+            // Backups use timestamped names: player1.json.bak.<timestamp>
+            try (var files1 = Files.list(file1.getParent())) {
+                assertThat(files1.anyMatch(p -> p.getFileName().toString().startsWith("player1.json.bak."))).isTrue();
+            }
+            try (var files2 = Files.list(file2.getParent())) {
+                assertThat(files2.anyMatch(p -> p.getFileName().toString().startsWith("player2.json.bak."))).isTrue();
+            }
         }
 
         @Test

aether-datafixers-core/src/main/java/de/splatgames/aether/datafixers/core/diagnostic/MigrationReportImpl.java

@@ -179,6 +179,11 @@ public static final class BuilderImpl implements MigrationReport.Builder {
         private final List<RuleApplication> currentRuleApplications = new ArrayList<>();
         private String currentFixBeforeSnapshot;
 
+        // Lifecycle state
+        private boolean migrationStarted;
+        private boolean fixInProgress;
+        private boolean built;
+
         BuilderImpl() {
         }
 
@@ -192,7 +197,10 @@ public Builder startMigration(
             Preconditions.checkNotNull(type, "type must not be null");
             Preconditions.checkNotNull(fromVersion, "fromVersion must not be null");
             Preconditions.checkNotNull(toVersion, "toVersion must not be null");
+            Preconditions.checkState(!this.built, "Report already built");
+            Preconditions.checkState(!this.migrationStarted, "Migration already started");
 
+            this.migrationStarted = true;
             this.type = type;
             this.fromVersion = fromVersion;
             this.toVersion = toVersion;
@@ -212,6 +220,11 @@ public Builder setInputSnapshot(@Nullable final String snapshot) {
         @NotNull
         public Builder startFix(@NotNull final DataFix<?> fix) {
             Preconditions.checkNotNull(fix, "fix must not be null");
+            Preconditions.checkState(!this.built, "Report already built");
+            Preconditions.checkState(this.migrationStarted, "Migration not started");
+            Preconditions.checkState(!this.fixInProgress,
+                    "Fix already in progress: " + this.currentFixName);
+            this.fixInProgress = true;
             this.currentFixName = fix.name();
             this.currentFixFromVersion = fix.fromVersion();
             this.currentFixToVersion = fix.toVersion();
@@ -244,6 +257,7 @@ public Builder endFix(
                 @Nullable final String afterSnapshot
         ) {
             Preconditions.checkNotNull(fix, "fix must not be null");
+            Preconditions.checkState(this.fixInProgress, "No fix in progress");
             Preconditions.checkNotNull(duration, "duration must not be null");
 
             final FixExecution execution = new FixExecution(
@@ -270,6 +284,7 @@ public Builder endFix(
          * to prevent stale state from corrupting subsequent fix records.
          */
         private void resetFixState() {
+            this.fixInProgress = false;
             this.currentFixName = null;
             this.currentFixFromVersion = null;
             this.currentFixToVersion = null;
@@ -304,13 +319,17 @@ public Builder setOutputSnapshot(@Nullable final String snapshot) {
         @Override
         @NotNull
         public MigrationReport build() {
+            Preconditions.checkState(!this.built, "Report already built");
+            Preconditions.checkState(!this.fixInProgress,
+                    "Cannot build report while fix is in progress: " + this.currentFixName);
             if (this.type == null) {
                 throw new IllegalStateException(
                         "Migration was not started. Call startMigration() first."
                 );
             }
 
             this.endTime = Instant.now();
+            this.built = true;
             return new MigrationReportImpl(this);
         }
     }

aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/analysis/MigrationAnalyzer.java

@@ -259,6 +259,13 @@ public MigrationPath analyze() {
     /**
      * Analyzes fix coverage for the migration between configured versions.
      *
+     * <p><b>Known limitation:</b> Coverage analysis operates at the <i>type</i> level, not the
+     * <i>field</i> level. While {@link CoverageGap.Reason} defines field-level reasons
+     * ({@code FIELD_ADDED}, {@code FIELD_REMOVED}, {@code FIELD_TYPE_CHANGED}), these are
+     * not currently populated because the analysis cannot determine which specific fields
+     * a DataFix handles. If any fix exists for a type at a given version, all field changes
+     * for that type are considered covered. See {@link #checkTypeDiffCoverage} for details.</p>
+     *
      * @return the coverage analysis result, never {@code null}
      * @throws IllegalStateException if from/to versions are not set
      */

aether-datafixers-schema-tools/src/main/java/de/splatgames/aether/datafixers/schematools/introspection/TypeIntrospector.java

@@ -186,14 +186,15 @@ public static TypeKind determineKind(@NotNull final Type<?> type) {
 
         // Check reference ID for other types
         final String refId = type.reference().getId();
+        final String description = type.describe();
 
         // Primitive types
         if (PRIMITIVE_TYPE_IDS.contains(refId)) {
             return TypeKind.PRIMITIVE;
         }
 
         // Passthrough
-        if ("passthrough".equals(refId) || "...".equals(type.describe())) {
+        if ("passthrough".equals(refId) || "...".equals(description)) {
             return TypeKind.PASSTHROUGH;
         }
 
@@ -218,7 +219,7 @@ public static TypeKind determineKind(@NotNull final Type<?> type) {
         }
 
         // Named type (has "=" in description)
-        if (type.describe().contains("=")) {
+        if (description.contains("=")) {
             return TypeKind.NAMED;
         }
 

aether-datafixers-testkit/src/main/java/de/splatgames/aether/datafixers/testkit/factory/MockSchemas.java

@@ -204,6 +204,11 @@ protected void registerTypes() {
 
     /**
      * A builder for creating custom mock schemas.
+     *
+     * <p><b>Note:</b> Parent schema types are <b>not</b> automatically inherited.
+     * You must explicitly add all types needed for each schema version via
+     * {@link #withType}. Setting a parent with {@link #withParent} only establishes
+     * the parent reference for schema chain traversal, not type inheritance.</p>
      */
     public static final class SchemaBuilder {