Migrated from: https://dev.bukkit.org/projects/shopkeepers/issues/535

Originally posted by FatherWh0 (Oct 4, 2017):

I've got shopkeepers losing their trades. Some trades persist a few are lost. It will happen when I restart but if the server stays up for several days it happens between restarts as well. I've been using shopkeepers for four years and haven't encountered anything like this. I have noted that all trades that are forgotten have been added recently. Some of the shopkeepers themselves are new but others losing trades have been around a very long time. I've tried deleting some of them and creating new ones. This changes which trades are forgotten but doesn't fix the issue. I've tried save-instantly set to both true and false but with no change.
I am including before and after screenshots, my config.yml and a link to download my save.yml.
This one is "Mob Eggs" by Backwordz standing at  x: -495 y: 58 z: -2335
Before: https://prnt.sc/grsun8
After: https://prnt.sc/grsuii
This one is "Illusionary Enchants" by Backwordz standing at  x: -493 y: 58 z: -2333
Before: https://prnt.sc/grsuzk
After: https://prnt.sc/grsusw
Config.yml: https://hastebin.com/ugacagocey.sql
save.yml: https://drive.google.com/file/d/0B4jJYrVOq7X5cGlfazhFb2xJbjg/view?usp=sharing
I do realize the first thing to try is to delete the save.yml and let it recreate fresh but as you can quickly tell by it's size this would be a major undertaking, requiring a lot of coordinating with players and a complete reconstruction of some admin shops and our library. Basically as pleasant an idea as being shot at dawn. :P

Originally commented by blablubbabc (Oct 8, 2017):

Those shops seem to have a lot of duplicate trades for the same item, sometimes with differing prices. Not sure how he got that though, because shopkeepers assumes that there is always only one trade for an item and it removes the old trades for items inside the editor view when saving. Maybe he got that through some legacy shop conversions, not sure.. I could update shopkeepers to remove all trades for the same item to slightly clean this up.
 
The "Illusionary Enchants" shop also has trades for the same item types but with differing internal data (for ex. sometimes only the repair cost differing).
 
My current guess is, that maybe he is refilling his shop chest with items which are slightly different to those for which he has specified prices already. And therefore it will list those in the editor empty just like when adding new items to the chest shop.
 
However, as I don't know which exact items (including their internal nbt data) he has in his chest when this occurs, I currently cannot reproduce it.
Maybe, when this issue occurs for the 'Mob Eggs' shop (I can more easily recreate those spawn egg items), you could create a small schematic with worldedit containing the shop chest, so I can try to recreate it on my testing server.
 
Might also be interesting to know which other plugins are running on the server. Some plugins secretly add internal data to items (ex. to track them). Some plugins even hide this information from players by modifying outgoing item packets of the server. So even if those items look similar, their internal data on the server could differ..
 
Another 'solution' to those many slightly differing trades could be to somehow officially support more than 8 trades and / or allow players to somehow cleanup no longer used trade offers. There is already a ticket about this, but I currently have no time to look into that.
Edited Oct 8, 2017

Originally commented by FatherWh0 (Oct 8, 2017):

The varying data is caused by the enchantments. Illusionary Enchants sells enchanted items. Sometimes replacement items aren't recognized as the originals. My theory is that the enchants might be added to the items in different orders. Also sometimes an enchanted book used is a composite of two and sometimes it's natural. For instance, an unbreaking III book used to enchant an item might be naturally unbreaking III or it may be a composite of two unbreaking II. This would cause the resulting enchanted item to have different data and the shopkeeper to recognize it as a different item.
Now I punch a small hole in this theory. Illusionary enchants was my attempt to fix this issue. It was a blaze called Max Enchants. I deleted the blaze and replaced it with the current shopkeeper which immediately began to lose trades from it's first creation. Could the new shopkeeper be keeping data from the previous that used the same chest? I ask because the lost trades is so bad that the player claims the new shopkeeper has never sold any items. I can't prove this is true. I will delete and recreate a few of them and use logging to test this.
Just to add helpful info I've included another shopkeeper, "MsKittens" location x: -583 y: 72 z: -2331, which loses it's trade for a signed and duplicated book. I've included the chest for this shopkeeper in the schems I'm providing. Here is an image to show the lost trade. MsKittens has also been deleted then recreated in a failed attempt to fix the issue.
You are likely on the right track about the item data. The trade "Mob Eggs" keeps losing isn't a normal villager spawn egg. It's got data that causes it to spawn farmer villagers. All the other eggs that don't lose their trades are normal.
Here is the link to the schematics.
A cleanup system for expired trades would definitely be useful and appreciated. I've got a few players selling enchanted items and doing a lot of volume sales. They often run into issues that I've been able to gather is because of the left-over trade still being in the save file.
Thank you for taking the time to look into this. I know your time is precious and limited. Your efforts are appreciated.

Originally commented by blablubbabc (Oct 9, 2017):

I submitted a ticket about the spawn egg serialization to spigot and they already fixed it: https://hub.spigotmc.org/jira/browse/SPIGOT-3605
Haven't tested yet though.

Originally commented by FatherWh0 (Oct 11, 2017):

I have new information pertaining to this issue. I created a new shopkeeper for testing.
Chest contents: https://prnt.sc/gvvfs8
Initial trade setup gui: https://prnt.sc/gvvg5g
Initial save.yml entry: https://hastebin.com/loduhesaro.scala
I then performed /shopkeeper reload. The shopkeeper immediately lost trades as seen.
Post-reload trade setup gui: https://prnt.sc/gvvfwj
Post-reload save.yml entry: https://hastebin.com/fepebobiro.scala
Trades are lost on items with nbt data but not necessarily items with duplicates. Also being a new shopkeeper no trades had been made with it.

Originally commented by blablubbabc (Oct 9, 2017):

So the issue seems indeed to be that the item in the chest differs to the items for which there already exist trades. So the real problem here is that these internal item differences are not distinguishable to the player..
 
The only real solution would be to control how your players get those special items (you could then for example consistently add lore to the items, visible to the player, so he always can see that this is a different item), and make sure that there is as less internal variation as possible regarding the custom items you provide to your players.
 
Things like enchantment order, which players have direct access to, would be something for bukkit / spigot to look into. However, there seems to have been a pull request about this 2 years ago,  which got not accepted for some reason. So I don't know how good the chances are for this..
A 'hack' might be to have a plugin which reacts to players enchanting items and then ordering the internal enchantments consistently in the resulting item.
 
Regarding cleanup of unused trades: This would have to be something the user controls, because I cannot differ between trade offers which the chest has simply run out of supplies, and actually no longer required trade offers.
A solution would be if the user could see all his setup trade offers in the editor view, even those for which there are no items in the chest anymore. Basically support for more than 8 trades. As I said above however, this would require some larger changes for which I currently don't have the time.
However, even then, even if he can now see all his setup trades, he will see that the trade he has previous setup doesn't match the current item in the chest, and he will have to define a new price for that new item. And as those items might differ only in their internal data (not distinguishable to the player), the user is still confused about why he has to setup a new price for the 'apparently' same item he already has setup a trade for. Which is basically the same issue we started with..
 
What I can do in the next update is to remove all duplicate trades for fully matching items. This should already result in quite some cleanup for those trades.
 
I will close this ticket, as there is not much I can do on my end about this issue. Its solution is mostly about preventing players from ending up with those differing but indistinguishable items.
 
Good luck!
 
Edit: Actually, at least for the villager eggs, I think I know how he ended up with those duplicate trades. Spigot/Bukkit doesn't seem to serialize the career and profession data at all. Meaning, after a save and reload it will become a default villager egg in the save file and shopkeeper trades.
So this is actually a bukkit bug. I could try to create an issue ticket at spigot for this the next days. But I don't know how well the chances are for it to get fixed soon.
 
Another possibility ('workaround') might be to not use those custom villager eggs at all and instead use a plugin to spawn the intended villager profession, if the spawn egg has a certain lore entry.
 
The enchanted items seem to internally differ in their repair cost, and possibly enchantment order. But at least they seem to get properly (fully) saved to the save.yml data.
Edited Oct 9, 2017

Originally commented by blablubbabc (Oct 11, 2017):

I don't seem to be able to reproduce the issue with those items.
According to the schametic they also don't contain any custom nbt data beyond the enchants and repair cost: http://prntscr.com/gw5fw1
Chest in game: http://prntscr.com/gw5gsd (those elytras seem to be all the same acording to their data shown above)
Shopkeeper editor: http://prntscr.com/gw5h7u
After plugin reload: http://prntscr.com/gw5hfn
 
Could you try to reproduce it with your own schematic? (maybe the schematic is for some reason not containing the correct data of those items..)
Also try to reproduce it on an empty up-to-date spigot server without other plugins possibly interfering. If you cannot reproduce it there, the issue might be something else. Maybe a bug in your server version, or some conflicting plugin being involved.
Edited Oct 11, 2017

Originally commented by FatherWh0 (Oct 11, 2017):

Tried loading the schem and creating a shopkeeper with the same effect. I then tried it on the test server with minimal plugins but with the same results.
http://prntscr.com/gw90pn
I then tried deleting the save.yml and starting the server with no shopkeepers. The issue still occurred. (Frankly I was a bit relieved on that one.)

Originally commented by blablubbabc (Oct 11, 2017):

Can you send me a schematic containing the chest with those items, so I can check their custom nbt data?
 
I assume those elytras have custom internal nbt data, which bukkit doesn't serialize properly to the save.yml file (bukkit only saves the enchantment and repairCost). So if you have trades for 2 different elytras with varying nbt data beyond that, and the nbt data doesn't get saved to the save.yml file, after the next reload shopkeepers will find 2 trades for the same item now (the item without the custom nbt data). And the actual items in the chest (the ones with custom nbt data) will show up as new 'unknown' items without any trade setup yet when you open the editor menu.
 
This is something for the spigot team to fix. Can you tell me how you create those items? If you are able to create those items in vanilla minecraft, the chances are a lot better that spigot fixes that. Just like they fixed it for the custom villager spawn eggs.
 
Edit: I just checked elytras in the schematic you already sent me, but I don't see any custom nbt there beyond the enchantments and repaircost. I also tried it and don't have this issue with the elytras included in the schematic. They only differ in their repairCost. See: http://prntscr.com/gw1c2q
Edited Oct 11, 2017

Originally commented by FatherWh0 (Oct 11, 2017):

Here is the schematic of the chest.
https://drive.google.com/file/d/0B4jJYrVOq7X5dzJtajBSX2xqRG8/view?usp=sharing
Thank you for your time and effort with this.

Originally commented by FatherWh0 (Oct 12, 2017):

I'm running the latest paper. Sry for the confusion. The viaversion is for viabackwards cuz some of my mods haven't updated so my client is 1.12. I'll make a minimal 1.12.2 server, test it, and zip it for you a.s.a.p.
F.y.i. I just noticed in our chestshop that the data for all potions and heads has changed some time recently. I wonder if this is connected.

Originally commented by blablubbabc (Oct 12, 2017):

Okay I have been able to reproduce the issue with paper, even without viaversion and viabackwards. Looking into it currently, but this is probably something for them to fix in the end. Might be something related to their custom handling of item enchantments (https://github.com/PaperMC/Paper/blob/master/Spigot-Server-Patches/0073-Handle-Item-Meta-Inconsistencies.patch).
 
Edit: This seems to be more tricky than I thought, but definitly a paper bug:
I wrote a little plugin to compare the items in the players off hand slot and main hand via bukkit's isSimilar function.
 
Scenario: I open the chest, click the first elytra to pick it up and click a hotbar slot to drop it there. I do the same for the third elytra in the chest. I close the chest inventory, scroll with the mouse wheel to where I dropped the first elytra and press F to move it into the offhand slot. Then I scroll the mousewheel to select the 2nd elytra in the hotbar and left-click into air to trigger an interaction. My plugin will react to the interaction, compare the 2 items and print the result: They are not similar.
 
Now the curios thing is the following: If I 'slide / drag' the elytra into a hotbar slot (usually you can drag a stack of items over multiple slots to evenly distribute it), the plugin will print 'the items are similar' at the end of the above scenario. The above scenario is therefore a bit tricky to consistently reproduce, because if minecraft assumes you didn't only click the hotbar slot to drop the item but you use the 'dragging', you might get a different result in the above scenario.
 
Very strange and hard to debug, but seems to indicate to me that something internal in the server is going wrong here. Maybe the paper devs can help you with this issue
 
Here is a screenshot of the code I used for testing: http://prntscr.com/gwj62n
The config loading stuff can be ignored, I have that in there because I initially assumed the issue is somehow related to the itemstack serialization.
I have roughly looked through the paper patches but have no explanation for this behavior so far.
 
Maybe one of those elytra nbt data isn't properly formatted or contains empty entries or something like that, which get automatically 'fixed' after the inventory dragging action (because it maybe create new items stacks there when it split the itemstack across multiple slots).
 
Edit: I checked and the item's enchantments seem to be differently ordered: http://prntscr.com/gwjxwk
It also makes a difference whether the first elytra is compared to the third one in the chest, or the third one to the first one. This is very likely an issue with paper's enchantment-order-does-not-matter patch.
Edited Oct 12, 2017

Originally commented by blablubbabc (Oct 12, 2017):

I have created a paper issue ticket here: PaperMC/Paper#915

Originally commented by blablubbabc (Oct 11, 2017):

Which server and client versions are you using? Have you tried on the latest version of spigot (1.12.2)? Could you try it without ViaVersion and ViaBackwards?
 
If you are still able to reproduce this on your (hopefully very minimal) testing server running 1.12.2 spigot, would you mind zipping and uploading that server? I will then try to reproduce it with exactly the same setup as yours.
 
Sorry for the extra effort, but without being able to reproduce this myself I won't be able to find out what's going on :(
Edited Oct 12, 2017

Originally commented by blablubbabc (Oct 13, 2017):

It's an issue of paper and their enchantment reordering. It's up to them now to fix this.

Originally commented by FatherWh0 (Oct 13, 2017):

Newest, stripped, tested, duplicated with same schem, zipped. Everything is included excluding server. Launch on paperclip #1238. Set yourself as op and/or 'owner'.
https://drive.google.com/open?id=0B4jJYrVOq7X5OG1sUkFHb2N2NzA

Originally closed by blablubbabc (Oct 9, 2017)