From da32de01c1606a9a072c63a6e0370244dd4d23fc Mon Sep 17 00:00:00 2001 From: Zach Bray Date: Thu, 19 Oct 2023 16:49:33 +0100 Subject: [PATCH] [C#] Extend tests to cover extended schemas. These tests showed some deficiencies in the DTO generation. For example, added variable-length data was not represented as nullable nor properly handled in `EncodeInto(...)`. During this work, I noticed composite "field" tokens (i.e., types) take their `token.version()` from their containing message/group field. I had to adjust some code that was using `token.version() > 0` to determine whether a field had been added, as this only works with message/group fields. --- .../generation/csharp/CSharpDtoGenerator.java | 87 ++++++++++++++----- .../properties/CSharpDtosPropertyTest.java | 11 +++ .../arbitraries/SbeArbitraries.java | 55 +++++++++--- .../sbe/properties/schema/FieldSchema.java | 11 ++- .../sbe/properties/schema/GroupSchema.java | 10 ++- .../sbe/properties/schema/MessageSchema.java | 35 +++++++- .../schema/TestXmlSchemaWriter.java | 3 + .../sbe/properties/schema/VarDataSchema.java | 10 ++- 8 files changed, 184 insertions(+), 38 deletions(-) diff --git a/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpDtoGenerator.java b/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpDtoGenerator.java index d50030351a..9335295663 100644 --- a/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpDtoGenerator.java +++ b/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpDtoGenerator.java @@ -93,8 +93,8 @@ public void generate() throws IOException collectVarData(messageBody, offset, varData); generateVarData(sb, ctorArgs, varData, BASE_INDENT + INDENT); - generateDecodeFrom(sb, className, codecClassName, fields, groups, varData, BASE_INDENT + INDENT); - generateEncodeInto(sb, codecClassName, fields, groups, varData, BASE_INDENT + INDENT); + generateMessageDecodeFrom(sb, className, codecClassName, fields, groups, varData, BASE_INDENT + INDENT); + generateMessageEncodeInto(sb, codecClassName, fields, groups, varData, BASE_INDENT + INDENT); generateDisplay(sb, codecClassName, "WrapForEncode", null, BASE_INDENT + INDENT); removeTrailingComma(ctorArgs); @@ -119,6 +119,12 @@ public void generate() throws IOException } } + private enum DefinitionKind + { + Composite, + Message + } + private void generateGroups( final StringBuilder sb, final StringBuilder ctorArgs, @@ -174,9 +180,9 @@ private void generateGroups( generateDecodeListFrom( groupRecordBody, groupClassName, qualifiedCodecClassName, indent + INDENT); - generateDecodeFrom( + generateMessageDecodeFrom( groupRecordBody, groupClassName, qualifiedCodecClassName, fields, groups, varData, indent + INDENT); - generateEncodeInto( + generateMessageEncodeInto( groupRecordBody, qualifiedCodecClassName, fields, groups, varData, indent + INDENT); removeTrailingComma(groupCtorArgs); @@ -210,7 +216,8 @@ private void generateCompositeDecodeFrom( { final Token token = tokens.get(i); - generateFieldDecodeFrom(sb, token, token, codecClassName, indent + INDENT + INDENT); + generateFieldDecodeFrom( + sb, DefinitionKind.Composite, token, token, codecClassName, indent + INDENT + INDENT); i += tokens.get(i).componentTokenCount(); } @@ -270,7 +277,7 @@ private void generateDecodeListFrom( .append(indent).append("}\n"); } - private void generateDecodeFrom( + private void generateMessageDecodeFrom( final StringBuilder sb, final String dtoClassName, final String codecClassName, @@ -285,7 +292,7 @@ private void generateDecodeFrom( .append(indent).append("{\n"); sb.append(indent).append(INDENT).append("return new ").append(dtoClassName).append("(\n"); - generateFieldsDecodeFrom(sb, fields, codecClassName, indent + INDENT + INDENT); + generateMessageFieldsDecodeFrom(sb, fields, codecClassName, indent + INDENT + INDENT); generateGroupsDecodeFrom(sb, groups, indent + INDENT + INDENT); generateVarDataDecodeFrom(sb, varData, indent + INDENT + INDENT); removeTrailingComma(sb); @@ -294,7 +301,7 @@ private void generateDecodeFrom( sb.append(indent).append("}\n"); } - private void generateFieldsDecodeFrom( + private void generateMessageFieldsDecodeFrom( final StringBuilder sb, final List tokens, final String codecClassName, @@ -307,35 +314,37 @@ private void generateFieldsDecodeFrom( { final Token encodingToken = tokens.get(i + 1); - generateFieldDecodeFrom(sb, signalToken, encodingToken, codecClassName, indent); + generateFieldDecodeFrom(sb, DefinitionKind.Message, signalToken, encodingToken, codecClassName, indent); } } } private void generateFieldDecodeFrom( final StringBuilder sb, + final DefinitionKind rootKind, final Token fieldToken, final Token typeToken, - final String codecClassName, final String indent) + final String codecClassName, + final String indent) { switch (typeToken.signal()) { case ENCODING: - generatePrimitiveDecodeFrom(sb, fieldToken, typeToken, codecClassName, indent); + generatePrimitiveDecodeFrom(sb, rootKind, fieldToken, typeToken, codecClassName, indent); break; case BEGIN_SET: - generatePropertyDecodeFrom(sb, fieldToken, "0", null, indent); + generatePropertyDecodeFrom(sb, rootKind, fieldToken, "0", null, indent); break; case BEGIN_ENUM: final String enumName = formatClassName(typeToken.applicableTypeName()); final String nullValue = formatNamespace(ir.packageName()) + "." + enumName + ".NULL_VALUE"; - generatePropertyDecodeFrom(sb, fieldToken, nullValue, null, indent); + generatePropertyDecodeFrom(sb, rootKind, fieldToken, nullValue, null, indent); break; case BEGIN_COMPOSITE: - generateComplexDecodeFrom(sb, fieldToken, typeToken, indent); + generateComplexDecodeFrom(sb, rootKind, fieldToken, typeToken, indent); break; default: @@ -345,6 +354,7 @@ private void generateFieldDecodeFrom( private void generatePrimitiveDecodeFrom( final StringBuilder sb, + final DefinitionKind rootKind, final Token fieldToken, final Token typeToken, final String codecClassName, @@ -360,16 +370,17 @@ private void generatePrimitiveDecodeFrom( if (arrayLength == 1) { final String codecNullValue = codecClassName + "." + formatPropertyName(fieldToken.name()) + "NullValue"; - generatePropertyDecodeFrom(sb, fieldToken, "null", codecNullValue, indent); + generatePropertyDecodeFrom(sb, rootKind, fieldToken, "null", codecNullValue, indent); } else if (arrayLength > 1) { - generateArrayDecodeFrom(sb, fieldToken, typeToken, indent); + generateArrayDecodeFrom(sb, rootKind, fieldToken, typeToken, indent); } } private void generateArrayDecodeFrom( final StringBuilder sb, + final DefinitionKind rootKind, final Token fieldToken, final Token typeToken, final String indent) @@ -386,6 +397,7 @@ private void generateArrayDecodeFrom( { generateRecordPropertyAssignment( sb, + rootKind, fieldToken, indent, "codec.Get" + formattedPropertyName + "()", @@ -397,6 +409,7 @@ private void generateArrayDecodeFrom( { generateRecordPropertyAssignment( sb, + rootKind, fieldToken, indent, "codec." + formattedPropertyName + "AsSpan().ToArray()", @@ -408,6 +421,7 @@ private void generateArrayDecodeFrom( private void generatePropertyDecodeFrom( final StringBuilder sb, + final DefinitionKind rootKind, final Token fieldToken, final String dtoNullValue, final String codecNullValue, @@ -423,6 +437,7 @@ private void generatePropertyDecodeFrom( generateRecordPropertyAssignment( sb, + rootKind, fieldToken, indent, "codec." + formattedPropertyName, @@ -433,6 +448,7 @@ private void generatePropertyDecodeFrom( private void generateComplexDecodeFrom( final StringBuilder sb, + final DefinitionKind rootKind, final Token fieldToken, final Token typeToken, final String indent) @@ -443,6 +459,7 @@ private void generateComplexDecodeFrom( generateRecordPropertyAssignment( sb, + rootKind, fieldToken, indent, dtoClassName + ".DecodeFrom(codec." + formattedPropertyName + ")", @@ -469,6 +486,7 @@ private void generateGroupsDecodeFrom( generateRecordPropertyAssignment( sb, + DefinitionKind.Message, groupToken, indent, groupDtoClassName + ".DecodeListFrom(codec." + formattedPropertyName + ")", @@ -511,6 +529,7 @@ private void generateVarDataDecodeFrom( generateRecordPropertyAssignment( sb, + DefinitionKind.Message, token, indent, "codec." + accessor + "()", @@ -523,6 +542,7 @@ private void generateVarDataDecodeFrom( private void generateRecordPropertyAssignment( final StringBuilder sb, + final DefinitionKind rootKind, final Token token, final String indent, final String presentExpression, @@ -534,8 +554,20 @@ private void generateRecordPropertyAssignment( sb.append(indent).append(formattedPropertyName).append(": "); - if (token.version() > 0) + // N.B., in the IR, the composite information is "embedded" into each message/group field. + // When we are looking at a composite "field" (i.e., a type) the `token.version()` method + // returns the "sinceVersion" of the containing field. Therefore, we cannot decide presence + // based on `token.version()` when dealing with composites. + if (rootKind.equals(DefinitionKind.Message) && token.version() > 0) { + if (token.signal() != Signal.BEGIN_VAR_DATA && !token.isOptionalEncoding()) + { + throw new IllegalStateException( + "Expected added field " + propertyName + + " (sinceVersion=" + token.version() + ") to have optional presence." + ); + } + sb.append("codec.").append(formattedPropertyName).append("InActingVersion()"); if (null != nullCodecValueOrNull) @@ -553,7 +585,7 @@ private void generateRecordPropertyAssignment( } } - private void generateEncodeInto( + private void generateMessageEncodeInto( final StringBuilder sb, final String codecClassName, final List fields, @@ -674,7 +706,7 @@ private String nullableConvertedExpression( final String expression, final String nullValue) { - return fieldToken.version() > 0 || fieldToken.isOptionalEncoding() ? + return fieldToken.isOptionalEncoding() ? expression + " ?? " + nullValue : expression; } @@ -791,9 +823,19 @@ private void generateVarDataEncodeInto( if (token.signal() == Signal.BEGIN_VAR_DATA) { final String propertyName = token.name(); + final String formattedPropertyName = formatPropertyName(propertyName); + + if (token.version() > 0) + { + sb.append(indent).append("if (null == ").append(formattedPropertyName).append(")\n") + .append(indent).append("{\n") + .append(indent).append(INDENT).append("throw new System.InvalidOperationException(\"") + .append(formattedPropertyName).append(" must not be null.\");\n") + .append(indent).append("}\n\n"); + } - sb.append(indent).append("codec.Set").append(formatPropertyName(propertyName)) - .append("(").append(formatPropertyName(propertyName)).append(");\n"); + sb.append(indent).append("codec.Set").append(formattedPropertyName) + .append("(").append(formattedPropertyName).append(");\n"); } } } @@ -1181,7 +1223,8 @@ private void generateVarData( final String propertyName = token.name(); final Token varDataToken = Generators.findFirst("varData", tokens, i); final String characterEncoding = varDataToken.encoding().characterEncoding(); - final String dtoType = characterEncoding == null ? "byte[]" : "string"; + final String nullableSuffix = token.version() > 0 ? "?" : ""; + final String dtoType = (characterEncoding == null ? "byte[]" : "string") + nullableSuffix; final String formattedPropertyName = formatPropertyName(propertyName); diff --git a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/CSharpDtosPropertyTest.java b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/CSharpDtosPropertyTest.java index 7a4dde3f49..8a8be0fdde 100644 --- a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/CSharpDtosPropertyTest.java +++ b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/CSharpDtosPropertyTest.java @@ -89,6 +89,17 @@ void dtoEncodeShouldBeTheInverseOfDtoDecode( "SCHEMA:\n" + encodedMessage.schema()); } + final byte[] errorBytes = Files.readAllBytes(stderr); + if (errorBytes.length != 0) + { + throw new AssertionError( + "Process wrote to stderr.\n\n" + + "STDOUT:\n" + new String(Files.readAllBytes(stdout)) + "\n\n" + + "STDERR:\n" + new String(errorBytes) + "\n\n" + + "SCHEMA:\n" + encodedMessage.schema() + "\n\n" + ); + } + final byte[] inputBytes = new byte[encodedMessage.length()]; encodedMessage.buffer().getBytes(0, inputBytes); final byte[] outputBytes = Files.readAllBytes(tempDir.resolve("output.dat")); diff --git a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/arbitraries/SbeArbitraries.java b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/arbitraries/SbeArbitraries.java index b1b025d4d9..13f027fb30 100644 --- a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/arbitraries/SbeArbitraries.java +++ b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/arbitraries/SbeArbitraries.java @@ -192,6 +192,37 @@ private static Arbitrary typeSchema(final int depth) } } + private static Arbitrary addedField() + { + return Combinators.combine( + typeSchema(MAX_COMPOSITE_DEPTH), + Arbitraries.of(Encoding.Presence.OPTIONAL), + Arbitraries.of((short)1, (short)2) + ).as(FieldSchema::new); + } + + private static Arbitrary originalField() + { + return Combinators.combine( + typeSchema(MAX_COMPOSITE_DEPTH), + presence(), + Arbitraries.of((short)0) + ).as(FieldSchema::new); + } + + private static Arbitrary skewedFieldDistribution() + { + final Arbitrary originalField = originalField(); + final Arbitrary addedField = addedField(); + + return Arbitraries.oneOf( + originalField, + originalField, + originalField, + addedField + ); + } + private static Arbitrary groupSchema(final int depth) { final Arbitrary> subGroups = depth == 1 ? @@ -201,10 +232,7 @@ private static Arbitrary groupSchema(final int depth) return Combinators.combine( withDuplicates( 2, - Combinators.combine( - typeSchema(MAX_COMPOSITE_DEPTH), - presence() - ).as(FieldSchema::new).list().ofMaxSize(5) + skewedFieldDistribution().list().ofMaxSize(5) ), subGroups, varDataSchema().list().ofMaxSize(3) @@ -227,6 +255,13 @@ private static Arbitrary varDataSchema() PrimitiveType.UINT8, PrimitiveType.UINT16, PrimitiveType.UINT32 + ), + Arbitraries.of( + (short)0, + (short)0, + (short)0, + (short)1, + (short)2 ) ).as(VarDataSchema::new); } @@ -236,10 +271,7 @@ public static Arbitrary messageSchema() return Combinators.combine( withDuplicates( 3, - Combinators.combine( - typeSchema(MAX_COMPOSITE_DEPTH), - presence() - ).as(FieldSchema::new).list().ofMaxSize(10) + skewedFieldDistribution().list().ofMaxSize(10) ), groupSchema(MAX_GROUP_DEPTH).list().ofMaxSize(3), varDataSchema().list().ofMaxSize(3) @@ -747,7 +779,7 @@ public static Arbitrary messageValueEncoder( buffer.putShort(0, (short)blockLength, ir.byteOrder()); buffer.putShort(2, messageId, ir.byteOrder()); buffer.putShort(4, (short)ir.id(), ir.byteOrder()); - buffer.putShort(6, (short)0, ir.byteOrder()); + buffer.putShort(6, (short)ir.version(), ir.byteOrder()); final int headerLength = 8; fields.encode(buffer, offset + headerLength, null); limit.set(offset + headerLength + blockLength); @@ -821,7 +853,10 @@ public static Arbitrary encodedMessage(final CharGenerationMode } catch (final Exception e) { - throw new RuntimeException(e); + throw new AssertionError( + "Failed to generate encoded value for schema.\n\n" + + "SCHEMA:\n" + xml, + e); } }).withoutEdgeCases(); } diff --git a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/FieldSchema.java b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/FieldSchema.java index 406ee65cdd..c1fad8abc4 100644 --- a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/FieldSchema.java +++ b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/FieldSchema.java @@ -22,14 +22,18 @@ public final class FieldSchema { private final TypeSchema type; private final Encoding.Presence presence; + private final short sinceVersion; public FieldSchema( final TypeSchema type, - final Encoding.Presence presence + final Encoding.Presence presence, + final short sinceVersion ) { + assert sinceVersion == 0 || presence.equals(Encoding.Presence.OPTIONAL); this.type = type; this.presence = presence; + this.sinceVersion = sinceVersion; } public TypeSchema type() @@ -41,4 +45,9 @@ public Encoding.Presence presence() { return presence; } + + public short sinceVersion() + { + return sinceVersion; + } } diff --git a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/GroupSchema.java b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/GroupSchema.java index 53c3c81df7..47cfe343a7 100644 --- a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/GroupSchema.java +++ b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/GroupSchema.java @@ -16,7 +16,9 @@ package uk.co.real_logic.sbe.properties.schema; +import java.util.Comparator; import java.util.List; +import java.util.stream.Collectors; public final class GroupSchema { @@ -29,9 +31,13 @@ public GroupSchema( final List groups, final List varData) { - this.blockFields = blockFields; + this.blockFields = blockFields.stream() + .sorted(Comparator.comparing(FieldSchema::sinceVersion)) + .collect(Collectors.toList()); this.groups = groups; - this.varData = varData; + this.varData = varData.stream() + .sorted(Comparator.comparing(VarDataSchema::sinceVersion)) + .collect(Collectors.toList()); } public List blockFields() diff --git a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/MessageSchema.java b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/MessageSchema.java index a0decb2d6b..2095628c39 100644 --- a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/MessageSchema.java +++ b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/MessageSchema.java @@ -16,13 +16,16 @@ package uk.co.real_logic.sbe.properties.schema; +import java.util.Comparator; import java.util.List; +import java.util.stream.Collectors; public final class MessageSchema { private final List blockFields; private final List groups; private final List varData; + private final short version; public MessageSchema( final List blockFields, @@ -30,9 +33,14 @@ public MessageSchema( final List varData ) { - this.blockFields = blockFields; + this.blockFields = blockFields.stream() + .sorted(Comparator.comparing(FieldSchema::sinceVersion)) + .collect(Collectors.toList()); this.groups = groups; - this.varData = varData; + this.varData = varData.stream() + .sorted(Comparator.comparing(VarDataSchema::sinceVersion)) + .collect(Collectors.toList()); + this.version = findMaxVersion(blockFields, groups, varData); } public short schemaId() @@ -45,6 +53,11 @@ public short templateId() return 1; } + public short version() + { + return version; + } + public List blockFields() { return blockFields; @@ -59,4 +72,22 @@ public List varData() { return varData; } + + private static short findMaxVersion( + final List fields, + final List groups, + final List varData + ) + { + final int maxFieldVersion = fields.stream() + .mapToInt(FieldSchema::sinceVersion) + .max().orElse(0); + final int maxGroupVersion = groups.stream() + .mapToInt(group -> findMaxVersion(group.blockFields(), group.groups(), group.varData())) + .max().orElse(0); + final int maxVarDataVersion = varData.stream() + .mapToInt(VarDataSchema::sinceVersion) + .max().orElse(0); + return (short)Math.max(maxFieldVersion, Math.max(maxGroupVersion, maxVarDataVersion)); + } } diff --git a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/TestXmlSchemaWriter.java b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/TestXmlSchemaWriter.java index 7ccd273017..3c43228099 100644 --- a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/TestXmlSchemaWriter.java +++ b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/TestXmlSchemaWriter.java @@ -70,6 +70,7 @@ private static void writeTo( final Element root = document.createElementNS("http://fixprotocol.io/2016/sbe", "sbe:messageSchema"); root.setAttribute("id", Short.toString(schema.schemaId())); root.setAttribute("package", "uk.co.real_logic.sbe.properties"); + root.setAttribute("version", Short.toString(schema.version())); document.appendChild(root); final Element topLevelTypes = createTypesElement(document); @@ -152,6 +153,7 @@ private static void appendMembers( element.setAttribute("name", "member" + id); element.setAttribute("type", typeName); element.setAttribute("presence", field.presence().name().toLowerCase()); + element.setAttribute("sinceVersion", Short.toString(field.sinceVersion())); parent.appendChild(element); } @@ -180,6 +182,7 @@ private static void appendMembers( element.setAttribute("id", Integer.toString(id)); element.setAttribute("name", "member" + id); element.setAttribute("type", requireNonNull(typeToName.get(data))); + element.setAttribute("sinceVersion", Short.toString(data.sinceVersion())); parent.appendChild(element); } } diff --git a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/VarDataSchema.java b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/VarDataSchema.java index b5e98c02c7..db3d5900a1 100644 --- a/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/VarDataSchema.java +++ b/sbe-tool/src/propertyTest/java/uk/co/real_logic/sbe/properties/schema/VarDataSchema.java @@ -22,13 +22,16 @@ public final class VarDataSchema { private final Encoding dataEncoding; private final PrimitiveType lengthEncoding; + private final short sinceVersion; public VarDataSchema( final Encoding dataEncoding, - final PrimitiveType lengthEncoding) + final PrimitiveType lengthEncoding, + final short sinceVersion) { this.dataEncoding = dataEncoding; this.lengthEncoding = lengthEncoding; + this.sinceVersion = sinceVersion; } public Encoding dataEncoding() @@ -41,6 +44,11 @@ public PrimitiveType lengthEncoding() return lengthEncoding; } + public short sinceVersion() + { + return sinceVersion; + } + public enum Encoding { ASCII,