Uploaded image for project: 'QuickFIX/J'
  1. QuickFIX/J
  2. QFJ-656

allowUnknownMsgFields has no effect if the field is not defined in the dictionary

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.0, 1.5.0, 1.5.1
    • Fix Version/s: 1.6.2
    • Component/s: Engine
    • Labels:
      None

      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) .

        Attachments

          Activity

          Hide
          chabala Greg Chabala added a comment -

          This is still an issue in version 1.5.2

          Show
          chabala Greg Chabala added a comment - This is still an issue in version 1.5.2
          Hide
          alphashock Marcin L added a comment -

          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.

          Show
          alphashock Marcin L added a comment - 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.
          Hide
          chrjohn Christoph John added a comment -

          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.

          Show
          chrjohn Christoph John added a comment - 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.
          Hide
          alphashock Marcin L added a comment -

          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)

          Show
          alphashock Marcin L added a comment - 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)
          Hide
          chrjohn Christoph John added a comment -

          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.

          Show
          chrjohn Christoph John added a comment - 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.
          Show
          alphashock Marcin L added a comment - https://github.com/quickfix-j/quickfixj/pull/57

            People

            • Assignee:
              alphashock Marcin L
              Reporter:
              test test
            • Votes:
              2 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: