search

MovingBlocks / Terasology

Terasology - open source voxel world
http://terasology.org
Apache License 2.0
3.64k stars 1.32k forks source link

feat: prevent serialisation of private component fields #5208

Closed BenjaminAmos closed 3 months ago

BenjaminAmos commented 5 months ago

Contains

This pull request adds a new ComponentTypeHandlerFactory type specifcally for serialising components. It is mostly the same as ObjectFieldMapTypeHandlerFactory (from TypeHandlerFactory) but with an additional check for private fields. It might be possible to merge the two classes instead, this is just a first attempt to see how it behaves.

It is not possible to exclude private field serialiation from ObjectFieldMapTypeHandlerFactory completely since NUI depends on this behaviour extensively for UI widgets.

This pull request removes the ability to serialise private fields in TypeHandlerLibrary, apart from in a few special cases (mainly NUI). It changes the generic ObjectFieldMapHandler to use getter and setter methods where available instead.

How to test

jdrueckert commented 4 months ago

cross-post from Discord for the record: I tested this branch with JS and ran into the following: https://pastebin.com/raw/rxsj7xWr

18:07:13.961 [parallel-1] ERROR reactor.core.publisher.Operators - Operator called default onErrorDropped
reactor.core.Exceptions$ErrorCallbackNotImplemented: java.lang.reflect.InaccessibleObjectException: Unable to make field private final java.lang.Class java.lang.Class.componentType accessible: module java.base does not "opens java.lang" to unnamed module @75eeccf5
Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make field private final java.lang.Class java.lang.Class.componentType accessible: module java.base does not "opens java.lang" to unnamed module @75eeccf5
    at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
[...]

    at java.base/java.lang.reflect.Field.setAccessible(Field.java:172)
    at org.terasology.persistence.typeHandling.coreTypes.factories.ObjectFieldMapTypeHandlerFactory.lambda$getResolvedFields$1(ObjectFieldMapTypeHandlerFactory.java:80)
    at java.base/java.security.AccessController.doPrivileged(AccessController.java:318)
    at org.terasology.persistence.typeHandling.coreTypes.factories.ObjectFieldMapTypeHandlerFactory.getResolvedFields(ObjectFieldMapTypeHandlerFactory.java:68)
    at org.terasology.persistence.typeHandling.coreTypes.factories.ObjectFieldMapTypeHandlerFactory.create(ObjectFieldMapTypeHandlerFactory.java:40)
    at org.terasology.persistence.typeHandling.TypeHandlerLibrary.getTypeHandler(TypeHandlerLibrary.java:274)

what I did in JS: I gave myself a chest and some furnaces and sifting tables, placed them, put some stuff in them and then tried to exit to main menu (and it was a fresh TS workspace with JS recursed)

@BenjaminAmos responded:

Yep, I knew about that issue. It's down to https://github.com/MovingBlocks/Terasology/blob/14937d24b851b300b3b8f569cfb86205a29626f2/engine/src/main/java/org/terasology/engine/logic/common/RetainComponentsComponent.java#L20. Technically it's a public field, so we try to serialise it. The actual serialiser then still looks at private fields inside of the public field value. It needs more thought.

soloturn commented 3 months ago

a lot better than before. with joshuasworld i get, and i seem recall that i had this before and it was my setup, some module missing isn't it?

19:26:59.664 [main] ERROR o.t.e.world.generation.WorldBuilder - Facet provider for class org.terasology.caves.CaveFacet is missing. It is required by class org.terasology.caves.CaveLocationProvider
19:26:59.664 [main] ERROR o.t.engine.core.modes.StateLoading - Error while loading org.terasology.engine.core.modes.loadProcesses.InitialiseWorldGenerator@28466003
java.lang.IllegalStateException: Missing facet provider
        at org.terasology.engine.world.generation.WorldBuilder.determineProviderChains(WorldBuilder.java:188)
        at org.terasology.engine.world.generation.WorldBuilder.build(WorldBuilder.java:95)
        at org.terasology.engine.world.generation.BaseFacetedWorldGenerator.getWorld(BaseFacetedWorldGenerator.java:83)
        at org.terasology.engine.world.generation.BaseFacetedWorldGenerator.initialize(BaseFacetedWorldGenerator.java:58)
        at org.terasology.engine.core.modes.loadProcesses.InitialiseWorldGenerator.step(InitialiseWorldGenerator.java:34)
jdrueckert commented 3 months ago

@soloturn yes you faced that before: https://github.com/Terasology/JoshariasSurvival/issues/76 and I also noticed it sporadically happening (actually relatively often), but it's also happening with develop, so it seems unrelated to this change. @BenjaminAmos IIRC you wanted to have a look at adding some test cases, right? should we hold off merging until then or do you want to add them with a follow-up PR?