diff --git a/engine-tests/src/test/java/org/terasology/engine/core/SimpleUriTest.java b/engine-tests/src/test/java/org/terasology/engine/core/SimpleUriTest.java new file mode 100644 index 00000000000..29384e72820 --- /dev/null +++ b/engine-tests/src/test/java/org/terasology/engine/core/SimpleUriTest.java @@ -0,0 +1,38 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.engine.core; + +import org.junit.jupiter.api.Test; +import org.terasology.gestalt.naming.Name; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class SimpleUriTest { + + @Test + void parsesModuleAndObject() { + SimpleUri uri = new SimpleUri("engine:something"); + + assertTrue(uri.isValid()); + assertEquals(new Name("engine"), uri.getModuleName()); + assertEquals(new Name("something"), uri.getObjectName()); + } + + @Test + void stringWithoutSeparatorIsInvalid() { + assertFalse(new SimpleUri("engine").isValid()); + } + + @Test + void nullStringIsInvalid() { + // Regression: null reached this constructor when deserializing a stale reference and + // threw on split(). It must land in the same empty/invalid state as the no-arg form. + SimpleUri uri = new SimpleUri((String) null); + + assertFalse(uri.isValid()); + assertTrue(uri.getModuleName().isEmpty()); + assertTrue(uri.getObjectName().isEmpty()); + } +} diff --git a/engine/src/main/java/org/terasology/engine/core/SimpleUri.java b/engine/src/main/java/org/terasology/engine/core/SimpleUri.java index 94c3bb59211..de6690d418a 100644 --- a/engine/src/main/java/org/terasology/engine/core/SimpleUri.java +++ b/engine/src/main/java/org/terasology/engine/core/SimpleUri.java @@ -58,11 +58,15 @@ public SimpleUri(Name moduleName, Name objectName) { } /** - * Creates a SimpleUri from a string in the format "module:object". If the string does not match this format, it will be marked invalid + * Creates a SimpleUri from a string in the format "module:object". If the string does not match this format + * (including {@code null}), it will be marked invalid. * * @param simpleUri */ public SimpleUri(String simpleUri) { + if (simpleUri == null) { + return; + } String[] split = simpleUri.split(MODULE_SEPARATOR, 2); if (split.length > 1) { moduleName = new Name(split[0]); diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java index 5d4ea3f0596..0d84c7bc4eb 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java +++ b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java @@ -2,10 +2,23 @@ // SPDX-License-Identifier: Apache-2.0 package org.terasology.persistence.typeHandling; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; public abstract class StringRepresentationTypeHandler extends TypeHandler { + private static final Logger logger = LoggerFactory.getLogger(StringRepresentationTypeHandler.class); + + /** + * Per-instance rather than static: a single damaged reference can appear on thousands of + * entities, so this reports once and then goes quiet - but MTE builds a fresh handler library + * per environment, and a static flag would silence every test after the first. + */ + private final AtomicBoolean warnedNullContent = new AtomicBoolean(); + public abstract String getAsString(T item); public abstract T getFromString(String representation); @@ -19,7 +32,20 @@ public PersistedData serializeNonNull(T value, PersistedDataSerializer serialize @Override public Optional deserialize(PersistedData data) { if (data.isString()) { - return Optional.ofNullable(getFromString(data.getAsString())); + String value = data.getAsString(); + if (value == null) { + // PersistedString reports isString() unconditionally, so stale save data can + // arrive with null content - treat as absent rather than call getFromString(null). + if (warnedNullContent.compareAndSet(false, true)) { + logger.warn("{}: persisted data reported isString() but held null content -" + + " treating it as absent. Further occurrences from this handler" + + " are silent. Usually a stale or renamed reference in older" + + " save data.", + getClass().getSimpleName()); + } + return Optional.empty(); + } + return Optional.ofNullable(getFromString(value)); } return Optional.empty(); } diff --git a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandlerTest.java b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandlerTest.java new file mode 100644 index 00000000000..d0400f31dc5 --- /dev/null +++ b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandlerTest.java @@ -0,0 +1,66 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.persistence.typeHandling; + +import org.junit.jupiter.api.Test; +import org.terasology.persistence.typeHandling.inMemory.PersistedInteger; +import org.terasology.persistence.typeHandling.inMemory.PersistedString; + +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class StringRepresentationTypeHandlerTest { + + /** + * Records whether {@link #getFromString} was reached. The contract under test is not just + * "returns empty" but "never forwards null to the subclass" — asserting only on the returned + * Optional would still pass if a subclass happened to tolerate null and return null itself. + */ + private static final class RecordingHandler extends StringRepresentationTypeHandler { + private boolean getFromStringCalled; + + @Override + public String getAsString(String item) { + return item; + } + + @Override + public String getFromString(String representation) { + getFromStringCalled = true; + return representation; + } + } + + private final RecordingHandler typeHandler = new RecordingHandler(); + + @Test + void deserializesStringContent() { + Optional result = typeHandler.deserialize(new PersistedString("engine:something")); + + assertTrue(result.isPresent()); + assertEquals("engine:something", result.get()); + assertTrue(typeHandler.getFromStringCalled); + } + + @Test + void treatsNullStringContentAsAbsent() { + // PersistedString returns true from isString() unconditionally, so a null payload reaches + // deserialize() with isString() == true. That is the shape that produced the NPE: null was + // forwarded to getFromString(), and ComponentClassTypeHandler built a SimpleUri from it. + Optional result = typeHandler.deserialize(new PersistedString(null)); + + assertFalse(result.isPresent()); + assertFalse(typeHandler.getFromStringCalled, "null content must not reach getFromString()"); + } + + @Test + void ignoresNonStringData() { + Optional result = typeHandler.deserialize(new PersistedInteger(42)); + + assertFalse(result.isPresent()); + assertFalse(typeHandler.getFromStringCalled); + } +}