[QFJ-656] allowUnknownMsgFields has no effect if the field is not defined in the dictionary Created: 28/Nov/11  Updated: 21/Apr/16  Resolved: 08/Jan/16

Status: Closed
Project: QuickFIX/J
Component/s: Engine
Affects Version/s: 1.4.0, 1.5.0, 1.5.1
Fix Version/s: 1.6.2

Type: Bug Priority: Major
Reporter: test Assignee: Marcin L
Resolution: Fixed Votes: 2
Labels: None

Attachments: Text File suggested_fix.txt    

 Description   

The documentation of allowUnknownMsgFields is a bit misleading, it has no effect if the field is not defined in the dictionary. allowUnknownMsgFields is not checked in void checkValidTagNumber(Field<?> field) .



 Comments   
Comment by Greg Chabala [ 25/Jun/12 ]

This is still an issue in version 1.5.2

Comment by Marcin L [ 30/Dec/15 ]

I wanted to produce a fix for this issue, but I have trouble understanding what the correct logic should be. I believe the question is if the behaviour should be respectively the same, regardless if a field exists in <fields> section or not. I did some analysis on the topic, but I need a second opinion if this is desired solution, before I submit a patch. Please see attachment.

Comment by Christoph John [ 31/Dec/15 ]

Hi Marcin L,

you forgot to mention if the fields are required. I was wondering why test case 2 with field 6000 (AllowUnknownMessageFields=false and CheckUserDefinedFields=true) failed. But this is because the field 6000 is required I assume?
Why should the case with field 6000 and AllowUnknownMessageFields and CheckUserDefinedFields both set to true be fixed?
IMHO you are right about the FIXMEs for case 3. If AllowUnknownMessageFields is set to true then it should not matter if the field is inside the dictionary or not.

Thanks,
Chris.

Comment by Marcin L [ 31/Dec/15 ]

Hello Christoph,

1) Actually none of the fields are required, because all them are missing in the message fields section:

<messages>
<message name="TestMessage" msgtype="T" msgcat="app">
<!-- no fields defined -->
</message>
</messages>

Field 6000, test case (2, 2) fails currently, because field 6000 is not defined in message + AllowUnknownMessageFields == false:

private void checkIsInMessage(Field<?> field, String msgType) {
if (!isMsgField(msgType, field.getField()) && !allowUnknownMessageFields)

{ throw new FieldException(SessionRejectReason.TAG_NOT_DEFINED_FOR_THIS_MESSAGE_TYPE, field.getField()); // line: 779 }

}

The above check is triggered, because of CheckUserDefinedFields == true.

2) I'm not sure about test case (2,4). It would make sense to FAIL according to documentation, because

  • 6000 is a user defined field
  • CheckUserDefinedFields == true
  • it's not defined in the message

In general I think that checks dependent on flags AllowUnknownMessageFields and CheckUserDefinedFields should be executed independently, although this is not the case at the moment.

From docoumentation. ValidateUserDefinedFields: If set to N, user defined fields will not be rejected if they are not defined in the data dictionary, or are present in messages they do not belong to.

This basically means that if value is true, then all fields >= 5000 should fail if not in message field section or global field section, which would imply that test case (2, 4) should result in FAILURE.

Additionally in my understanding of these flags:

AllowUnknownMessageFields == !CheckUserDefinedFields (however the first flag applies to fields < 5000 only, second to fields >=5000 only)

Comment by Christoph John [ 04/Jan/16 ]

Hi Marcin L and happy new year,

you are of course right about points 1) and 2). Sorry, it was late.
As you said, according to the documentation ValidateUserDefinedFields applies to fields >= 5000 and AllowUnknownMsgFields to tags < 5000. Currently the behaviour is not as expected, as your analysis has shown.

It would be great if you could provide a patch and unit test for this.
Thanks in advance.

Comment by Marcin L [ 04/Jan/16 ]

https://github.com/quickfix-j/quickfixj/pull/57

Generated at Sat Nov 23 07:49:45 UTC 2024 using JIRA 7.5.2#75007-sha1:9f5725bb824792b3230a5d8716f0c13e296a3cae.