Validation fails when an optional component of a required group is absent from message since 0.101 release

[writeoncereadmany]

On upgrading from 0.100 to 0.101, we're seeing a lot of message validations on generated decoders fail. We're seeing this in validation blocks like:

        {
            int count = 0;
            final LegsGroupIterator iterator = legsGroupIterator.iterator();
            for (final LegsGroupDecoder group : iterator)
            {
                count++;                if (!group.validate())
                {
                    invalidTagId = group.invalidTagId();
                    rejectReason = group.rejectReason();
                    return false;
                }
            }
            if (count != iterator.numberFieldValue())
            {
                invalidTagId = 555;
                rejectReason = 16;
                return false;
            }
        }

(note: there is no if (hasNoLegsGroupCounter) check before this block)
The implementation of iterator.numberFieldValue() has changed between 0.100 to 0.101 from:

        public int numberFieldValue()
        {
            return parent.hasNoLegsGroupCounter() ? parent.noLegsGroupCounter() : 0;
        }

to

        public int numberFieldValue()
        {
            return parent.hasNoLegsGroupCounter() ? parent.noLegsGroupCounter() : MISSING_INT;
        }

(where MISSING_INT is Integer.MIN_VALUE)
meaning that we're failing the count check.

This is not the case for all optional repeating groups: most repeating groups are correctly putting the if (hasNoXXXGroupCounter) check in place before the validation block. The difference appears to be that the failing repeating groups are specified in the data dictionary as optional components of required groups

<messages>
    <message name="TradeCaptureReport" msgtype="AE" msgcat="app">
        fields...
        <component name="TrdInstmtLegGrp" required="N"/>
    </message>
</messages
...
<components>
    <component name="TrdInstmtLegGrp">
        <group name="NoLegs" required="Y">
            fields...
        </group>
    </component>
</components>

whereas we're seeing the guards being properly applied to optional components of optional repeating groups:

<messages>
    <message name="TradeCaptureReport" msgtype="AE" msgcat="app">
        fields...
        <component name="Stipulations" required="N"/>
    </message>
</messages
...
<components>
    <component name="Stipulations">
        <group name="NoStipulations" required="N">
            fields...
        </group>
    </component>
</components>

This means we're now (incorrectly?) rejecting messages as invalid.

I'm not sure if it's coherent for an optional repeating group component to be a required group, under the definition of FIX, but data dictionaries we've received from vendors include data of this form, and at this point updating all our data dictionaries is impractically huge.

The DecoderGenerator is aware that the group is optional, because it's doing the correct thing in DecoderGenerator.generateGroupIterator() (see the check on line 1008).

The behaviour was changed in commit 4d6dcf1 with the justification "fix bug where a reset call with an invalid message wrapped by a flyweight decoder could throw an exception, fix #327", but if I change line 1010 back to returning a default value of 0 (which would fix our problem), no tests fail (I also don't see how this particular check is related to the bug, but I may be missing something).

It also seems coherent to me that an absent group should report a count of 0 rather than null, but I can see arguments both ways.

This could also be fixed by fixing the logic around the if-check on the validation block, in DecoderGenerator.generateGroupValidation() (line 629) to be consistent with the determination of optionality on line 1008, but I'm not entirely sure where that's breaking down.

I haven't been able to construct a replicating test in the Artio codebase, but to be fair I didn't try for very long. If I had I'd be reporting this with a pull request, but I am sufficiently uncertain of the context and cause to stick to an issue for now.

[RichardWarburton]

Hi Tom,

tl;dr Can you send in a PR with a test that would have failed for this issue?

Eurgh, this is an annoying interaction of things, here's an explanation for posterity's sake.

So FYI the original bug that this was fixing was that because performing a reset operation required knowing the number of elements in a repeating group (so it could iterate and reset all codecs each group element) and flyweight codecs trying to decode things as late as possible doing a reset on a flyweight codec that had rewrapped another buffer could throw an exception because it would try to re-read the num-in-group field from the rewrapped buffer at a now invalid offset. The fix was to read num-in-group fields when we parse the message.

The consequence of that is that the decoder needs to differentiate between "I've not read the num-in-group field" and "the group size is 0" - therefore the MISSING_INT value is used to distinguish "i've not read the num-in-group field". I think we can still return 0 for the missing case and I've just pushed a fix for that which, according to my reading of the issue should fix the issue.

With respect to "I'm not sure if it's coherent for an optional repeating group component to be a required group" - it's clearly common-nonsense isn't it? See also #293 . It would be nice to have a sensible thing to do here.

I think it would still be good to have a test for our test-suite that capture your original issue though. So can you send one in a PR?

[writeoncereadmany]

Thanks! I have verified that this does fix the issue.

To quote my original issue:

I haven't been able to construct a replicating test in the Artio codebase, but to be fair I didn't try for very long.

I can put a bit more time into this, but it was non-trivial how to represent the data dictionary xml input data in the programmatically-constructed messages in ExampleDictionary.java

With respect to "I'm not sure if it's coherent for an optional repeating group component to be a required group" - it's clearly common-nonsense isn't it? See also #293 . It would be nice to have a sensible thing to do here.

I mean, yes, it doesn't seem to make a lot of sense, but it's something we see in real-world fix data dictionaries, so I think we need coherent handling of them.

This is coloured by the observation that we've seen real messages with groups absent where the spec has an optional component representing a required group, implying that in those contexts this group is intended to be treated as optional. Which implies that optionality dominates over requirement?