fix(behavior): reject null children in behavior tree JSON at load time

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<AssetDataFile> 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<String> getPath() {
+                return Collections.emptyList();
+            }
+
+            @Override
+            public InputStream open() {
+                return new ByteArrayInputStream(json.getBytes(StandardCharsets.UTF_8));
+            }
+        };
+    }
+}

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"));
+    }
+}

engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairToolTest.java

@@ -0,0 +1,95 @@
+// 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 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));
+    }
+}

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<AssetDataFile> 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);
         }
     }
 

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<BehaviorNode> children = context.deserialize(jsonElement, new TypeToken<List<BehaviorNode>>() {
             }.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);
             }
         }