feat: prevent serialisation of private component fields #5208

BenjaminAmos · 2024-01-21T20:31:35Z

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

Create a new game
Play around for a bit (destroying blocks, loading chunks, etc.)
Save the game
Restart the game and load that save
The world and surrounding entities should be similar to how you left it
There should also not be any additional exceptions thrown due to these changes

jdrueckert · 2024-02-03T18:53:53Z

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

Terasology/engine/src/main/java/org/terasology/engine/logic/common/RetainComponentsComponent.java

Line 20 in 14937d2

public Set<Class<? extends Component>> components = Sets.newHashSet();

. 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.

jdrueckert

We need to figure out how to deal with the RetainComponentsComponent issue before we can merge this.
Although I'm wondering why it shows up ... didn't I change all private component fields to public? 🤔

…into feat/no-private-component-field-serialisation

…eldMapTypeHandler by default

…tion

jdrueckert

Tests out fine and looks good to me.
Only question: can we have some additional tests for the new behavior?

jdrueckert · 2024-02-25T18:19:37Z

...c/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java

+            } catch (InvocationTargetException e) {
+                logger.error("Failed to involve getter for field {}", field);
+                continue;


do we need something similar below for the setter?

I believe that the setter is caught in the general catch block here:

Terasology/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java

Lines 110 to 112 in ae83461

} catch (Exception e) {

logger.error("Unable to deserialize {}", data, e);

}

I will add a more specific catch for that error to make it less ambiguous though.

jdrueckert · 2024-02-25T18:23:25Z

...erasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactory.java

+                            logger.atWarn().addArgument(field.getName()).addArgument(rawType.getTypeName()).
+                                    log("Field {}#{} will not be serialised. Terasology no longer supports serialising private fields.");


Why fluent API usage here?
I would expect getName() and getTypeName() to be O(1) so pretty much neglectable... also this is not on the happy path so it's not expected to be evaluated often...
In my test run, I got the warning only twice:

19:18:19.310 [main] WARN o.t.p.t.c.f.ObjectFieldMapTypeHandlerFactory - Field exists#org.terasology.engine.entitySystem.entity.internal.PojoEntityRef will not be serialised. Terasology no longer supports serialising private fields. 19:18:19.396 [parallel-1] WARN o.t.p.t.c.f.ObjectFieldMapTypeHandlerFactory - Field entityRef#org.terasology.engine.persistence.internal.DelayedEntityRef will not be serialised. Terasology no longer supports serialising private fields.

I did not make that change. @soloturn pushed 873fe5b to this branch, possibly as a well-intentioned fix for a newly-introduced checkstyle warning.

okay, guess it might've been a PMD warning in that case.
I'd prefer suppressing the warning (suffix with //NOPMD) here due to the above-mentioned reasons.

i am a little confused - git blame says @BenjaminAmos changed that line. commit 873fe5b changed ComponentTypeHandlerFactory, different file, but the change looks the same? in case i really did this, feel free to undo. 2 warnings, one line too long, and one for the logger are there in the line before.

soloturn · 2024-03-01T18:30:15Z

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 · 2024-03-02T18:02:23Z

@soloturn yes you faced that before: Terasology/JoshariasSurvival#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?

soloturn

not hold off merging. this improves a lot and finally saves stuff again.

github-actions bot added the Type: Improvement Request for or addition/enhancement of a feature label Jan 21, 2024

feat: prevent serialisation of private component fields

a0c9384

soloturn force-pushed the feat/no-private-component-field-serialisation branch from 295928c to a0c9384 Compare January 29, 2024 01:26

soloturn previously approved these changes Jan 29, 2024

View reviewed changes

soloturn dismissed their stale review via 873fe5b January 29, 2024 01:51

log line shorter for ComponentTypeHandlerFactory

873fe5b

jdrueckert requested changes Feb 3, 2024

View reviewed changes

BenjaminAmos and others added 8 commits February 11, 2024 11:45

Merge branch 'develop' of https://github.com/MovingBlocks/Terasology …

d992dbf

…into feat/no-private-component-field-serialisation

feat(TypeHandlerLibrary): do not serialise private fields in ObjectFi…

7178325

…eldMapTypeHandler by default

fix: fix ObjectFieldMapTypeHandlerFactoryTest tests

7701cc1

fix: fix TypeSerializerTest tests

9aa53fe

fix: fix VectorTypeSerializerTest tests

456efca

fix: fix BlockAreaTypeHandlerTest tests

2cbdba0

style: fix checkstyle issues in TypeSerializerTest.java

6f284d7

Merge branch 'develop' into feat/no-private-component-field-serialisa…

17b2b9e

…tion

jdrueckert previously approved these changes Feb 25, 2024

View reviewed changes

fix: catch setter errors explicitly in ObjectFieldTypeMapHandler

91c852f

BenjaminAmos dismissed jdrueckert’s stale review via 91c852f February 25, 2024 18:45

soloturn approved these changes Mar 3, 2024

View reviewed changes

soloturn merged commit 23b83d6 into develop Mar 3, 2024
9 checks passed

soloturn deleted the feat/no-private-component-field-serialisation branch March 3, 2024 08:00

BenjaminAmos mentioned this pull request Mar 3, 2024

test: add private serialisation cases to type serialiser tests #5217

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: prevent serialisation of private component fields #5208

feat: prevent serialisation of private component fields #5208

BenjaminAmos commented Jan 21, 2024 •

edited

Loading

jdrueckert commented Feb 3, 2024

jdrueckert left a comment

jdrueckert left a comment

jdrueckert Feb 25, 2024

BenjaminAmos Feb 25, 2024

jdrueckert Feb 25, 2024

BenjaminAmos Feb 25, 2024

jdrueckert Feb 25, 2024

soloturn Mar 1, 2024

soloturn commented Mar 1, 2024

jdrueckert commented Mar 2, 2024

soloturn left a comment

	} catch (Exception e) {
	logger.error("Unable to deserialize {}", data, e);
	}

		logger.atWarn().addArgument(field.getName()).addArgument(rawType.getTypeName()).
		log("Field {}#{} will not be serialised. Terasology no longer supports serialising private fields.");

feat: prevent serialisation of private component fields #5208

feat: prevent serialisation of private component fields #5208

Conversation

BenjaminAmos commented Jan 21, 2024 • edited Loading

Contains

How to test

jdrueckert commented Feb 3, 2024

jdrueckert left a comment

Choose a reason for hiding this comment

jdrueckert left a comment

Choose a reason for hiding this comment

jdrueckert Feb 25, 2024

Choose a reason for hiding this comment

BenjaminAmos Feb 25, 2024

Choose a reason for hiding this comment

jdrueckert Feb 25, 2024

Choose a reason for hiding this comment

BenjaminAmos Feb 25, 2024

Choose a reason for hiding this comment

jdrueckert Feb 25, 2024

Choose a reason for hiding this comment

soloturn Mar 1, 2024

Choose a reason for hiding this comment

soloturn commented Mar 1, 2024

jdrueckert commented Mar 2, 2024

soloturn left a comment

Choose a reason for hiding this comment

BenjaminAmos commented Jan 21, 2024 •

edited

Loading