Skip to content

Commit

Permalink
Merge pull request EsotericSoftware#424 from xfrendx/master
Browse files Browse the repository at this point in the history
Fix for CompatibleFieldSerializer inheritance issue
  • Loading branch information
magro authored Jun 10, 2016
2 parents cf253eb + 5a7b7c5 commit 3520bc4
Show file tree
Hide file tree
Showing 7 changed files with 253 additions and 13 deletions.
15 changes: 14 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,20 @@ VersionFieldSerializer extends FieldSerializer and allows fields to have a `@Sin

TaggedFieldSerializer extends FieldSerializer to only serialize fields that have a `@Tag(int)` annotation, providing backward compatibility so new fields can be added. And it also provides forward compatibility by `setIgnoreUnknownTags(true)`, thus any unknown field tags will be ignored. TaggedFieldSerializer has two advantages over VersionFieldSerializer: 1) fields can be renamed and 2) fields marked with the `@Deprecated` annotation will be ignored when reading old bytes and won't be written to new bytes. Deprecation effectively removes the field from serialization, though the field and `@Tag` annotation must remain in the class. Deprecated fields can optionally be made private and/or renamed so they don't clutter the class (eg, `ignored`, `ignored2`). For these reasons, TaggedFieldSerializer generally provides more flexibility for classes to evolve. The downside is that it has a small amount of additional overhead compared to VersionFieldSerializer (an additional varint per field).

CompatibleFieldSerializer extends FieldSerializer to provide both forward and backward compatibility, meaning fields can be added or removed without invalidating previously serialized bytes. Changing the type of a field is not supported. Like FieldSerializer, it can serialize most classes without needing annotations. The forward and backward compatibility comes at a cost: the first time the class is encountered in the serialized bytes, a simple schema is written containing the field name strings. Also, during serialization and deserialization buffers are allocated to perform chunked encoding. This is what enables CompatibleFieldSerializer to skip bytes for fields it does not know about. When Kryo is configured to use references, there can be a [problem](https://github.com/EsotericSoftware/kryo/issues/286#issuecomment-74870545) with CompatibleFieldSerializer if a field is removed.
CompatibleFieldSerializer extends FieldSerializer to provide both forward and backward compatibility, meaning fields can be added or removed without invalidating previously serialized bytes. Changing the type of a field is not supported. Like FieldSerializer, it can serialize most classes without needing annotations. The forward and backward compatibility comes at a cost: the first time the class is encountered in the serialized bytes, a simple schema is written containing the field name strings. Also, during serialization and deserialization buffers are allocated to perform chunked encoding. This is what enables CompatibleFieldSerializer to skip bytes for fields it does not know about. When Kryo is configured to use references, there can be a [problem](https://github.com/EsotericSoftware/kryo/issues/286#issuecomment-74870545) with CompatibleFieldSerializer if a field is removed. In case your class inheritance hierarchy contains same named fields, use the `CachedFieldNameStrategy.EXTENDED` strategy.

```java
class A {
String a;
}

class B extends A {
String a;
}
...
// use `EXTENDED` name strategy, otherwise serialized object can't be deserialized correctly. Attention, `EXTENDED` strategy increases the serialized footprint.
kryo.getFieldSerializerConfig().setCachedFieldNameStrategy(FieldSerializer.CachedFieldNameStrategy.EXTENDED);
```

Additional serializers can easily be developed for forward and backward compatibility, such as a serializer that uses an external, hand written schema.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public void write (Kryo kryo, Output output, T object) {
if (TRACE) trace("kryo", "Write " + fields.length + " field names.");
output.writeVarInt(fields.length, true);
for (int i = 0, n = fields.length; i < n; i++)
output.writeString(fields[i].field.getName());
output.writeString(getCachedFieldName(fields[i]));
}

OutputChunked outputChunked = new OutputChunked(output, 1024);
Expand Down Expand Up @@ -88,7 +88,7 @@ public T read (Kryo kryo, Input input, Class<T> type) {
for (int i = 0; i < length; i++) {
String schemaName = names[i];
for (int ii = 0, nn = allFields.length; ii < nn; ii++) {
if (allFields[ii].field.getName().equals(schemaName)) {
if (getCachedFieldName(allFields[ii]).equals(schemaName)) {
fields[i] = allFields[ii];
continue outer;
}
Expand All @@ -108,7 +108,7 @@ public T read (Kryo kryo, Input input, Class<T> type) {

while (low <= high) {
mid = (low + high) >>> 1;
String midVal = allFields[mid].field.getName();
String midVal = getCachedFieldName(allFields[mid]);
compare = schemaName.compareTo(midVal);

if (compare < 0) {
Expand Down Expand Up @@ -137,7 +137,7 @@ else if (compare > 0) {
// Generic type used to instantiate this field could have
// been changed in the meantime. Therefore take the most
// up-to-date definition of a field
cachedField = getField(cachedField.field.getName());
cachedField = getField(getCachedFieldName(cachedField));
}
if (cachedField == null) {
if (TRACE) trace("kryo", "Skip obsolete field.");
Expand Down
31 changes: 27 additions & 4 deletions src/com/esotericsoftware/kryo/serializers/FieldSerializer.java
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ private CachedFieldFactory getUnsafeFieldFactory () {

public int compare (CachedField o1, CachedField o2) {
// Fields are sorted by alpha so the order of the data is known.
return o1.field.getName().compareTo(o2.field.getName());
return getCachedFieldName(o1).compareTo(getCachedFieldName(o2));
}

/** Sets the default value for {@link CachedField#setCanBeNull(boolean)}. Calling this method resets the {@link #getFields()
Expand Down Expand Up @@ -542,15 +542,19 @@ protected T create (Kryo kryo, Input input, Class<T> type) {
/** Allows specific fields to be optimized. */
public CachedField getField (String fieldName) {
for (CachedField cachedField : fields)
if (cachedField.field.getName().equals(fieldName)) return cachedField;
if (getCachedFieldName(cachedField).equals(fieldName)) return cachedField;
throw new IllegalArgumentException("Field \"" + fieldName + "\" not found on class: " + type.getName());
}

protected String getCachedFieldName(CachedField cachedField) {
return config.getCachedFieldNameStrategy().getName(cachedField);
}

/** Removes a field so that it won't be serialized. */
public void removeField (String fieldName) {
for (int i = 0; i < fields.length; i++) {
CachedField cachedField = fields[i];
if (cachedField.field.getName().equals(fieldName)) {
if (getCachedFieldName(cachedField).equals(fieldName)) {
CachedField[] newFields = new CachedField[fields.length - 1];
System.arraycopy(fields, 0, newFields, 0, i);
System.arraycopy(fields, i + 1, newFields, i, newFields.length - i);
Expand All @@ -562,7 +566,7 @@ public void removeField (String fieldName) {

for (int i = 0; i < transientFields.length; i++) {
CachedField cachedField = transientFields[i];
if (cachedField.field.getName().equals(fieldName)) {
if (getCachedFieldName(cachedField).equals(fieldName)) {
CachedField[] newFields = new CachedField[transientFields.length - 1];
System.arraycopy(transientFields, 0, newFields, 0, i);
System.arraycopy(transientFields, i + 1, newFields, i, newFields.length - i);
Expand Down Expand Up @@ -721,6 +725,25 @@ public static interface CachedFieldFactory {
public CachedField createCachedField (Class fieldClass, Field field, FieldSerializer ser);
}

public interface CachedFieldNameStrategy {

CachedFieldNameStrategy DEFAULT = new CachedFieldNameStrategy() {
@Override
public String getName(CachedField cachedField) {
return cachedField.field.getName();
}
};

CachedFieldNameStrategy EXTENDED = new CachedFieldNameStrategy() {
@Override
public String getName(CachedField cachedField) {
return cachedField.field.getDeclaringClass().getSimpleName() + "." + cachedField.field.getName();
}
};

String getName(CachedField cachedField);
}

/** Indicates a field should be ignored when its declaring class is registered unless the {@link Kryo#getContext() context} has
* a value set for the specified key. This can be useful when a field must be serialized for one purpose, but not for another.
* Eg, a class for a networked application could have a field that should not be serialized and sent to clients, but should be
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ public class FieldSerializerConfig implements Cloneable {
/** If set, transient fields will be serialized */
private boolean serializeTransient = false;

private FieldSerializer.CachedFieldNameStrategy cachedFieldNameStrategy = FieldSerializer.CachedFieldNameStrategy.DEFAULT;

{
useAsm = !FieldSerializer.unsafeAvailable;
if (TRACE) trace("kryo.FieldSerializerConfig", "useAsm: " + useAsm);
Expand Down Expand Up @@ -136,4 +138,13 @@ public boolean isCopyTransient() {
public boolean isSerializeTransient() {
return serializeTransient;
}

public FieldSerializer.CachedFieldNameStrategy getCachedFieldNameStrategy() {
return cachedFieldNameStrategy;
}

public void setCachedFieldNameStrategy(FieldSerializer.CachedFieldNameStrategy cachedFieldNameStrategy) {
this.cachedFieldNameStrategy = cachedFieldNameStrategy;
if (TRACE) trace("kryo.FieldSerializerConfig", "CachedFieldNameStrategy: " + cachedFieldNameStrategy);
}
}
51 changes: 48 additions & 3 deletions test/com/esotericsoftware/kryo/CompatibleFieldSerializerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@
* SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
* NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */

* NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */

package com.esotericsoftware.kryo;

import java.io.FileNotFoundException;

import com.esotericsoftware.kryo.serializers.CompatibleFieldSerializer;
import com.esotericsoftware.kryo.serializers.FieldSerializer;

/** @author Nathan Sweet <[email protected]> */
public class CompatibleFieldSerializerTest extends KryoTestCase {
Expand Down Expand Up @@ -71,6 +72,21 @@ public void testRemovedField () throws FileNotFoundException {
assertEquals(object1, object2);
}

public void testExtendedClass() throws FileNotFoundException {
ExtendedTestClass extendedObject = new ExtendedTestClass();

// this test would fail with DEFAULT field name strategy
kryo.getFieldSerializerConfig().setCachedFieldNameStrategy(FieldSerializer.CachedFieldNameStrategy.EXTENDED);

CompatibleFieldSerializer serializer = new CompatibleFieldSerializer(kryo, ExtendedTestClass.class);
kryo.register(ExtendedTestClass.class, serializer);
roundTrip(286, 286, extendedObject);

ExtendedTestClass object2 = (ExtendedTestClass) kryo.readClassAndObject(input);
assertEquals(extendedObject, object2);
}


static public class TestClass {
public String text = "something";
public int moo = 120;
Expand All @@ -97,7 +113,36 @@ public boolean equals (Object obj) {
}
}

static public class ExtendedTestClass extends TestClass {
// keep the same names of attributes like TestClass
public String text = "extendedSomething";
public int moo = 127;
public long moo2 = 5555;
public TestClass child;
public int zzz = 222;
public AnotherClass other;

public boolean equals (Object obj) {
if (this == obj) return true;
if (obj == null) return false;
if (getClass() != obj.getClass()) return false;
ExtendedTestClass other = (ExtendedTestClass) obj;

if (!super.equals(obj)) return false;
if (child == null) {
if (other.child != null) return false;
} else if (!child.equals(other.child)) return false;
if (moo != other.moo) return false;
if (moo2 != other.moo2) return false;
if (text == null) {
if (other.text != null) return false;
} else if (!text.equals(other.text)) return false;
if (zzz != other.zzz) return false;
return true;
}
}

static public class AnotherClass {
String value;
}
}
}
148 changes: 148 additions & 0 deletions test/com/esotericsoftware/kryo/FieldSerializerInheritanceTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
/* Copyright (c) 2008, Nathan Sweet
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following
* conditions are met:
*
* - Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
* - Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following
* disclaimer in the documentation and/or other materials provided with the distribution.
* - Neither the name of Esoteric Software nor the names of its contributors may be used to endorse or promote products derived
* from this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING,
* BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT
* SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
* NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */

package com.esotericsoftware.kryo;

import com.esotericsoftware.kryo.serializers.FieldSerializer;
import org.junit.Assert;

/**
* Created by phamrak on 8.6.2016.
*/
public class FieldSerializerInheritanceTest extends KryoTestCase {
public void testDefaultStrategyForDefaultClass() {
TestDefault testDefault = new TestDefault();
testDefault.a = "someDefaultValue";
kryo.setDefaultSerializer(FieldSerializer.class);
kryo.register(TestDefault.class);

roundTrip(17, 17, testDefault);

FieldSerializer serializer = (FieldSerializer) kryo.getSerializer(TestDefault.class);
assertNotNull(serializer.getField("a"));
serializer.removeField("a");
assertFieldRemoved(serializer, "a");
}

public void testDefaultStrategyForExtendedClass() {
TestExtended testExtended = new TestExtended();
((TestDefault) testExtended).a = "someDefaultValue";
testExtended.a = "someExtendedValue";
kryo.setDefaultSerializer(FieldSerializer.class);
kryo.register(TestExtended.class);

roundTrip(34, 34, testExtended);

FieldSerializer serializer = (FieldSerializer) kryo.getSerializer(TestExtended.class);

// the "a" field needs to be removed 2x, once for TestDefault.a and once for TestExtended.a. You
// can't remove the second one without removing the first one (in DEFAULT field name strategy)
assertNotNull(serializer.getField("a"));
serializer.removeField("a");
assertNotNull(serializer.getField("a"));
serializer.removeField("a");
assertFieldRemoved(serializer, "a");
}

public void testExtendedStrategyForExtendedClass() {
TestExtended testExtended = new TestExtended();
((TestDefault) testExtended).a = "someDefaultValue";
testExtended.a = "someExtendedValue";
kryo.getFieldSerializerConfig().setCachedFieldNameStrategy(FieldSerializer.CachedFieldNameStrategy.EXTENDED);
kryo.setDefaultSerializer(FieldSerializer.class);
kryo.register(TestExtended.class);

roundTrip(34, 34, testExtended);

FieldSerializer serializer = (FieldSerializer) kryo.getSerializer(TestExtended.class);

// Simple class name is part of field name in EXTENDED field name strategy.
assertNotNull(serializer.getField("TestDefault.a"));
serializer.removeField("TestDefault.a");
assertFieldRemoved(serializer, "TestDefault.a");
assertNotNull(serializer.getField("TestExtended.a"));
serializer.removeField("TestExtended.a");
assertFieldRemoved(serializer, "TestExtended.a");
}

protected void assertFieldRemoved(FieldSerializer serializer, String fieldName) {
try {
assertNull(serializer.getField(fieldName));
Assert.fail("Expected IllegalArgumentException to be thrown for serializer.getField(" + fieldName + ")");
} catch (IllegalArgumentException iae) {
assertTrue(true);
}
}

static public class TestDefault {
private String a;

public String getA() {
return a;
}

public void setA(String a) {
this.a = a;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;

TestDefault that = (TestDefault) o;

return a != null ? a.equals(that.a) : that.a == null;

}

@Override
public int hashCode() {
return a != null ? a.hashCode() : 0;
}
}

static public class TestExtended extends TestDefault {
private String a;

public String getA() {
return a;
}

public void setA(String a) {
this.a = a;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;

if (!super.equals(o)) return false;

TestExtended that = (TestExtended) o;
return a != null ? a.equals(that.a) : that.a == null;
}

@Override
public int hashCode() {
return a != null ? a.hashCode() : 0;
}
}
}
2 changes: 1 addition & 1 deletion test/com/esotericsoftware/kryo/FieldSerializerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,7 @@ public void testMultipleTimesAnnotatedMapFields () {

assertFalse("Exception was expected", true);
}

static public class DefaultTypes {
// Primitives.
public boolean booleanField;
Expand Down

0 comments on commit 3520bc4

Please sign in to comment.