Generates invalid CSharp code when using sinceVersion attribute inside groups

[ZachBray]

Thanks for this great library! :-)

I think this is an issue for @billsegall, as it relates to the CSharp code generation.

We're generating code using the sbe-tool:1.7.6 JAR.

When we have the following structure in the schema file

• message
  • group (sinceVersion = 42)
  • field (sinceVersion = 42)

the C# code that is generated does not compile.

This code fragment is representative of the problem:

            public SimpleCompositeV0 Value
            {
                get
                {
                if (actingVersion < 42) return null;

                    _value.Wrap(_buffer, _offset + 0, _actingVersion);
                    return _value;
                }
            }

If actingVersion were prefixed with an underscore I believe it would fix the issue.

It looks like it may come from this line: https://github.com/real-logic/simple-binary-encoding/blob/4b64c6f4a678f6ccb5604c5f5e0b2cade3e5bfa2/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpGenerator.java#L700

---

Full schema

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<ns2:messageSchema xmlns:ns2="http://fixprotocol.io/2016/sbe" package="N.B., package is required but not used" id="32353" version="42" 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="varStringEncoding" description="Variable length string encoding">
        <type name="length" maxValue="2147483647" primitiveType="uint32"/>
        <type name="varData" length="0" primitiveType="uint8" characterEncoding="UTF-8"/>
    </composite>
    <composite name="varDataEncoding" description="Variable length data encoding">
        <type name="length" maxValue="2147483647" primitiveType="uint32"/>
        <type name="varData" length="0" primitiveType="uint8"/>
    </composite>
    <composite name="groupSizeEncoding" description="Group size encoding">
        <type name="blockLength" primitiveType="uint16"/>
        <type name="numInGroup" primitiveType="uint16"/>
    </composite>
</types>
<types>
    <composite name="simpleCompositeV0">
        <type name="foo" primitiveType="int32"/>
    </composite>
</types>
<ns2:message name="brokenMessageV0" id="5746">
    <group dimensionType="groupSizeEncoding" name="brokenGroup" id="1" sinceVersion="42">
        <field name="value" id="2" type="simpleCompositeV0" sinceVersion="42"/>
    </group>
</ns2:message>
</ns2:messageSchema>

Generated class

    public sealed partial class BrokenMessageV0
    {
        public const ushort BlockLength = (ushort)0;
        public const ushort TemplateId = (ushort)5746;
        public const ushort SchemaId = (ushort)32353;
        public const ushort SchemaVersion = (ushort)42;
        public const string SemanticType = "";

        private readonly BrokenMessageV0 _parentMessage;
        private DirectBuffer _buffer;
        private int _offset;
        private int _limit;
        private int _actingBlockLength;
        private int _actingVersion;

        public int Offset { get { return _offset; } }

        public BrokenMessageV0()
        {
            _parentMessage = this;
        }

        public void WrapForEncode(DirectBuffer buffer, int offset)
        {
            _buffer = buffer;
            _offset = offset;
            _actingBlockLength = BlockLength;
            _actingVersion = SchemaVersion;
            Limit = offset + _actingBlockLength;
        }

        public void WrapForDecode(DirectBuffer buffer, int offset, int actingBlockLength, int actingVersion)
        {
            _buffer = buffer;
            _offset = offset;
            _actingBlockLength = actingBlockLength;
            _actingVersion = actingVersion;
            Limit = offset + _actingBlockLength;
        }

        public int Size
        {
            get
            {
                return _limit - _offset;
            }
        }

        public int Limit
        {
            get
            {
                return _limit;
            }
            set
            {
                _buffer.CheckLimit(value);
                _limit = value;
            }
        }

        private readonly BrokenGroupGroup _brokenGroup = new BrokenGroupGroup();

        public const long BrokenGroupId = 1;
        public const int BrokenGroupSinceVersion = 42;
        public const int BrokenGroupDeprecated = 0;
        public bool BrokenGroupInActingVersion()
        {
            return _actingVersion >= BrokenGroupSinceVersion;
        }

        public BrokenGroupGroup BrokenGroup
        {
            get
            {
                _brokenGroup.WrapForDecode(_parentMessage, _buffer, _actingVersion);
                return _brokenGroup;
            }
        }

        public BrokenGroupGroup BrokenGroupCount(int count)
        {
            _brokenGroup.WrapForEncode(_parentMessage, _buffer, count);
            return _brokenGroup;
        }

        public sealed partial class BrokenGroupGroup
        {
            private readonly GroupSizeEncoding _dimensions = new GroupSizeEncoding();
            private BrokenMessageV0 _parentMessage;
            private DirectBuffer _buffer;
            private int _blockLength;
            private int _actingVersion;
            private int _count;
            private int _index;
            private int _offset;

            public void WrapForDecode(BrokenMessageV0 parentMessage, DirectBuffer buffer, int actingVersion)
            {
                _parentMessage = parentMessage;
                _buffer = buffer;
                _dimensions.Wrap(buffer, parentMessage.Limit, actingVersion);
                _blockLength = _dimensions.BlockLength;
                _count = _dimensions.NumInGroup;
                _actingVersion = actingVersion;
                _index = -1;
                _parentMessage.Limit = parentMessage.Limit + SbeHeaderSize;
            }

            public void WrapForEncode(BrokenMessageV0 parentMessage, DirectBuffer buffer, int count)
            {
                _parentMessage = parentMessage;
                _buffer = buffer;
                _dimensions.Wrap(buffer, parentMessage.Limit, _actingVersion);
                _dimensions.BlockLength = (ushort)4;
                _dimensions.NumInGroup = (ushort)count;
                _index = -1;
                _count = count;
                _blockLength = 4;
                parentMessage.Limit = parentMessage.Limit + SbeHeaderSize;
            }

            public const int SbeBlockLength = 4;
            public const int SbeHeaderSize = 4;
            public int ActingBlockLength { get { return _blockLength; } }

            public int Count { get { return _count; } }

            public bool HasNext { get { return (_index + 1) < _count; } }

            public BrokenGroupGroup Next()
            {
                if (_index + 1 >= _count)
                {
                    throw new InvalidOperationException();
                }

                _offset = _parentMessage.Limit;
                _parentMessage.Limit = _offset + _blockLength;
                ++_index;

                return this;
            }

            public System.Collections.IEnumerator GetEnumerator()
            {
                while (this.HasNext)
                {
                    yield return this.Next();
                }
            }

            public const int ValueId = 2;
            public const int ValueSinceVersion = 42;
            public const int ValueDeprecated = 0;
            public bool ValueInActingVersion()
            {
                return _actingVersion >= ValueSinceVersion;
            }

            public static string ValueMetaAttribute(MetaAttribute metaAttribute)
            {
                switch (metaAttribute)
                {
                    case MetaAttribute.Epoch: return "unix";
                    case MetaAttribute.TimeUnit: return "nanosecond";
                    case MetaAttribute.SemanticType: return "";
                    case MetaAttribute.Presence: return "required";
                }

                return "";
            }

            private readonly SimpleCompositeV0 _value = new SimpleCompositeV0();

            public SimpleCompositeV0 Value
            {
                get
                {
                if (actingVersion < 42) return null;

                    _value.Wrap(_buffer, _offset + 0, _actingVersion);
                    return _value;
                }
            }
        }
    }

[mjpt777]

Thanks for the report @ZachBray. Can you try the latest to confirm it works for you?

[ZachBray]

It looks much healthier now. Thank you!

[ZachBray]

@billsegall, will this fix make it into the "sbe-tool" NuGet package or should I package it up myself for use on our project?

[billsegall]

I'll try and get a new package out this week.

[billsegall]

nuget.org has an update. Thanks for the report and patch.

[ZachBray]

Great! Thank you for the package update.

[ZachBray]

Hi guys,

Sorry I've discovered another issue related to this. Not sure how I didn't notice it earlier.

The code generated is now valid C# and it works for decoding. When encoding we use the same property.

            public SimpleCompositeV0 Value
            {
                get
                {
                if (_actingVersion < 42) return null;

                    _value.Wrap(_buffer, _offset + 0, _actingVersion);
                    return _value;
                }
            }

Unfortunately WrapForEncode(...) does not initialize _actingVersion to anything. Therefore, encoding fails.

I'll attach a PR that fixes this momentarily.

[mjpt777]

I've merged the change. However it feels wrong to wrapForEncode and then decode. I think the Java design of separate encoders and decoders is a safer and cleaner approach.

[ZachBray]

it feels wrong to wrapForEncode and then decode

FYI we're not calling WrapForEncode(...) then decoding, we're still encoding, but we need to access the property in order to encode/write the composite fields which necessitates a Wrap(...). I believe composites use the same Wrap(...) mechanism for encoding and decoding on the C# side.

separate encoders and decoders is a safer and cleaner approach

👍