Skip to content

Commit

Permalink
feat: prevent serialisation of private component fields (#5208)
Browse files Browse the repository at this point in the history 
* feat: prevent serialisation of private component fields

* log line shorter for ComponentTypeHandlerFactory

* feat(TypeHandlerLibrary): do not serialise private fields in ObjectFieldMapTypeHandler by default

* fix: fix ObjectFieldMapTypeHandlerFactoryTest tests

* fix: fix TypeSerializerTest tests

* fix: fix VectorTypeSerializerTest tests

* fix: fix BlockAreaTypeHandlerTest tests

* style: fix checkstyle issues in TypeSerializerTest.java

* fix: catch setter errors explicitly in ObjectFieldTypeMapHandler

---------

Co-authored-by: soloturn <soloturn@gmail.com>
Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
  • Loading branch information
@BenjaminAmos @soloturn @jdrueckert
3 people authored Mar 3, 2024
1 parent ae83461 commit 23b83d6
Show file tree
Hide file tree
Showing 6 changed files with 63 additions and 28 deletions.
32 changes: 16 additions & 16 deletions ...tests/src/test/java/org/terasology/engine/persistence/serializers/TypeSerializerTest.java
View file
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
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
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
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
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.");
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
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;
}
}

0 comments on commit 23b83d6

Please sign in to comment.