UserDefinedFields is only validated on message parsing in a very specific circumstance

[philipwhiuk]

Describe the bug

We only check field validity in a specific circumstance

To Reproduce

String ioiWithUserDefinedField = 
    "8=FIX.4.2|9=75|35=6|49=Target|56=Sender|128=BUYSIDE|115=BIG-BANK-PLC|34=1|52=20160531-09:22:01.625|" +
    "23=001|28=N|55=VOD.C|62=20170531-09:22:01.625|130=N|15=GBP|44=13.37|54=1|58=This is a comment|27=122|22=1|48=BH4HKS3|5001=ABC|10=206|";
String ioiInFixFormat = ioiWithUserDefinedField.replaceAll("\\|","\u0001")
DataDictionary fix42Dictionary = new DataDictionary("FIX42.xml");
fix42Dictionary.setCheckUserDefinedFields(true)
Message message = new Message(ioiInFixFormat, fix42Dictionary, true);

Expected behavior Constructor should throw FieldException with message: "Invalid tag number, field=5001"

system information:

• Linux
• Java version: JDK 8
• QFJ Version: 2.2.0-SNAPSHOT

Additional context

This may be by design but it's not clear that it'll happen from the API. The boolean you pass is called validate, it's reasonable to expect that it should validate according to the dictionary settings.

The only circumstance we check for user defined fields is in parseGroup:

https://github.com/quickfix-j/quickfixj/blob/027e82bbb3d5a674b695d875b77e1b84f9bdd2f3/quickfixj-core/src/main/java/quickfix/Message.java#L738

[chrjohn]

FieldException is not thrown because of this:

https://github.com/quickfix-j/quickfixj/blob/027e82bbb3d5a674b695d875b77e1b84f9bdd2f3/quickfixj-core/src/main/java/quickfix/Message.java#L568-L583

So you can check if Message.getException() is not NULL after your call. But yes, in a way it is unexpected. Should maybe be dependent on doValidation flag.

[philipwhiuk]

UserDefinedFields isn't checked at all though. getException() returns null.

[the-thing]

This was always the case (at least going back 6 years back.)

The validation happens in two places. One in DataDictionary and the other in the Message. The one in Message iterates over fields in the message and not in the dictionary so extra fields are not checked. There are probably some other nuances.

It is not nice, but at the when message is received the failure will be detected in one of two places

https://github.com/quickfix-j/quickfixj/blob/027e82bbb3d5a674b695d875b77e1b84f9bdd2f3/quickfixj-core/src/main/java/quickfix/mina/AbstractIoHandler.java#L135

or

https://github.com/quickfix-j/quickfixj/blob/027e82bbb3d5a674b695d875b77e1b84f9bdd2f3/quickfixj-core/src/main/java/quickfix/Session.java#L994

I remember that in the past every time we got invalid message from a venue we had to pass it via both validations to find out what was wrong.

[drosso64]

A side-effect of this is that, if an unknown tag is placed just after a group, then the field is rejected and the parsing stops. In all other cases, the parsing continues. I'd rather the parsing to continue in all cases, with no exception. Probably the test for unknown tags in groups should be made "smarter" and determine if the tag is to be considered still in the group or not before calling the validation. In this way the behavior of the parser is asymmetric.

[chrjohn]

@drosso64 , with which version did you test this? There were changes done with https://www.quickfixj.org/jira/browse/QFJ-169 to consider the unknown field to be part of the repeating group. Actually, the parser cannot decide whether the unknown tag is part of the repeating group or part of the parent message since the tag is unknown. Especially when there is only one repeating group instance there is no way to know whether the tag is part of the group or not. You can only guess at this point.

[drosso64]

Hi @chrjohn, I'm testing the 2.1.1 version. I've seen the code affected by https://www.quickfixj.org/jira/browse/QFJ-169 and actually I understand there's no way to determine whether the tag is part of the repeating group or not. I'm testing the library in the context of [http://fixpusher.sourceforge.net/](url, a FIX testing tool I'm changing to my needs; the tool is used to test some FIX components of a trading market platform, and I needed a complete parsing of messages even in case of tags not defined in the dictionary. That's how I noticed that when an unknown tags is at the end of a repeating group the parser throws an exception and stops the processing; on the other hand this didn't happen if the tag was simply moved in another position. I was however able to circumvent the problem by setting AllowUnknownMsgFields to 'Y'.

[philipwhiuk]

If the tag is not in the dictionary you have no way of knowing whether it should be in the group or not. With a Dictionary and AllowUnknownMsgFields you will find that tags after a group might get placed in the group rather than in the message