Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Field of type MULTIPLEVALUESTRING is not treated #66

Closed
raphaelms opened this issue Apr 30, 2012 · 7 comments
Closed

Fix Field of type MULTIPLEVALUESTRING is not treated #66

raphaelms opened this issue Apr 30, 2012 · 7 comments
Labels

Comments

@raphaelms
Copy link

Hi All,

Trying to decode a the field with Tag number 277 (TradeCondition) of type MULTIPLEVALUESTRING resulted in an exception of type IncorrectTagValue. The code could not understand a message with "277=R L". I think it can't decode any message with the specification of MULTIPLEVALUESTRING when it comes more than one value delimited by space.
To replicate this issue try to receive a message that contains any field of type MULTIPLEVALUESTRING with more than one value delimited by space.

Thanks,

Raphael
@cbusbey
Copy link
Contributor

cbusbey commented Apr 30, 2012

Raphael,

Could you post an example of a message that causes this issue? Do you have a stack trace?

Thanks

@cbusbey
Copy link
Contributor

cbusbey commented Apr 30, 2012

Nevermind Raphael, I think I found the issue. It looks like the DataDictionary doesn't take into account multiple value strings when validating.

        /// <summary>
        /// If field is an enum, make sure the value is valid.
        /// </summary>
        /// <param name="field"></param>
        public void CheckValue(Fields.IField field)
        {
            DDField fld = FieldsByTag[field.Tag];
            if (fld.HasEnums() && !fld.Enums.Contains(field.ToString()))
            {
                throw new IncorrectTagValue(field.Tag);
            }

Until this issue is resolved, is it possible to turn off the UseDataDictionary configuration option? If your messages contain repeating groups this may not be an option.

@raphaelms
Copy link
Author

Yes, the code do not split multiple values separated by spaces. To resolve this issue a class named multiplestringvalues deriving from FieldBase< IList < string>> could decode this field type, but i think that it wouldn't comply with the standard quickfix. Other approach would be to check if the IField is multiplevalue and split the string inside CheckValue.
To handle this issue now I'm changing the type of the field to STRING in the XML and decoding the field externally.

Thanks

@cbusbey
Copy link
Contributor

cbusbey commented May 1, 2012

Ah yes, swizzling the data dictionary to string is a good work around. As you suggest, I think the solution is to split the string inside CheckValue and verify the values. We encourage pull requests for fixes if you have the time, otherwise we'll prioritize the bug and try to get a fix into a future release.

Thanks again for logging the issue, Raphael

@jcwild
Copy link

jcwild commented Apr 2, 2013

We have also just experienced this problem on tag 18, defined as MULTIPLEVALUESTRING. We received the value "1 B" which should be perfectly valid, but it was rejected with the error "Value is incorrect (out of range) for this tag".

@gbirchmeier
Copy link
Member

The reason this is failing is because QF/n is trying to enforce that the field's string value be one of the enumerated values as specified in the DD. For tag 18, it will accept "B", but not "1 2 B". Obviously, that's just wrong.

(Note: All MultipleString fields are enums. Spec doesn't say they this type must be an enum, but it just happens that they are.)

So, do these types get used enough that we want to fully implement these field types?

I see some options:

No-code workaround

Go into your DD and delete/comment out all enums for the specific field that's giving you trouble. Now you can get/set the field as a string and do the split-on-space yourself.

Not a solution, but I figured that might be useful to someone to have written down.

Easy minimum-effort fix

We change QF/n to ignore the enum validation for these multiple-value types. Same result as workaround, except you don't have to kludge your DD.

Full Feature Requirements (If we want to do this.)

According to specs, these are what we need to support:

  • MultipleValueString (FIX4) / MultipleStringValue (FIX5) (same things but different name)
  • MultipleCharValue (FIX5 only)

Functionally, these are the same: a string made up of space-separated values. Technically, MultipleCharValue should only allow single-character values, but I'm kinda thinking we can ignore enforcement of that.

  1. So, one type: MultipleValueField. Unless whoever writes this really wants to make separate String and Char types.
  2. If the DD has enums defined for the field, the validator should check that all values are valid enum values or else reject. If no enums, accept anything.
  3. The field type should provide an interface to get/set the field's multiple values. Brackets? Get(index)? AsArray? Implementer's choice. We can add synonym methods later if desired. @raphaelms's FieldBase<IList<string>> suggestion sounds like a logical way to do it, but there may be complications.
  4. Generator will need to be altered to create the new field type.
  5. Looks like classes in QuickFIXn/DataDictionary are already ok.

gbirchmeier added a commit that referenced this issue Sep 26, 2013
(#66) MultipleValueString type validation bug
@gbirchmeier
Copy link
Member

See Harv's fix in PR #232. It's not a full-blown MultipleValueField class, but it does correct the deficiency that causes incoming messages to fail. Such fields are still StringFields, and users must interact with them as such.

I've opened #233 to note that we don't have a MultipleValueField class, and the requirements to keep in mind if someone wanted to take a stab at it.

Thus I'm considering #66 to be done. Please have a look at #233 if you think we should go further, and make comments over there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants