Fix serializer recursion handling and bump version to 1.0.3

common/pom.xml

@@ -7,7 +7,7 @@
     <parent>
         <groupId>dev.vality.geck</groupId>
         <artifactId>parent</artifactId>
-        <version>1.0.2</version>
+        <version>1.0.3</version>
         <relativePath>../pom.xml</relativePath>
     </parent>
 

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;
+    }
+}

filter/pom.xml

@@ -7,7 +7,7 @@
     <parent>
         <groupId>dev.vality.geck</groupId>
         <artifactId>parent</artifactId>
-        <version>1.0.2</version>
+        <version>1.0.3</version>
         <relativePath>../pom.xml</relativePath>
     </parent>
 

migrator/pom.xml

@@ -7,7 +7,7 @@
     <parent>
         <groupId>dev.vality.geck</groupId>
         <artifactId>parent</artifactId>
-        <version>1.0.2</version>
+        <version>1.0.3</version>
         <relativePath>../pom.xml</relativePath>
     </parent>
 

migrator/src/main/java/dev/vality/geck/migrator/kit/BaseMigrationManager.java

@@ -42,7 +42,7 @@ public <I, O> O migrate(I src, ThriftSpec thriftSpec, SerializerSpec<I, O> 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());
         }
 

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<MigrationPoint> migrationPoints;
+    /**
+     * Собирает простой упорядоченный маршрут миграции для теста.
+     * Этого достаточно, чтобы проверить chaining без лишнего проектного окружения.
+     */
+    private static class MigrationPointProviderStub implements MigrationPointProvider {
+        private final List<MigrationPoint> migrationPoints;
 
-        public MigrationPointProviderImpl(Collection<ThriftSpec> tSpecs) {
+        MigrationPointProviderStub(Collection<ThriftSpec> 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<String> {
-        private String spec;
+    /**
+     * Хранит тестовый идентификатор шага, например TEST1 или TEST2.
+     * Нужен, чтобы было видно, какие шаги реально применились и в каком порядке.
+     */
+    private static class TestMigrationSpec implements MigrationSpec<String> {
+        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 <I, O> O migrate(I data, MigrationPoint mPoint, SerializerSpec<I, O> serializerSpec) throws MigrationException {
@@ -83,4 +123,22 @@ public String getMigrationType() {
             return "TEST";
         }
     }
+
+    /**
+     * Дописывает к payload номер текущего шага миграции.
+     * Так видно, получают ли поздние шаги накопленный результат или снова исходный input.
+     */
+    private static class StepNumberAppendingMigrator implements Migrator {
+
+        @Override
+        public <I, O> O migrate(I data, MigrationPoint mPoint, SerializerSpec<I, O> serializerSpec) {
+            String stepNumber = ((TestMigrationSpec) mPoint.getMigrationSpec()).getSpec().replace("TEST", "");
+            return (O) (String.valueOf(data) + stepNumber);
+        }
+
+        @Override
+        public String getMigrationType() {
+            return "TEST";
+        }
+    }
 }

pom.xml

@@ -12,7 +12,7 @@
 
     <groupId>dev.vality.geck</groupId>
     <artifactId>parent</artifactId>
-    <version>1.0.2</version>
+    <version>1.0.3</version>
     <packaging>pom</packaging>
 
     <name>Geck</name>

serializer/pom.xml

@@ -7,7 +7,7 @@
     <parent>
         <groupId>dev.vality.geck</groupId>
         <artifactId>parent</artifactId>
-        <version>1.0.2</version>
+        <version>1.0.3</version>
         <relativePath>../pom.xml</relativePath>
     </parent>
 

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);
                 }
             }
         }

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<String, FieldHandler> fieldHandlers = new HashMap<>();
+    private final Map<String, FieldHandler> fieldHandlers = new HashMap<>();
+    private final ThreadLocal<Set<Class<? extends TBase>>> structTypesInProgress = new ThreadLocal<>();
 
     public MockTBaseProcessor() {
         this(MockMode.ALL);
@@ -52,15 +55,28 @@ public void addFieldHandler(FieldHandler handler, String... fieldNames) {
         }
     }
 
+    @Override
+    public <R> R process(TBase value, StructHandler<R> 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<? extends TBase> structClass = structMetaData.getStructClass();
+        Set<Class<? extends TBase>> 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<Class<? extends TBase>> currentStructTypes() {
+        Set<Class<? extends TBase>> currentStructTypes = structTypesInProgress.get();
+        if (currentStructTypes == null) {
+            throw new IllegalStateException("Mock generation context is not initialized");
+        }
+        return currentStructTypes;
+    }
+
 }

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;
         }
     }
+
 }