CanDestroy and CanPlaceOn tags from serialized item stacks are lost when switching between Spigot and Paper.

blablubbabc commented 3 years ago

Expected behavior

Server owners being able to freely switch between Spigot and Paper back and forth without risking serialized item data to be lost.

Observed/Actual behavior

A user of one of my plugins, which uses the Bukkit ItemStack config serialization API to save and load item stacks to and from config files, complained about the CanDestroy and CanPlaceOn item tags being lost when they switched from using Spigot to using Paper. I assume the same issue applies in the opposite direction as well, when switching from Paper back to Spigot.

Steps/models to reproduce

Create an item stack with a CanDestroy tag. For example: /minecraft:give @s minecraft:diamond_pickaxe{CanDestroy:["minecraft:iron_ore"]}

Use the Bukkit config serialization API to save this item stack on Spigot. Example output:

==: org.bukkit.inventory.ItemStack
v: 2730
type: DIAMOND_PICKAXE
meta:
==: ItemMeta
meta-type: UNSPECIFIC
internal: H4sIAAAAAAAA/+NiYOBk4HJOzHNJLS4pyq/kYGBgYGQQys3MS00uSkwrscosys+Lzy9KZQAAMhDaySoAAAA=

Switch over to using Paper, or copy this data to a server that runs Paper.
Load this item stack using the Bukkit config serialization API.
Observation: The item's CanDestroy tag is not loaded.
Saving the just loaded item stack back to the config yields this output (no traces of the item's previous CanDestroy data remains):
```
==: org.bukkit.inventory.ItemStack
v: 2730
type: DIAMOND_PICKAXE
```

As per below explanation for the cause of this issue, I assume the same issue also applies when serializing an item stack with this CanDestroy tag on Paper, switching from Paper to Spigot, and attempting to load the item stack there. The serialized item stack data on Paper, which Spigot will not know and therefore ignore, will look like this:

==: org.bukkit.inventory.ItemStack
v: 2730
type: DIAMOND_PICKAXE
meta:
  ==: ItemMeta
  meta-type: UNSPECIFIC
  CanDestroy:
  - minecraft:iron_ore

Plugin list

Shopkeepers

The underlying cause of the issue should be apparent, but if wanted, I can prepare a more minimal reproduction plugin that simply saves and loads an item stack to and from the plugin's config file.

Paper version

Checking version, please wait... This server is running Paper version git-Paper-295 (MC: 1.17.1) (Implementing API version 1.17.1-R0.1-SNAPSHOT) (Git: f905057) You are running the latest version Previous version: git-Paper-280 (MC: 1.17.1)

Agreements

[X] I am running the latest version of Paper available from https://papermc.io/downloads.
[X] I have searched for and ensured there isn't already an open issue regarding this.
[X] My version of Minecraft is supported by Paper.

Other

My assumption is that the issue is caused by the following patch: https://github.com/PaperMC/Paper/blob/master/patches/server/0267-Implement-an-API-for-CanPlaceOn-and-CanDestroy-NBT-v.patch

On Spigot, the CanDestroy and CanPlaceOn item tags are serialized as part of the unhandledTags (under key internal). On Paper, these tags have been removed from Spigot's unhandledTags and treated separately during serialization and deserialization: They are serialized under the keys CanDestroy and CanPlaceOn inside configs.

The problem with this is that Paper ignores any already serialized data for these tags that may previously have been saved by Spigot as part of the internal / unhandledTags. And the other way around, Spigot ignores any data that Paper may have previously saved under the CanDestroy and CanPlaceOn keys.

I see two possible solutions:

Paper could additionally import the data for these tags from Spigot's serialized internal data. However, this would only resolve one side of the issue: When the serialized item data is transferred from a Spigot server over to a Paper server. Copying the serialized item data from a Paper server over to a Spigot server will not work with this solution.
Paper could stick to reading and writing the data for these tags from/to Spigot's unhandledTags (under key internal). The Paper-specific API could still be implemented on top of that. The benefit of this would be that any items saved on the Paper server can also be transferred over to a Spigot server without these tags being lost.
Since Paper servers will already have serialized items with these tags, Paper would additionally need to import any tags that have previously been saved under the CanDestroy and CanPlaceOn keys.

electronicboy commented 3 years ago

I can imagine supporting migrating from spigot, but, trying to support both of them would be pretty shoddy, the item meta system was never designed to deal with such cases, nor is bouncing between platforms something we support

Machine-Maker commented 2 months ago

We've decided to close this as won't fix. Since this, we've introduced other changes in the form of fixes to the serialization/deserialization logic for stacks that just makes trying to maintain backwards compat not feasible.

PaperMC / Paper