Prevent NBTBlock read access from creating empty chunk PDC compounds#351
Prevent NBTBlock read access from creating empty chunk PDC compounds#351Kepchyk1101 wants to merge 1 commit intotr7zw:masterfrom
Conversation
WalkthroughThe changes implement lazy storage creation and smart cleanup for block-specific NBT data. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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)
item-nbt-plugin/src/main/java/de/tr7zw/nbtapi/plugin/tests/blocks/BlockNBTTest.java (1)
28-55: Good coverage for the read/remove-on-missing paths; consider also asserting the cleanup-after-write path.The new assertions cover the two regressions this PR fixes (read does not create, remove-missing does not create). One gap that matches the PR description ("Empty block compounds are removed after the last key is deleted") is not actually exercised end-to-end: after
setString("Too","Bar")+ a subsequentremoveKey("Too"), we never verify that the"blocks"compound is fully cleaned up from the chunk PDC.🧪 Suggested extra assertion at the end of the test
comp.getData().setString("Too", "Bar"); if (!new NBTBlock(block).getData().getString("Too").equals("Bar")) { throw new NbtApiException("Custom Data did not save to a Block!"); } + comp.getData().removeKey("Too"); + blocks = persistentData.getCompound("blocks"); + if (blocks != null && blocks.hasTag(blockKey)) { + throw new NbtApiException("Removing the last Block key did not clean up the persistent block compound!"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@item-nbt-plugin/src/main/java/de/tr7zw/nbtapi/plugin/tests/blocks/BlockNBTTest.java` around lines 28 - 55, Add an assertion that verifies the "blocks" compound is removed after writing then deleting the block key: after comp.getData().setString("Too","Bar") and confirming the write via new NBTBlock(block).getData().getString("Too"), call comp.getData().removeKey("Too") (or new NBTBlock(block).getData().removeKey("Too")) and then re-check the chunk persistent data (via new NBTChunk(chunk).getPersistentDataContainer()) to ensure persistentData.getCompound("blocks") is null or does not contain blockKey, throwing an NbtApiException if the cleanup did not occur. Ensure you reference the same blockKey/persistentData/blocks variables so the test exercises the empty-compound removal path end-to-end.item-nbt-api/src/main/java/de/tr7zw/changeme/nbtapi/NBTBlock.java (1)
44-114: Design looks solid; a couple of small robustness notes.Nice approach overall — making
NBTBlockCompoundact as a virtual root (parent=null,name=null) meansNBTReflectionUtil.getToCompountleaves the tag unchanged andgetCompound()/getResolvedObject()correctly return the per-block NMS compound. The self-cleaningsetCompoundalso handles the merge-empty-source and remove-last-key cases cleanly, so there's no path I could find that leaves an orphaned empty block compound.A few minor observations:
Write path does redundant work. On every write,
prepareWrite()walksgetOrCreateCompound("blocks").getOrCreateCompound(blockKey)to materialize the entry, and thensetCompound(compound)immediately writes the same reference back viapersistentDataContainer.getOrCreateCompound("blocks").set(blockKey, compound). It's correct (theseton the same reference is effectively a no-op on modern NMS), but the two navigations + twosaveCompound()propagations per write are avoidable. Not blocking — just something to keep in mind if this becomes a hot path.
isEmptyCompoundmirrors established codebase patterns.ReflectionMethod.COMPOUND_GET_KEYS.run(compound)is used identically inNBTItem.java:170andNBTReflectionUtil.java:764without null-checks, reflecting consistent reliance on the contract thatgetKeys(),getAllKeys(), andkeySet()return non-null across all supported NMS versions (1.7–1.21+). Current implementation is aligned with that pattern.
readOnlyis not propagated.super(null, null)always constructs withreadOnly=false, regardless of whethernbtChunk.getPersistentDataContainer()is writable. Current API always returns a writable PDC, so this isn't a bug today, but it's worth noting for any future API that would add a read-only chunk PDC.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@item-nbt-api/src/main/java/de/tr7zw/changeme/nbtapi/NBTBlock.java` around lines 44 - 114, The NBTBlockCompound currently does duplicate navigation on writes and always constructs as writable; fix by (1) eliminating the redundant getOrCreateCompound call in prepareWrite()/setCompound: have prepareWrite() call getOrCreateData() and store the resulting NBTCompound in a transient field (e.g., cachedWriteTarget) that setCompound() uses to write or remove (clear the cache after use), so you don't re-traverse persistentDataContainer.getOrCreateCompound("blocks").getOrCreateCompound(blockKey); and (2) propagate the persistentDataContainer's readOnly state into the NBTBlockCompound constructor instead of always calling super(null, null) as writable (e.g., call super with the persistentDataContainer's readOnly flag or otherwise pass its mutability) so read-only PDCs remain read-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@item-nbt-api/src/main/java/de/tr7zw/changeme/nbtapi/NBTBlock.java`:
- Around line 44-114: The NBTBlockCompound currently does duplicate navigation
on writes and always constructs as writable; fix by (1) eliminating the
redundant getOrCreateCompound call in prepareWrite()/setCompound: have
prepareWrite() call getOrCreateData() and store the resulting NBTCompound in a
transient field (e.g., cachedWriteTarget) that setCompound() uses to write or
remove (clear the cache after use), so you don't re-traverse
persistentDataContainer.getOrCreateCompound("blocks").getOrCreateCompound(blockKey);
and (2) propagate the persistentDataContainer's readOnly state into the
NBTBlockCompound constructor instead of always calling super(null, null) as
writable (e.g., call super with the persistentDataContainer's readOnly flag or
otherwise pass its mutability) so read-only PDCs remain read-only.
In
`@item-nbt-plugin/src/main/java/de/tr7zw/nbtapi/plugin/tests/blocks/BlockNBTTest.java`:
- Around line 28-55: Add an assertion that verifies the "blocks" compound is
removed after writing then deleting the block key: after
comp.getData().setString("Too","Bar") and confirming the write via new
NBTBlock(block).getData().getString("Too"), call comp.getData().removeKey("Too")
(or new NBTBlock(block).getData().removeKey("Too")) and then re-check the chunk
persistent data (via new NBTChunk(chunk).getPersistentDataContainer()) to ensure
persistentData.getCompound("blocks") is null or does not contain blockKey,
throwing an NbtApiException if the cleanup did not occur. Ensure you reference
the same blockKey/persistentData/blocks variables so the test exercises the
empty-compound removal path end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05ab2a56-8b96-4abf-8b0a-b12a521502e2
📒 Files selected for processing (4)
item-nbt-api/src/main/java/de/tr7zw/changeme/nbtapi/NBTBlock.javaitem-nbt-api/src/main/java/de/tr7zw/changeme/nbtapi/NBTCompound.javaitem-nbt-api/src/main/java/de/tr7zw/changeme/nbtapi/NBTReflectionUtil.javaitem-nbt-plugin/src/main/java/de/tr7zw/nbtapi/plugin/tests/blocks/BlockNBTTest.java
|
|
||
| @Override | ||
| protected void prepareWrite() { | ||
| getOrCreateData(); |
There was a problem hiding this comment.
This logic is a bit wonky. Before a write, it getOrCreates the tag so it's implicitly there for the later getCompound call. Trying to think if there's some cleaner way. Issue is, this api is not correctly changed to the interface, otherwise could have some "empty" impl for known empty data. But can't do that without issues at this point.
2 participants
Summary
This PR prevents
NBTBlock#getData()from creating persistent chunk PDC compounds during read-only access.Currently, simply checking a tag like this:
creates the backing
blockscompound and the block-location compound inside the chunkPersistentDataContainer, even when no data exists and the caller only wanted to read.On larger servers, plugins may check many block locations. This can cause unnecessary persistent chunk data growth from empty compounds.
Expected Behavior
Read-only access should not mutate chunk PDC data.
should only check whether the tag exists.
Previous Behavior
NBTBlock#getData()used:so even read-only access created persistent data.
Changes
NBTBlock#getData()now returns a lazy block compound wrapper.hasTagandremoveKeyon missing data do not create persistent block compounds.Compatibility
Existing write usage continues to work:
The only behavior change is that read-only access no longer creates missing persistent compounds.