GH-221 NPE in CollectionGui from null parcel content items

[Jakubk15]

Problem

Closes #221. Opening the parcel collection GUI threw a NullPointerException:

Cannot invoke "org.bukkit.inventory.ItemStack.getType()" because "itemStack" is null
  at ...CollectionGui.lambda$createParcelItemAsync$11(CollectionGui.java:133)

CollectionGui.createParcelItemAsync iterates a parcel's stored content list and calls itemStack.getType() with no null guard. The list contained a literal null element.

Root cause

The content list survives a serialize → DB → deserialize round-trip through ItemStackPersister, which was configured with legacy (Spigot map) ItemStack serialization. Legacy serialization drops empty/air stacks to null. Empty/air inventory slots can slip past the GUI write filters (if (item == null) continue; does not catch isEmpty()/air), get persisted, and come back as null list elements — which then NPE on read.

Fix

Two layers:

1. Defensive guard at the model boundary (ParcelContent): the record's compact constructor now strips null and isEmpty() stacks and exposes an immutable list. Every read and write path funnels through ParcelContent, so this heals already-corrupted rows on read and prevents bad writes.
2. Upstream fix (ItemStackPersister): stop using legacy serialization. The jackson-bukkit Paper module defaults to an NBT byte-array format (ItemStack#serializeAsBytes) that round-trips empties safely. Its deserializer auto-detects the format, so previously stored legacy-format rows still read correctly and are rewritten in the new format on next save. This is the configuration the library docs explicitly recommend for paper plugins ("make sure that you are not using legacy serialization").

Tests

• New ParcelContentTest covers null-stripping and empty-stripping (written test-first, watched fail, then fixed).
• Added Mockito as a test dependency: paper-api ItemStacks cannot be constructed in plain unit tests (they need a running server's RegistryAccess), so ItemStack is mocked.

Verification notes

./gradlew compileJava test passes. The serialization round-trip itself is not covered by automated tests: paper-api ItemStacks require a running server and the existing integration tests are Docker-gated, so the format change relies on the library's documented backward-compatible auto-detection. Worth a manual smoke test (send a parcel, collect it) on a real server before release.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request addresses issue #221 by adding a guard in the ParcelContent constructor to filter out null and empty item stacks, and by updating ItemStackPersister to use default Paper serialization instead of legacy serialization. Additionally, Mockito was added to the test dependencies and new unit tests were introduced. The reviewer's feedback suggests optimizing the ParcelContent constructor to avoid stream overhead when the list is empty, and adding a unit test to verify that a null items list is handled correctly.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

It would be beneficial to add a unit test to verify that the compact constructor correctly handles a null items list by defaulting to an empty list, ensuring full coverage of the defensive guard.

        ParcelContent content = new ParcelContent(UUID.randomUUID(), List.of(stone, air));

        assertEquals(List.of(stone), content.items());
    }

    @Test
    void handlesNullItemsList() {
        ParcelContent content = new ParcelContent(UUID.randomUUID(), null);

        assertEquals(List.of(), content.items());
    }

[Jakubk15]

@claude review

[claude]

Claude finished @Jakubk15's task in 2s —— View job

---

I'll analyze this and get back to you.

[Jakubk15]

@claude review

[claude]

Claude finished @Jakubk15's task in 1m 49s —— View job

---

Reviewing PR #223

• Gather context (PR diff, description, existing review comments)
• Review ParcelContent.java changes
• Review ItemStackPersister.java changes
• Review AdminParcelEditGui.java changes
• Review new tests
• Post final review

Review

Solid fix, and both of Gemini's suggestions were already incorporated in the final version I checked out (ParcelContent.java:14-18 short-circuits on null/empty without building a stream, and ParcelContentTest.java:39-44 adds handlesNullItemsList). No further action needed on those two points.

What's good:

• ParcelContent's compact constructor (src/main/java/com/eternalcode/parcellockers/content/ParcelContent.java:10-19) is the right layer for this guard — I traced both the read path (ParcelContentTable.toParcelContent()) and write path (ParcelContentTable.from(), ParcelContentManager.java:45,55, ParcelServiceImpl.java:123) and every one funnels through this constructor, so the filtering is genuinely universal rather than patched at one call site.
• Confirmed the original NPE site (CollectionGui.java:133-137, iterating itemStack.getType()) now consumes content.items(), which can no longer contain nulls.
• Switching ItemStackPersister off legacy serialization (ItemStackPersister.java:24-26) is the correct root-cause fix per the jackson-bukkit docs, and the "auto-detects old format on read" behavior means existing DB rows aren't broken by the change.
• New AdminParcelEditGuiTest covering nullToEmpty is a reasonable, low-cost addition given AdminParcelEditGui.java:98-100 passes a nullable parcel.description() into String#replace.

Notes / residual risk (not blockers, already called out in the PR description):

• The serialization format switch itself isn't covered by an automated test — understandable given paper-api ItemStack needs a live server, but worth the manual smoke test (send → collect a parcel) mentioned in the PR description before this reaches a release build, particularly to confirm the auto-detecting deserializer really does transparently upgrade rows written in the old legacy format.
• ParcelContentTest mocks ItemStack rather than using a real instance; that's a reasonable trade-off for a plain unit test (real ItemStacks need RegistryAccess), and the behavior under test (null/empty filtering) doesn't depend on real Bukkit internals, so this is fine as-is.

No changes requested — this PR is already merged and the follow-up review comments were addressed in the final commits.