[Java] 1.6.0 -> 1.6.1 Backwards compatibility change when decoding nested groups

[tom-smalls]

Hi,

When upgrading to SBE 1.6.1 from 1.6.0, two of our backwards compatibility tests failed.

Looking into this in more detail, we believe it is to do with commit 6852773

Given version 1 of a schema

<?xml version="1.0" encoding="US-ASCII" standalone="yes"?>
<messageSchema package="com.transficc.sbecompatibility.test2.protocol.v1"
               id="0"
               version="1"
               semanticVersion="1.0"
               description="Unit Test For Compatibility"
               byteOrder="littleEndian">
    <types>
        <composite name="messageHeader" description="Message identifiers and length of message root">
            <type name="blockLength" primitiveType="uint16"/>
            <type name="templateId" primitiveType="uint16"/>
            <type name="schemaId" primitiveType="uint16"/>
            <type name="version" primitiveType="uint16"/>
        </composite>
        <composite name="groupSizeEncoding" description="Repeating group dimensions">
            <type name="blockLength" primitiveType="uint16"/>
            <type name="numInGroup" primitiveType="uint8" semanticType="NumInGroup"/>
        </composite>
    </types>

    <message name="Message" id="2" description="Message">
        <field name="longField1" id="0100000000" type="int64"/>

        <group dimensionType="groupSizeEncoding" name="group1" id="0300000000">
            <field name="longField1" id="0301000000" type="int64"/>
        </group>
    </message>
</messageSchema>

And version 2 of the schema where we add a nested group

<?xml version="1.0" encoding="US-ASCII" standalone="yes"?>
<messageSchema package="com.transficc.sbecompatibility.test2.protocol.v2"
               id="0"
               version="2"
               semanticVersion="1.1"
               description="Unit Test For Compatibility"
               byteOrder="littleEndian">
    <types>
        <composite name="messageHeader" description="Message identifiers and length of message root">
            <type name="blockLength" primitiveType="uint16"/>
            <type name="templateId" primitiveType="uint16"/>
            <type name="schemaId" primitiveType="uint16"/>
            <type name="version" primitiveType="uint16"/>
        </composite>
        <composite name="groupSizeEncoding" description="Repeating group dimensions">
            <type name="blockLength" primitiveType="uint16"/>
            <type name="numInGroup" primitiveType="uint8" semanticType="NumInGroup"/>
        </composite>
    </types>

    <message name="Message" id="2" description="Message">
        <field name="longField1" id="0100000000" type="int64"/>
        <group dimensionType="groupSizeEncoding" name="group1" id="0300000000">
            <field name="longField1" id="0301000000" type="int64"/>

            <group dimensionType="groupSizeEncoding" name="nestedGroup" id="0304000000" presence="optional" sinceVersion="2">
                <field name="longField1" id="0304010000" type="int64" presence="optional" sinceVersion="2"/>
            </group>
        </group>
    </message>
</messageSchema>

When we use the version 1 encoder to encode a message and the version 2 decode to decode the message, the generated decoder for the nested group thinks there is an entry for the repeated group and attempts to decode. This occurs because the hasNext has been changed to check return index < count rather than return (index + 1) < count;\n"

I have a unit test that proves this but wasn't sure where in the SBE code base to to add:

@Test
void shouldNotAttemptToDecodeNestedGroupForMessageEncodedUsingV1Encoder()
{
    final int expectedLongField1Value = 435;
    final int expectedGroupCount = 1;
    final int expectedGroupLongField1Value = 678;
    final int expectedNestedGroupCount = 0;
    final MutableDirectBuffer buffer = new ExpandableArrayBuffer(1024);

    //Encode using v1 encoder
    final MessageEncoder v1MessageEncoder = new MessageEncoder();
    final MessageHeaderEncoder v1HeaderEncoder = new MessageHeaderEncoder();
    v1MessageEncoder.wrapAndApplyHeader(buffer, OFFSET, v1HeaderEncoder);
    v1MessageEncoder.longField1(expectedLongField1Value);
    final MessageEncoder.Group1Encoder group1Encoder = v1MessageEncoder.group1Count(expectedGroupCount);
    group1Encoder
            .next()
            .longField1(expectedGroupLongField1Value);

    //Decode using v2 decode
    final MessageDecoder v2MessageDecoder = new MessageDecoder();
    final MessageHeaderDecoder v2HeaderDecoder = new MessageHeaderDecoder();
    v2HeaderDecoder.wrap(buffer, OFFSET);
    v2MessageDecoder.wrap(buffer, OFFSET + v2HeaderDecoder.encodedLength(), v2HeaderDecoder.blockLength(), v2HeaderDecoder.version());

    Assertions.assertEquals(expectedLongField1Value, v2MessageDecoder.longField1());
    final MessageDecoder.Group1Decoder v2Group1Decoder = v2MessageDecoder.group1();
    Assertions.assertEquals(expectedGroupCount, v2Group1Decoder.count());

    for (final MessageDecoder.Group1Decoder decoder : v2Group1Decoder)
    {
        Assertions.assertEquals(expectedGroupLongField1Value, decoder.longField1());

        final MessageDecoder.Group1Decoder.NestedGroupDecoder v2NestedGroupDecoder1 = decoder.nestedGroup();
        Assertions.assertEquals(v2NestedGroupDecoder1.count(), expectedNestedGroupCount);

        for (final MessageDecoder.Group1Decoder.NestedGroupDecoder nestedGroupDecoder : v2NestedGroupDecoder1)
        {
            final long nestedGroupField = nestedGroupDecoder.longField1();
            Assertions.fail("Should never enter this loop. Read value for nested group field: " + nestedGroupField);
        }
    }
}

Was this change in behaviour expected / intentional?

Thanks,
Tom

[mjpt777]

Your v2 schema does not pass validation because optional groups are not valid. SBE 1.0 does not support extension via groups or var-data. The real-logic implementation a little more flexible but we don't test for things outside the spec.

That being said I've pushed a change which should allow the old behaviour.

[tom-smalls]

Thanks for fixing and for clarifying that features not in the spec can be broken / changed.