Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
Expand Down
1 change: 1 addition & 0 deletions buildSrc/src/main/kotlin/Versions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,14 @@

public record ParcelContent(UUID uniqueId, List<ItemStack> 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 || items.isEmpty()
? List.of()
: items.stream()
.filter(item -> item != null && !item.isEmpty())
.toList();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,17 @@ public void show(Player player) {

private GuiItem button(ConfigItem template, String placeholder, String value, dev.triumphteam.gui.components.GuiAction<org.bukkit.event.inventory.InventoryClickEvent> 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<EditResult> future) {
future.thenAccept(result -> {
this.notifyResult(player, result);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
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());
}
Comment on lines +34 to +37

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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());
    }


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

assertEquals(List.of(), content.items());
}
}
Original file line number Diff line number Diff line change
@@ -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)));
}
}
Loading