Fixed FIX message XML conversion

[the-thing]

This should fix quickfix.MessageTest#shouldConvertToXmlWhenDataDictionaryLoadedWithExternalDTD failing in #319

When using INDENT it is mandatory to specify indent-amount as well, otherwise the generated XML will not be indented properly.

quickfixj/quickfixj-core/src/main/java/quickfix/Message.java

Line 346 in 83c0c98

serializer.setOutputProperty(OutputKeys.INDENT, "yes");

However, this is not that case in Java 11 anymore. It seems to be using default indent-amount equal to 4, when not explicitly defined.

Changes

• added indent-amount so the formatting is the same on Java 8/11/15. We are breaking backwards compatibility here as Java 8 will convert FIX messages with indent from now on.
• test slightly adjusted to pass on different Java versions

[chrjohn]

Hi @the-thing , thanks for the PR.

We are breaking backwards compatibility here as Java 8 will convert FIX messages with indent from now on.

But actually it is only a whitespace change, isn't it? I'm not the XML guy, but this only matters when textually comparing the XML output and parsers shouldn't care, right?

[the-thing]

Yes - only whitespace/formatting. We could only change the test and the XML document would still be valid, but we would get different method behaviour between Java versions.

[chrjohn]

Closing/re-opening to trigger rebuild with CI pipeline.

Edit: does not work, new CI pipeline is not used. :-/

[the-thing]

I have seen. I rebased from master and forced pushed, but it did not help.

[chrjohn]

Maybe this is the reason? https://docs.github.com/en/free-pro-team@latest/actions/reference/events-that-trigger-workflows#pull-request-events-for-forked-repositories

Note: Workflows do not run on private base repositories when you open a pull request from a forked repository.

[the-thing]

Yeah, but repo is not private. I think you need to add something like

on: pull_request: types: [assigned, opened, synchronize, reopened]

https://docs.github.com/en/free-pro-team@latest/actions/reference/events-that-trigger-workflows#pull_request

[chrjohn]

Was just about to add that, when I found the statement about the private repos. But you are right, will try.

[chrjohn]

Works now.

[the-thing]

BTW. Are you planning to phase out Travis CI completely at some point? Just asking.

[chrjohn]

Actually I just wanted to try out the github actions. Personally I found them easier to configure but maybe there are some features of Travis CI that are better? Not played around with them that much up to now. What do you think?

[the-thing]

No opinion, but it looks like compiling and running on different Java versions is easier in GitHub. In Travis you have to use jdk_switcher etc. If the actions work, I would probably get rid of Travis at some point.

Edit: I think it would be useful to add "opened, synchronize" to pull request triggers.

[chrjohn]

Here https://docs.github.com/en/free-pro-team@latest/actions/reference/events-that-trigger-workflows#pull_request it is stated that

Note: By default, a workflow only runs when a pull_request's activity type is opened, synchronize, or reopened. To trigger workflows for more activity types, use the types keyword.

But did not seem to work though. Maybe because your PR was newer than the latest CI workflow changes.

[the-thing]

Not sure.

Other thing. It seems like this particular pull request fix does work, although on my branch there are two additional race condition problems detected - https://github.com/the-thing/quickfixj/actions/runs/317548182.

• quickfix.test.acceptance.timer.TimerTest#testAcceptorTimer
• quickfix.mina.SingleThreadedEventHandlingStrategyTest#shouldCleanUpInitiatorQFJMessageProcessorThreadAfterStop

[chrjohn]

Other thing. It seems like this particular pull request fix does work, although on my branch there are two additional race condition problems detected - https://github.com/the-thing/quickfixj/actions/runs/317548182.

* `quickfix.test.acceptance.timer.TimerTest#testAcceptorTimer`

* `quickfix.mina.SingleThreadedEventHandlingStrategyTest#shouldCleanUpInitiatorQFJMessageProcessorThreadAfterStop`

Hmm, interestingly these only occur for JDK15. But might be coincidence.

[the-thing]

TimerTest is just a flaky test, although the other one is a problem. SingleThreadedEventHandlingStrategyTest.shouldCleanUpInitiatorQFJMessageProcessorThreadAfterStop fails repeateadly on Windows even locally and it is actually an error related to stopping the initiator (acceptor might have the same problem, but harder to recreate because of timing). Stopping (possibly starting as well) SocketInitiator from different threads will lead to problems.

[chrjohn]

@the-thing , w.r.t. SocketInitiator/Acceptor restarts: tried to correct the problem in #324 but somehow most of the build jobs are now failing with some other errors (seem to be git-related).

[the-thing]

#326 should help with Maven connectivity. I run from a branch based on this PR and it was only failing on SocketInitiator/Acceptor related tests. Might be useful to start merging these small PR to make sure we are moving forward.

[chrjohn]

@the-thing w.r.t. backwards compatibility: do you think we could introduce a boolean to the method to decide whether the indentation should be done or not? So the behaviour would be unchanged in the default way but could be overriden on request.

[the-thing]

Added the appropriate flag to method signatures. By default (old method signatures) indent is disabled.

[the-thing]

Only SingleThreadedEventHandlingStrategyTest.shouldCleanUpInitiatorQFJMessageProcessorThreadAfterStop is failing at the moment.

Do you think we should make a change to be fully compatible with previous versions as you pointed out above? (we will be having inconsistent behaviour across Java versions).

            if (indent) {
                serializer.setOutputProperty(OutputKeys.INDENT, "yes");
                serializer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount", "4");
            } else {
                serializer.setOutputProperty(OutputKeys.INDENT, "no");
            }

I'm actually curious if anyone is using XML serialisation and actually how many companies have deployed QuickFIXJ on Java 11. Adoption rate of Java versions > 8 is quite low and probably even lower in finance, because of procedures and regulations.

[chrjohn]

The test shouldCleanUpInitiatorQFJMessageProcessorThreadAfterStop should be fixed once #324 is merged.

My gut feeling is to be backward compatible instead of consistent across Java versions mainly due to the reason that this is a more or less pretty-print method and that only white-space is added.

[the-thing]

Ok. But then I'm not sure what the behaviour should be at this stage. Just looks awkward.

• if indent flag is true, then

serializer.setOutputProperty(OutputKeys.INDENT, "yes");
serializer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount", "4");

This gives consistent behaviour across all Java versions (with exception to CDATA element but - https://bugs.openjdk.java.net/browse/JDK-8215543)

• if indent flag is false, then

serializer.setOutputProperty(OutputKeys.INDENT, "yes");

Old behaviour. Obviously for Java version >= 11 will behave as true. Is that what we want?

[chrjohn]

OK, what about fixing this in the next major release then instead of 2.2.1? Because until we set up the build jobs for Java11 no-one ever seemed to notice that there was a problem.

But actually, my statement about "pretty-print method and that only white-space is added" could also be interpreted as: we should fix it in the minor release because probably no-one will notice the change in the XML output.

[the-thing]

Yeah exactly. Nobody noticed until GitHub build actions. How about we add @ignore to this nasty test via another PR and this can be merged later, once the GA work.

[chrjohn]

Yeah exactly. Nobody noticed until GitHub build actions. How about we add @ignore to this nasty test via another PR and this can be merged later, once the GA work.

Agreed. :)

[chrjohn]

@the-thing I don't remember exactly what we wanted to do with this but IMHO it could be integrated into 3.0.0.

[the-thing]

There was a test disabled in #329 because of the JDK bug described in https://bugs.openjdk.java.net/browse/JDK-8215543 to help pass new GitHub CI builds across different VM versions. This test was fixed here by explicitely removing whitespace characters.

Also toXml method has additional indent parameter to specify different behaviour.

Previously the default behaviour was serializer.setOutputProperty(OutputKeys.INDENT, "yes") when calling toXML method. Now this has changed to false/no. It changes formatting, but it makes sure that toXML gives the same result on all versions for the old method.