Sponge: "FailedData" not removed after successful re-attempt of deserialization

[randombyte-developer]

I am currently running

• SpongeCommon version: Latest

Issue Description

My issue is that with DataManager#registerLegacyManipulatorIds() the old legacy data which apparently also appears in FailedData NBT list is not removed. This means that everytime the data is tried to be loaded, FailedData is tried to be deserialized. As long as the legacy ids registration is present it works fine. But in the future the legacy registration will be removed and the legacy data is still in FailedData. Then it throws an error because it couldn't find a legacy registration for that data.

I am pretty pretty sure that somewhere here the successful deserialized FailedData must be removed to not be re-attempted to be deserialized at the next time the method is called.

[randombyte-developer]

It seems to be a "gamebreaking" bug for plugins that are doing the conversion from the old data registration to the new one: the failed data constantly overwrites the actual data thus making the specific data read-only.

[gabizou]

The reason why it re-adds failed data is that the failed data is supposed to be upgraded to current data, if the upgrade fails, it's still considered failed data, and therefor will be re-attempted. However, if it's failing, then it should not be "overwriting" anything, except being put back into place as failed data. If it IS being upgraded, but still considered failed data, then that is indeed a bug, as the transaction for deserialization is supposed to trim out the failed data that succeeded at that point, since a deserialization was successful.

If you can explain how to reproduce this issue with some code examples, I can fix the issue, but without exact detail of "what" is going wrong, I'm not quite grasping as to what is actually going wrong here as it sounds like it's performing it's function as intended.

Lastly, saying the "latest" version of SpongeCommon doesn't work, you know that, name an implementation that you tested on and use that version, but don't say the latest commit.

[randombyte-developer]

the transaction for deserialization is supposed to trim out the failed data that succeeded at that point, since a deserialization was successful. That doesn't seem to work. I'll create a repo for that to test.
Sorry about the version/implementation note. I tested it on sponge vanilla beta-3 1.12.2 for API v7.0.0.

[randombyte-developer]

Here is the test plugin repo: https://github.com/randombyte-developer/data-issue
It is a tedious process.

Clone the repo. Checkout to apiv5 branch. Build the plugin. Run it on a SpongeVanilla server for API v5.x.
Setup an entity with some data:

1. Stand near an entity(I prefer a cow trapped by fences)
2. Execute /test

Now the cow has the SampleData on it, with the DataClass identification method.

Setup a SpongeVanilla server for API v7.x, ofc keep the world. Start the server without the test plugin to make Sponge put the data into FailedData. The warning about a missing manipulator appears.

Checkout to branch master on the first commit(ba5c129). Build the plugin. Start the server with the plugin. registerLegacyManipulatorIds is used to update the data to manipulator IDs. No warning appears. (Here the bug is happening, the FailedData which is successfully deserialized isn't removed)

Checkout to the following commit c6f67db. Build the plugin. Start the server. This removed registerLegacyManipulatorIds, it just uses the manipulator ID method. And a warning appears, which it shouldn't do. The warning is for the remaining DataClass FailedData of SampleDate.

To check what data is on the cow during this process, I recommend /entityData @e[r=3] {} while standing nearby. Check that no other entities are around(the test command and the entityData command pick the nearest entity).

I hope I explained it well, if not please tell me how I can help you to reproduce the issue.

Edit: The read-only aspect can be seen when changing the data value in the test command from SampleData(true) to SampleData(false), so basically a different value that is saved on the entity. On the 1.12.2 server with registerLegacyManipulatorIds the data is set on the entity but is reset to the value in FailedData at server restart.

[gabizou]

@randombyte-developer Can you re-test with the latest? I added some failed data redundancy and duplication removal, working on adding a config for registered and existing data stuff with a command to allow server admins to filter out old failed data from being deserialized or attempted. Until I hear back, I'll leave the issue open.

[parlough]

Sorry was automatically closed with the commit!

[gabizou]

@randombyte-developer I'm guessing by the closure of your PR that it's resolved?

[randombyte-developer]

It works in my test environment. I haven't tested it yet in production. If it doesn't work, I'll reopen the issue.
Thanks for the quick fix!