fix/issue 222

[Jakubk15]

• fix: strip null/empty item stacks from parcel content (Exception in future due to NPE #221)
• fix: stop using legacy ItemStack serialization for parcel content
• Update src/main/java/com/eternalcode/parcellockers/content/ParcelContent.java
• test: cover null items list in ParcelContent guard
• fix: guard against null parcel description in AdminParcelEditGui
• fix: restrict parcel collection to destination locker (Every parcel can be collected from every locker #222)
• refactor: filter collectible parcels in the query, not after paging

[gemini-code-assist]

Code Review

This pull request introduces a new configuration option allowCollectingFromAnyLocker to control whether players can collect parcels from any locker or only their designated destination locker. It refactors parcel retrieval to filter collectible parcels (status DELIVERED and matching locker, if restricted) directly in the database query rather than in-memory, improving pagination consistency. Additionally, it addresses NPE issues (such as Issue #221) by filtering out null/empty items in ParcelContent and removing legacy item stack serialization in ItemStackPersister, and adds a helper to handle null placeholders in AdminParcelEditGui. Feedback is provided regarding a potential logic bypass in GuiManager#getCollectibleParcels where a null currentLocker could unintentionally bypass the locker restriction when allowCollectingFromAnyLocker is false.

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]

If allowCollectingFromAnyLocker is false but currentLocker is null, destinationLocker will be set to null. In ParcelRepositoryOrmLite#findCollectible, a null destination locker bypasses the locker restriction filter entirely, allowing the player to collect parcels from any locker. To prevent this security/logic bypass, we should ensure that a non-existent/dummy UUID is used when currentLocker is null and restriction is active.

Suggested change

	public CompletableFuture<PageResult<Parcel>> getCollectibleParcels(UUID receiver, UUID currentLocker, Page page) {
	UUID destinationLocker = this.allowCollectingFromAnyLocker ? null : currentLocker;
	return this.parcelService.getCollectible(receiver, destinationLocker, page);
	}
	public CompletableFuture<PageResult<Parcel>> getCollectibleParcels(UUID receiver, UUID currentLocker, Page page) {
	UUID destinationLocker = this.allowCollectingFromAnyLocker ? null : (currentLocker != null ? currentLocker : new UUID(0L, 0L));
	return this.parcelService.getCollectible(receiver, destinationLocker, page);
	}

[Jakubk15]

@claude review

[claude]

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

---

Review of fix/issue-222 (merged)

Todo list:

• Gather context (PR diff, changed files, prior review comments)
• Read changed files in detail
• Verify/assess the Gemini bot's flagged issue in GuiManager.getCollectibleParcels
• Review remaining changed files for bugs/quality issues
• Post final review

Gemini's flagged bug — resolved correctly

Gemini flagged that a null currentLocker with allowCollectingFromAnyLocker = false would silently bypass the locker restriction (since findCollectible treats null destination locker as "any locker"). Checking GuiManager.java:67-74 on the current master, this has already been fixed — but with a better approach than Gemini's suggested dummy-UUID sentinel:

public CompletableFuture<PageResult<Parcel>> getCollectibleParcels(UUID receiver, UUID currentLocker, Page page) {
    if (this.allowCollectingFromAnyLocker) {
        return this.parcelService.getCollectible(receiver, null, page);
    }
    Objects.requireNonNull(currentLocker,
        "currentLocker must not be null when collection is restricted to the destination locker");
    return this.parcelService.getCollectible(receiver, currentLocker, page);
}

Failing fast with requireNonNull is preferable to a dummy new UUID(0L, 0L) sentinel — the latter would silently return an empty (but technically not bypassed) result set if a caller ever passed null, masking a caller bug. requireNonNull surfaces it immediately.

I traced the only caller path (LockerInteractionController → LockerGui.show → CollectionGui) and currentLocker/entryLocker always originates from a real Locker.uuid(), so this can't currently throw in practice — the guard is a correct defensive invariant, not a live bug.

Rest of the diff

• ParcelRepositoryOrmLite.findCollectible — filtering DELIVERED status + optional destination-locker match directly in the ORMLite query builder is the right fix for the original pagination bug (previously filtering post-page would under-fill/mis-count pages).
• ParcelFindCollectibleIntegrationTest — good coverage: verifies pagination counts only eligible parcels across the restricted-locker case and the allowCollectingFromAnyLocker/null-locker case.
• PluginConfig.allowCollectingFromAnyLocker — defaults to false (secure by default, matches the new restrictive behavior), with a clear comment describing both modes.
• ParcelService/ParcelRepository interfaces and impls — consistent null-checks (Objects.requireNonNull) added on the new parameter path; Javadoc on findCollectible/getCollectible clearly documents the null = "any locker" contract, which is what made the original bug easy to introduce and now makes it easy to avoid.

No further issues found. Nice fix — the requireNonNull fail-fast approach is arguably safer than the review suggestion since it converts a security-relevant silent-bypass class of bug into a loud, unmissable failure.

Branch ·