From 9ccf34c8425d94508031d607d688fbd208edc618 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Mon, 22 Jun 2026 18:59:33 +0200 Subject: [PATCH 1/8] fix: strip null/empty item stacks from parcel content (#221) Empty/air inventory slots could slip past the GUI write filters and round-trip through the persister as null elements, causing a NullPointerException when CollectionGui read itemStack.getType() on a collected parcel. Normalize the item list in the ParcelContent record's compact constructor so null and empty stacks are dropped and the exposed list is immutable. This is the single chokepoint every read and write path goes through, so it heals already-corrupted rows on read and prevents bad writes. Adds Mockito to mock ItemStack in unit tests, since paper-api ItemStacks cannot be constructed without a running server (RegistryAccess). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01R4YqhTV42FvN95HanC7XJc --- build.gradle.kts | 2 + buildSrc/src/main/kotlin/Versions.kt | 1 + .../parcellockers/content/ParcelContent.java | 10 +++++ .../content/ParcelContentTest.java | 38 +++++++++++++++++++ 4 files changed, 51 insertions(+) create mode 100644 src/test/java/com/eternalcode/parcellockers/content/ParcelContentTest.java diff --git a/build.gradle.kts b/build.gradle.kts index be08d5547..5c470ab48 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -88,6 +88,8 @@ dependencies { testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:${Versions.JUNIT}") testRuntimeOnly("org.junit.platform:junit-platform-launcher") + testImplementation("org.mockito:mockito-core:${Versions.MOCKITO}") + testImplementation("org.testcontainers:junit-jupiter:${Versions.TESTCONTAINERS}") testImplementation("org.testcontainers:mysql:${Versions.TESTCONTAINERS}") testImplementation("mysql:mysql-connector-java:${Versions.MYSQL_CONNECTOR}") diff --git a/buildSrc/src/main/kotlin/Versions.kt b/buildSrc/src/main/kotlin/Versions.kt index d0dc40f10..8e318c21c 100644 --- a/buildSrc/src/main/kotlin/Versions.kt +++ b/buildSrc/src/main/kotlin/Versions.kt @@ -32,4 +32,5 @@ object Versions { const val JUNIT = "6.1.0" const val TESTCONTAINERS = "1.21.4" const val MYSQL_CONNECTOR = "8.0.33" + const val MOCKITO = "5.14.2" } diff --git a/src/main/java/com/eternalcode/parcellockers/content/ParcelContent.java b/src/main/java/com/eternalcode/parcellockers/content/ParcelContent.java index 0a10c3a7e..1df6d6880 100644 --- a/src/main/java/com/eternalcode/parcellockers/content/ParcelContent.java +++ b/src/main/java/com/eternalcode/parcellockers/content/ParcelContent.java @@ -7,4 +7,14 @@ public record ParcelContent(UUID uniqueId, List items) { + public ParcelContent { + // Guard against null/empty stacks leaking into the content (issue #221): empty/air slots + // can slip past the GUI write filters and round-trip through the persister as nulls, which + // would later NPE when the collection GUI reads itemStack.getType(). + items = items == null + ? List.of() + : items.stream() + .filter(item -> item != null && !item.isEmpty()) + .toList(); + } } diff --git a/src/test/java/com/eternalcode/parcellockers/content/ParcelContentTest.java b/src/test/java/com/eternalcode/parcellockers/content/ParcelContentTest.java new file mode 100644 index 000000000..26fe03734 --- /dev/null +++ b/src/test/java/com/eternalcode/parcellockers/content/ParcelContentTest.java @@ -0,0 +1,38 @@ +package com.eternalcode.parcellockers.content; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.Arrays; +import java.util.List; +import java.util.UUID; +import org.bukkit.inventory.ItemStack; +import org.junit.jupiter.api.Test; + +class ParcelContentTest { + + @Test + void dropsNullItems() { + // Reproduces issue #221: a null element in the content list makes CollectionGui + // NPE on itemStack.getType(). The model must never expose null items. + ItemStack stone = mock(ItemStack.class); + + ParcelContent content = new ParcelContent(UUID.randomUUID(), Arrays.asList(stone, null)); + + assertEquals(List.of(stone), content.items()); + } + + @Test + void dropsEmptyItems() { + // Empty/air slots can slip past the GUI write filters; they must not be exposed + // as content, otherwise they round-trip through the persister as nulls. + ItemStack stone = mock(ItemStack.class); + ItemStack air = mock(ItemStack.class); + when(air.isEmpty()).thenReturn(true); + + ParcelContent content = new ParcelContent(UUID.randomUUID(), List.of(stone, air)); + + assertEquals(List.of(stone), content.items()); + } +} From 1ecdb8ebe9df51181ed0fd30fc4d8cbd8b2c640a Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Mon, 22 Jun 2026 19:00:57 +0200 Subject: [PATCH 2/8] fix: stop using legacy ItemStack serialization for parcel content Legacy (Spigot map) ItemStack serialization drops empty/air stacks to null, the upstream cause of the issue #221 NPE. The jackson-bukkit Paper module defaults to an NBT byte-array format (ItemStack#serializeAsBytes) that round-trips empties safely, and 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. The ParcelContent null/empty guard remains as defense in depth. Note: the serialization round-trip cannot be exercised by the existing test harness (paper-api ItemStacks require a running server, and the integration tests are Docker-gated), so this relies on the library's documented backward-compatible auto-detection. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01R4YqhTV42FvN95HanC7XJc --- .../database/persister/ItemStackPersister.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/database/persister/ItemStackPersister.java b/src/main/java/com/eternalcode/parcellockers/database/persister/ItemStackPersister.java index 63d87811c..301b12c7f 100644 --- a/src/main/java/com/eternalcode/parcellockers/database/persister/ItemStackPersister.java +++ b/src/main/java/com/eternalcode/parcellockers/database/persister/ItemStackPersister.java @@ -17,11 +17,12 @@ public class ItemStackPersister extends BaseDataType { private static final ItemStackPersister instance = new ItemStackPersister(); + // Paper plugins must NOT use legacy (Spigot map) ItemStack serialization: it drops empty/air + // stacks to null, which caused the NPE in issue #221. The default Paper serializer uses an NBT + // byte array (ItemStack#serializeAsBytes) that round-trips empties safely. The deserializer + // auto-detects and still reads any data previously written in the legacy format. private static final ObjectMapper JSON = JsonMapper.builder() - .addModule(JacksonPaper.builder() - .useLegacyItemStackSerialization() - .build() - ) + .addModule(JacksonPaper.builder().build()) .build(); private ItemStackPersister() { From dbfde431fb922033600be0ced582e739d264974f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20K=C4=99dziora?= <77227023+Jakubk15@users.noreply.github.com> Date: Mon, 22 Jun 2026 19:05:43 +0200 Subject: [PATCH 3/8] Update src/main/java/com/eternalcode/parcellockers/content/ParcelContent.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- .../com/eternalcode/parcellockers/content/ParcelContent.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/eternalcode/parcellockers/content/ParcelContent.java b/src/main/java/com/eternalcode/parcellockers/content/ParcelContent.java index 1df6d6880..3e04bfb0b 100644 --- a/src/main/java/com/eternalcode/parcellockers/content/ParcelContent.java +++ b/src/main/java/com/eternalcode/parcellockers/content/ParcelContent.java @@ -11,7 +11,7 @@ public record ParcelContent(UUID uniqueId, List items) { // Guard against null/empty stacks leaking into the content (issue #221): empty/air slots // can slip past the GUI write filters and round-trip through the persister as nulls, which // would later NPE when the collection GUI reads itemStack.getType(). - items = items == null + items = items == null || items.isEmpty() ? List.of() : items.stream() .filter(item -> item != null && !item.isEmpty()) From a1ed767515e0024dc97963d82213aeca34578305 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Mon, 22 Jun 2026 19:07:37 +0200 Subject: [PATCH 4/8] test: cover null items list in ParcelContent guard Adds a case verifying the compact constructor defaults a null items list to an empty list, closing the coverage gap on the defensive guard. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01R4YqhTV42FvN95HanC7XJc --- .../parcellockers/content/ParcelContentTest.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/test/java/com/eternalcode/parcellockers/content/ParcelContentTest.java b/src/test/java/com/eternalcode/parcellockers/content/ParcelContentTest.java index 26fe03734..2983c662f 100644 --- a/src/test/java/com/eternalcode/parcellockers/content/ParcelContentTest.java +++ b/src/test/java/com/eternalcode/parcellockers/content/ParcelContentTest.java @@ -35,4 +35,11 @@ void dropsEmptyItems() { assertEquals(List.of(stone), content.items()); } + + @Test + void handlesNullItemsList() { + ParcelContent content = new ParcelContent(UUID.randomUUID(), null); + + assertEquals(List.of(), content.items()); + } } From c8c0a0c29851c3de7544c46d9acc9374b3fa173a Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Mon, 22 Jun 2026 21:06:32 +0200 Subject: [PATCH 5/8] fix: guard against null parcel description in AdminParcelEditGui A parcel description may legitimately be null (SendingGui stores a blank description as null). AdminParcelEditGui.button() passed the value straight to String.replace(), which throws NPE when the replacement is null, crashing the admin edit GUI on InventoryClickEvent. Coerce null placeholder values to an empty string via a testable nullToEmpty() helper and cover it with unit tests. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Egi4whB4fVKQL3XH4PpkX9 --- .../admin/AdminParcelEditGui.java | 10 +++++-- .../admin/AdminParcelEditGuiTest.java | 30 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 src/test/java/com/eternalcode/parcellockers/gui/implementation/admin/AdminParcelEditGuiTest.java diff --git a/src/main/java/com/eternalcode/parcellockers/gui/implementation/admin/AdminParcelEditGui.java b/src/main/java/com/eternalcode/parcellockers/gui/implementation/admin/AdminParcelEditGui.java index bea26810a..60fa2f476 100644 --- a/src/main/java/com/eternalcode/parcellockers/gui/implementation/admin/AdminParcelEditGui.java +++ b/src/main/java/com/eternalcode/parcellockers/gui/implementation/admin/AdminParcelEditGui.java @@ -137,11 +137,17 @@ public void show(Player player) { private GuiItem button(ConfigItem template, String placeholder, String value, dev.triumphteam.gui.components.GuiAction action) { ConfigItem item = template.clone(); - return item.name(item.name().replace(placeholder, value)) - .lore(item.lore().stream().map(line -> line.replace(placeholder, value)).toList()) + String replacement = nullToEmpty(value); + return item.name(item.name().replace(placeholder, replacement)) + .lore(item.lore().stream().map(line -> line.replace(placeholder, replacement)).toList()) .toGuiItem(action); } + /** Coerces a nullable placeholder value to empty so {@link String#replace} never sees a null replacement. */ + static String nullToEmpty(String value) { + return value == null ? "" : value; + } + private void apply(Player player, CompletableFuture future) { future.thenAccept(result -> { this.notifyResult(player, result); diff --git a/src/test/java/com/eternalcode/parcellockers/gui/implementation/admin/AdminParcelEditGuiTest.java b/src/test/java/com/eternalcode/parcellockers/gui/implementation/admin/AdminParcelEditGuiTest.java new file mode 100644 index 000000000..32b466e5b --- /dev/null +++ b/src/test/java/com/eternalcode/parcellockers/gui/implementation/admin/AdminParcelEditGuiTest.java @@ -0,0 +1,30 @@ +package com.eternalcode.parcellockers.gui.implementation.admin; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +class AdminParcelEditGuiTest { + + @Test + @DisplayName("Should return empty string for a null placeholder value") + void nullToEmptyWhenValueIsNull() { + assertEquals("", AdminParcelEditGui.nullToEmpty(null)); + } + + @Test + @DisplayName("Should return the original value when not null") + void nullToEmptyWhenValueIsNotNull() { + assertEquals("desc", AdminParcelEditGui.nullToEmpty("desc")); + } + + @Test + @DisplayName("Should not throw when substituting a null parcel description into a template") + void replaceWithNullDescriptionDoesNotThrow() { + String template = "Description: {DESCRIPTION}"; + assertDoesNotThrow(() -> template.replace("{DESCRIPTION}", AdminParcelEditGui.nullToEmpty(null))); + assertEquals("Description: ", template.replace("{DESCRIPTION}", AdminParcelEditGui.nullToEmpty(null))); + } +} From 84b3de1fce0905cae10fea597aa560605559c908 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Mon, 22 Jun 2026 19:36:24 +0200 Subject: [PATCH 6/8] fix: restrict parcel collection to destination locker (#222) Parcels were collectible from any locker because CollectionGui listed every DELIVERED parcel of the receiver regardless of the locker opened. Filter the collection list to parcels destined for the current locker, guarded by the new PluginConfig.Settings#allowCollectingFromAnyLocker flag (default false) which restores the legacy collect-anywhere behavior. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01R4YqhTV42FvN95HanC7XJc --- .../parcellockers/ParcelLockers.java | 3 +- .../implementation/PluginConfig.java | 8 +++ .../parcellockers/gui/GuiManager.java | 9 ++- .../implementation/locker/CollectionGui.java | 8 ++- .../gui/implementation/locker/LockerGui.java | 3 +- .../parcellockers/parcel/Parcel.java | 10 ++++ .../parcel/ParcelDestinationTest.java | 59 +++++++++++++++++++ 7 files changed, 96 insertions(+), 4 deletions(-) create mode 100644 src/test/java/com/eternalcode/parcellockers/parcel/ParcelDestinationTest.java diff --git a/src/main/java/com/eternalcode/parcellockers/ParcelLockers.java b/src/main/java/com/eternalcode/parcellockers/ParcelLockers.java index 7f897ca12..c7342143b 100644 --- a/src/main/java/com/eternalcode/parcellockers/ParcelLockers.java +++ b/src/main/java/com/eternalcode/parcellockers/ParcelLockers.java @@ -161,7 +161,8 @@ public void onEnable() { itemStorageManager, parcelDispatchService, parcelContentManager, - deliveryManager + deliveryManager, + config.settings.allowCollectingFromAnyLocker ); MainGui mainGUI = new MainGui( diff --git a/src/main/java/com/eternalcode/parcellockers/configuration/implementation/PluginConfig.java b/src/main/java/com/eternalcode/parcellockers/configuration/implementation/PluginConfig.java index 476fc3399..1ae9d486c 100644 --- a/src/main/java/com/eternalcode/parcellockers/configuration/implementation/PluginConfig.java +++ b/src/main/java/com/eternalcode/parcellockers/configuration/implementation/PluginConfig.java @@ -78,6 +78,14 @@ public static class Settings extends OkaeriConfig { @Comment({"", "# Maximum number of parcels that can be stored in a single locker"}) public int maxParcelsPerLocker = 30; + @Comment({ + "", + "# Whether a parcel can be collected from any locker instead of only its destination locker.", + "# false (default): parcels can only be collected from the locker they were sent to.", + "# true: keeps the legacy behavior where every parcel can be collected from every locker." + }) + public boolean allowCollectingFromAnyLocker = false; + @Comment({"", "# Small parcel fee in in-game currency"}) public double smallParcelFee = 10.0; diff --git a/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java b/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java index f6852e7a6..c7f12c135 100644 --- a/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java +++ b/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java @@ -33,6 +33,7 @@ public class GuiManager { private final ParcelDispatchService parcelDispatchService; private final ParcelContentManager parcelContentManager; private final DeliveryManager deliveryManager; + private final boolean allowCollectingFromAnyLocker; public GuiManager( ParcelService parcelService, @@ -41,7 +42,8 @@ public GuiManager( ItemStorageManager itemStorageManager, ParcelDispatchService parcelDispatchService, ParcelContentManager parcelContentManager, - DeliveryManager deliveryManager + DeliveryManager deliveryManager, + boolean allowCollectingFromAnyLocker ) { this.parcelService = parcelService; this.lockerManager = lockerManager; @@ -50,6 +52,11 @@ public GuiManager( this.parcelDispatchService = parcelDispatchService; this.parcelContentManager = parcelContentManager; this.deliveryManager = deliveryManager; + this.allowCollectingFromAnyLocker = allowCollectingFromAnyLocker; + } + + public boolean isCollectingFromAnyLockerAllowed() { + return this.allowCollectingFromAnyLocker; } public void sendParcel(Player sender, Parcel parcel, List items) { diff --git a/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/CollectionGui.java b/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/CollectionGui.java index cd101250d..b94205451 100644 --- a/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/CollectionGui.java +++ b/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/CollectionGui.java @@ -18,6 +18,7 @@ import dev.triumphteam.gui.guis.PaginatedGui; import java.util.ArrayList; import java.util.List; +import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.function.Supplier; import net.kyori.adventure.text.Component; @@ -35,17 +36,20 @@ public class CollectionGui implements GuiView { private final Scheduler scheduler; private final GuiManager guiManager; private final MiniMessage miniMessage; + private final UUID currentLocker; public CollectionGui( GuiSettings guiSettings, Scheduler scheduler, GuiManager guiManager, - MiniMessage miniMessage + MiniMessage miniMessage, + UUID currentLocker ) { this.guiSettings = guiSettings; this.scheduler = scheduler; this.guiManager = guiManager; this.miniMessage = miniMessage; + this.currentLocker = currentLocker; } @Override @@ -79,6 +83,8 @@ public void show(Player player, Page page) { result.items().stream() .filter(parcel -> parcel.status() == ParcelStatus.DELIVERED) + .filter(parcel -> this.guiManager.isCollectingFromAnyLockerAllowed() + || parcel.isDestinedFor(this.currentLocker)) .map(parcel -> this.createParcelItemAsync(parcel, parcelItem, player, refresher)) .collect(CompletableFutures.joinList()) .thenAccept(suppliers -> { diff --git a/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/LockerGui.java b/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/LockerGui.java index 16dae36e5..3ee62762a 100644 --- a/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/LockerGui.java +++ b/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/LockerGui.java @@ -49,7 +49,8 @@ public void show(Player player, UUID entryLocker) { this.guiSettings, this.scheduler, this.guiManager, - this.miniMessage + this.miniMessage, + entryLocker ); gui.setItem(21, this.guiSettings.parcelLockerCollectItem.toGuiItem(event -> collectionGui.show(player))); diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/Parcel.java b/src/main/java/com/eternalcode/parcellockers/parcel/Parcel.java index c248c7ed4..7d3b92284 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/Parcel.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/Parcel.java @@ -14,4 +14,14 @@ public record Parcel( UUID destinationLocker, ParcelStatus status ) { + + /** + * Checks whether this parcel is destined for (and therefore collectible from) the given locker. + * + * @param locker the locker the player is currently interacting with + * @return {@code true} only when the locker matches this parcel's destination locker + */ + public boolean isDestinedFor(UUID locker) { + return locker != null && locker.equals(this.destinationLocker); + } } diff --git a/src/test/java/com/eternalcode/parcellockers/parcel/ParcelDestinationTest.java b/src/test/java/com/eternalcode/parcellockers/parcel/ParcelDestinationTest.java new file mode 100644 index 000000000..4fb3ef9c5 --- /dev/null +++ b/src/test/java/com/eternalcode/parcellockers/parcel/ParcelDestinationTest.java @@ -0,0 +1,59 @@ +package com.eternalcode.parcellockers.parcel; + +import java.util.UUID; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class ParcelDestinationTest { + + private static Parcel parcelWithDestination(UUID destinationLocker) { + return new Parcel( + UUID.randomUUID(), + UUID.randomUUID(), + "name", + "description", + false, + UUID.randomUUID(), + ParcelSize.SMALL, + UUID.randomUUID(), + destinationLocker, + ParcelStatus.DELIVERED + ); + } + + @Test + @DisplayName("Should be destined for the locker matching its destination locker") + void isDestinedForMatchingLocker() { + UUID locker = UUID.randomUUID(); + Parcel parcel = parcelWithDestination(locker); + + assertTrue(parcel.isDestinedFor(locker)); + } + + @Test + @DisplayName("Should not be destined for a locker other than its destination locker") + void isDestinedForOtherLocker() { + Parcel parcel = parcelWithDestination(UUID.randomUUID()); + + assertFalse(parcel.isDestinedFor(UUID.randomUUID())); + } + + @Test + @DisplayName("Should not be destined for any locker when its destination locker is null") + void isDestinedForWhenDestinationIsNull() { + Parcel parcel = parcelWithDestination(null); + + assertFalse(parcel.isDestinedFor(UUID.randomUUID())); + } + + @Test + @DisplayName("Should not be destined for a null locker") + void isDestinedForNullLocker() { + Parcel parcel = parcelWithDestination(UUID.randomUUID()); + + assertFalse(parcel.isDestinedFor(null)); + } +} From 6425e972e8bbb85e7e80f17dad9b1484e5961fef Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Mon, 22 Jun 2026 19:41:15 +0200 Subject: [PATCH 7/8] refactor: filter collectible parcels in the query, not after paging CollectionGui paged receiver parcels (28/page) then filtered DELIVERED and destination-locker client-side, so a page could render fewer than its size and the has-next probe counted ineligible rows. Move the receiver + DELIVERED (+ optional destination locker) filter into the ORMLite query via findCollectible, so pagination operates on the eligible set. Drops the now redundant client-side filters and the unused Parcel#isDestinedFor helper; adds a Docker-gated integration test for the paged query. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01R4YqhTV42FvN95HanC7XJc --- .../parcellockers/gui/GuiManager.java | 9 +- .../implementation/locker/CollectionGui.java | 6 +- .../parcellockers/parcel/Parcel.java | 10 -- .../parcel/repository/ParcelRepository.java | 12 ++ .../repository/ParcelRepositoryOrmLite.java | 18 +++ .../parcel/service/ParcelService.java | 8 ++ .../parcel/service/ParcelServiceImpl.java | 12 ++ .../ParcelFindCollectibleIntegrationTest.java | 110 ++++++++++++++++++ .../parcel/ParcelDestinationTest.java | 59 ---------- 9 files changed, 168 insertions(+), 76 deletions(-) create mode 100644 src/test/java/com/eternalcode/parcellockers/database/ParcelFindCollectibleIntegrationTest.java delete mode 100644 src/test/java/com/eternalcode/parcellockers/parcel/ParcelDestinationTest.java diff --git a/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java b/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java index c7f12c135..f3e51bdd9 100644 --- a/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java +++ b/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java @@ -55,8 +55,13 @@ public GuiManager( this.allowCollectingFromAnyLocker = allowCollectingFromAnyLocker; } - public boolean isCollectingFromAnyLockerAllowed() { - return this.allowCollectingFromAnyLocker; + /** + * Returns the delivered parcels the receiver may collect at the locker they are interacting with. + * When {@code allowCollectingFromAnyLocker} is enabled the locker restriction is dropped. + */ + public CompletableFuture> getCollectibleParcels(UUID receiver, UUID currentLocker, Page page) { + UUID destinationLocker = this.allowCollectingFromAnyLocker ? null : currentLocker; + return this.parcelService.getCollectible(receiver, destinationLocker, page); } public void sendParcel(Player sender, Parcel parcel, List items) { diff --git a/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/CollectionGui.java b/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/CollectionGui.java index b94205451..b1e105174 100644 --- a/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/CollectionGui.java +++ b/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/CollectionGui.java @@ -8,7 +8,6 @@ import com.eternalcode.parcellockers.gui.GuiView; import com.eternalcode.parcellockers.gui.PaginatedGuiRefresher; import com.eternalcode.parcellockers.parcel.Parcel; -import com.eternalcode.parcellockers.parcel.ParcelStatus; import com.eternalcode.parcellockers.parcel.util.PlaceholderUtil; import com.eternalcode.parcellockers.shared.Page; import com.eternalcode.parcellockers.util.MaterialUtil; @@ -70,7 +69,7 @@ public void show(Player player, Page page) { this.setupStaticItems(player, gui); - this.guiManager.getParcelsByReceiver(player.getUniqueId(), page).thenAccept(result -> { + this.guiManager.getCollectibleParcels(player.getUniqueId(), this.currentLocker, page).thenAccept(result -> { if (result == null || result.items().isEmpty()) { gui.setItem(22, this.guiSettings.noParcelsItem.toGuiItem()); this.scheduler.run(() -> gui.open(player)); @@ -82,9 +81,6 @@ public void show(Player player, Page page) { this.setupNavigation(gui, page, result, player, this.guiSettings); result.items().stream() - .filter(parcel -> parcel.status() == ParcelStatus.DELIVERED) - .filter(parcel -> this.guiManager.isCollectingFromAnyLockerAllowed() - || parcel.isDestinedFor(this.currentLocker)) .map(parcel -> this.createParcelItemAsync(parcel, parcelItem, player, refresher)) .collect(CompletableFutures.joinList()) .thenAccept(suppliers -> { diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/Parcel.java b/src/main/java/com/eternalcode/parcellockers/parcel/Parcel.java index 7d3b92284..c248c7ed4 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/Parcel.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/Parcel.java @@ -14,14 +14,4 @@ public record Parcel( UUID destinationLocker, ParcelStatus status ) { - - /** - * Checks whether this parcel is destined for (and therefore collectible from) the given locker. - * - * @param locker the locker the player is currently interacting with - * @return {@code true} only when the locker matches this parcel's destination locker - */ - public boolean isDestinedFor(UUID locker) { - return locker != null && locker.equals(this.destinationLocker); - } } diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepository.java b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepository.java index 7f62feca9..43f5b49f0 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepository.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepository.java @@ -31,6 +31,18 @@ public interface ParcelRepository { CompletableFuture> findByReceiver(UUID receiver, Page page); + /** + * Finds the parcels a receiver is allowed to collect: those addressed to them whose status is + * {@link com.eternalcode.parcellockers.parcel.ParcelStatus#DELIVERED}. Filtering happens in the + * query so pagination operates on the eligible set rather than on a raw receiver page. + * + * @param receiver the receiver whose parcels are collected + * @param destinationLocker when non-null, only parcels addressed to this locker are returned; + * when null, delivered parcels from any locker are returned + * @param page the requested page + */ + CompletableFuture> findCollectible(UUID receiver, UUID destinationLocker, Page page); + /** * Counts the parcels currently occupying a destination locker. Collected parcels are removed * from storage, so every parcel addressed to the locker (in-transit or delivered) occupies a diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java index 5bdd549ff..43c560084 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java @@ -4,6 +4,7 @@ import com.eternalcode.parcellockers.database.DatabaseManager; import com.eternalcode.parcellockers.database.wrapper.AbstractRepositoryOrmLite; import com.eternalcode.parcellockers.parcel.Parcel; +import com.eternalcode.parcellockers.parcel.ParcelStatus; import com.eternalcode.parcellockers.shared.Page; import com.eternalcode.parcellockers.shared.PageResult; import java.util.List; @@ -17,6 +18,7 @@ public class ParcelRepositoryOrmLite extends AbstractRepositoryOrmLite implement private static final String RECEIVER_COLUMN = "receiver"; private static final String SENDER_COLUMN = "sender"; private static final String DESTINATION_LOCKER_COLUMN = "destination_locker"; + private static final String STATUS_COLUMN = "status"; public ParcelRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) { super(databaseManager, scheduler); @@ -78,6 +80,22 @@ public CompletableFuture> findByReceiver(UUID receiver, Page return this.findByPaged(receiver, page, RECEIVER_COLUMN); } + @Override + public CompletableFuture> findCollectible(UUID receiver, UUID destinationLocker, Page page) { + Objects.requireNonNull(receiver, "Receiver UUID cannot be null"); + Objects.requireNonNull(page, "Page cannot be null"); + return this.queryPage(ParcelTable.class, page, builder -> { + var where = builder.where() + .eq(RECEIVER_COLUMN, receiver) + .and() + .eq(STATUS_COLUMN, ParcelStatus.DELIVERED); + if (destinationLocker != null) { + where.and().eq(DESTINATION_LOCKER_COLUMN, destinationLocker); + } + return builder; + }, ParcelTable::toParcel); + } + @Override public CompletableFuture countParcelsByDestinationLocker(UUID destinationLocker) { Objects.requireNonNull(destinationLocker, "Destination locker UUID cannot be null"); diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelService.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelService.java index e733114a8..abaa9cb55 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelService.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelService.java @@ -36,6 +36,14 @@ public interface ParcelService { CompletableFuture> getByReceiver(UUID receiver, Page page); + /** + * Returns the delivered parcels a receiver may collect, optionally restricted to a single + * destination locker. Filtering is applied in the query so pagination stays consistent. + * + * @param destinationLocker the locker to collect from, or null to allow any locker + */ + CompletableFuture> getCollectible(UUID receiver, UUID destinationLocker, Page page); + CompletableFuture> getAll(Page page); CompletableFuture delete(UUID uuid); diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java index 0ecde6da3..27a687f88 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java @@ -309,6 +309,18 @@ public CompletableFuture> getByReceiver(UUID receiver, Page p }); } + @Override + public CompletableFuture> getCollectible(UUID receiver, UUID destinationLocker, Page page) { + Objects.requireNonNull(receiver, "Receiver UUID cannot be null"); + Objects.requireNonNull(page, "Page cannot be null"); + + return this.parcelRepository.findCollectible(receiver, destinationLocker, page) + .thenApply(result -> { + result.items().forEach(parcel -> this.parcelsByUuid.put(parcel.uuid(), parcel)); + return result; + }); + } + @Override public CompletableFuture> getAll(Page page) { Objects.requireNonNull(page, "Page cannot be null"); diff --git a/src/test/java/com/eternalcode/parcellockers/database/ParcelFindCollectibleIntegrationTest.java b/src/test/java/com/eternalcode/parcellockers/database/ParcelFindCollectibleIntegrationTest.java new file mode 100644 index 000000000..87a2da64f --- /dev/null +++ b/src/test/java/com/eternalcode/parcellockers/database/ParcelFindCollectibleIntegrationTest.java @@ -0,0 +1,110 @@ +package com.eternalcode.parcellockers.database; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.eternalcode.parcellockers.TestScheduler; +import com.eternalcode.parcellockers.configuration.implementation.PluginConfig; +import com.eternalcode.parcellockers.parcel.Parcel; +import com.eternalcode.parcellockers.parcel.ParcelSize; +import com.eternalcode.parcellockers.parcel.ParcelStatus; +import com.eternalcode.parcellockers.parcel.repository.ParcelRepository; +import com.eternalcode.parcellockers.parcel.repository.ParcelRepositoryOrmLite; +import com.eternalcode.parcellockers.shared.Page; +import com.eternalcode.parcellockers.shared.PageResult; +import java.nio.file.Path; +import java.sql.SQLException; +import java.util.UUID; +import java.util.logging.Logger; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.testcontainers.containers.MySQLContainer; +import org.testcontainers.junit.jupiter.Container; +import org.testcontainers.junit.jupiter.Testcontainers; +import org.testcontainers.utility.DockerImageName; + +@Testcontainers(disabledWithoutDocker = true) +class ParcelFindCollectibleIntegrationTest extends IntegrationTestSpec { + + @Container + private static final MySQLContainer mySQLContainer = new MySQLContainer<>(DockerImageName.parse("mysql:latest")); + + @TempDir + private Path tempDir; + + private DatabaseManager databaseManager; + + private ParcelRepository repository() throws SQLException { + PluginConfig config = new PluginConfig(); + config.settings.databaseType = DatabaseType.MYSQL; + config.settings.host = mySQLContainer.getHost(); + config.settings.port = String.valueOf(mySQLContainer.getFirstMappedPort()); + config.settings.databaseName = mySQLContainer.getDatabaseName(); + config.settings.user = mySQLContainer.getUsername(); + config.settings.password = mySQLContainer.getPassword(); + + DatabaseManager databaseManager = new DatabaseManager(config, Logger.getLogger("ParcelLockers"), this.tempDir.toFile()); + databaseManager.connect(); + this.databaseManager = databaseManager; + + return new ParcelRepositoryOrmLite(databaseManager, new TestScheduler()); + } + + private void save(ParcelRepository repository, UUID receiver, UUID destinationLocker, ParcelStatus status) { + this.await(repository.save(new Parcel( + UUID.randomUUID(), UUID.randomUUID(), "p", "d", false, + receiver, ParcelSize.SMALL, UUID.randomUUID(), destinationLocker, status))); + } + + @Test + void findCollectibleReturnsOnlyDeliveredParcelsForTheGivenLockerAndReceiver() throws SQLException { + ParcelRepository repository = this.repository(); + + UUID receiver = UUID.randomUUID(); + UUID locker = UUID.randomUUID(); + UUID otherLocker = UUID.randomUUID(); + + for (int i = 0; i < 3; i++) { + this.save(repository, receiver, locker, ParcelStatus.DELIVERED); + } + this.save(repository, receiver, locker, ParcelStatus.SENT); // not yet delivered + this.save(repository, receiver, otherLocker, ParcelStatus.DELIVERED); // different locker + this.save(repository, UUID.randomUUID(), locker, ParcelStatus.DELIVERED); // different receiver + + // Pagination must count only the 3 eligible parcels, not the raw receiver page. + PageResult firstPage = this.await(repository.findCollectible(receiver, locker, new Page(0, 2))); + assertEquals(2, firstPage.items().size()); + assertTrue(firstPage.hasNextPage()); + + PageResult secondPage = this.await(repository.findCollectible(receiver, locker, new Page(1, 2))); + assertEquals(1, secondPage.items().size()); + assertFalse(secondPage.hasNextPage()); + } + + @Test + void findCollectibleWithNullLockerReturnsDeliveredParcelsFromAnyLocker() throws SQLException { + ParcelRepository repository = this.repository(); + + UUID receiver = UUID.randomUUID(); + + this.save(repository, receiver, UUID.randomUUID(), ParcelStatus.DELIVERED); + this.save(repository, receiver, UUID.randomUUID(), ParcelStatus.DELIVERED); + this.save(repository, receiver, UUID.randomUUID(), ParcelStatus.SENT); // excluded by status + this.save(repository, UUID.randomUUID(), UUID.randomUUID(), ParcelStatus.DELIVERED); // other receiver + + PageResult page = this.await(repository.findCollectible(receiver, null, new Page(0, 10))); + assertEquals(2, page.items().size()); + assertFalse(page.hasNextPage()); + assertTrue(page.items().stream().allMatch(parcel -> parcel.receiver().equals(receiver))); + assertTrue(page.items().stream().allMatch(parcel -> parcel.status() == ParcelStatus.DELIVERED)); + } + + @AfterEach + void tearDown() { + if (this.databaseManager != null) { + this.databaseManager.disconnect(); + } + } +} diff --git a/src/test/java/com/eternalcode/parcellockers/parcel/ParcelDestinationTest.java b/src/test/java/com/eternalcode/parcellockers/parcel/ParcelDestinationTest.java deleted file mode 100644 index 4fb3ef9c5..000000000 --- a/src/test/java/com/eternalcode/parcellockers/parcel/ParcelDestinationTest.java +++ /dev/null @@ -1,59 +0,0 @@ -package com.eternalcode.parcellockers.parcel; - -import java.util.UUID; -import org.junit.jupiter.api.DisplayName; -import org.junit.jupiter.api.Test; - -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; - -class ParcelDestinationTest { - - private static Parcel parcelWithDestination(UUID destinationLocker) { - return new Parcel( - UUID.randomUUID(), - UUID.randomUUID(), - "name", - "description", - false, - UUID.randomUUID(), - ParcelSize.SMALL, - UUID.randomUUID(), - destinationLocker, - ParcelStatus.DELIVERED - ); - } - - @Test - @DisplayName("Should be destined for the locker matching its destination locker") - void isDestinedForMatchingLocker() { - UUID locker = UUID.randomUUID(); - Parcel parcel = parcelWithDestination(locker); - - assertTrue(parcel.isDestinedFor(locker)); - } - - @Test - @DisplayName("Should not be destined for a locker other than its destination locker") - void isDestinedForOtherLocker() { - Parcel parcel = parcelWithDestination(UUID.randomUUID()); - - assertFalse(parcel.isDestinedFor(UUID.randomUUID())); - } - - @Test - @DisplayName("Should not be destined for any locker when its destination locker is null") - void isDestinedForWhenDestinationIsNull() { - Parcel parcel = parcelWithDestination(null); - - assertFalse(parcel.isDestinedFor(UUID.randomUUID())); - } - - @Test - @DisplayName("Should not be destined for a null locker") - void isDestinedForNullLocker() { - Parcel parcel = parcelWithDestination(UUID.randomUUID()); - - assertFalse(parcel.isDestinedFor(null)); - } -} From be438662063c6c04d7bf39ef3d8784cc722e1502 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Thu, 2 Jul 2026 22:10:48 +0200 Subject: [PATCH 8/8] fix: require a concrete locker when parcel collection is restricted Addresses PR #230 review: getCollectibleParcels passed currentLocker straight through, and getCollectible/findCollectible treat a null destination locker as "any locker". A null currentLocker while allowCollectingFromAnyLocker is false would therefore silently drop the locker restriction (fail-open authorization bypass). Not reachable today (the locker UUID always comes from a real Locker via LockerInteractionController), but the invariant was implicit. Make it explicit and fail-closed with Objects.requireNonNull, and only pass null on the deliberate collect-from-any-locker path. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UWGThsFyex5LiEyN63L3Wu --- .../eternalcode/parcellockers/gui/GuiManager.java | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java b/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java index f3e51bdd9..5ed683ef6 100644 --- a/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java +++ b/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java @@ -17,6 +17,7 @@ import com.eternalcode.parcellockers.user.User; import com.eternalcode.parcellockers.user.UserManager; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; @@ -58,10 +59,18 @@ public GuiManager( /** * Returns the delivered parcels the receiver may collect at the locker they are interacting with. * When {@code allowCollectingFromAnyLocker} is enabled the locker restriction is dropped. + * + *

{@code getCollectible} treats a {@code null} destination locker as "collect from any locker", + * so when the restriction is active a concrete {@code currentLocker} is required — passing + * {@code null} here would silently drop the restriction and let the receiver collect from any locker. */ public CompletableFuture> getCollectibleParcels(UUID receiver, UUID currentLocker, Page page) { - UUID destinationLocker = this.allowCollectingFromAnyLocker ? null : currentLocker; - return this.parcelService.getCollectible(receiver, destinationLocker, 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); } public void sendParcel(Player sender, Parcel parcel, List items) {