fix: guard against null in SimpleUri(String) and StringRepresentationTypeHandler - #5360
Conversation
…TypeHandler
Loading a saved player entity can crash the whole game with:
java.lang.NullPointerException: Cannot invoke "String.split(String, int)" because "simpleUri" is null
at org.terasology.engine.core.SimpleUri.<init>(SimpleUri.java:66)
at org.terasology.engine.persistence.typeHandling.reflection.ModuleEnvironmentSandbox.doesSubclassMatch(...)
...
at org.terasology.engine.persistence.typeHandling.extensionTypes.ComponentClassTypeHandler.getFromString(...)
at org.terasology.persistence.typeHandling.StringRepresentationTypeHandler.deserialize(...)
at org.terasology.persistence.typeHandling.coreTypes.CollectionTypeHandler.deserialize(...)
...
at org.terasology.engine.persistence.internal.PlayerStoreInternal.restoreEntities(...)
This happens when deserializing a persisted list of component-class
references and one entry no longer resolves to a real class (e.g. a
component that existed in an older engine/module build was renamed or
removed) - PersistedData.isString() can report true while
getAsString() still yields null for that entry, and that null was
passed straight through getFromString() into "new SimpleUri(null)",
which called .split() on it unconditionally.
Two layered fixes:
- SimpleUri(String): treat a null input the same as a malformed one -
mark the URI invalid instead of throwing. SimpleUri already has this
"invalid but not exceptional" behavior for non-null malformed input,
this just extends it to null, consistent with the class's own
documented contract ("If the string does not match this format, it
will be marked invalid").
- StringRepresentationTypeHandler.deserialize(): also guard at the
actual entry point where the null appears, so any subclass (not just
ComponentClassTypeHandler) gets Optional.empty() instead of a null
forwarded into its own getFromString().
Verified against a real save that reproduced this exact crash while
testing Windows/ARM64 support (see TerasologyLauncher#727,
Terasology#5359): patched the compiled classes into a running
engine-5.4.0-SNAPSHOT.jar / TypeHandlerLibrary-5.4.0-SNAPSHOT.jar
locally; the malformed reference is now skipped instead of crashing
character spawn.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes add explicit null handling to ChangesNull handling
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
engine/src/main/java/org/terasology/engine/core/SimpleUri.java (1)
61-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an automated regression test for the null input path.
The implementation correctly leaves
new SimpleUri((String) null)invalid. Add a test that verifiesisValid()returnsfalse, so this deserialization fix remains protected from regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/src/main/java/org/terasology/engine/core/SimpleUri.java` around lines 61 - 69, Add a regression test for the String constructor of SimpleUri that passes a null value and asserts isValid() returns false, preserving the existing invalid-state behavior.subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java (1)
22-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an automated regression test for null persisted content.
Provide string-typed
PersistedDatawhosegetAsString()returnsnull. Assert thatdeserialize(data)returnsOptional.empty(). Keep a non-null case to verify that valid values still reachgetFromString.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java` around lines 22 - 30, Add a regression test for StringRepresentationTypeHandler.deserialize using string-typed PersistedData whose getAsString() returns null, asserting Optional.empty(). Also retain or add a non-null input case that verifies valid content is passed to getFromString and deserialized successfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@engine/src/main/java/org/terasology/engine/core/SimpleUri.java`:
- Around line 61-69: Add a regression test for the String constructor of
SimpleUri that passes a null value and asserts isValid() returns false,
preserving the existing invalid-state behavior.
In
`@subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java`:
- Around line 22-30: Add a regression test for
StringRepresentationTypeHandler.deserialize using string-typed PersistedData
whose getAsString() returns null, asserting Optional.empty(). Also retain or add
a non-null input case that verifies valid content is passed to getFromString and
deserialized successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f658fd9-7e45-48b0-b405-90e6ca3afbc2
📒 Files selected for processing (2)
engine/src/main/java/org/terasology/engine/core/SimpleUri.javasubsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java
`PersistedString` returns true from `isString()` unconditionally and hands back whatever it was constructed with, so `new PersistedString(null)` reproduces the reported NPE condition with a real type rather than a mock — which also confirms the guard is reachable and not dead code. The handler test asserts `getFromString` is never entered, not just that the result is empty; a subclass tolerating null would pass the weaker assertion while the contract was broken. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Agent-assisted review, driven by @Cervator via GDD. Added regression tests for both guards, covering the two nitpicks raised above. What pinned it down
public String getAsString() { return data; }
public boolean isString() { return true; }So Tests
The null case asserts
Both green, 3 tests each, verified from the JUnit XML rather than the exit code. One suggestion, take it or leave itNeither guard logs anything, so a stale or renamed reference now disappears silently — the component quietly fails to restore and nothing says why. That trades an NPE for a harder-to-diagnose symptom. A single Also trimmed the inline comment to two lines and pointed it at |
Returning empty on null content trades an NPE for a component that quietly fails to restore, which is harder to diagnose than the crash was. Bounded by construction rather than by a cap: the bucket is the handler, and there are 14 subclasses, so the worst possible load emits 14 lines however many entities carry the damaged reference. No sample counters or reset window needed, unlike `WorldProviderCoreImpl.logDroppedWrite` whose bucket space is chunk positions. Per-instance, not static — MTE builds a fresh handler library per environment, and a static flag would silence every test after the first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cervator
left a comment
Cervator
left a comment
There was a problem hiding this comment.
I had my agent add the extra logging that'll be one-time per type per instance - useful, but not overwhelming. CodeRabbit is happy and the prior commit ran fine including new tests.
3 participants
Summary
Spawning a character (loading player entity data) can crash the whole game with:
Root cause:
CollectionTypeHandler.deserializeis walking a persisted list of component-class references attached to the player entity. One entry no longer resolves to a real class (e.g. a component that existed in an older engine/module build was renamed or removed since the save was written).PersistedData.isString()can reporttruefor that entry whilegetAsString()still yieldsnull, andStringRepresentationTypeHandler.deserializeforwards thatnullstraight intogetFromString()without checking - which forComponentClassTypeHandlerends up callingnew SimpleUri(null), andSimpleUri's String constructor calls.split()on it unconditionally.Confirmed reproducible on a genuinely fresh world (new seed, not just a stale save) - the corrupt reference lives in the player's global profile data, not the per-world save.
Changes
SimpleUri(String): treat anullinput the same as a malformed one - mark the URI invalid instead of throwing.SimpleUrialready has this "invalid but not exceptional" behavior for non-null malformed input (see its own javadoc: "If the string does not match this format, it will be marked invalid") - this just extends that same contract tonull.StringRepresentationTypeHandler.deserialize(): also guard at the actual entry point where the null appears, so any subclass of it (not justComponentClassTypeHandler) getsOptional.empty()for a null-valued string entry instead of forwardingnullinto its owngetFromString().Neither change alters behavior for well-formed input; both are purely defensive against data that no longer round-trips cleanly.
Test plan
engine-5.4.0-SNAPSHOT.jar/TypeHandlerLibrary-5.4.0-SNAPSHOT.jarlocally and confirmed the malformed component-class reference is now skipped (Optional.empty()) instead of throwing, letting character spawn proceed past the point that previously crashed the whole engine.SimpleUriandStringRepresentationTypeHandlerdon't currently have a test file I found in this pass; happy to add anew SimpleUri(null).isValid() == falseregression test if a reviewer wants one before merge.Related