From a8038e5caf1c5a877b038ad2e31deba9cc91e702 Mon Sep 17 00:00:00 2001 From: Anatoly Karlov Date: Tue, 24 Mar 2026 17:15:09 +0700 Subject: [PATCH 1/4] Fix serializer recursion handling and bump version --- common/pom.xml | 2 +- filter/pom.xml | 2 +- migrator/pom.xml | 2 +- .../migrator/kit/BaseMigrationManager.java | 2 +- .../kit/TestBaseMigrationManager.java | 28 ++++ pom.xml | 2 +- serializer/pom.xml | 2 +- .../serializer/kit/json/JsonProcessor.java | 5 +- .../kit/mock/MockTBaseProcessor.java | 19 ++- .../kit/object/ObjectProcessor.java | 14 +- .../serializer/kit/tbase/TBaseProcessor.java | 65 +++++--- .../geck/serializer/kit/xml/XMLProcessor.java | 2 +- .../geck/serializer/kit/json/JsonTest.java | 14 ++ .../kit/mock/MockTBaseProcessorTest.java | 139 ++++++++++++++++ .../kit/object/ObjectProcessorTest.java | 23 +++ .../kit/tbase/TBaseProcessorTest.java | 157 ++++++++++++++++++ .../geck/serializer/kit/xml/XMLTest.java | 28 ++++ 17 files changed, 468 insertions(+), 38 deletions(-) create mode 100644 serializer/src/test/java/dev/vality/geck/serializer/kit/object/ObjectProcessorTest.java 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/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..7574816 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 @@ -25,6 +25,21 @@ public void test() throws MigrationException { Assert.assertEquals("A", res); } + @Test + public void shouldPassIntermediateMigrationResultToNextStep() throws MigrationException { + BaseMigrationStore migrationStore = new BaseMigrationStore(Arrays.asList(new MigrationPointProviderImpl(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 AccumulatingMigrator())); + + String res = migrationManager.migrate("A", new ThriftDef(1), new SerializerDef<>("TEST")); + + Assert.assertEquals("A123", res); + } + private static class MigrationPointProviderImpl implements MigrationPointProvider { private List migrationPoints; @@ -83,4 +98,17 @@ public String getMigrationType() { return "TEST"; } } + + private static class AccumulatingMigrator implements Migrator { + + @Override + public O migrate(I data, MigrationPoint mPoint, SerializerSpec serializerSpec) { + return (O) (String.valueOf(data) + ((MigrationSpecImpl) mPoint.getMigrationSpec()).getSpec().replace("TEST", "")); + } + + @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..ec56821 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 @@ -8,6 +8,8 @@ import org.apache.thrift.meta_data.*; import java.io.IOException; +import java.util.ArrayDeque; +import java.util.Deque; import java.util.HashMap; import java.util.Map; @@ -22,6 +24,7 @@ public class MockTBaseProcessor extends TBaseProcessor { private ValueGenerator valueGenerator; private Map fieldHandlers = new HashMap<>(); + private final Deque> structPath = new ArrayDeque<>(); public MockTBaseProcessor() { this(MockMode.ALL); @@ -56,7 +59,7 @@ public void addFieldHandler(FieldHandler handler, String... fieldNames) { 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)) { + if (fieldHandlers.containsKey(tFieldIdEnum.getFieldName())) { fieldHandlers.get(tFieldIdEnum.getFieldName()).handle(handler); } else { processFieldValue(fieldMetaData.valueMetaData, handler); @@ -70,7 +73,7 @@ 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)) { + if (fieldHandlers.containsKey(tFieldIdEnum.getFieldName())) { fieldHandlers.get(tFieldIdEnum.getFieldName()).handle(handler); } else { processFieldValue(fieldMetaData.valueMetaData, handler); @@ -130,11 +133,21 @@ protected void processFieldValue(FieldValueMetaData valueMetaData, StructHandler } private void processStruct(StructMetaData structMetaData, StructHandler handler) throws IOException { + Class structClass = structMetaData.getStructClass(); + if (structPath.contains(structClass)) { + throw new IllegalStateException(String.format( + "Recursive thrift type detected while generating mock for '%s'", + structClass.getName() + )); + } + structPath.push(structClass); try { - TBase tBase = structMetaData.getStructClass().newInstance(); + TBase tBase = structClass.newInstance(); super.processStruct(tBase, handler); } catch (InstantiationException | IllegalAccessException ex) { throw new IOException(ex); + } finally { + structPath.pop(); } } 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..2412398 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 @@ -41,9 +41,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 +85,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); @@ -115,7 +120,10 @@ private void processValue(Object value, StructType type, boolean named, StructHa } else if (value instanceof Boolean) { handler.value(((Boolean) value).booleanValue()); } else if (value instanceof ByteBuffer) { - handler.value(((ByteBuffer) value).array()); + ByteBuffer duplicate = ((ByteBuffer) value).duplicate(); + byte[] bytes = new byte[duplicate.remaining()]; + duplicate.get(bytes); + handler.value(bytes); } else if (value == null) { handler.nullValue(); } else { 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..a49e2d3 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 @@ -17,6 +17,7 @@ public class TBaseProcessor implements StructProcessor { private final boolean checkRequiredFields; + private final IdentityHashMap visitingStructs = new IdentityHashMap<>(); public TBaseProcessor() { this(true); @@ -38,34 +39,44 @@ public R process(TBase value, StructHandler handler) throws IOException { } 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)) { + if (visitingStructs.put(value, Boolean.TRUE) != null) { + 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); } 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); + } else { + processUnsetField(tFieldIdEnum, fieldMetaData, handler); + } } } - } - handler.endStruct(); + handler.endStruct(); + } finally { + visitingStructs.remove(value); + } } protected void processUnsetUnion(TUnion tUnion, StructHandler handler) throws IOException { @@ -120,7 +131,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(copyBinary((ByteBuffer) object)); } else { throw new IllegalStateException(String.format("Unknown binary type, type='%s'", object.getClass().getName())); } @@ -178,5 +189,11 @@ private void processMap(Map objectMap, MapMetaData metaData, StructHandler handl handler.endMap(); } + protected byte[] copyBinary(ByteBuffer buffer) { + ByteBuffer duplicate = buffer.duplicate(); + byte[] bytes = new byte[duplicate.remaining()]; + duplicate.get(bytes); + return bytes; + } } 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/kit/json/JsonTest.java b/serializer/src/test/java/dev/vality/geck/serializer/kit/json/JsonTest.java index c2c4dd2..d8c1358 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,10 @@ package dev.vality.geck.serializer.kit.json; import com.rbkmoney.damsel.v130.payment_processing.InvoicePaymentStarted; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; import dev.vality.geck.serializer.GeckTestUtil; +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; @@ -15,6 +18,8 @@ import java.io.IOException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + public class JsonTest { @Test @@ -60,4 +65,13 @@ public void testPretty() throws IOException { String json1 = new TBaseProcessor().process(testObject, handler).toString(); System.out.println(json1); } + + @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..c8f9082 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 @@ -4,19 +4,28 @@ import dev.vality.geck.serializer.domain.TestObject; import dev.vality.geck.serializer.kit.tbase.TBaseHandler; import dev.vality.geck.serializer.kit.tbase.ThriftType; +import org.apache.thrift.TException; import org.apache.thrift.TBase; import org.apache.thrift.TFieldIdEnum; import org.apache.thrift.TFieldRequirementType; import org.apache.thrift.TUnion; +import org.apache.thrift.meta_data.StructMetaData; import org.apache.thrift.meta_data.FieldMetaData; +import org.apache.thrift.protocol.TField; +import org.apache.thrift.protocol.TProtocol; +import org.apache.thrift.protocol.TStruct; +import org.apache.thrift.protocol.TType; import org.junit.Assert; import org.junit.Test; import java.io.IOException; +import java.util.Collections; +import java.util.EnumMap; import java.util.Map; import static junit.framework.Assert.assertNotNull; import static junit.framework.TestCase.*; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class MockTBaseProcessorTest { @@ -62,6 +71,14 @@ public void allFieldsTest() throws IOException { assertTrue(checkFields(testObject, false)); } + @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) { Map fieldMetaDataMap = tBase.getFieldMetaData(); @@ -89,4 +106,126 @@ public boolean checkFields(TBase tBase, boolean requiredOnly) { return check; } + public static class RecursiveStruct implements TBase { + private static final TStruct STRUCT_DESC = new TStruct("RecursiveStruct"); + private static final TField NEXT_FIELD_DESC = new TField("next", TType.STRUCT, (short) 1); + 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); + } + + public RecursiveStruct next; + + @Override + public void clear() { + next = null; + } + + @Override + public RecursiveStruct deepCopy() { + RecursiveStruct copy = new RecursiveStruct(); + copy.next = next; + return copy; + } + + @Override + public void setFieldValue(_Fields field, Object value) { + if (field == _Fields.NEXT) { + next = (RecursiveStruct) value; + } + } + + @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 boolean equals(Object other) { + return this == other; + } + + @Override + public int hashCode() { + return System.identityHashCode(this); + } + + @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 String toString() { + return STRUCT_DESC.name; + } + + @Override + public _Fields[] getFields() { + return _Fields.values(); + } + + @Override + public Map<_Fields, FieldMetaData> getFieldMetaData() { + return META_DATA_MAP; + } + + 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/kit/object/ObjectProcessorTest.java b/serializer/src/test/java/dev/vality/geck/serializer/kit/object/ObjectProcessorTest.java new file mode 100644 index 0000000..278d113 --- /dev/null +++ b/serializer/src/test/java/dev/vality/geck/serializer/kit/object/ObjectProcessorTest.java @@ -0,0 +1,23 @@ +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 { + + @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..bc2388a 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 @@ -3,11 +3,22 @@ import dev.vality.geck.serializer.domain.*; import dev.vality.geck.serializer.handler.HandlerStub; import dev.vality.geck.serializer.kit.mock.MockTBaseProcessor; +import dev.vality.geck.serializer.StructHandler; import org.junit.Assert; import org.junit.Test; +import org.apache.thrift.TBase; +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.TType; +import org.mockito.Mockito; import java.io.IOException; +import java.nio.ByteBuffer; import java.util.ArrayList; +import java.util.Collections; +import java.util.Map; import java.util.List; import static dev.vality.geck.serializer.GeckTestUtil.getTestObject; @@ -70,4 +81,150 @@ public void binaryUnknownTypeDataTest() throws IOException { .hasMessage("Unknown binary type, type='java.lang.Integer'"); } + @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.binaryValue); + } + + @Test + public void cyclicThriftGraphShouldFailFastInsteadOfStackOverflow() { + TBase first = Mockito.mock(TBase.class); + TBase second = Mockito.mock(TBase.class); + TFieldIdEnum firstField = mockStructField((short) 1, "next"); + TFieldIdEnum secondField = mockStructField((short) 1, "next"); + + Mockito.when(first.getFields()).thenReturn(new TFieldIdEnum[]{firstField}); + Mockito.when(first.getFieldMetaData()).thenReturn(singleStructFieldMeta(firstField, "next")); + Mockito.when(first.isSet(firstField)).thenReturn(true); + Mockito.when(first.getFieldValue(firstField)).thenReturn(second); + + Mockito.when(second.getFields()).thenReturn(new TFieldIdEnum[]{secondField}); + Mockito.when(second.getFieldMetaData()).thenReturn(singleStructFieldMeta(secondField, "next")); + Mockito.when(second.isSet(secondField)).thenReturn(true); + Mockito.when(second.getFieldValue(secondField)).thenReturn(first); + + assertThatThrownBy(() -> new TBaseProcessor().process(first, new HandlerStub())) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("Cyclic reference detected"); + } + + private static TFieldIdEnum mockStructField(short id, String name) { + TFieldIdEnum field = Mockito.mock(TFieldIdEnum.class); + Mockito.when(field.getThriftFieldId()).thenReturn(id); + Mockito.when(field.getFieldName()).thenReturn(name); + return field; + } + + private static Map singleStructFieldMeta(TFieldIdEnum field, String fieldName) { + return Collections.singletonMap( + field, + new FieldMetaData( + fieldName, + TFieldRequirementType.DEFAULT, + new StructMetaData(TType.STRUCT, TestObject.class) + ) + ); + } + + private static class BinaryCapturingHandler implements StructHandler { + private byte[] binaryValue; + + @Override + public void beginStruct(int size) { + } + + @Override + public void endStruct() { + } + + @Override + public void beginList(int size) { + } + + @Override + public void endList() { + } + + @Override + public void beginSet(int size) { + } + + @Override + public void endSet() { + } + + @Override + public void beginMap(int size) { + } + + @Override + public void endMap() { + } + + @Override + public void beginKey() { + } + + @Override + public void endKey() { + } + + @Override + public void beginValue() { + } + + @Override + public void endValue() { + } + + @Override + public void name(String name) { + } + + @Override + public void value(boolean value) { + } + + @Override + public void value(String value) { + } + + @Override + public void value(double value) { + } + + @Override + public void value(long value) { + } + + @Override + public void value(byte[] value) { + if (binaryValue == null) { + binaryValue = value; + } + } + + @Override + public void nullValue() { + } + + @Override + public byte[] getResult() { + return binaryValue; + } + } + } 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..af28780 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,6 +2,9 @@ import com.rbkmoney.damsel.v130.payment_processing.InvoicePaymentStarted; import dev.vality.geck.serializer.GeckTestUtil; +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; @@ -10,9 +13,15 @@ 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,23 @@ public void xmlKebabTest() throws Exception { new TBaseProcessor().process(invoice, handler); System.out.println(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"); + } } From bd980dae9a9a1299ec7af8aced1d386ebb39586b Mon Sep 17 00:00:00 2001 From: Anatoly Karlov Date: Thu, 26 Mar 2026 18:54:25 +0700 Subject: [PATCH 2/4] Simplify serializer recursion tests and helpers --- .../kit/mock/MockTBaseProcessor.java | 38 ++-- .../kit/object/ObjectProcessor.java | 21 +- .../serializer/kit/tbase/TBaseProcessor.java | 10 +- .../serializer/domain/RecursiveStruct.java | 131 ++++++++++++ .../handler/BinaryCapturingHandler.java | 18 ++ .../kit/mock/MockTBaseProcessorTest.java | 191 +++--------------- .../kit/tbase/TBaseProcessorTest.java | 137 +------------ 7 files changed, 227 insertions(+), 319 deletions(-) create mode 100644 serializer/src/test/java/dev/vality/geck/serializer/domain/RecursiveStruct.java create mode 100644 serializer/src/test/java/dev/vality/geck/serializer/handler/BinaryCapturingHandler.java 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 ec56821..d9deb32 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 @@ -8,10 +8,10 @@ import org.apache.thrift.meta_data.*; import java.io.IOException; -import java.util.ArrayDeque; -import java.util.Deque; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Set; public class MockTBaseProcessor extends TBaseProcessor { @@ -21,10 +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 Deque> structPath = new ArrayDeque<>(); + private final Map fieldHandlers = new HashMap<>(); + private final Set> structTypesInProgress = new HashSet<>(); public MockTBaseProcessor() { this(MockMode.ALL); @@ -59,11 +59,10 @@ public void addFieldHandler(FieldHandler handler, String... fieldNames) { protected void processUnsetField(TFieldIdEnum tFieldIdEnum, FieldMetaData fieldMetaData, StructHandler handler) throws IOException { if (needProcess(fieldMetaData)) { handler.name((byte) tFieldIdEnum.getThriftFieldId(), tFieldIdEnum.getFieldName()); - if (fieldHandlers.containsKey(tFieldIdEnum.getFieldName())) { - fieldHandlers.get(tFieldIdEnum.getFieldName()).handle(handler); - } else { - processFieldValue(fieldMetaData.valueMetaData, handler); + if (handleField(tFieldIdEnum, handler)) { + return; } + processFieldValue(fieldMetaData.valueMetaData, handler); } } @@ -73,11 +72,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(tFieldIdEnum.getFieldName())) { - 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 { @@ -134,20 +132,19 @@ protected void processFieldValue(FieldValueMetaData valueMetaData, StructHandler private void processStruct(StructMetaData structMetaData, StructHandler handler) throws IOException { Class structClass = structMetaData.getStructClass(); - if (structPath.contains(structClass)) { + if (!structTypesInProgress.add(structClass)) { throw new IllegalStateException(String.format( "Recursive thrift type detected while generating mock for '%s'", structClass.getName() )); } - structPath.push(structClass); try { TBase tBase = structClass.newInstance(); super.processStruct(tBase, handler); } catch (InstantiationException | IllegalAccessException ex) { throw new IOException(ex); } finally { - structPath.pop(); + structTypesInProgress.remove(structClass); } } @@ -193,4 +190,13 @@ 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; + } + } 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 2412398..8962b20 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 @@ -96,7 +96,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: @@ -112,18 +112,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) { - ByteBuffer duplicate = ((ByteBuffer) value).duplicate(); - byte[] bytes = new byte[duplicate.remaining()]; - duplicate.get(bytes); - handler.value(bytes); + handler.value(readBinary((ByteBuffer) value)); } else if (value == null) { handler.nullValue(); } else { @@ -183,4 +181,11 @@ private String unescapeString(String name) { return name; } } + + private byte[] readBinary(ByteBuffer buffer) { + ByteBuffer duplicate = buffer.duplicate(); + byte[] bytes = new byte[duplicate.remaining()]; + duplicate.get(bytes); + return bytes; + } } 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 a49e2d3..602b978 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 @@ -17,7 +17,7 @@ public class TBaseProcessor implements StructProcessor { private final boolean checkRequiredFields; - private final IdentityHashMap visitingStructs = new IdentityHashMap<>(); + private final Set structsInProgress = newIdentitySet(); public TBaseProcessor() { this(true); @@ -39,7 +39,7 @@ public R process(TBase value, StructHandler handler) throws IOException { } protected void processStruct(TBase value, StructHandler handler) throws IOException { - if (visitingStructs.put(value, Boolean.TRUE) != null) { + if (!structsInProgress.add(value)) { throw new IllegalStateException(String.format( "Cyclic reference detected while processing thrift struct '%s'", value.getClass().getName() @@ -75,7 +75,7 @@ protected void processStruct(TBase value, StructHandler handler) throws IOExcept handler.endStruct(); } finally { - visitingStructs.remove(value); + structsInProgress.remove(value); } } @@ -196,4 +196,8 @@ protected byte[] copyBinary(ByteBuffer buffer) { return bytes; } + private static Set newIdentitySet() { + return Collections.newSetFromMap(new IdentityHashMap<>()); + } + } 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..1cea137 --- /dev/null +++ b/serializer/src/test/java/dev/vality/geck/serializer/domain/RecursiveStruct.java @@ -0,0 +1,131 @@ +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; + +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; + } + + 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..d8791e6 --- /dev/null +++ b/serializer/src/test/java/dev/vality/geck/serializer/handler/BinaryCapturingHandler.java @@ -0,0 +1,18 @@ +package dev.vality.geck.serializer.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/mock/MockTBaseProcessorTest.java b/serializer/src/test/java/dev/vality/geck/serializer/kit/mock/MockTBaseProcessorTest.java index c8f9082..f45cb85 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,26 +1,19 @@ 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; -import org.apache.thrift.TException; import org.apache.thrift.TBase; import org.apache.thrift.TFieldIdEnum; import org.apache.thrift.TFieldRequirementType; import org.apache.thrift.TUnion; -import org.apache.thrift.meta_data.StructMetaData; import org.apache.thrift.meta_data.FieldMetaData; -import org.apache.thrift.protocol.TField; -import org.apache.thrift.protocol.TProtocol; -import org.apache.thrift.protocol.TStruct; -import org.apache.thrift.protocol.TType; import org.junit.Assert; import org.junit.Test; import java.io.IOException; -import java.util.Collections; -import java.util.EnumMap; import java.util.Map; import static junit.framework.Assert.assertNotNull; @@ -31,25 +24,25 @@ 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 @@ -58,17 +51,17 @@ 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)); } @Test @@ -79,153 +72,29 @@ public void recursiveSchemaShouldFailFastInsteadOfInfiniteMockGeneration() { .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; - } - - public static class RecursiveStruct implements TBase { - private static final TStruct STRUCT_DESC = new TStruct("RecursiveStruct"); - private static final TField NEXT_FIELD_DESC = new TField("next", TType.STRUCT, (short) 1); - 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); - } - - public RecursiveStruct next; - - @Override - public void clear() { - next = null; - } - - @Override - public RecursiveStruct deepCopy() { - RecursiveStruct copy = new RecursiveStruct(); - copy.next = next; - return copy; - } - - @Override - public void setFieldValue(_Fields field, Object value) { - if (field == _Fields.NEXT) { - next = (RecursiveStruct) value; - } - } - - @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 boolean equals(Object other) { - return this == other; - } - - @Override - public int hashCode() { - return System.identityHashCode(this); - } - - @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 String toString() { - return STRUCT_DESC.name; - } - - @Override - public _Fields[] getFields() { - return _Fields.values(); - } - - @Override - public Map<_Fields, FieldMetaData> getFieldMetaData() { - return META_DATA_MAP; - } - - 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; - } - } + return fieldsAreSet; } - } 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 bc2388a..cb8f0e1 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,24 +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 dev.vality.geck.serializer.StructHandler; import org.junit.Assert; import org.junit.Test; -import org.apache.thrift.TBase; -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.TType; -import org.mockito.Mockito; import java.io.IOException; import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Collections; -import java.util.Map; import java.util.List; import static dev.vality.geck.serializer.GeckTestUtil.getTestObject; @@ -96,135 +88,18 @@ public void binaryByteBufferShouldRespectPositionAndLimit() throws IOException { BinaryCapturingHandler handler = new BinaryCapturingHandler(); new TBaseProcessor().process(binaryTest, handler); - Assert.assertArrayEquals(new byte[]{1, 2}, handler.binaryValue); + Assert.assertArrayEquals(new byte[]{1, 2}, handler.getResult()); } @Test public void cyclicThriftGraphShouldFailFastInsteadOfStackOverflow() { - TBase first = Mockito.mock(TBase.class); - TBase second = Mockito.mock(TBase.class); - TFieldIdEnum firstField = mockStructField((short) 1, "next"); - TFieldIdEnum secondField = mockStructField((short) 1, "next"); - - Mockito.when(first.getFields()).thenReturn(new TFieldIdEnum[]{firstField}); - Mockito.when(first.getFieldMetaData()).thenReturn(singleStructFieldMeta(firstField, "next")); - Mockito.when(first.isSet(firstField)).thenReturn(true); - Mockito.when(first.getFieldValue(firstField)).thenReturn(second); - - Mockito.when(second.getFields()).thenReturn(new TFieldIdEnum[]{secondField}); - Mockito.when(second.getFieldMetaData()).thenReturn(singleStructFieldMeta(secondField, "next")); - Mockito.when(second.isSet(secondField)).thenReturn(true); - Mockito.when(second.getFieldValue(secondField)).thenReturn(first); + 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"); } - - private static TFieldIdEnum mockStructField(short id, String name) { - TFieldIdEnum field = Mockito.mock(TFieldIdEnum.class); - Mockito.when(field.getThriftFieldId()).thenReturn(id); - Mockito.when(field.getFieldName()).thenReturn(name); - return field; - } - - private static Map singleStructFieldMeta(TFieldIdEnum field, String fieldName) { - return Collections.singletonMap( - field, - new FieldMetaData( - fieldName, - TFieldRequirementType.DEFAULT, - new StructMetaData(TType.STRUCT, TestObject.class) - ) - ); - } - - private static class BinaryCapturingHandler implements StructHandler { - private byte[] binaryValue; - - @Override - public void beginStruct(int size) { - } - - @Override - public void endStruct() { - } - - @Override - public void beginList(int size) { - } - - @Override - public void endList() { - } - - @Override - public void beginSet(int size) { - } - - @Override - public void endSet() { - } - - @Override - public void beginMap(int size) { - } - - @Override - public void endMap() { - } - - @Override - public void beginKey() { - } - - @Override - public void endKey() { - } - - @Override - public void beginValue() { - } - - @Override - public void endValue() { - } - - @Override - public void name(String name) { - } - - @Override - public void value(boolean value) { - } - - @Override - public void value(String value) { - } - - @Override - public void value(double value) { - } - - @Override - public void value(long value) { - } - - @Override - public void value(byte[] value) { - if (binaryValue == null) { - binaryValue = value; - } - } - - @Override - public void nullValue() { - } - - @Override - public byte[] getResult() { - return binaryValue; - } - } - } From d810d35d340f5135f75fa9d1d845adbfc63c39bf Mon Sep 17 00:00:00 2001 From: Anatoly Karlov Date: Thu, 26 Mar 2026 22:43:32 +0700 Subject: [PATCH 3/4] review fixes --- .../kit/TestBaseMigrationManager.java | 46 +++++++++++-------- .../kit/object/ObjectProcessor.java | 6 +-- 2 files changed, 29 insertions(+), 23 deletions(-) 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 7574816..f4944bb 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 @@ -14,41 +14,46 @@ public class TestBaseMigrationManager { @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 MigrationPointProviderImpl(Arrays.asList( + 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 AccumulatingMigrator())); + new BaseMigrationManager(migrationStore, Arrays.asList(new StepNumberAppendingMigrator())); - String res = migrationManager.migrate("A", new ThriftDef(1), new SerializerDef<>("TEST")); + String result = migrationManager.migrate("A", new ThriftDef(1), new SerializerDef<>("TEST")); - Assert.assertEquals("A123", res); + Assert.assertEquals("A123", result); } - private static class MigrationPointProviderImpl implements MigrationPointProvider { - private List migrationPoints; + 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()); } @@ -68,10 +73,10 @@ public MigrationPoint getMappedSpec(ThriftSpec thriftSpec) throws MigrationExcep } } - private static class MigrationSpecImpl implements MigrationSpec { - private String spec; + private static class TestMigrationSpec implements MigrationSpec { + private final String spec; - public MigrationSpecImpl(String spec) { + TestMigrationSpec(String spec) { this.spec = spec; } @@ -86,7 +91,7 @@ public String getType() { } } - private static class MigratorImpl extends AbstractMigrator { + private static class PassThroughMigrator extends AbstractMigrator { @Override public O migrate(I data, MigrationPoint mPoint, SerializerSpec serializerSpec) throws MigrationException { @@ -99,11 +104,12 @@ public String getMigrationType() { } } - private static class AccumulatingMigrator implements Migrator { + private static class StepNumberAppendingMigrator implements Migrator { @Override public O migrate(I data, MigrationPoint mPoint, SerializerSpec serializerSpec) { - return (O) (String.valueOf(data) + ((MigrationSpecImpl) mPoint.getMigrationSpec()).getSpec().replace("TEST", "")); + String stepNumber = ((TestMigrationSpec) mPoint.getMigrationSpec()).getSpec().replace("TEST", ""); + return (O) (String.valueOf(data) + stepNumber); } @Override 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 8962b20..9f065ed 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 @@ -183,9 +183,9 @@ private String unescapeString(String name) { } private byte[] readBinary(ByteBuffer buffer) { - ByteBuffer duplicate = buffer.duplicate(); - byte[] bytes = new byte[duplicate.remaining()]; - duplicate.get(bytes); + ByteBuffer currentRange = buffer.slice(); + byte[] bytes = new byte[currentRange.remaining()]; + currentRange.get(bytes); return bytes; } } From 511ee5e9168928d1e6a0bcc756175498d108fb0b Mon Sep 17 00:00:00 2001 From: Anatoly Karlov Date: Fri, 27 Mar 2026 00:37:29 +0700 Subject: [PATCH 4/4] review fixes --- .../vality/geck/common/util/BinaryUtil.java | 16 ++++ .../kit/TestBaseMigrationManager.java | 24 ++++++ .../kit/mock/MockTBaseProcessor.java | 29 +++++++- .../kit/object/ObjectProcessor.java | 9 +-- .../serializer/kit/tbase/TBaseProcessor.java | 73 ++++++++++++------- .../serializer/domain/RecursiveStruct.java | 8 ++ .../handler/BinaryCapturingHandler.java | 4 + .../geck/serializer/kit/json/JsonTest.java | 8 +- .../kit/mock/MockTBaseProcessorTest.java | 4 + .../kit/object/ObjectProcessorTest.java | 4 + .../kit/tbase/TBaseProcessorTest.java | 8 ++ .../geck/serializer/kit/xml/XMLTest.java | 6 +- 12 files changed, 153 insertions(+), 40 deletions(-) create mode 100644 common/src/main/java/dev/vality/geck/common/util/BinaryUtil.java 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/migrator/src/test/java/dev/vality/geck/migrator/kit/TestBaseMigrationManager.java b/migrator/src/test/java/dev/vality/geck/migrator/kit/TestBaseMigrationManager.java index f4944bb..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,6 +13,10 @@ public class TestBaseMigrationManager { + /** + * Проверяет, что сама цепочка миграций не меняет данные. + * Если мигратор только перегоняет значение дальше, на выходе должен остаться исходный payload. + */ @Test public void shouldKeepSourceDataWhenMigratorDoesNotTransformIt() throws MigrationException { BaseMigrationStore migrationStore = @@ -29,6 +33,10 @@ public void shouldKeepSourceDataWhenMigratorDoesNotTransformIt() throws Migratio Assert.assertEquals("A", result); } + /** + * Проверяет, что каждый следующий шаг получает результат предыдущего шага. + * Ловит баг, при котором все шаги вызывались с исходным значением. + */ @Test public void shouldPassIntermediateMigrationResultToNextStep() throws MigrationException { BaseMigrationStore migrationStore = @@ -45,6 +53,10 @@ public void shouldPassIntermediateMigrationResultToNextStep() throws MigrationEx Assert.assertEquals("A123", result); } + /** + * Собирает простой упорядоченный маршрут миграции для теста. + * Этого достаточно, чтобы проверить chaining без лишнего проектного окружения. + */ private static class MigrationPointProviderStub implements MigrationPointProvider { private final List migrationPoints; @@ -73,6 +85,10 @@ public MigrationPoint getMappedSpec(ThriftSpec thriftSpec) throws MigrationExcep } } + /** + * Хранит тестовый идентификатор шага, например TEST1 или TEST2. + * Нужен, чтобы было видно, какие шаги реально применились и в каком порядке. + */ private static class TestMigrationSpec implements MigrationSpec { private final String spec; @@ -91,6 +107,10 @@ public String getType() { } } + /** + * Имитирует мигратор, который просто пропускает данные через сериализацию. + * Нужен, чтобы проверить поведение самого migration manager без дополнительных преобразований. + */ private static class PassThroughMigrator extends AbstractMigrator { @Override @@ -104,6 +124,10 @@ public String getMigrationType() { } } + /** + * Дописывает к payload номер текущего шага миграции. + * Так видно, получают ли поздние шаги накопленный результат или снова исходный input. + */ private static class StepNumberAppendingMigrator implements Migrator { @Override 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 d9deb32..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 @@ -24,7 +24,7 @@ public class MockTBaseProcessor extends TBaseProcessor { private final ValueGenerator valueGenerator; private final Map fieldHandlers = new HashMap<>(); - private final Set> structTypesInProgress = new HashSet<>(); + private final ThreadLocal>> structTypesInProgress = new ThreadLocal<>(); public MockTBaseProcessor() { this(MockMode.ALL); @@ -55,6 +55,20 @@ 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)) { @@ -132,7 +146,8 @@ protected void processFieldValue(FieldValueMetaData valueMetaData, StructHandler private void processStruct(StructMetaData structMetaData, StructHandler handler) throws IOException { Class structClass = structMetaData.getStructClass(); - if (!structTypesInProgress.add(structClass)) { + Set> currentStructTypes = currentStructTypes(); + if (!currentStructTypes.add(structClass)) { throw new IllegalStateException(String.format( "Recursive thrift type detected while generating mock for '%s'", structClass.getName() @@ -144,7 +159,7 @@ private void processStruct(StructMetaData structMetaData, StructHandler handler) } catch (InstantiationException | IllegalAccessException ex) { throw new IOException(ex); } finally { - structTypesInProgress.remove(structClass); + currentStructTypes.remove(structClass); } } @@ -199,4 +214,12 @@ private boolean handleField(TFieldIdEnum field, StructHandler handler) throws IO 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 9f065ed..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; @@ -121,7 +122,7 @@ private void processValue(Object value, StructType type, boolean named, StructHa } else if (value instanceof Boolean) { handler.value((Boolean) value); } else if (value instanceof ByteBuffer) { - handler.value(readBinary((ByteBuffer) value)); + handler.value(BinaryUtil.toByteArray((ByteBuffer) value)); } else if (value == null) { handler.nullValue(); } else { @@ -182,10 +183,4 @@ private String unescapeString(String name) { } } - private byte[] readBinary(ByteBuffer buffer) { - ByteBuffer currentRange = buffer.slice(); - byte[] bytes = new byte[currentRange.remaining()]; - currentRange.get(bytes); - return bytes; - } } 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 602b978..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; @@ -17,7 +18,6 @@ public class TBaseProcessor implements StructProcessor { private final boolean checkRequiredFields; - private final Set structsInProgress = newIdentitySet(); public TBaseProcessor() { this(true); @@ -32,13 +32,17 @@ 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 { + 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'", @@ -57,7 +61,8 @@ protected void processStruct(TBase value, StructHandler handler) throws IOExcept if (union.isSet()) { TFieldIdEnum tFieldIdEnum = union.getSetField(); handler.name((byte) tFieldIdEnum.getThriftFieldId(), tFieldIdEnum.getFieldName()); - process(union.getFieldValue(), fieldMetaDataMap.get(tFieldIdEnum).valueMetaData, handler); + process(union.getFieldValue(), fieldMetaDataMap.get(tFieldIdEnum).valueMetaData, handler, + structsInProgress); } else { processUnsetUnion(union, handler); } @@ -66,7 +71,8 @@ protected void processStruct(TBase value, StructHandler handler) throws IOExcept FieldMetaData fieldMetaData = fieldMetaDataMap.get(tFieldIdEnum); if (value.isSet(tFieldIdEnum)) { handler.name((byte) tFieldIdEnum.getThriftFieldId(), tFieldIdEnum.getFieldName()); - process(value.getFieldValue(tFieldIdEnum), fieldMetaData.valueMetaData, handler); + process(value.getFieldValue(tFieldIdEnum), fieldMetaData.valueMetaData, handler, + structsInProgress); } else { processUnsetField(tFieldIdEnum, fieldMetaData, handler); } @@ -89,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(); } @@ -131,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(copyBinary((ByteBuffer) object)); + handler.value(BinaryUtil.toByteArray((ByteBuffer) object)); } else { throw new IllegalStateException(String.format("Unknown binary type, type='%s'", object.getClass().getName())); } @@ -139,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)); @@ -158,44 +168,53 @@ 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(); } - protected byte[] copyBinary(ByteBuffer buffer) { - ByteBuffer duplicate = buffer.duplicate(); - byte[] bytes = new byte[duplicate.remaining()]; - duplicate.get(bytes); - return bytes; - } - private static Set newIdentitySet() { return Collections.newSetFromMap(new IdentityHashMap<>()); } 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 index 1cea137..4409f80 100644 --- a/serializer/src/test/java/dev/vality/geck/serializer/domain/RecursiveStruct.java +++ b/serializer/src/test/java/dev/vality/geck/serializer/domain/RecursiveStruct.java @@ -13,6 +13,10 @@ import java.util.EnumMap; import java.util.Map; +/** + * Минимальная рекурсивная thrift-подобная структура только для тестов. + * Нужна, чтобы собрать цикл в ссылках без зависимости от сгенерированных моделей проекта. + */ public class RecursiveStruct implements TBase { private static final Map<_Fields, FieldMetaData> META_DATA_MAP; @@ -103,6 +107,10 @@ public Map<_Fields, FieldMetaData> getFieldMetaData() { return META_DATA_MAP; } + /** + * Описывает единственное рекурсивное поле тестовой структуры. + * TBase требует такой enum, чтобы выдавать id и имя поля так же, как обычный thrift-класс. + */ public enum _Fields implements TFieldIdEnum { NEXT((short) 1, "next"); 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 index d8791e6..dc8dc39 100644 --- a/serializer/src/test/java/dev/vality/geck/serializer/handler/BinaryCapturingHandler.java +++ b/serializer/src/test/java/dev/vality/geck/serializer/handler/BinaryCapturingHandler.java @@ -1,5 +1,9 @@ package dev.vality.geck.serializer.handler; +/** + * Тестовый handler, который запоминает первое бинарное значение. + * Нужен, чтобы проверить, какие байты процессор реально отдал в результат. + */ public class BinaryCapturingHandler extends HandlerStub { private byte[] 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 d8c1358..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,8 +1,9 @@ package dev.vality.geck.serializer.kit.json; -import com.rbkmoney.damsel.v130.payment_processing.InvoicePaymentStarted; 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; @@ -12,7 +13,6 @@ 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; @@ -66,6 +66,10 @@ public void testPretty() throws IOException { System.out.println(json1); } + /** + * Проверяет, что JSON с неподдерживаемым маркером массива падает сразу. + * Без этого парсер уходил дальше с некорректным типом узла и ломался менее явно. + */ @Test public void unknownArrayTypeShouldFailFast() { assertThatThrownBy(() -> new JsonProcessor().process( 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 f45cb85..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 @@ -64,6 +64,10 @@ public void allFieldsTest() throws IOException { assertTrue(hasExpectedFields(result, false)); } + /** + * Проверяет, что mock-генерация останавливается на рекурсивном thrift-типе. + * Без этой защиты генератор продолжал создавать вложенные объекты, пока не заканчивался стек. + */ @Test public void recursiveSchemaShouldFailFastInsteadOfInfiniteMockGeneration() { assertThatThrownBy(() -> new MockTBaseProcessor(MockMode.ALL) 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 index 278d113..50188d1 100644 --- 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 @@ -11,6 +11,10 @@ public class ObjectProcessorTest { + /** + * Проверяет, что set не меняется после сериализации и обратного чтения. + * Ловит баг, при котором после обработки set код проваливался в следующую ветку switch. + */ @Test public void shouldRoundTripSetsWithoutFallingThroughToOtherBranch() throws IOException { SetTest source = new MockTBaseProcessor().process(new SetTest(), new TBaseHandler<>(SetTest.class)); 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 cb8f0e1..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 @@ -73,6 +73,10 @@ 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(); @@ -91,6 +95,10 @@ public void binaryByteBufferShouldRespectPositionAndLimit() throws IOException { Assert.assertArrayEquals(new byte[]{1, 2}, handler.getResult()); } + /** + * Проверяет, что циклический граф объектов падает с понятной ошибкой, а не со stack overflow. + * Процессор умеет сериализовать дерево, но должен остановиться на петле в ссылках. + */ @Test public void cyclicThriftGraphShouldFailFastInsteadOfStackOverflow() { RecursiveStruct first = new RecursiveStruct(); 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 af28780..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,6 +2,7 @@ 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; @@ -10,7 +11,6 @@ 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; @@ -42,6 +42,10 @@ public void xmlKebabTest() throws Exception { System.out.println(xml); } + /** + * Проверяет, что XML с неподдерживаемым типом узла падает сразу. + * Без этого некорректный XML проходил глубже в парсер и ломался уже в полусобранном состоянии. + */ @Test public void unknownNodeTypeShouldFailFast() throws Exception { Document document = DocumentBuilderFactory.newInstance()