fix(persistence): stop ProtobufPersistedData.isArray() lying for maps

[soloturn]

Fixes #5069 - PersistedData.isArray() can return true for data that is actually a value map, forcing callers to add their own isValueMap() check to tell the two apart, as GenericMapTypeHandler had to when reading save-game data (#5062).

Root cause

Gson-backed PersistedData (JSON config/prefabs) already scopes isArray() correctly - AbstractGsonPersistedData just delegates to JsonElement.isJsonArray(), which is never true for a JsonObject.

ProtobufPersistedData - the class backing save-game entity serialization - is different. A single EntityData.Value protobuf message doubles as a scalar, an array (repeated scalar/nested-value fields) and a value map (name-value pairs), and isArray() ignored all of that:

public boolean isArray() {
    return true;
}

So for any save-game field actually shaped as a map, isArray() lied, and the only way to tell was the separate isValueMap() check GenericMapTypeHandler already had to add. Every other isArray()/getAsArray() caller in the type-handling code has the same latent exposure, just less likely to hit it in practice since Map fields are where array/map shape confusion naturally arises.

Fix

Mirror isValueMap()'s own check instead of hardcoding true:

public boolean isArray() {
    return data.getNameValueCount() == 0;
}

Every other kind this class represents - scalars (already treated as size-1 arrays by getAsArray()/getArrayItem() elsewhere in this class), genuine repeated-value arrays, and null - keeps reporting isArray() as true exactly as before; only the one case that was actually wrong (a real value map) now correctly reports false. getAsArray() is gated on isArray(), so this also means it now correctly throws instead of silently succeeding when called on map-shaped data.

GenericMapTypeHandler's existing isValueMap() check is now redundant for this case but harmless, so it's left in place rather than touched here.

Verification

:engine:compileJava clean. engine-tests:unitTest and subsystems:TypeHandlerLibrary:test both pass in full, no failures - notably EntitySerializerTest (14 tests), which round-trips components through the real EntityData.Entity protobuf path this class backs, and GenericMapTypeHandlerTest/CollectionTypeHandlerTest/ArrayTypeHandlerTest, which cover the array/map-shape-detection callers most exposed to this bug.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 262e636a-df82-40e1-806c-e2f3dcc4b81f

📥 Commits

Reviewing files that changed from the base of the PR and between 338d7dd and 2c4a746.

📒 Files selected for processing (1)

• engine/src/main/java/org/terasology/engine/persistence/typeHandling/protobuf/ProtobufPersistedData.java

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Corrected handling of persisted protobuf data so map entries are no longer incorrectly identified as arrays.
  • Preserved array detection for other supported data types.

Walkthrough

ProtobufPersistedData.isArray() now identifies protobuf values with name-value entries as maps instead of arrays. Other value forms continue to return true.

Changes

Protobuf persisted data

Layer / File(s)	Summary
Map-aware array classification engine/src/main/java/org/terasology/engine/persistence/typeHandling/protobuf/ProtobufPersistedData.java	isArray() returns false for values containing name-value entries and retains true for other value forms.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 2c4a7

This localized fix makes map-shaped persisted data report the correct array status and prevents invalid array access; no actionable merge-blocking risk remains after normal checks and review.

Poem

I’m a rabbit guarding maps,
With protobuf paths and hops.
Name-value pairs stay arrays no more,
isArray() knows what they’re for.
A tidy fix, then off I spring!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states that ProtobufPersistedData.isArray() is corrected for value maps.
Description check	✅ Passed	The description explains the root cause, implementation, preserved behavior, and verification for the persistence fix.
Linked Issues check	✅ Passed	The change directly addresses issue #5069 by making map-shaped protobuf data report isArray() as false.
Out of Scope Changes check	✅ Passed	The pull request changes only ProtobufPersistedData.isArray(), which matches the linked issue and stated objectives.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch soloturn-fix-protobuf-isarray

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}