Broken custom message humanize with 1.12.0

[enricobenedos]

Hello,

we updated the library from 1.11.2 to 1.12.0 and that's broke a custom message parsing.

We are using the FIX42.xml dictionary with addition of a custom broker commissions message (included fields definition)

<message name="CommissionMessage" msgtype="U" msgcat="app">
    <field name="OrderID" required="Y"/>
    <field name="ExecID" required="Y"/>
    <field name="CustomMsgTypeID" required="Y"/>
    <field name="CommissionValue" required="Y"/>
    <field name="CommissionCurrency" required="Y"/>
    <field name="RealizedPNL" required="Y"/>
</message>

Then in the code we simply check for U message types in this way message.Header.GetString(Tags.MsgType).Equals("U") and then we convert the message from json humanized format to an internal object:

JsonNode? jNode = JsonNode.Parse(message.ToJSON(humanReadableValues: true));

Starting with 1.12.0 the humanReadableValues: true doesn't work anymore for this custom message and this cause a deserialize error when trying to convert it.

[gbirchmeier]

Oh! I see the problem. This was a breaking change, noted in the RELEASE_NOTES.md file.

See here:
https://github.com/connamara/quickfixn/blob/02dcb0537000b3d7110624d145f1d31cfff612cf/RELEASE_NOTES.md?plain=1#L37

I'll paste it here for posterity:

• Message: ToXML() and ToJSON() require DD params for any tag/enum translation. (Message no longer stores a DD instance var.) These functions are new, probably not widely used yet. Function docs are now clear on this behavior.

There was some sloppiness in the code where Message was storing a reference to the DD, but only for this purpose. The logic was weird and not intuitive, which is bad for maintenance, so I took it out.

The function public string ToJSON(bool humanReadableValues) has been removed.

The new function signature is: public string ToJSON(DD? dataDictionary = null, bool humanReadableValues = false) But... per the function's doc comments:

    /// <param name="humanReadableValues">
    ///   True will cause enums to be converted to human strings.
    ///   Will not (and cannot!) work if dataDictionary is null.
    /// </param>

Therefore, when you call message.ToJSON(humanReadableValues: true) you are now actually calling message.ToJSON(dataDictionary: null, humanReadableValues: true) ^^^^^^^^^^^^^^^^^^^^

The solution for you is to pass in the dataDictionary parameter for your custom DD.

(Maybe I should make this function throw an exception if dd=null and humanReadable=true. Or maybe make a separate function message.ToHumanReadableJSON where dd is required.)

Please let me know if you have any questions!

[enricobenedos]

Thanks @gbirchmeier,

that's my mistake, I don't read the method docs. Maybe it's a little bit weird that dataDictionary parameter is not mandatory.. I'm not sure this point about the library behaviour, let me try explain it with an example:

• UseDataDictionary=N and humanReadableValues= false --> no problems
• UseDataDictionary=Y and humanReadableValues= false --> do you load the given external data dictionary? Anyway no translation, no problem
• UseDataDictionary=Y and humanReadableValues= true --> Am I not already passing it using this configuration?

(I'm not able to reach new website to explore newly the documentation)

This workflow is adopted due to impossibility to define custom messages at build time for example (we know the manual tool), we cannot every time build the library due to CI/CD limits.

Sorry for inaccuracy in some arguments, the implementations is working from months and in a stable way, so I'm a little bit rusty on this library usage.

[gbirchmeier]

The UseDataDictionary setting is not relevant to your problem.

I'm saying, when you call ToJSON, you need to pass an actual DataDictionary reference into it.

You need to call this:

message.ToJSON(enricosDictionary, true)

For the dictionary param, you can either create a new DD reference with

new QuickFix.DataDictionary.DataDictionary(path_to_your_dd_xml)

Or you can get it from your session, with session.ApplicationDataDictionary

[enricobenedos]

Ok, that's clear, I was looking for a more global way to set it once.

We have a lot of code lines in different places using ToJSON (also different apps) with humanReadableValues=true both for logging or debugging purposes; that new implementation force us to pass a dictionary over all functions or method in different places with a reference that decrease our code quality.

I don't want to be annoying 🙂, but why I need to specify every time the dictionary reference if the sessions already know it?

Probably we will continue using the old package at the moment.

[gbirchmeier]

why I need to specify every time the dictionary reference if the sessions already know it?

Ha, see, that's the problem. A message doesn't "know" its session. Message never had a reference to a Session. Maybe it has the session header fields set, or maybe it doesn't. You can create a Message without specifying a Session -- in fact, that's usually what you do, and the engine fills in the session header info when you call Send.

When the original dev (whoever it was) wrote ToJSON, they added the internal DD reference in Message, but it was kind of a hack. It was nullable (which is bad practice), and would only be set on messages that came from the counterparty. It was an internal field added only for use by ToJSON and ToXML, functions that probably aren't used by many people.

Furthermore, every single message received on a session has the exact same DD anyway! That's silly and unnecessary. You can just create a single DD variable, set it once, and put it in all the ToJSON calls. Make it global to your app if you want.

Or make a static utility class so you don't have to pass references around. Here's a hacky example, but you can probably do something better with a little more thought.

public static JsonConverter {
    private static DataDictionary _dd;

    public void Init(DataDictionary dd) {
        _dd = dd;
    }

    public string ToJSON(Message m) {
        if(_dd is null)
            throw new Exception("dd is not set; call Init() first");

        return m.ToJSON(_dd, true);
    }
}

[gbirchmeier]

I made it throw an exception if dd=null and humanReadable=true.

And I renamed humanReadable to convertEnumsToDescriptions, to be a more accurate descriptor of what it does.