diff --git a/engine-tests/src/test/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormatTest.java b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormatTest.java new file mode 100644 index 00000000000..893a3ea04e2 --- /dev/null +++ b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormatTest.java @@ -0,0 +1,58 @@ +// Copyright 2021 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.engine.logic.behavior.asset; + +import org.junit.jupiter.api.Test; +import org.terasology.gestalt.assets.ResourceUrn; +import org.terasology.gestalt.assets.format.AssetDataFile; +import org.terasology.gestalt.module.resources.FileReference; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.util.Collections; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression coverage for https://github.com/MovingBlocks/Terasology/issues/5099. A malformed behavior tree must + * fail this single asset's load with the checked {@link IOException} the {@code AssetFileFormat} contract expects + * - that's what lets gestalt's asset-loading machinery isolate the failure to just this asset (log it, move on) + * instead of an unchecked exception blowing past that safety net and crashing whatever triggered the load. + */ +public class BehaviorTreeFormatTest { + + @Test + public void malformedTreeFailsWithCheckedIOExceptionNotAnUncheckedOne() { + BehaviorTreeFormat format = new BehaviorTreeFormat(); + ResourceUrn urn = new ResourceUrn("engine:malformed"); + List source = Collections.singletonList(new AssetDataFile(jsonFile("{ selector: [success, null, success] }"))); + + IOException exception = assertThrows(IOException.class, () -> format.load(urn, source)); + + assertTrue(exception.getMessage().contains("engine:malformed")); + assertTrue(exception.getMessage().contains("selector")); + } + + private FileReference jsonFile(String json) { + return new FileReference() { + @Override + public String getName() { + return "malformed.behavior"; + } + + @Override + public List getPath() { + return Collections.emptyList(); + } + + @Override + public InputStream open() { + return new ByteArrayInputStream(json.getBytes(StandardCharsets.UTF_8)); + } + }; + } +} diff --git a/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilderTest.java b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilderTest.java new file mode 100644 index 00000000000..a3fe5026e45 --- /dev/null +++ b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilderTest.java @@ -0,0 +1,60 @@ +// Copyright 2021 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.engine.logic.behavior.core; + +import com.google.gson.JsonParseException; +import org.junit.jupiter.api.Test; +import org.terasology.engine.logic.behavior.actions.InvertAction; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression coverage for https://github.com/MovingBlocks/Terasology/issues/5099: a malformed behavior tree + * (e.g. a stray/trailing comma producing a null array entry) used to parse "successfully" with a silent + * {@code null} child, only to crash later with an unrelated NPE deep in tree execution/copying + * ({@link org.terasology.engine.logic.behavior.core.SelectorNode#deepCopy()} or + * {@link org.terasology.engine.logic.behavior.DefaultBehaviorTreeRunner}). It should instead fail fast, at + * load time, with a message that points at the malformed tree. + */ +public class BehaviorTreeBuilderTest { + + private final BehaviorTreeBuilder builder = new BehaviorTreeBuilder(); + + @Test + public void validTreeStillParses() { + BehaviorNode node = builder.fromJson("{ selector: [success, failure, success] }"); + + assertEquals(3, node.getChildrenCount()); + } + + @Test + public void nullChildInCompositeArrayFailsLoudlyInsteadOfLater() { + JsonParseException exception = assertThrows(JsonParseException.class, + () -> builder.fromJson("{ selector: [success, null, success] }")); + + assertTrue(exception.getMessage().contains("selector")); + assertTrue(exception.getMessage().contains("index 1")); + } + + @Test + public void nullChildInDecoratorFailsLoudlyInsteadOfLater() { + builder.registerDecorator("invert", InvertAction.class); + + JsonParseException exception = assertThrows(JsonParseException.class, + () -> builder.fromJson("{ invert: { child: null } }")); + + assertTrue(exception.getMessage().contains("invert")); + } + + @Test + public void missingChildInDecoratorFailsLoudlyInsteadOfLater() { + builder.registerDecorator("invert", InvertAction.class); + + JsonParseException exception = assertThrows(JsonParseException.class, + () -> builder.fromJson("{ invert: {} }")); + + assertTrue(exception.getMessage().contains("invert")); + } +} diff --git a/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairToolTest.java b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairToolTest.java new file mode 100644 index 00000000000..a6f7a5a4c19 --- /dev/null +++ b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairToolTest.java @@ -0,0 +1,130 @@ +// Copyright 2021 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.engine.logic.behavior.core; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * See https://github.com/MovingBlocks/Terasology/issues/5099. The tool exists so a content author who hits the + * load-time rejection from {@link BehaviorTreeBuilder} doesn't have to hand-edit the file's JSON to find and + * remove the offending comma - it does the same fix mechanically. + */ +public class BehaviorTreeRepairToolTest { + + @Test + public void trailingCommaInCompositeArrayIsRemoved() { + BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.clean( + "{ \"selector\": [\"success\", \"success\",] }"); + + assertTrue(result.changed); + assertEquals(1, result.nullArrayEntriesRemoved); + assertTrue(result.unfixableIssues.isEmpty()); + // The cleaned JSON should load through the real builder with no custom actions/decorators needed. + BehaviorNode node = new BehaviorTreeBuilder().fromJson(result.cleanedJson); + assertEquals(2, node.getChildrenCount()); + } + + @Test + public void explicitNullInCompositeArrayIsRemoved() { + BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.clean( + "{ \"selector\": [\"success\", null, \"success\"] }"); + + assertTrue(result.changed); + assertEquals(1, result.nullArrayEntriesRemoved); + BehaviorNode node = new BehaviorTreeBuilder().fromJson(result.cleanedJson); + assertEquals(2, node.getChildrenCount()); + } + + @Test + public void nothingToFixIsReportedAsUnchanged() { + BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.clean( + "{ \"selector\": [\"success\", \"success\"] }"); + + assertFalse(result.changed); + assertEquals(0, result.nullArrayEntriesRemoved); + assertTrue(result.unfixableIssues.isEmpty()); + } + + @Test + public void explicitNullDecoratorChildIsReportedNotSilentlyDropped() { + BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.clean( + "{ \"invert\": { \"child\": null } }"); + + assertFalse(result.unfixableIssues.isEmpty()); + } + + @Test + public void repairWritesTheFileAndKeepsABackup(@TempDir Path dir) throws IOException { + Path file = dir.resolve("broken.behavior"); + String original = "{ \"selector\": [\"success\", \"success\",] }"; + Files.write(file, original.getBytes(StandardCharsets.UTF_8)); + + BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.repair(file); + + assertTrue(result.changed); + Path backup = dir.resolve("broken.behavior.bak"); + assertTrue(Files.exists(backup)); + assertEquals(original, new String(Files.readAllBytes(backup), StandardCharsets.UTF_8)); + BehaviorNode node = new BehaviorTreeBuilder().fromJson( + new String(Files.readAllBytes(file), StandardCharsets.UTF_8)); + assertEquals(2, node.getChildrenCount()); + } + + @Test + public void repairLeavesUnfixableFileUntouched(@TempDir Path dir) throws IOException { + Path file = dir.resolve("broken.behavior"); + String original = "{ \"invert\": { \"child\": null } }"; + Files.write(file, original.getBytes(StandardCharsets.UTF_8)); + + BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.repair(file); + + assertFalse(result.changed); + assertFalse(Files.exists(dir.resolve("broken.behavior.bak"))); + assertEquals(original, new String(Files.readAllBytes(file), StandardCharsets.UTF_8)); + } + + @Test + public void repairNeverClobbersAnExistingBackup(@TempDir Path dir) throws IOException { + Path file = dir.resolve("broken.behavior"); + String original = "{ \"selector\": [\"success\", \"success\",] }"; + Files.write(file, original.getBytes(StandardCharsets.UTF_8)); + Path existingBackup = dir.resolve("broken.behavior.bak"); + String someoneElsesBackup = "not the original - already there before repair() ran"; + Files.write(existingBackup, someoneElsesBackup.getBytes(StandardCharsets.UTF_8)); + + BehaviorTreeRepairTool.repair(file); + + assertEquals(someoneElsesBackup, new String(Files.readAllBytes(existingBackup), StandardCharsets.UTF_8)); + Path versionedBackup = dir.resolve("broken.behavior.bak.1"); + assertTrue(Files.exists(versionedBackup)); + assertEquals(original, new String(Files.readAllBytes(versionedBackup), StandardCharsets.UTF_8)); + } + + @Test + public void repairLeavesOnlyTheFinalFileBehindNoStrayTempFiles(@TempDir Path dir) throws IOException { + Path file = dir.resolve("broken.behavior"); + Files.write(file, "{ \"selector\": [\"success\", \"success\",] }".getBytes(StandardCharsets.UTF_8)); + + BehaviorTreeRepairTool.repair(file); + + try (Stream entries = Files.list(dir)) { + List names = entries.map(p -> p.getFileName().toString()) + .sorted().collect(Collectors.toList()); + assertEquals(Arrays.asList("broken.behavior", "broken.behavior.bak"), names); + } + } +} diff --git a/engine/src/main/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormat.java b/engine/src/main/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormat.java index e86669e61a7..926686d01cf 100644 --- a/engine/src/main/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormat.java +++ b/engine/src/main/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormat.java @@ -3,6 +3,7 @@ package org.terasology.engine.logic.behavior.asset; import com.google.common.base.Charsets; +import com.google.gson.JsonParseException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.terasology.gestalt.assets.ResourceUrn; @@ -53,6 +54,12 @@ public BehaviorTreeData load(ResourceUrn resourceUrn, List list) } try (InputStream stream = list.get(0).openStream()) { return load(stream); + } catch (JsonParseException e) { + // Gestalt only isolates a single asset's load failure (logs it, returns Optional.empty()) for + // *checked* exceptions - an unchecked JsonParseException would instead propagate all the way out + // and abort whatever triggered the load (e.g. crash the whole game on startup, see #5099). + // Rethrowing as the IOException this method already declares routes it through that safety net. + throw new IOException("Malformed behavior tree asset '" + resourceUrn + "': " + e.getMessage(), e); } } diff --git a/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilder.java b/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilder.java index 6a39fa87ce3..43c2e489246 100644 --- a/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilder.java +++ b/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilder.java @@ -231,12 +231,20 @@ private BehaviorNode getCompositeNode(JsonElement json, JsonDeserializationConte addAction((ActionNode) node, action); JsonElement childJson = jsonElement.getAsJsonObject().get("child"); BehaviorNode child = context.deserialize(childJson, BehaviorNode.class); + if (child == null) { + throw new JsonParseException("Malformed behavior tree: decorator '" + type + + "' has no valid child (check for a missing/null \"child\" entry)"); + } node.insertChild(0, child); } else if (jsonElement.isJsonArray()) { List children = context.deserialize(jsonElement, new TypeToken>() { }.getType()); for (int i = 0; i < children.size(); i++) { BehaviorNode child = children.get(i); + if (child == null) { + throw new JsonParseException("Malformed behavior tree: '" + type + + "' has a null child at index " + i + " (check for a stray/trailing comma)"); + } node.insertChild(i, child); } } diff --git a/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java b/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java new file mode 100644 index 00000000000..bd0de4aa220 --- /dev/null +++ b/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java @@ -0,0 +1,185 @@ +// Copyright 2021 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.engine.logic.behavior.core; + +import com.google.gson.GsonBuilder; +import com.google.gson.JsonArray; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; +import com.google.gson.JsonParser; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardCopyOption; +import java.nio.file.StandardOpenOption; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +/** + * Repairs a class of malformed {@code .behavior} JSON files: a stray/trailing comma in a composite node's child + * array (e.g. {@code { selector: [success, success,] } }) parses as an extra, phantom {@code null} array entry - + * see {@link BehaviorTreeBuilder#getCompositeNode} and https://github.com/MovingBlocks/Terasology/issues/5099. + * That slot never represented a real child (there's nothing between the comma and the closing bracket), so + * removing it loses nothing and is safe to do automatically. + *

+ * A decorator with an explicit {@code "child": null} is a different, not safely auto-fixable case: a + * real child was supposed to be there and isn't, and there's no way to guess what it should have been. Those are + * reported, not touched. A decorator missing the {@code child} key entirely can't be told apart from a legitimate + * childless action without knowing which JSON keys are registered as decorators (that needs a live module + * environment, which this offline tool doesn't have) - it will slip through undetected here and still needs + * {@link BehaviorTreeBuilder}'s own load-time check to be caught. + */ +public final class BehaviorTreeRepairTool { + private static final Logger logger = LoggerFactory.getLogger(BehaviorTreeRepairTool.class); + + private BehaviorTreeRepairTool() { + } + + /** + * Cleans the given behavior tree JSON: strips phantom null entries from composite child arrays, and reports + * (without touching) any null/missing decorator child it finds along the way. Does not touch any file. + * + * @param json the raw file contents + * @return the result of the scan - {@link Result#changed} is false if there was nothing to remove + */ + public static Result clean(String json) { + JsonElement root = JsonParser.parseString(json); + List unfixable = new ArrayList<>(); + int[] removed = {0}; + JsonElement cleaned = stripPhantomNulls(root, unfixable, removed); + if (removed[0] == 0) { + return new Result(false, 0, unfixable, null); + } + String cleanedJson = new GsonBuilder().setPrettyPrinting().create().toJson(cleaned); + return new Result(true, removed[0], unfixable, cleanedJson); + } + + /** + * Repairs a {@code .behavior} file on disk in place, if it has any safely-fixable phantom null array entries. + * The original is preserved alongside as {@code .bak} - or {@code .bak.1}, {@code .bak.2}, etc. if + * an earlier repair already left a {@code .bak} there, so a repeat run never clobbers a previous backup. + * The repaired content is written to a temporary file first and moved into place atomically, so a failure + * partway through can't leave the source file truncated or corrupt. + *

+ * If the file also has an unfixable issue (a decorator's null/missing child - see the class doc), nothing is + * written: fixing the array entries alone would still leave a broken file, and this tool can't guess what the + * decorator's missing child was supposed to be. Deliberately does not validate the result by loading it + * through {@link BehaviorTreeBuilder}: that requires a live module environment to resolve custom + * action/decorator names, which an offline repair tool doesn't have. + * + * @param file the {@code .behavior} file to repair + * @return the result of the repair attempt + * @throws IOException if the file can't be read, backed up or written + */ + public static Result repair(Path file) throws IOException { + String original = new String(Files.readAllBytes(file), StandardCharsets.UTF_8); + Result result = clean(original); + if (!result.changed) { + return result; + } + if (!result.unfixableIssues.isEmpty()) { + logger.warn("Not repairing '{}': found unfixable issues too - {}", file, result.unfixableIssues); + return new Result(false, result.nullArrayEntriesRemoved, result.unfixableIssues, null); + } + Path backup = nextFreeBackupPath(file); + Files.write(backup, original.getBytes(StandardCharsets.UTF_8), StandardOpenOption.CREATE_NEW); + + Path temporary = Files.createTempFile(file.toAbsolutePath().getParent(), file.getFileName().toString(), ".tmp"); + try { + Files.write(temporary, result.cleanedJson.getBytes(StandardCharsets.UTF_8)); + Files.move(temporary, file, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING); + } finally { + Files.deleteIfExists(temporary); + } + + logger.info("Repaired '{}': removed {} phantom null entr{} (backup at '{}')", + file, result.nullArrayEntriesRemoved, result.nullArrayEntriesRemoved == 1 ? "y" : "ies", backup); + return result; + } + + private static Path nextFreeBackupPath(Path file) { + Path candidate = file.resolveSibling(file.getFileName() + ".bak"); + int suffix = 1; + while (Files.exists(candidate)) { + candidate = file.resolveSibling(file.getFileName() + ".bak." + suffix); + suffix++; + } + return candidate; + } + + /** + * Command-line entry point, since a file broken this way stops the game from even starting - + * {@code gradle :engine:run... } isn't an option, this needs to run standalone against the raw file(s). + * + * @param args one or more paths to {@code .behavior} files + */ + public static void main(String[] args) throws IOException { + if (args.length == 0) { + System.err.println("Usage: BehaviorTreeRepairTool [more files...]"); + System.exit(1); + } + for (String arg : args) { + Path file = Path.of(arg); + Result result = repair(file); + if (result.changed) { + System.out.println(file + ": removed " + result.nullArrayEntriesRemoved + " phantom null entr" + + (result.nullArrayEntriesRemoved == 1 ? "y" : "ies") + ", backup saved alongside it."); + } else if (!result.unfixableIssues.isEmpty()) { + System.out.println(file + ": NOT repaired, needs manual fixing - " + result.unfixableIssues); + } else { + System.out.println(file + ": nothing to repair."); + } + } + } + + private static JsonElement stripPhantomNulls(JsonElement element, List unfixable, int[] removedCount) { + if (element.isJsonArray()) { + JsonArray source = element.getAsJsonArray(); + JsonArray cleaned = new JsonArray(); + for (JsonElement child : source) { + if (child.isJsonNull()) { + removedCount[0]++; + } else { + cleaned.add(stripPhantomNulls(child, unfixable, removedCount)); + } + } + return cleaned; + } else if (element.isJsonObject()) { + JsonObject source = element.getAsJsonObject(); + JsonObject cleaned = new JsonObject(); + for (Map.Entry entry : source.entrySet()) { + JsonElement value = entry.getValue(); + if ("child".equals(entry.getKey()) && value.isJsonNull()) { + // A decorator missing its one required child - there's nothing to safely infer here, unlike + // an array's phantom trailing-comma slot. Leave it as-is and flag it instead. + unfixable.add("decorator has a null/missing 'child' - needs a real child added by hand"); + cleaned.add(entry.getKey(), value); + } else { + cleaned.add(entry.getKey(), stripPhantomNulls(value, unfixable, removedCount)); + } + } + return cleaned; + } + return element; + } + + public static final class Result { + public final boolean changed; + public final int nullArrayEntriesRemoved; + public final List unfixableIssues; + /** The cleaned JSON, or {@code null} if {@link #changed} is false. */ + public final String cleanedJson; + + private Result(boolean changed, int nullArrayEntriesRemoved, List unfixableIssues, String cleanedJson) { + this.changed = changed; + this.nullArrayEntriesRemoved = nullArrayEntriesRemoved; + this.unfixableIssues = unfixableIssues; + this.cleanedJson = cleanedJson; + } + } +}