GUI Shop

GUI Shop

273k Downloads

NPE on fresh install with 7.3.8 and Paper 1.12.2

A248 opened this issue ยท 7 comments

commented

GuiShop reports "Warming items" then this error immediately results:
https://pastebin.com/Ff6wWrVn

I used GuiShop 7.3.8 and Paper 1.12.2 to reproduce this error because a user in the Discord, Josh, reported the problem on these versions. By browsing git history at 7.3.8, the source of the NPE is identified:

.

Evidently, the ItemMeta itemMeta is null. The last commit which changed this is: adcc0d9, where:
ItemMeta itemMeta = itemStack.getItemMeta(); was replaced with
ItemMeta itemMeta = gItem.getItem().getItemMeta(); a few lines later.

ItemStack#getItemMeta is marked as @nullable, so it's perfectly possible that gItem.getItem().getItemMeta() returns null. However, it's confusing why itemStack.getItemMeta wouldn't return null by the same logic.

I'll complete some more testing and report the results back here.

commented

Yes, it does actually

commented

Added commit to fix this in the PR

commented

Am I good to merge this into master? I'd like to address one more issue that was reported, with arrows showing up on a full page of items, even if the second page might be emtpy. Something minor, but a nice touchup nonetheless

commented

Go ahead. Also, I was thinking about adding a display name to the buttons; I'm sure that wouldn't take long.

commented

The issue results whenever the material is invalid. Upon further testing, removing REDSTONE_LAMP and LAPIS_LAZULI from the shops.yml solved the problem configuration-wise. These items' materials are different in 1.12.2.

I initially thought this problem could be solved with a simple null-check, in #48 . Indeed, the stacktrace vanished. However, I realised that the OFF fix should have worked for REDSTONE_LAMP, since REDSTONE_LAMP_OFF is a valid material in 1.12.2. In fact, with debug logging enabled, I never received a message for the OFF fix. So what happened? Why wasn't the OFF fix triggered? To find out, I added REDSTONE_LAMP back to the shops.yml with a simple CTRL+Z.

First, I refactored material parsing to Item#parseMaterial, so that changes there will be more manageable.
Second, I noticed XMaterial is outdated. GuiShop's XMaterial has this entry:
REDSTONE_LAMP("REDSTONE_LAMP", "REDSTONE_LAMP"),
whereas the latest XMaterial has this:
REDSTONE_LAMP("REDSTONE_LAMP_OFF", "REDSTONE_LAMP_ON"),
So, updating XMaterial should solve the problem. Indeed,

The Problem
Still, even if XMaterial fails, GuiShop should use the OFF fix, right? Incorrect, actually. gItem is nonnull but itemStack is null.

After considerable testing, I modified the first try block:

			try {
				gItem = new GuiItem(itemStack);
				if (gItem == null) {
					Main.debugLog("gItem is null!");
				} else if (itemStack == null) {
					Main.debugLog("itemStack is null, but gItem is nonnull!");
				}
			} catch (Exception ex2) {
				Main.debugLog("Failed to find item by Material: " + item.getMaterial() + ". Attempting OFF Fix...");

inside the first try block.
Result in console:

WARN]: [GUIShop] [GUISHOP][DEBUG]: itemStack is null, but gItem is nonnull!

Explanation
It would seem that the first try block would fail because GuiItem's constructor GuiItem(@nonnull ItemStack) will throw an exception. Indeed, looking at the source of InventoryFramework (Eclipse > Open Declaration), this appears to be the case:
GuiShop calls GuiItem(itemStack), which calls ItemNBTUtil. The nbt utility then gets a nmsItem object. If the nms item copy is null, it throws an exception, which should propagate back to GuiShop's first try block, causing GuiShop to move on to the catch block.
In the catch block, GuiShop should complete the OFF fix.

Yet, GuiItem(ItemStack) does not throw an exception for a null itemstack. Hence, itemStack is null, gItem is nonnull, gItem.getItemStack() is nonnull, and gItem.getItemStack().getItemMeta() is null.

Solution
Check for nulls wherever applicable. Change Item#parseMaterial to ensure it continues with the OFF fix if itemStack is null, even if no exception is thrown.

Note: All of these tests were completed using Paper 1.12.2 and the master branch of GUIShop, with some locally-applied changes (e.g., null checking, more debug messages)

commented

XMaterial in its latest form, seems to fail when looking up a REDSTONE_LAMP, it will return REDSTONE_LAMP_ON, regardless of which version this is being looked up on, this is a problem because REDSTONE_LAMP_OFF is the only ItemStack that can have a NMS dupe created of it (spawnable). I personally went into XMaterial to level those out, as you can successfully spawn a REDSTONE_LAMP alone on newer versions, while the OFF fix will add _OFF to the end to ensure it works in ealier versions. In short, REDSTONE_LAMP_ON shouldn't even be an option to select, as it immediately folds support for 1.8-. Off fix is designed to correct users who accidentally use anything with _ON. It will be automatically replaced with _OFF to attempt resolution for an item.

commented

Oh, I see. If that's the case, though, shouldn't this:
itemStack = new ItemStack(Material.valueOf(getMaterial() + "_OFF"));
read like this:
itemStack = new ItemStack(Material.valueOf(getMaterial().substring(0, getMaterial().length() - "_ON".length()) + "_OFF"));
Because the "_ON" would have to be removed.