Skip to content

Commit 45638b0

Browse files
soloturnsoloturn
committed
Merge PR #5366 (fix/behavior-tree-null-child-validation) into merge-train
2 parents 44b2663 + 78ae9ff commit 45638b0

6 files changed

Lines changed: 448 additions & 0 deletions

File tree

‎engine-tests/src/test/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormatTest.java‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
// Copyright 2021 The Terasology Foundation
2+
// SPDX-License-Identifier: Apache-2.0
3+
package org.terasology.engine.logic.behavior.asset;
4+
5+
import org.junit.jupiter.api.Test;
6+
import org.terasology.gestalt.assets.ResourceUrn;
7+
import org.terasology.gestalt.assets.format.AssetDataFile;
8+
import org.terasology.gestalt.module.resources.FileReference;
9+
10+
import java.io.ByteArrayInputStream;
11+
import java.io.IOException;
12+
import java.io.InputStream;
13+
import java.nio.charset.StandardCharsets;
14+
import java.util.Collections;
15+
import java.util.List;
16+
17+
import static org.junit.jupiter.api.Assertions.assertThrows;
18+
import static org.junit.jupiter.api.Assertions.assertTrue;
19+
20+
/**
21+
* Regression coverage for https://github.com/MovingBlocks/Terasology/issues/5099. A malformed behavior tree must
22+
* fail this single asset's load with the checked {@link IOException} the {@code AssetFileFormat} contract expects
23+
* - that's what lets gestalt's asset-loading machinery isolate the failure to just this asset (log it, move on)
24+
* instead of an unchecked exception blowing past that safety net and crashing whatever triggered the load.
25+
*/
26+
public class BehaviorTreeFormatTest {
27+
28+
@Test
29+
public void malformedTreeFailsWithCheckedIOExceptionNotAnUncheckedOne() {
30+
BehaviorTreeFormat format = new BehaviorTreeFormat();
31+
ResourceUrn urn = new ResourceUrn("engine:malformed");
32+
List<AssetDataFile> source = Collections.singletonList(new AssetDataFile(jsonFile("{ selector: [success, null, success] }")));
33+
34+
IOException exception = assertThrows(IOException.class, () -> format.load(urn, source));
35+
36+
assertTrue(exception.getMessage().contains("engine:malformed"));
37+
assertTrue(exception.getMessage().contains("selector"));
38+
}
39+
40+
private FileReference jsonFile(String json) {
41+
return new FileReference() {
42+
@Override
43+
public String getName() {
44+
return "malformed.behavior";
45+
}
46+
47+
@Override
48+
public List<String> getPath() {
49+
return Collections.emptyList();
50+
}
51+
52+
@Override
53+
public InputStream open() {
54+
return new ByteArrayInputStream(json.getBytes(StandardCharsets.UTF_8));
55+
}
56+
};
57+
}
58+
}

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

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
// Copyright 2021 The Terasology Foundation
2+
// SPDX-License-Identifier: Apache-2.0
3+
package org.terasology.engine.logic.behavior.core;
4+
5+
import com.google.gson.JsonParseException;
6+
import org.junit.jupiter.api.Test;
7+
import org.terasology.engine.logic.behavior.actions.InvertAction;
8+
9+
import static org.junit.jupiter.api.Assertions.assertEquals;
10+
import static org.junit.jupiter.api.Assertions.assertThrows;
11+
import static org.junit.jupiter.api.Assertions.assertTrue;
12+
13+
/**
14+
* Regression coverage for https://github.com/MovingBlocks/Terasology/issues/5099: a malformed behavior tree
15+
* (e.g. a stray/trailing comma producing a null array entry) used to parse "successfully" with a silent
16+
* {@code null} child, only to crash later with an unrelated NPE deep in tree execution/copying
17+
* ({@link org.terasology.engine.logic.behavior.core.SelectorNode#deepCopy()} or
18+
* {@link org.terasology.engine.logic.behavior.DefaultBehaviorTreeRunner}). It should instead fail fast, at
19+
* load time, with a message that points at the malformed tree.
20+
*/
21+
public class BehaviorTreeBuilderTest {
22+
23+
private final BehaviorTreeBuilder builder = new BehaviorTreeBuilder();
24+
25+
@Test
26+
public void validTreeStillParses() {
27+
BehaviorNode node = builder.fromJson("{ selector: [success, failure, success] }");
28+
29+
assertEquals(3, node.getChildrenCount());
30+
}
31+
32+
@Test
33+
public void nullChildInCompositeArrayFailsLoudlyInsteadOfLater() {
34+
JsonParseException exception = assertThrows(JsonParseException.class,
35+
() -> builder.fromJson("{ selector: [success, null, success] }"));
36+
37+
assertTrue(exception.getMessage().contains("selector"));
38+
assertTrue(exception.getMessage().contains("index 1"));
39+
}
40+
41+
@Test
42+
public void nullChildInDecoratorFailsLoudlyInsteadOfLater() {
43+
builder.registerDecorator("invert", InvertAction.class);
44+
45+
JsonParseException exception = assertThrows(JsonParseException.class,
46+
() -> builder.fromJson("{ invert: { child: null } }"));
47+
48+
assertTrue(exception.getMessage().contains("invert"));
49+
}
50+
51+
@Test
52+
public void missingChildInDecoratorFailsLoudlyInsteadOfLater() {
53+
builder.registerDecorator("invert", InvertAction.class);
54+
55+
JsonParseException exception = assertThrows(JsonParseException.class,
56+
() -> builder.fromJson("{ invert: {} }"));
57+
58+
assertTrue(exception.getMessage().contains("invert"));
59+
}
60+
}

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

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
// Copyright 2021 The Terasology Foundation
2+
// SPDX-License-Identifier: Apache-2.0
3+
package org.terasology.engine.logic.behavior.core;
4+
5+
import org.junit.jupiter.api.Test;
6+
import org.junit.jupiter.api.io.TempDir;
7+
8+
import java.io.IOException;
9+
import java.nio.charset.StandardCharsets;
10+
import java.nio.file.Files;
11+
import java.nio.file.Path;
12+
import java.util.Arrays;
13+
import java.util.List;
14+
import java.util.stream.Collectors;
15+
import java.util.stream.Stream;
16+
17+
import static org.junit.jupiter.api.Assertions.assertEquals;
18+
import static org.junit.jupiter.api.Assertions.assertFalse;
19+
import static org.junit.jupiter.api.Assertions.assertTrue;
20+
21+
/**
22+
* See https://github.com/MovingBlocks/Terasology/issues/5099. The tool exists so a content author who hits the
23+
* load-time rejection from {@link BehaviorTreeBuilder} doesn't have to hand-edit the file's JSON to find and
24+
* remove the offending comma - it does the same fix mechanically.
25+
*/
26+
public class BehaviorTreeRepairToolTest {
27+
28+
@Test
29+
public void trailingCommaInCompositeArrayIsRemoved() {
30+
BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.clean(
31+
"{ \"selector\": [\"success\", \"success\",] }");
32+
33+
assertTrue(result.changed);
34+
assertEquals(1, result.nullArrayEntriesRemoved);
35+
assertTrue(result.unfixableIssues.isEmpty());
36+
// The cleaned JSON should load through the real builder with no custom actions/decorators needed.
37+
BehaviorNode node = new BehaviorTreeBuilder().fromJson(result.cleanedJson);
38+
assertEquals(2, node.getChildrenCount());
39+
}
40+
41+
@Test
42+
public void explicitNullInCompositeArrayIsRemoved() {
43+
BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.clean(
44+
"{ \"selector\": [\"success\", null, \"success\"] }");
45+
46+
assertTrue(result.changed);
47+
assertEquals(1, result.nullArrayEntriesRemoved);
48+
BehaviorNode node = new BehaviorTreeBuilder().fromJson(result.cleanedJson);
49+
assertEquals(2, node.getChildrenCount());
50+
}
51+
52+
@Test
53+
public void nothingToFixIsReportedAsUnchanged() {
54+
BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.clean(
55+
"{ \"selector\": [\"success\", \"success\"] }");
56+
57+
assertFalse(result.changed);
58+
assertEquals(0, result.nullArrayEntriesRemoved);
59+
assertTrue(result.unfixableIssues.isEmpty());
60+
}
61+
62+
@Test
63+
public void explicitNullDecoratorChildIsReportedNotSilentlyDropped() {
64+
BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.clean(
65+
"{ \"invert\": { \"child\": null } }");
66+
67+
assertFalse(result.unfixableIssues.isEmpty());
68+
}
69+
70+
@Test
71+
public void repairWritesTheFileAndKeepsABackup(@TempDir Path dir) throws IOException {
72+
Path file = dir.resolve("broken.behavior");
73+
String original = "{ \"selector\": [\"success\", \"success\",] }";
74+
Files.write(file, original.getBytes(StandardCharsets.UTF_8));
75+
76+
BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.repair(file);
77+
78+
assertTrue(result.changed);
79+
Path backup = dir.resolve("broken.behavior.bak");
80+
assertTrue(Files.exists(backup));
81+
assertEquals(original, new String(Files.readAllBytes(backup), StandardCharsets.UTF_8));
82+
BehaviorNode node = new BehaviorTreeBuilder().fromJson(
83+
new String(Files.readAllBytes(file), StandardCharsets.UTF_8));
84+
assertEquals(2, node.getChildrenCount());
85+
}
86+
87+
@Test
88+
public void repairLeavesUnfixableFileUntouched(@TempDir Path dir) throws IOException {
89+
Path file = dir.resolve("broken.behavior");
90+
String original = "{ \"invert\": { \"child\": null } }";
91+
Files.write(file, original.getBytes(StandardCharsets.UTF_8));
92+
93+
BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.repair(file);
94+
95+
assertFalse(result.changed);
96+
assertFalse(Files.exists(dir.resolve("broken.behavior.bak")));
97+
assertEquals(original, new String(Files.readAllBytes(file), StandardCharsets.UTF_8));
98+
}
99+
100+
@Test
101+
public void repairNeverClobbersAnExistingBackup(@TempDir Path dir) throws IOException {
102+
Path file = dir.resolve("broken.behavior");
103+
String original = "{ \"selector\": [\"success\", \"success\",] }";
104+
Files.write(file, original.getBytes(StandardCharsets.UTF_8));
105+
Path existingBackup = dir.resolve("broken.behavior.bak");
106+
String someoneElsesBackup = "not the original - already there before repair() ran";
107+
Files.write(existingBackup, someoneElsesBackup.getBytes(StandardCharsets.UTF_8));
108+
109+
BehaviorTreeRepairTool.repair(file);
110+
111+
assertEquals(someoneElsesBackup, new String(Files.readAllBytes(existingBackup), StandardCharsets.UTF_8));
112+
Path versionedBackup = dir.resolve("broken.behavior.bak.1");
113+
assertTrue(Files.exists(versionedBackup));
114+
assertEquals(original, new String(Files.readAllBytes(versionedBackup), StandardCharsets.UTF_8));
115+
}
116+
117+
@Test
118+
public void repairLeavesOnlyTheFinalFileBehindNoStrayTempFiles(@TempDir Path dir) throws IOException {
119+
Path file = dir.resolve("broken.behavior");
120+
Files.write(file, "{ \"selector\": [\"success\", \"success\",] }".getBytes(StandardCharsets.UTF_8));
121+
122+
BehaviorTreeRepairTool.repair(file);
123+
124+
try (Stream<Path> entries = Files.list(dir)) {
125+
List<String> names = entries.map(p -> p.getFileName().toString())
126+
.sorted().collect(Collectors.toList());
127+
assertEquals(Arrays.asList("broken.behavior", "broken.behavior.bak"), names);
128+
}
129+
}
130+
}

‎engine/src/main/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormat.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
package org.terasology.engine.logic.behavior.asset;
44

55
import com.google.common.base.Charsets;
6+
import com.google.gson.JsonParseException;
67
import org.slf4j.Logger;
78
import org.slf4j.LoggerFactory;
89
import org.terasology.gestalt.assets.ResourceUrn;
@@ -53,6 +54,12 @@ public BehaviorTreeData load(ResourceUrn resourceUrn, List<AssetDataFile> list)
5354
}
5455
try (InputStream stream = list.get(0).openStream()) {
5556
return load(stream);
57+
} catch (JsonParseException e) {
58+
// Gestalt only isolates a single asset's load failure (logs it, returns Optional.empty()) for
59+
// *checked* exceptions - an unchecked JsonParseException would instead propagate all the way out
60+
// and abort whatever triggered the load (e.g. crash the whole game on startup, see #5099).
61+
// Rethrowing as the IOException this method already declares routes it through that safety net.
62+
throw new IOException("Malformed behavior tree asset '" + resourceUrn + "': " + e.getMessage(), e);
5663
}
5764
}
5865

‎engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilder.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,12 +231,20 @@ private BehaviorNode getCompositeNode(JsonElement json, JsonDeserializationConte
231231
addAction((ActionNode) node, action);
232232
JsonElement childJson = jsonElement.getAsJsonObject().get("child");
233233
BehaviorNode child = context.deserialize(childJson, BehaviorNode.class);
234+
if (child == null) {
235+
throw new JsonParseException("Malformed behavior tree: decorator '" + type
236+
+ "' has no valid child (check for a missing/null \"child\" entry)");
237+
}
234238
node.insertChild(0, child);
235239
} else if (jsonElement.isJsonArray()) {
236240
List<BehaviorNode> children = context.deserialize(jsonElement, new TypeToken<List<BehaviorNode>>() {
237241
}.getType());
238242
for (int i = 0; i < children.size(); i++) {
239243
BehaviorNode child = children.get(i);
244+
if (child == null) {
245+
throw new JsonParseException("Malformed behavior tree: '" + type
246+
+ "' has a null child at index " + i + " (check for a stray/trailing comma)");
247+
}
240248
node.insertChild(i, child);
241249
}
242250
}

0 commit comments

Comments
 (0)