Avoid using Inventory.getHolder() in hopper check

[nouish]

See RazielMartelus96's thread on SpigotMC for discussion related to use of Inventory.getHolder().

TL;DR

Inventory.getHolder() return state snapshots, and this can be very slow for items that hold a lot of NBT data, like shulker boxes or custom items.

GriefPrevention has a single call to this method:
https://github.com/TechFortress/GriefPrevention/blob/a8d55dbdcef13acad59fa5d14c77cd291ebaf830/src/main/java/me/ryanhamshire/GriefPrevention/BlockEventHandler.java#L1105-L1106

... and it's not needed.

Testing

Using the following code:

GriefPrevention.instance.getLogger().info("-".repeat(25));
GriefPrevention.instance.getLogger().info("Item: " + event.getItem().getName());
GriefPrevention.instance.getLogger().info("hashCode: " + System.identityHashCode(event.getInventory().getHolder()));
GriefPrevention.instance.getLogger().info("hashCode: " + System.identityHashCode(event.getInventory().getHolder()));

Then dropping a protected item spawned with:

@EventHandler
void onItemSpawn(ItemSpawnEvent event)
{
    event.getEntity().setMetadata("GP_ITEMOWNER", new FixedMetadataValue(GriefPrevention.instance,
        UUID.fromString("<UUID OF AN ONLINE PLAYER - YOU!?>")));
}

Will reveal that different instances are indeed given for subsequent calls:

[02:05:02 INFO]: [GriefPrevention] -------------------------
[02:05:02 INFO]: [GriefPrevention] Item: Iron Trapdoor
[02:05:02 INFO]: [GriefPrevention] hashCode: 1407605097
[02:05:02 INFO]: [GriefPrevention] hashCode: 522167492
[02:05:02 INFO]: [GriefPrevention] -------------------------
[02:05:02 INFO]: [GriefPrevention] Item: Cyan Shulker Box
[02:05:02 INFO]: [GriefPrevention] hashCode: 2098715974
[02:05:02 INFO]: [GriefPrevention] hashCode: 692044206
[02:05:02 INFO]: [GriefPrevention] -------------------------
[02:05:02 INFO]: [GriefPrevention] Item: Iron Trapdoor
[02:05:02 INFO]: [GriefPrevention] hashCode: 1223944433
[02:05:02 INFO]: [GriefPrevention] hashCode: 715676847
[02:05:02 INFO]: [GriefPrevention] -------------------------
[02:05:02 INFO]: [GriefPrevention] Item: Cyan Shulker Box
[02:05:02 INFO]: [GriefPrevention] hashCode: 1763068241
[02:05:02 INFO]: [GriefPrevention] hashCode: 1047775084

Solution

Simply change the GriefPrevention code referenced above to this:

if (event.getInventory().getType() == InventoryType.HOPPER)

This PR tidies up the related code, and as a result is not a one-line change.

In addition to the screenshot below proving behaviour is unchanged, there are also some tests included with the same intent. Feel free to omit those, if you don't want to add the approx. 2 second increase in build time Mockito adds.

[Jikoo]

Looks good to me. If Robo is particularly concerned about the holder being a real in-world holder, it's possible, just a bit messy.

[nouish]

Looks good to me. If Robo is particularly concerned about the holder being a real in-world holder, it's possible, just a bit messy.

The InventoryHolder is org.bukkit.inventory.InventoryHolder.

[nouish]

It just occured to me that the current behaviour here is strange.

To me it would make sense to allow hoppers picking up items for players with access to containers in the same claim.

Thoughts, @Jikoo?

[Jikoo]

Looks good to me. If Robo is particularly concerned about the holder being a real in-world holder, it's possible, just a bit messy.

The InventoryHolder is org.bukkit.inventory.InventoryHolder.

I'm referring to the fact that the inventory type could be HOPPER but not actually be backed by a hopper minecart or a hopper. I don't think it's worth being concerned about (if anything, does the inventory type check even need to exist in vanilla? Would this offer better weird hybrid server support or futureproofing if it didn't check at all?) but if Robo does want that, it is possible to verify the holder without snapshotting it via inventory location. I made a post in the linked thread about it.

It just occured to me that the current behaviour here is strange.

To me it would make sense to allow hoppers picking up items for players with access to containers in the same claim.

It prevents some abuse, i.e. I make a deathtrap over a hopper in a 1x1 subclaim with /containertrust public, then the hopper feeds all the drops into my inaccessible personal storage. This way at least the items might actually be retrievable if you can arrange surviving the trap, and if they're not, admin intervention is required either way. Weird, niche abuse to be sure, but actually something I've seen done.

[nouish]

Looks good to me. If Robo is particularly concerned about the holder being a real in-world holder, it's possible, just a bit messy.

The InventoryHolder is org.bukkit.inventory.InventoryHolder.

I'm referring to the fact that the inventory type could be HOPPER but not actually be backed by a hopper minecart or a hopper. I don't think it's worth being concerned about (if anything, does the inventory type check even need to exist in vanilla? Would this offer better weird hybrid server support or futureproofing if it didn't check at all?) but if Robo does want that, it is possible to verify the holder without snapshotting it via inventory location. I made a post in the linked thread about it.

I haven't checked the implementation, but do you reckon a InventoryPickupItemEvent would be called without a 'physical' block/minecart, without an actual hopper? That would surprise me for sure.

Per documentation the event is Called when a hopper or hopper minecart picks up a dropped item., so no, the check is not needed in vanilla. InventoryPickupItemEvent is a bad name for this event. I thought it was generic, and not specific for hoppers.

So it could either be removed entirely, or this current PR might avoid an xkcd#1172 situation, if another plugin is doing something weird.

It just occured to me that the current behaviour here is strange.
To me it would make sense to allow hoppers picking up items for players with access to containers in the same claim.

It prevents some abuse, i.e. I make a deathtrap over a hopper in a 1x1 subclaim with /containertrust public, then the hopper feeds all the drops into my inaccessible personal storage. This way at least the items might actually be retrievable if you can arrange surviving the trap, and if they're not, admin intervention is required either way. Weird, niche abuse to be sure, but actually something I've seen done.

Interesting point.

[RoboMWM]

This sort of stuff are things I kinda wanna remove, but I'll need to update statistics to see precisely how much PvE rules are in use. My guess is it's higher than creative claim users, and it is a very niche player-side grief.