diff --git a/common/pom.xml b/common/pom.xml index aab7de9..acdf1c7 100644 --- a/common/pom.xml +++ b/common/pom.xml @@ -7,7 +7,7 @@ dev.vality.geck parent - 1.0.2 + 1.0.3 ../pom.xml diff --git a/common/src/main/java/dev/vality/geck/common/util/BinaryUtil.java b/common/src/main/java/dev/vality/geck/common/util/BinaryUtil.java new file mode 100644 index 0000000..b4f962d --- /dev/null +++ b/common/src/main/java/dev/vality/geck/common/util/BinaryUtil.java @@ -0,0 +1,16 @@ +package dev.vality.geck.common.util; + +import java.nio.ByteBuffer; + +public final class BinaryUtil { + + private BinaryUtil() { + } + + public static byte[] toByteArray(ByteBuffer buffer) { + ByteBuffer currentRange = buffer.slice(); + byte[] bytes = new byte[currentRange.remaining()]; + currentRange.get(bytes); + return bytes; + } +} diff --git a/filter/pom.xml b/filter/pom.xml index 8114149..624292b 100644 --- a/filter/pom.xml +++ b/filter/pom.xml @@ -7,7 +7,7 @@ dev.vality.geck parent - 1.0.2 + 1.0.3 ../pom.xml diff --git a/migrator/pom.xml b/migrator/pom.xml index cbd3ef6..7eb21cb 100644 --- a/migrator/pom.xml +++ b/migrator/pom.xml @@ -7,7 +7,7 @@ dev.vality.geck parent - 1.0.2 + 1.0.3 ../pom.xml diff --git a/migrator/src/main/java/dev/vality/geck/migrator/kit/BaseMigrationManager.java b/migrator/src/main/java/dev/vality/geck/migrator/kit/BaseMigrationManager.java index a4b33b5..95dc183 100644 --- a/migrator/src/main/java/dev/vality/geck/migrator/kit/BaseMigrationManager.java +++ b/migrator/src/main/java/dev/vality/geck/migrator/kit/BaseMigrationManager.java @@ -42,7 +42,7 @@ public O migrate(I src, ThriftSpec thriftSpec, SerializerSpec seria throw new MigrationException("Not migrator for type: "+ mPoint.getMigrationType()); } transitionSerSpec.setOutDef( ((i < mPoints.size() - 1) ? mPoints.get(i + 1).getSerializerDef() : serializerSpec.getOutDef())); - data = migrator.migrate(src, mPoint, transitionSerSpec); + data = migrator.migrate(data, mPoint, transitionSerSpec); transitionSerSpec.setInDef(mPoint.getSerializerDef()); } diff --git a/migrator/src/test/java/dev/vality/geck/migrator/kit/TestBaseMigrationManager.java b/migrator/src/test/java/dev/vality/geck/migrator/kit/TestBaseMigrationManager.java index 2285b08..a679069 100644 --- a/migrator/src/test/java/dev/vality/geck/migrator/kit/TestBaseMigrationManager.java +++ b/migrator/src/test/java/dev/vality/geck/migrator/kit/TestBaseMigrationManager.java @@ -13,27 +13,59 @@ public class TestBaseMigrationManager { + /** + * Проверяет, что сама цепочка миграций не меняет данные. + * Если мигратор только перегоняет значение дальше, на выходе должен остаться исходный payload. + */ @Test - public void test() throws MigrationException { - BaseMigrationStore migrationStore = new BaseMigrationStore(Arrays.asList(new MigrationPointProviderImpl(Arrays.asList( + public void shouldKeepSourceDataWhenMigratorDoesNotTransformIt() throws MigrationException { + BaseMigrationStore migrationStore = + new BaseMigrationStore(Arrays.asList(new MigrationPointProviderStub(Arrays.asList( new ThriftSpec(new ThriftDef(1, "t1"), new ThriftDef(2, "t2")), new ThriftSpec(new ThriftDef(2, "t2"), new ThriftDef(3, "t2")), new ThriftSpec(new ThriftDef(3, "t2"), new ThriftDef(30, "t3")) )))); - BaseMigrationManager migrationManager = new BaseMigrationManager(migrationStore, Arrays.asList(new MigratorImpl())); - String res = migrationManager.migrate("A", new ThriftDef(1), new SerializerDef<>("TEST")); - Assert.assertEquals("A", res); + BaseMigrationManager migrationManager = + new BaseMigrationManager(migrationStore, Arrays.asList(new PassThroughMigrator())); + + String result = migrationManager.migrate("A", new ThriftDef(1), new SerializerDef<>("TEST")); + + Assert.assertEquals("A", result); + } + + /** + * Проверяет, что каждый следующий шаг получает результат предыдущего шага. + * Ловит баг, при котором все шаги вызывались с исходным значением. + */ + @Test + public void shouldPassIntermediateMigrationResultToNextStep() throws MigrationException { + BaseMigrationStore migrationStore = + new BaseMigrationStore(Arrays.asList(new MigrationPointProviderStub(Arrays.asList( + new ThriftSpec(new ThriftDef(1, "t1"), new ThriftDef(2, "t2")), + new ThriftSpec(new ThriftDef(2, "t2"), new ThriftDef(3, "t2")), + new ThriftSpec(new ThriftDef(3, "t2"), new ThriftDef(4, "t3")) + )))); + BaseMigrationManager migrationManager = + new BaseMigrationManager(migrationStore, Arrays.asList(new StepNumberAppendingMigrator())); + + String result = migrationManager.migrate("A", new ThriftDef(1), new SerializerDef<>("TEST")); + + Assert.assertEquals("A123", result); } - private static class MigrationPointProviderImpl implements MigrationPointProvider { - private List migrationPoints; + /** + * Собирает простой упорядоченный маршрут миграции для теста. + * Этого достаточно, чтобы проверить chaining без лишнего проектного окружения. + */ + private static class MigrationPointProviderStub implements MigrationPointProvider { + private final List migrationPoints; - public MigrationPointProviderImpl(Collection tSpecs) { + MigrationPointProviderStub(Collection thriftSpecs) { AtomicInteger idx = new AtomicInteger(); SerializerDef serializerDef = new SerializerDef("TEST"); - migrationPoints = tSpecs.stream().map(thriftSpec -> { + migrationPoints = thriftSpecs.stream().map(thriftSpec -> { int i = idx.incrementAndGet(); - return new MigrationPoint(i, thriftSpec, serializerDef, new MigrationSpecImpl("TEST"+i)); + return new MigrationPoint(i, thriftSpec, serializerDef, new TestMigrationSpec("TEST" + i)); }).collect(Collectors.toList()); } @@ -53,10 +85,14 @@ public MigrationPoint getMappedSpec(ThriftSpec thriftSpec) throws MigrationExcep } } - private static class MigrationSpecImpl implements MigrationSpec { - private String spec; + /** + * Хранит тестовый идентификатор шага, например TEST1 или TEST2. + * Нужен, чтобы было видно, какие шаги реально применились и в каком порядке. + */ + private static class TestMigrationSpec implements MigrationSpec { + private final String spec; - public MigrationSpecImpl(String spec) { + TestMigrationSpec(String spec) { this.spec = spec; } @@ -71,7 +107,11 @@ public String getType() { } } - private static class MigratorImpl extends AbstractMigrator { + /** + * Имитирует мигратор, который просто пропускает данные через сериализацию. + * Нужен, чтобы проверить поведение самого migration manager без дополнительных преобразований. + */ + private static class PassThroughMigrator extends AbstractMigrator { @Override public O migrate(I data, MigrationPoint mPoint, SerializerSpec serializerSpec) throws MigrationException { @@ -83,4 +123,22 @@ public String getMigrationType() { return "TEST"; } } + + /** + * Дописывает к payload номер текущего шага миграции. + * Так видно, получают ли поздние шаги накопленный результат или снова исходный input. + */ + private static class StepNumberAppendingMigrator implements Migrator { + + @Override + public O migrate(I data, MigrationPoint mPoint, SerializerSpec serializerSpec) { + String stepNumber = ((TestMigrationSpec) mPoint.getMigrationSpec()).getSpec().replace("TEST", ""); + return (O) (String.valueOf(data) + stepNumber); + } + + @Override + public String getMigrationType() { + return "TEST"; + } + } } diff --git a/pom.xml b/pom.xml index 37339de..952f605 100644 --- a/pom.xml +++ b/pom.xml @@ -12,7 +12,7 @@ dev.vality.geck parent - 1.0.2 + 1.0.3 pom Geck diff --git a/serializer/pom.xml b/serializer/pom.xml index c63f2a1..ca2a7ee 100644 --- a/serializer/pom.xml +++ b/serializer/pom.xml @@ -7,7 +7,7 @@ dev.vality.geck parent - 1.0.2 + 1.0.3 ../pom.xml diff --git a/serializer/src/main/java/dev/vality/geck/serializer/kit/json/JsonProcessor.java b/serializer/src/main/java/dev/vality/geck/serializer/kit/json/JsonProcessor.java index a488941..5fa5749 100644 --- a/serializer/src/main/java/dev/vality/geck/serializer/kit/json/JsonProcessor.java +++ b/serializer/src/main/java/dev/vality/geck/serializer/kit/json/JsonProcessor.java @@ -59,6 +59,9 @@ private void processNode(JsonNode jsonNode, String name, StructHandler handler) } String arrCode = elements.next().textValue(); StructType arrType = StructType.valueOfKey(arrCode); + if (arrType == null) { + throw new BadFormatException("Unknown type of node: " + arrCode + ". Must be on of them : " + StructType.LIST + ", " + StructType.SET + ", " + StructType.MAP); + } int size = jsonNode.size() - 1; switch (arrType) { case LIST: @@ -85,7 +88,7 @@ private void processNode(JsonNode jsonNode, String name, StructHandler handler) handler.endMap(); break; default: - new BadFormatException("Unknown type of node: " + arrType + ". Must be on of them : " + StructType.LIST + ", " + StructType.SET + ", " + StructType.MAP); + throw new BadFormatException("Unknown type of node: " + arrType + ". Must be on of them : " + StructType.LIST + ", " + StructType.SET + ", " + StructType.MAP); } } } diff --git a/serializer/src/main/java/dev/vality/geck/serializer/kit/mock/MockTBaseProcessor.java b/serializer/src/main/java/dev/vality/geck/serializer/kit/mock/MockTBaseProcessor.java index 33011ff..ea4ddac 100644 --- a/serializer/src/main/java/dev/vality/geck/serializer/kit/mock/MockTBaseProcessor.java +++ b/serializer/src/main/java/dev/vality/geck/serializer/kit/mock/MockTBaseProcessor.java @@ -9,7 +9,9 @@ import java.io.IOException; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Set; public class MockTBaseProcessor extends TBaseProcessor { @@ -19,9 +21,10 @@ public class MockTBaseProcessor extends TBaseProcessor { private final int maxContainerSize; - private ValueGenerator valueGenerator; + private final ValueGenerator valueGenerator; - private Map fieldHandlers = new HashMap<>(); + private final Map fieldHandlers = new HashMap<>(); + private final ThreadLocal>> structTypesInProgress = new ThreadLocal<>(); public MockTBaseProcessor() { this(MockMode.ALL); @@ -52,15 +55,28 @@ public void addFieldHandler(FieldHandler handler, String... fieldNames) { } } + @Override + public R process(TBase value, StructHandler handler) throws IOException { + structTypesInProgress.set(new HashSet<>()); + try { + if (value == null) { + handler.nullValue(); + return handler.getResult(); + } + return super.process(value, handler); + } finally { + structTypesInProgress.remove(); + } + } + @Override protected void processUnsetField(TFieldIdEnum tFieldIdEnum, FieldMetaData fieldMetaData, StructHandler handler) throws IOException { if (needProcess(fieldMetaData)) { handler.name((byte) tFieldIdEnum.getThriftFieldId(), tFieldIdEnum.getFieldName()); - if (fieldHandlers.containsKey(fieldMetaData.fieldName)) { - fieldHandlers.get(tFieldIdEnum.getFieldName()).handle(handler); - } else { - processFieldValue(fieldMetaData.valueMetaData, handler); + if (handleField(tFieldIdEnum, handler)) { + return; } + processFieldValue(fieldMetaData.valueMetaData, handler); } } @@ -70,11 +86,10 @@ protected void processUnsetUnion(TUnion tUnion, StructHandler handler) throws IO TFieldIdEnum tFieldIdEnum = valueGenerator.getField(tUnion); FieldMetaData fieldMetaData = fieldMetaDataMap.get(tFieldIdEnum); handler.name((byte) tFieldIdEnum.getThriftFieldId(), tFieldIdEnum.getFieldName()); - if (fieldHandlers.containsKey(fieldMetaData.fieldName)) { - fieldHandlers.get(tFieldIdEnum.getFieldName()).handle(handler); - } else { - processFieldValue(fieldMetaData.valueMetaData, handler); + if (handleField(tFieldIdEnum, handler)) { + return; } + processFieldValue(fieldMetaData.valueMetaData, handler); } protected void processFieldValue(FieldValueMetaData valueMetaData, StructHandler handler) throws IOException { @@ -130,11 +145,21 @@ protected void processFieldValue(FieldValueMetaData valueMetaData, StructHandler } private void processStruct(StructMetaData structMetaData, StructHandler handler) throws IOException { + Class structClass = structMetaData.getStructClass(); + Set> currentStructTypes = currentStructTypes(); + if (!currentStructTypes.add(structClass)) { + throw new IllegalStateException(String.format( + "Recursive thrift type detected while generating mock for '%s'", + structClass.getName() + )); + } try { - TBase tBase = structMetaData.getStructClass().newInstance(); + TBase tBase = structClass.newInstance(); super.processStruct(tBase, handler); } catch (InstantiationException | IllegalAccessException ex) { throw new IOException(ex); + } finally { + currentStructTypes.remove(structClass); } } @@ -180,4 +205,21 @@ private boolean needProcess(FieldMetaData fieldMetaData) { || mode == MockMode.ALL; } + private boolean handleField(TFieldIdEnum field, StructHandler handler) throws IOException { + FieldHandler fieldHandler = fieldHandlers.get(field.getFieldName()); + if (fieldHandler == null) { + return false; + } + fieldHandler.handle(handler); + return true; + } + + private Set> currentStructTypes() { + Set> currentStructTypes = structTypesInProgress.get(); + if (currentStructTypes == null) { + throw new IllegalStateException("Mock generation context is not initialized"); + } + return currentStructTypes; + } + } diff --git a/serializer/src/main/java/dev/vality/geck/serializer/kit/object/ObjectProcessor.java b/serializer/src/main/java/dev/vality/geck/serializer/kit/object/ObjectProcessor.java index 3421bc3..7f7d632 100644 --- a/serializer/src/main/java/dev/vality/geck/serializer/kit/object/ObjectProcessor.java +++ b/serializer/src/main/java/dev/vality/geck/serializer/kit/object/ObjectProcessor.java @@ -1,5 +1,6 @@ package dev.vality.geck.serializer.kit.object; +import dev.vality.geck.common.util.BinaryUtil; import dev.vality.geck.common.util.TypeUtil; import dev.vality.geck.serializer.StructHandler; import dev.vality.geck.serializer.StructProcessor; @@ -41,9 +42,13 @@ private void processMap(List mapList, boolean named, StructHandler handler) thro handler.beginMap(mapList.size()); Iterator it = mapList.iterator(); if (!named) { - assert it.hasNext(); + if (!it.hasNext()) { + throw new BadFormatException("Incorrect structure of map. First element should exist!"); + } Object mapSign = it.next(); - assert getType(String.valueOf(mapSign)) == StructType.MAP; + if (getType(String.valueOf(mapSign)) != StructType.MAP) { + throw new BadFormatException("Incorrect structure of map. First element should contain map marker!"); + } } for (Object entry; it.hasNext(); ) { entry = it.next(); @@ -81,6 +86,7 @@ private void processValue(Object value, StructType type, boolean named, StructHa break; case SET: processSet(TypeUtil.convertType(Collection.class, value), named, handler); + break; case OTHER: if (value instanceof Map) { processStruct((Map) value, handler); @@ -91,7 +97,7 @@ private void processValue(Object value, StructType type, boolean named, StructHa if (named) { processList(list, handler); } else { - if (list.size() > 0) { + if (!list.isEmpty()) { StructType lType = getType(String.valueOf(list.get(0))); switch (lType) { case OTHER: @@ -107,15 +113,16 @@ private void processValue(Object value, StructType type, boolean named, StructHa } else if (value instanceof String) { handler.value(unescapeString((String) value)); } else if (value instanceof Number) { + Number number = (Number) value; if (value instanceof Double || value instanceof Float) { - handler.value(((Number) value).doubleValue()); + handler.value(number.doubleValue()); } else { - handler.value(((Number) value).longValue()); + handler.value(number.longValue()); } } else if (value instanceof Boolean) { - handler.value(((Boolean) value).booleanValue()); + handler.value((Boolean) value); } else if (value instanceof ByteBuffer) { - handler.value(((ByteBuffer) value).array()); + handler.value(BinaryUtil.toByteArray((ByteBuffer) value)); } else if (value == null) { handler.nullValue(); } else { @@ -175,4 +182,5 @@ private String unescapeString(String name) { return name; } } + } diff --git a/serializer/src/main/java/dev/vality/geck/serializer/kit/tbase/TBaseProcessor.java b/serializer/src/main/java/dev/vality/geck/serializer/kit/tbase/TBaseProcessor.java index 1adb3c1..d261867 100644 --- a/serializer/src/main/java/dev/vality/geck/serializer/kit/tbase/TBaseProcessor.java +++ b/serializer/src/main/java/dev/vality/geck/serializer/kit/tbase/TBaseProcessor.java @@ -1,5 +1,6 @@ package dev.vality.geck.serializer.kit.tbase; +import dev.vality.geck.common.util.BinaryUtil; import dev.vality.geck.common.util.TBaseUtil; import dev.vality.geck.common.util.TypeUtil; import dev.vality.geck.serializer.StructHandler; @@ -31,41 +32,57 @@ public R process(TBase value, StructHandler handler) throws IOException { if (value == null) { handler.nullValue(); } else { - processStruct(value, handler); + processStruct(value, handler, newIdentitySet()); } return handler.getResult(); } protected void processStruct(TBase value, StructHandler handler) throws IOException { - TFieldIdEnum[] tFieldIdEnums = value.getFields(); - Map fieldMetaDataMap = value.getFieldMetaData(); - - int size = TBaseUtil.getSetFieldsCount(value); - handler.beginStruct(size); - - if (value instanceof TUnion) { - TUnion union = (TUnion) value; - if (union.isSet()) { - TFieldIdEnum tFieldIdEnum = union.getSetField(); - handler.name((byte) tFieldIdEnum.getThriftFieldId(), tFieldIdEnum.getFieldName()); - process(union.getFieldValue(), fieldMetaDataMap.get(tFieldIdEnum).valueMetaData, handler); - } else { - processUnsetUnion(union, handler); - } - } else { - for (TFieldIdEnum tFieldIdEnum : tFieldIdEnums) { - FieldMetaData fieldMetaData = fieldMetaDataMap.get(tFieldIdEnum); - if (value.isSet(tFieldIdEnum)) { + processStruct(value, handler, newIdentitySet()); + } + + protected void processStruct(TBase value, StructHandler handler, Set structsInProgress) throws IOException { + if (!structsInProgress.add(value)) { + throw new IllegalStateException(String.format( + "Cyclic reference detected while processing thrift struct '%s'", + value.getClass().getName() + )); + } + try { + TFieldIdEnum[] tFieldIdEnums = value.getFields(); + Map fieldMetaDataMap = value.getFieldMetaData(); + + int size = TBaseUtil.getSetFieldsCount(value); + handler.beginStruct(size); + + if (value instanceof TUnion) { + TUnion union = (TUnion) value; + if (union.isSet()) { + TFieldIdEnum tFieldIdEnum = union.getSetField(); handler.name((byte) tFieldIdEnum.getThriftFieldId(), tFieldIdEnum.getFieldName()); - process(value.getFieldValue(tFieldIdEnum), fieldMetaData.valueMetaData, handler); + process(union.getFieldValue(), fieldMetaDataMap.get(tFieldIdEnum).valueMetaData, handler, + structsInProgress); } else { - processUnsetField(tFieldIdEnum, fieldMetaData, handler); + processUnsetUnion(union, handler); + } + } else { + for (TFieldIdEnum tFieldIdEnum : tFieldIdEnums) { + FieldMetaData fieldMetaData = fieldMetaDataMap.get(tFieldIdEnum); + if (value.isSet(tFieldIdEnum)) { + handler.name((byte) tFieldIdEnum.getThriftFieldId(), tFieldIdEnum.getFieldName()); + process(value.getFieldValue(tFieldIdEnum), fieldMetaData.valueMetaData, handler, + structsInProgress); + } else { + processUnsetField(tFieldIdEnum, fieldMetaData, handler); + } } } - } - handler.endStruct(); + handler.endStruct(); + } finally { + structsInProgress.remove(value); + } } protected void processUnsetUnion(TUnion tUnion, StructHandler handler) throws IOException { @@ -78,13 +95,17 @@ protected void processUnsetField(TFieldIdEnum tFieldIdEnum, FieldMetaData fieldM } } - private void process(Object object, FieldValueMetaData fieldValueMetaData, StructHandler handler) throws IOException { + private void process( + Object object, + FieldValueMetaData fieldValueMetaData, + StructHandler handler, + Set structsInProgress) throws IOException { if (object == null) { handler.nullValue(); } else if (object instanceof Optional optional) { if (optional.isPresent()) { Object value = optional.get(); - process(value, fieldValueMetaData, handler); + process(value, fieldValueMetaData, handler, structsInProgress); } else { handler.nullValue(); } @@ -120,7 +141,7 @@ private void process(Object object, FieldValueMetaData fieldValueMetaData, Struc if (object instanceof byte[]) { handler.value((byte[]) object); } else if (object instanceof ByteBuffer) { - handler.value(((ByteBuffer) object).array()); + handler.value(BinaryUtil.toByteArray((ByteBuffer) object)); } else { throw new IllegalStateException(String.format("Unknown binary type, type='%s'", object.getClass().getName())); } @@ -128,18 +149,18 @@ private void process(Object object, FieldValueMetaData fieldValueMetaData, Struc case LIST: List list = TypeUtil.convertType(List.class, object); ListMetaData listMetaData = TypeUtil.convertType(ListMetaData.class, fieldValueMetaData); - processList(list, listMetaData, handler); + processList(list, listMetaData, handler, structsInProgress); break; case SET: Set set = TypeUtil.convertType(Set.class, object); SetMetaData setMetaData = TypeUtil.convertType(SetMetaData.class, fieldValueMetaData); - processSet(set, setMetaData, handler); + processSet(set, setMetaData, handler, structsInProgress); break; case MAP: - processMap((Map) object, (MapMetaData) fieldValueMetaData, handler); + processMap((Map) object, (MapMetaData) fieldValueMetaData, handler, structsInProgress); break; case STRUCT: - processStruct((TBase) object, handler); + processStruct((TBase) object, handler, structsInProgress); break; default: throw new IllegalStateException(String.format("Type '%s' not found", type)); @@ -147,36 +168,55 @@ private void process(Object object, FieldValueMetaData fieldValueMetaData, Struc } } - private void processList(List list, ListMetaData listMetaData, StructHandler handler) throws IOException { + private void processList( + List list, + ListMetaData listMetaData, + StructHandler handler, + Set structsInProgress) throws IOException { handler.beginList(list.size()); - processCollection(list, listMetaData.getElementMetaData(), handler); + processCollection(list, listMetaData.getElementMetaData(), handler, structsInProgress); handler.endList(); } - private void processSet(Set set, SetMetaData setMetaData, StructHandler handler) throws IOException { + private void processSet( + Set set, + SetMetaData setMetaData, + StructHandler handler, + Set structsInProgress) throws IOException { handler.beginSet(set.size()); - processCollection(set, setMetaData.getElementMetaData(), handler); + processCollection(set, setMetaData.getElementMetaData(), handler, structsInProgress); handler.endSet(); } - private void processCollection(Collection collection, FieldValueMetaData valueMetaData, StructHandler handler) throws IOException { + private void processCollection( + Collection collection, + FieldValueMetaData valueMetaData, + StructHandler handler, + Set structsInProgress) throws IOException { for (Object object : collection) { - process(object, valueMetaData, handler); + process(object, valueMetaData, handler, structsInProgress); } } - private void processMap(Map objectMap, MapMetaData metaData, StructHandler handler) throws IOException { + private void processMap( + Map objectMap, + MapMetaData metaData, + StructHandler handler, + Set structsInProgress) throws IOException { handler.beginMap(objectMap.size()); for (Map.Entry entry : (Set) objectMap.entrySet()) { handler.beginKey(); - process(entry.getKey(), metaData.getKeyMetaData(), handler); + process(entry.getKey(), metaData.getKeyMetaData(), handler, structsInProgress); handler.endKey(); handler.beginValue(); - process(entry.getValue(), metaData.getValueMetaData(), handler); + process(entry.getValue(), metaData.getValueMetaData(), handler, structsInProgress); handler.endValue(); } handler.endMap(); } + private static Set newIdentitySet() { + return Collections.newSetFromMap(new IdentityHashMap<>()); + } } diff --git a/serializer/src/main/java/dev/vality/geck/serializer/kit/xml/XMLProcessor.java b/serializer/src/main/java/dev/vality/geck/serializer/kit/xml/XMLProcessor.java index 6a3cd7f..da5365b 100644 --- a/serializer/src/main/java/dev/vality/geck/serializer/kit/xml/XMLProcessor.java +++ b/serializer/src/main/java/dev/vality/geck/serializer/kit/xml/XMLProcessor.java @@ -88,7 +88,7 @@ protected void processNode(Element node, StructHandler handler, boolean printNam handler.endValue(); break; default: - new BadFormatException("Unknown type of node: "+type+". Must be on of them : "+ Arrays.toString(StructType.values())); + throw new BadFormatException("Unknown type of node: "+type+". Must be on of them : "+ Arrays.toString(StructType.values())); } } } diff --git a/serializer/src/test/java/dev/vality/geck/serializer/domain/RecursiveStruct.java b/serializer/src/test/java/dev/vality/geck/serializer/domain/RecursiveStruct.java new file mode 100644 index 0000000..4409f80 --- /dev/null +++ b/serializer/src/test/java/dev/vality/geck/serializer/domain/RecursiveStruct.java @@ -0,0 +1,139 @@ +package dev.vality.geck.serializer.domain; + +import org.apache.thrift.TBase; +import org.apache.thrift.TException; +import org.apache.thrift.TFieldIdEnum; +import org.apache.thrift.TFieldRequirementType; +import org.apache.thrift.meta_data.FieldMetaData; +import org.apache.thrift.meta_data.StructMetaData; +import org.apache.thrift.protocol.TProtocol; +import org.apache.thrift.protocol.TType; + +import java.util.Collections; +import java.util.EnumMap; +import java.util.Map; + +/** + * Минимальная рекурсивная thrift-подобная структура только для тестов. + * Нужна, чтобы собрать цикл в ссылках без зависимости от сгенерированных моделей проекта. + */ +public class RecursiveStruct implements TBase { + + private static final Map<_Fields, FieldMetaData> META_DATA_MAP; + + static { + Map<_Fields, FieldMetaData> metaData = new EnumMap<>(_Fields.class); + metaData.put(_Fields.NEXT, new FieldMetaData( + "next", + TFieldRequirementType.REQUIRED, + new StructMetaData(TType.STRUCT, RecursiveStruct.class) + )); + META_DATA_MAP = Collections.unmodifiableMap(metaData); + FieldMetaData.addStructMetaDataMap(RecursiveStruct.class, META_DATA_MAP); + } + + private RecursiveStruct next; + + @Override + public void clear() { + next = null; + } + + @Override + public RecursiveStruct deepCopy() { + RecursiveStruct copy = new RecursiveStruct(); + copy.next = next; + return copy; + } + + public RecursiveStruct setNext(RecursiveStruct next) { + this.next = next; + return this; + } + + public RecursiveStruct getNext() { + return next; + } + + @Override + public void setFieldValue(_Fields field, Object value) { + if (field == _Fields.NEXT) { + next = (RecursiveStruct) value; + return; + } + throw new IllegalArgumentException("Unknown field: " + field); + } + + @Override + public Object getFieldValue(_Fields field) { + if (field == _Fields.NEXT) { + return next; + } + throw new IllegalArgumentException("Unknown field: " + field); + } + + @Override + public boolean isSet(_Fields field) { + return field == _Fields.NEXT && next != null; + } + + @Override + public _Fields fieldForId(int fieldId) { + return _Fields.findByThriftId(fieldId); + } + + @Override + public int compareTo(RecursiveStruct other) { + return 0; + } + + @Override + public void read(TProtocol iprot) throws TException { + throw new UnsupportedOperationException(); + } + + @Override + public void write(TProtocol oprot) throws TException { + throw new UnsupportedOperationException(); + } + + @Override + public _Fields[] getFields() { + return _Fields.values(); + } + + @Override + public Map<_Fields, FieldMetaData> getFieldMetaData() { + return META_DATA_MAP; + } + + /** + * Описывает единственное рекурсивное поле тестовой структуры. + * TBase требует такой enum, чтобы выдавать id и имя поля так же, как обычный thrift-класс. + */ + public enum _Fields implements TFieldIdEnum { + NEXT((short) 1, "next"); + + private final short thriftId; + private final String fieldName; + + _Fields(short thriftId, String fieldName) { + this.thriftId = thriftId; + this.fieldName = fieldName; + } + + @Override + public short getThriftFieldId() { + return thriftId; + } + + @Override + public String getFieldName() { + return fieldName; + } + + public static _Fields findByThriftId(int fieldId) { + return fieldId == 1 ? NEXT : null; + } + } +} diff --git a/serializer/src/test/java/dev/vality/geck/serializer/handler/BinaryCapturingHandler.java b/serializer/src/test/java/dev/vality/geck/serializer/handler/BinaryCapturingHandler.java new file mode 100644 index 0000000..dc8dc39 --- /dev/null +++ b/serializer/src/test/java/dev/vality/geck/serializer/handler/BinaryCapturingHandler.java @@ -0,0 +1,22 @@ +package dev.vality.geck.serializer.handler; + +/** + * Тестовый handler, который запоминает первое бинарное значение. + * Нужен, чтобы проверить, какие байты процессор реально отдал в результат. + */ +public class BinaryCapturingHandler extends HandlerStub { + + private byte[] binaryValue; + + @Override + public void value(byte[] value) { + if (binaryValue == null) { + binaryValue = value; + } + } + + @Override + public byte[] getResult() { + return binaryValue; + } +} diff --git a/serializer/src/test/java/dev/vality/geck/serializer/kit/json/JsonTest.java b/serializer/src/test/java/dev/vality/geck/serializer/kit/json/JsonTest.java index c2c4dd2..ced19c0 100644 --- a/serializer/src/test/java/dev/vality/geck/serializer/kit/json/JsonTest.java +++ b/serializer/src/test/java/dev/vality/geck/serializer/kit/json/JsonTest.java @@ -1,7 +1,11 @@ package dev.vality.geck.serializer.kit.json; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; import com.rbkmoney.damsel.v130.payment_processing.InvoicePaymentStarted; import dev.vality.geck.serializer.GeckTestUtil; +import dev.vality.geck.serializer.domain.TestObject; +import dev.vality.geck.serializer.exception.BadFormatException; +import dev.vality.geck.serializer.handler.HandlerStub; import dev.vality.geck.serializer.kit.mock.FixedValueGenerator; import dev.vality.geck.serializer.kit.mock.MockMode; import dev.vality.geck.serializer.kit.mock.MockTBaseProcessor; @@ -9,12 +13,13 @@ import dev.vality.geck.serializer.kit.msgpack.MsgPackProcessor; import dev.vality.geck.serializer.kit.tbase.TBaseHandler; import dev.vality.geck.serializer.kit.tbase.TBaseProcessor; -import dev.vality.geck.serializer.domain.TestObject; import org.junit.Assert; import org.junit.Test; import java.io.IOException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + public class JsonTest { @Test @@ -60,4 +65,17 @@ public void testPretty() throws IOException { String json1 = new TBaseProcessor().process(testObject, handler).toString(); System.out.println(json1); } + + /** + * Проверяет, что JSON с неподдерживаемым маркером массива падает сразу. + * Без этого парсер уходил дальше с некорректным типом узла и ломался менее явно. + */ + @Test + public void unknownArrayTypeShouldFailFast() { + assertThatThrownBy(() -> new JsonProcessor().process( + JsonNodeFactory.instance.arrayNode().add("unsupported"), + new HandlerStub())) + .isInstanceOf(BadFormatException.class) + .hasMessageContaining("Unknown type of node: unsupported"); + } } diff --git a/serializer/src/test/java/dev/vality/geck/serializer/kit/mock/MockTBaseProcessorTest.java b/serializer/src/test/java/dev/vality/geck/serializer/kit/mock/MockTBaseProcessorTest.java index 32f95c5..fad3ded 100644 --- a/serializer/src/test/java/dev/vality/geck/serializer/kit/mock/MockTBaseProcessorTest.java +++ b/serializer/src/test/java/dev/vality/geck/serializer/kit/mock/MockTBaseProcessorTest.java @@ -1,6 +1,7 @@ package dev.vality.geck.serializer.kit.mock; import dev.vality.geck.serializer.domain.BinaryTest; +import dev.vality.geck.serializer.domain.RecursiveStruct; import dev.vality.geck.serializer.domain.TestObject; import dev.vality.geck.serializer.kit.tbase.TBaseHandler; import dev.vality.geck.serializer.kit.tbase.ThriftType; @@ -17,30 +18,31 @@ import static junit.framework.Assert.assertNotNull; import static junit.framework.TestCase.*; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class MockTBaseProcessorTest { @Test public void requiredFieldsOnlyTest() throws IOException { - TestObject testObject = new TestObject(); - testObject = new MockTBaseProcessor(MockMode.REQUIRED_ONLY).process(testObject, new TBaseHandler<>(TestObject.class)); - assertTrue(checkFields(testObject, true)); - assertFalse(checkFields(testObject, false)); + TestObject source = new TestObject(); + TestObject result = new MockTBaseProcessor(MockMode.REQUIRED_ONLY).process(source, new TBaseHandler<>(TestObject.class)); + assertTrue(hasExpectedFields(result, true)); + assertFalse(hasExpectedFields(result, false)); } @Test public void binaryTest() throws IOException { - BinaryTest binaryTest = new BinaryTest(); - binaryTest = new MockTBaseProcessor(MockMode.REQUIRED_ONLY) - .process(binaryTest, new TBaseHandler<>(BinaryTest.class)); - assertNotNull(binaryTest.getData()); - assertNotNull(binaryTest.getDataInList()); - binaryTest.getDataInList().stream().forEach(Assert::assertNotNull); - assertNotNull(binaryTest.getDataInSet()); - binaryTest.getDataInSet().stream().forEach(Assert::assertNotNull); - assertNotNull(binaryTest.getDataInMap()); - binaryTest.getDataInMap().keySet().stream().forEach(Assert::assertNotNull); - binaryTest.getDataInMap().values().stream().forEach(Assert::assertNotNull); + BinaryTest source = new BinaryTest(); + BinaryTest result = new MockTBaseProcessor(MockMode.REQUIRED_ONLY) + .process(source, new TBaseHandler<>(BinaryTest.class)); + assertNotNull(result.getData()); + assertNotNull(result.getDataInList()); + result.getDataInList().forEach(Assert::assertNotNull); + assertNotNull(result.getDataInSet()); + result.getDataInSet().forEach(Assert::assertNotNull); + assertNotNull(result.getDataInMap()); + result.getDataInMap().keySet().forEach(Assert::assertNotNull); + result.getDataInMap().values().forEach(Assert::assertNotNull); } @Test @@ -49,44 +51,54 @@ public void fieldHandlerTest() throws IOException { MockTBaseProcessor processor = new MockTBaseProcessor(); processor.addFieldHandler(handler -> handler.value(testValue), "description", "another_string"); - TestObject testObject = new TestObject(); - testObject = processor.process(testObject, new TBaseHandler<>(TestObject.class)); - assertEquals(testValue, testObject.getDescription()); - assertEquals(testValue, testObject.getAnotherString()); + TestObject source = new TestObject(); + TestObject result = processor.process(source, new TBaseHandler<>(TestObject.class)); + assertEquals(testValue, result.getDescription()); + assertEquals(testValue, result.getAnotherString()); } @Test public void allFieldsTest() throws IOException { - TestObject testObject = new TestObject(); - testObject = new MockTBaseProcessor(MockMode.ALL).process(testObject, new TBaseHandler<>(TestObject.class)); - assertTrue(checkFields(testObject, false)); + TestObject source = new TestObject(); + TestObject result = new MockTBaseProcessor(MockMode.ALL).process(source, new TBaseHandler<>(TestObject.class)); + assertTrue(hasExpectedFields(result, false)); } + /** + * Проверяет, что mock-генерация останавливается на рекурсивном thrift-типе. + * Без этой защиты генератор продолжал создавать вложенные объекты, пока не заканчивался стек. + */ + @Test + public void recursiveSchemaShouldFailFastInsteadOfInfiniteMockGeneration() { + assertThatThrownBy(() -> new MockTBaseProcessor(MockMode.ALL) + .process(new RecursiveStruct(), new TBaseHandler<>(RecursiveStruct.class))) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("Recursive thrift type detected"); + } - public boolean checkFields(TBase tBase, boolean requiredOnly) { + private static boolean hasExpectedFields(TBase tBase, boolean requiredOnly) { Map fieldMetaDataMap = tBase.getFieldMetaData(); - boolean check = true; + boolean fieldsAreSet = true; if (tBase instanceof TUnion) { TUnion tUnion = (TUnion) tBase; - check &= tUnion.isSet(); + fieldsAreSet &= tUnion.isSet(); FieldMetaData fieldMetaData = fieldMetaDataMap.get(tUnion.getSetField()); if (ThriftType.findByMetaData(fieldMetaData.valueMetaData) == ThriftType.STRUCT) { - check &= checkFields((TBase) tUnion.getFieldValue(), requiredOnly); + fieldsAreSet &= hasExpectedFields((TBase) tUnion.getFieldValue(), requiredOnly); } } else { for (TFieldIdEnum tFieldIdEnum : tBase.getFields()) { FieldMetaData fieldMetaData = fieldMetaDataMap.get(tFieldIdEnum); if (fieldMetaData.requirementType == TFieldRequirementType.REQUIRED || !requiredOnly) { - check &= tBase.isSet(tFieldIdEnum); + fieldsAreSet &= tBase.isSet(tFieldIdEnum); if (ThriftType.findByMetaData(fieldMetaData.valueMetaData) == ThriftType.STRUCT) { - check &= checkFields((TBase) tBase.getFieldValue(tFieldIdEnum), requiredOnly); + fieldsAreSet &= hasExpectedFields((TBase) tBase.getFieldValue(tFieldIdEnum), requiredOnly); } } } } - return check; + return fieldsAreSet; } - } diff --git a/serializer/src/test/java/dev/vality/geck/serializer/kit/object/ObjectProcessorTest.java b/serializer/src/test/java/dev/vality/geck/serializer/kit/object/ObjectProcessorTest.java new file mode 100644 index 0000000..50188d1 --- /dev/null +++ b/serializer/src/test/java/dev/vality/geck/serializer/kit/object/ObjectProcessorTest.java @@ -0,0 +1,27 @@ +package dev.vality.geck.serializer.kit.object; + +import dev.vality.geck.serializer.domain.SetTest; +import dev.vality.geck.serializer.kit.mock.MockTBaseProcessor; +import dev.vality.geck.serializer.kit.tbase.TBaseHandler; +import dev.vality.geck.serializer.kit.tbase.TBaseProcessor; +import org.junit.Assert; +import org.junit.Test; + +import java.io.IOException; + +public class ObjectProcessorTest { + + /** + * Проверяет, что set не меняется после сериализации и обратного чтения. + * Ловит баг, при котором после обработки set код проваливался в следующую ветку switch. + */ + @Test + public void shouldRoundTripSetsWithoutFallingThroughToOtherBranch() throws IOException { + SetTest source = new MockTBaseProcessor().process(new SetTest(), new TBaseHandler<>(SetTest.class)); + + Object encoded = new TBaseProcessor().process(source, new ObjectHandler()); + SetTest restored = new ObjectProcessor().process(encoded, new TBaseHandler<>(SetTest.class)); + + Assert.assertEquals(source, restored); + } +} diff --git a/serializer/src/test/java/dev/vality/geck/serializer/kit/tbase/TBaseProcessorTest.java b/serializer/src/test/java/dev/vality/geck/serializer/kit/tbase/TBaseProcessorTest.java index 44a89a5..df1669d 100644 --- a/serializer/src/test/java/dev/vality/geck/serializer/kit/tbase/TBaseProcessorTest.java +++ b/serializer/src/test/java/dev/vality/geck/serializer/kit/tbase/TBaseProcessorTest.java @@ -1,13 +1,16 @@ package dev.vality.geck.serializer.kit.tbase; import dev.vality.geck.serializer.domain.*; +import dev.vality.geck.serializer.handler.BinaryCapturingHandler; import dev.vality.geck.serializer.handler.HandlerStub; import dev.vality.geck.serializer.kit.mock.MockTBaseProcessor; import org.junit.Assert; import org.junit.Test; import java.io.IOException; +import java.nio.ByteBuffer; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import static dev.vality.geck.serializer.GeckTestUtil.getTestObject; @@ -70,4 +73,41 @@ public void binaryUnknownTypeDataTest() throws IOException { .hasMessage("Unknown binary type, type='java.lang.Integer'"); } + /** + * Проверяет, что бинарные данные берутся только из активного диапазона ByteBuffer. + * Ловит случай, когда сериализация читала весь backing array и подтягивала лишние байты. + */ + @Test + public void binaryByteBufferShouldRespectPositionAndLimit() throws IOException { + BinaryTest binaryTest = new BinaryTest(); + ByteBuffer buffer = ByteBuffer.wrap(new byte[]{9, 1, 2, 8}); + buffer.position(1); + buffer.limit(3); + + binaryTest.setData(buffer); + binaryTest.setDataInList(Collections.emptyList()); + binaryTest.setDataInSet(Collections.emptySet()); + binaryTest.setDataInMap(Collections.emptyMap()); + + BinaryCapturingHandler handler = new BinaryCapturingHandler(); + new TBaseProcessor().process(binaryTest, handler); + + Assert.assertArrayEquals(new byte[]{1, 2}, handler.getResult()); + } + + /** + * Проверяет, что циклический граф объектов падает с понятной ошибкой, а не со stack overflow. + * Процессор умеет сериализовать дерево, но должен остановиться на петле в ссылках. + */ + @Test + public void cyclicThriftGraphShouldFailFastInsteadOfStackOverflow() { + RecursiveStruct first = new RecursiveStruct(); + RecursiveStruct second = new RecursiveStruct(); + first.setNext(second); + second.setNext(first); + + assertThatThrownBy(() -> new TBaseProcessor().process(first, new HandlerStub())) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("Cyclic reference detected"); + } } diff --git a/serializer/src/test/java/dev/vality/geck/serializer/kit/xml/XMLTest.java b/serializer/src/test/java/dev/vality/geck/serializer/kit/xml/XMLTest.java index ab27c84..6a3973b 100644 --- a/serializer/src/test/java/dev/vality/geck/serializer/kit/xml/XMLTest.java +++ b/serializer/src/test/java/dev/vality/geck/serializer/kit/xml/XMLTest.java @@ -2,17 +2,26 @@ import com.rbkmoney.damsel.v130.payment_processing.InvoicePaymentStarted; import dev.vality.geck.serializer.GeckTestUtil; +import dev.vality.geck.serializer.domain.TestObject; +import dev.vality.geck.serializer.exception.BadFormatException; +import dev.vality.geck.serializer.handler.HandlerStub; +import dev.vality.geck.serializer.kit.StructType; import dev.vality.geck.serializer.kit.mock.FixedValueGenerator; import dev.vality.geck.serializer.kit.mock.MockMode; import dev.vality.geck.serializer.kit.mock.MockTBaseProcessor; import dev.vality.geck.serializer.kit.tbase.TBaseHandler; import dev.vality.geck.serializer.kit.tbase.TBaseProcessor; -import dev.vality.geck.serializer.domain.TestObject; import org.junit.Assert; import org.junit.Test; +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.transform.dom.DOMResult; import java.io.IOException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + public class XMLTest { @Test public void testInvoiceBackTransform1() throws IOException { @@ -32,4 +41,27 @@ public void xmlKebabTest() throws Exception { new TBaseProcessor().process(invoice, handler); System.out.println(xml); } + + /** + * Проверяет, что XML с неподдерживаемым типом узла падает сразу. + * Без этого некорректный XML проходил глубже в парсер и ломался уже в полусобранном состоянии. + */ + @Test + public void unknownNodeTypeShouldFailFast() throws Exception { + Document document = DocumentBuilderFactory.newInstance() + .newDocumentBuilder() + .newDocument(); + Element root = document.createElement(XMLConstants.ROOT); + root.setAttribute(XMLConstants.ATTRIBUTE_TYPE, StructType.STRUCT.getKey()); + document.appendChild(root); + Element child = document.createElement("field"); + child.setAttribute(XMLConstants.ATTRIBUTE_TYPE, "unsupported"); + root.appendChild(child); + + DOMResult domResult = new DOMResult(document); + + assertThatThrownBy(() -> new XMLProcessor().process(domResult, new HandlerStub())) + .isInstanceOf(BadFormatException.class) + .hasMessageContaining("Attribute 'type' must not be null"); + } }