Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: prevent serialisation of private component fields #5208

Merged
merged 11 commits into from
Mar 3, 2024
Merged

feat: prevent serialisation of private component fields #5208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a0c9384
feat: prevent serialisation of private component fields
BenjaminAmos Jan 21, 2024
873fe5b
log line shorter for ComponentTypeHandlerFactory
soloturn Jan 29, 2024
d992dbf
Merge branch 'develop' of https://github.com/MovingBlocks/Terasology …
BenjaminAmos Feb 11, 2024
7178325
feat(TypeHandlerLibrary): do not serialise private fields in ObjectFi…
BenjaminAmos Feb 18, 2024
7701cc1
fix: fix ObjectFieldMapTypeHandlerFactoryTest tests
BenjaminAmos Feb 18, 2024
9aa53fe
fix: fix TypeSerializerTest tests
BenjaminAmos Feb 18, 2024
456efca
fix: fix VectorTypeSerializerTest tests
BenjaminAmos Feb 18, 2024
2cbdba0
fix: fix BlockAreaTypeHandlerTest tests
BenjaminAmos Feb 21, 2024
6f284d7
style: fix checkstyle issues in TypeSerializerTest.java
BenjaminAmos Feb 21, 2024
17b2b9e
Merge branch 'develop' into feat/no-private-component-field-serialisa…
jdrueckert Feb 25, 2024
91c852f
fix: catch setter errors explicitly in ObjectFieldTypeMapHandler
BenjaminAmos Feb 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions ...tests/src/test/java/org/terasology/engine/persistence/serializers/TypeSerializerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,14 @@ void testJsonSerializeDeserialize() throws IOException {
assertEquals(INSTANCE, deserializedInstance);
}

private static final class SomeClass<T> {
public static final class SomeClass<T> {
@SerializedName("generic-t")
private T data;
private final List<T> list = Lists.newArrayList();
private final Set<Animal<?>> animals = Sets.newHashSet();
private Animal<T> singleAnimal;
public T data;
public List<T> list = Lists.newArrayList();
public Set<Animal<?>> animals = Sets.newHashSet();
public Animal<T> singleAnimal;

private SomeClass(T data) {
SomeClass(T data) {
this.data = data;
}

Expand Down Expand Up @@ -151,10 +151,10 @@ public String toString() {
}

@SuppressWarnings("checkstyle:FinalClass")
private static class Animal<T> {
protected final T data;
public static class Animal<T> {
public T data;

private Animal(T data) {
Animal(T data) {
this.data = data;
}

Expand All @@ -176,11 +176,11 @@ public int hashCode() {
}
}

private static final class Dog<T> extends Animal<T> {
private final Vector3f tailPosition;
private final Vector3f headPosition;
public static final class Dog<T> extends Animal<T> {
public Vector3f tailPosition;
public Vector3f headPosition;

private Dog(T data, Vector3f tailPosition, Vector3f headPosition) {
Dog(T data, Vector3f tailPosition, Vector3f headPosition) {
super(data);
this.tailPosition = tailPosition;
this.headPosition = headPosition;
Expand Down Expand Up @@ -216,10 +216,10 @@ public int hashCode() {
}
}

private static final class Cheetah<T> extends Animal<T> {
private final Color spotColor;
public static final class Cheetah<T> extends Animal<T> {
public Color spotColor;

private Cheetah(T data, Color spotColor) {
Cheetah(T data, Color spotColor) {
super(data);
this.spotColor = spotColor;
}
Expand Down
4 changes: 2 additions & 2 deletions ...src/test/java/org/terasology/engine/persistence/serializers/VectorTypeSerializerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,13 @@ void testSerializationConstant() {
assertEquals(new Vector2f(1.0f, 2.0f), o.v3, .00001f);
}

static class TestObject {
public static class TestObject {
public Vector3f v1;
public Vector2f v2;
public Vector4f v3;
}

static class TestObject2 {
public static class TestObject2 {
public Vector3fc v1;
public Vector4fc v2;
public Vector2fc v3;
Expand Down
2 changes: 1 addition & 1 deletion ...va/org/terasology/engine/persistence/typeHandling/mathTypes/BlockAreaTypeHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public void testGsonSerialization() throws IOException {
assertEquals(new BlockArea(0, 0, 1, 1), o.b2);
}

static class TestObject {
public static class TestObject {
public BlockArea b1;
public BlockAreac b2;
}
Expand Down
24 changes: 21 additions & 3 deletions ...ain/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@
import org.terasology.persistence.typeHandling.TypeHandler;
import org.terasology.persistence.typeHandling.TypeHandlerLibrary;
import org.terasology.persistence.typeHandling.annotations.SerializedName;
import org.terasology.reflection.ReflectionUtil;
import org.terasology.reflection.reflect.ObjectConstructor;

import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Modifier;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
Expand Down Expand Up @@ -48,10 +51,17 @@ public PersistedData serializeNonNull(T value, PersistedDataSerializer serialize
Object val;

try {
val = field.get(value);
if (Modifier.isPrivate(field.getModifiers())) {
val = ReflectionUtil.findGetter(field).invoke(value);
} else {
val = field.get(value);
}
} catch (IllegalAccessException e) {
logger.error("Field {} is inaccessible", field);
continue;
} catch (InvocationTargetException e) {
logger.error("Failed to invoke getter for field {}", field);
continue;
}

if (!Objects.equals(val, Defaults.defaultValue(field.getType()))) {
Expand Down Expand Up @@ -93,15 +103,23 @@ public Optional<T> deserialize(PersistedData data) {
Field field = fieldByName.get(fieldName);

if (field == null) {
logger.error("Cound not find field with name {}", fieldName);
logger.error("Could not find field with name {}", fieldName);
continue;
}

TypeHandler handler = mappedFields.get(field);
Optional<?> fieldValue = handler.deserialize(entry.getValue());

if (fieldValue.isPresent()) {
field.set(result, fieldValue.get());
if (Modifier.isPrivate(field.getModifiers())) {
try {
ReflectionUtil.findSetter(field).invoke(result);
} catch (InvocationTargetException e) {
logger.error("Failed to invoke setter for field {}", field);
}
} else {
field.set(result, fieldValue.get());
}
} else {
logger.error("Could not deserialize field {}", field.getName());
}
Expand Down
19 changes: 18 additions & 1 deletion ...sology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
package org.terasology.persistence.typeHandling.coreTypes.factories;

import com.google.common.collect.Maps;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.terasology.persistence.typeHandling.TypeHandler;
import org.terasology.persistence.typeHandling.TypeHandlerContext;
import org.terasology.persistence.typeHandling.TypeHandlerFactory;
Expand All @@ -17,10 +19,14 @@
import java.lang.reflect.Type;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Optional;

public class ObjectFieldMapTypeHandlerFactory implements TypeHandlerFactory {
private static final Logger logger = LoggerFactory.getLogger(ObjectFieldMapTypeHandlerFactory.class);
private static final List<String> PRIVATE_OVERRIDE_ANNOTATIONS = List.of("org.terasology.nui.LayoutConfig");

private ConstructorLibrary constructorLibrary;

Expand Down Expand Up @@ -77,7 +83,18 @@ private <T> Map<Field, Type> getResolvedFields(TypeInfo<T> typeInfo) {
continue;
}

field.setAccessible(true);
if (Arrays.stream(field.getAnnotations())
.anyMatch(annotation -> PRIVATE_OVERRIDE_ANNOTATIONS.contains(annotation.getClass().getCanonicalName()))) {
field.setAccessible(true);
} else {
if (Modifier.isPrivate(field.getModifiers()) && ReflectionUtil.findGetter(field) == null
&& ReflectionUtil.findSetter(field) == null) {
logger.atWarn().addArgument(field.getName()).addArgument(rawType.getTypeName()).
log("Field {}#{} will not be serialised. Terasology no longer supports serialising private fields.");
Comment on lines +92 to +93
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

continue;
}
}

Type fieldType = ReflectionUtil.resolveType(type, field.getGenericType());
fields.put(field, fieldType);
}
Expand Down
10 changes: 5 additions & 5 deletions ...gy/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,12 @@ public void testMultipleObjectsOfDifferentType() {
}

private static class SingleTypeClass<T> {
private T t;
public T t;
}

private static class MultiTypeClass<T, U> {
private T t;
private U u;
public T t;
public U u;
}

@edu.umd.cs.findbugs.annotations.SuppressFBWarnings(
Expand All @@ -119,7 +119,7 @@ private static class MultiTypeClass<T, U> {
" creation based on member types of input class TypeInfo. ")
@SuppressWarnings("PMD.UnusedPrivateField")
private static class SomeClass<T> {
private T t;
private List<T> list;
public T t;
public List<T> list;
}
}
Loading