quickfixj
quickfixj copied to clipboard
quickfix-j
Fix orchestra with updated project structure
Support for FIX Latest #317
This is not ready to merge and is submitted to attract comments.
This branch has structural changes that are intended to make it quicker to work with core.
Core no longer has a mutual dependency with ALL the source code for fields, components and messages for all the FIX versions. Core depends on a new module "Base" as do the generated Message, Component and Field classes. Base does depend on a small number of generated classes that are used in Standard Header and Footer. These classes are generated with and packaged with Base, but only these classes. This is to help decouple the messages resources from the core distribution and facilitate the build and maintenance of custom distributions. Core now has only test dependencies on the message packages. This is described at length in project markdown files.
There is a benefit to this structure; "core" can be developed without rebuilding the message packages. Build times for the message packages are long. The FIX Orchestration now has nearly 6000 fields. One only needs to rebuild the message packages when code generation is altered, when a new FIX Standard Orchestration is incorporated or when Base is changed. As a further convenience there is a minimal profile to reduce the size -P"minimal-fix-latest". This This makes fix latest much smaller but includes resources that are needed to test core.
Use of the FTC Code Generator copied from the https://github.com/FIXTradingCommunity/fix-orchestra-quickfix is introduced. The XSL based QFJ code generation takes too long for FIXLatest and this was causing C.I. builds to fail. This FTC Code Generator is used for fixt11 and fixlatest to help with future compatibility. Other protocol versions are built with the old code generator to all options to build custom packages with very high backward compatibility but benefitting from the new core (and base).
The FTC Code Generator is copied into the repository-quickfix module in this repo. This is because: the "reason for change" for this component is changes to QFJ and also to avoid a cyclic dependency between the fix-orchestra-quickfix build and the QFJ build. The code in this repository has been extended and has many changes that are not in the original repo.
The convention used in Orchestra for codeset enumerations can lead to sub-optimal constant names. Work is in progress with the FTC to work around this. Where FIX latest is part of the build some constant names will be different from legacy builds. As mentioned above, there are work arounds for this.
You'll see that the new code generation for fixlatest does not de-normalize components. I had extended the code generation to do so and more closely mirror the QFJ code generator but this resulted in enormous classes with extreme build times for compilation and javadoc. I suggest that we adopt normalised components for fixlatest as the convention for this qfj distribution. If you build from orchestra you will get normalised components in the fixlatest package. This could be an optional setting but I think it's a bad idea. There are no actual "components" in fixt11, only groups.
This project now also publishes a QuickFIXJ orchestration, modified to help with a correct QFJ build. This can then be used to "easily" derive and customise QFJ builds, without needing to understand the work required to conform the standard orchestration to QFJ. Customised builds should now be more maintainable, there is no need to manage forks of QFJ to cope with ROE differences and this will enable more succinct code, documents and test generation.
I've tested this approach in branch "use-orchestration-from-qfj-project" of ig-orchestrations. see: https://github.com/IG-Group/ig-orchestrations/tree/use-orchestration-from-qfj-project. https://github.com/IG-Group/ig-orchestrations/tree/use-orchestration-from-qfj-project/ig-us-rfed/orchestration https://github.com/IG-Group/ig-orchestrations/blob/use-orchestration-from-qfj-project/ig-us-rfed/quickfixj-messages-fixLatest/pom.xml I have also tested the resulting customized message package end to end with private QFJ FIX applications.
Also note :
I apologize for too much whitespace change, my Eclipse just keeps crashing so I was unable to apply the formatter. If this work is otherwise acceptable I'm sure we can format the artefacts.
Build with profile "-PskipBundlePlugin" speeds things up a little. I might suggest that we can get rid of these bundles entirely for v3.0.0. I don't know how to ensure that they are correct.
This pull request introduces 5 alerts when merging d927ba2ba01d4bb305fbbb21170df3661ba6cdc9 into 2f4fda1f0dbcfd1185cf01533b0a614e00808478 - view on LGTM.com
new alerts:
- 3 for Spurious Javadoc @param tags
- 2 for Potential input resource leak
![lgtm-com[bot] avatar](https://avatars.githubusercontent.com/in/17324?v=4 "Published by a pub.dev verified publisher"){kind=small}
@chrjohn Hi, I would really welcome some feedback on this, as there are some structural changes that will make this really hard to maintain in line with master as time goes on. I think that the best way to work with this is to first check out the branch and build it, and to look at the readmes. There are some things I don't like about this structure but it works, it builds fast and can help with productive maintenance of QFJ. It is intended to make it quite easy for users to maintain their custom orchestration with type safety without needing manipulate the base/core QFJ distribution.
Hi @david-gibbs-ig , thanks for all your work. At a first glance the changes look good to me. :+1: However, please keep in mind that these are a lot of changes and I will need more time to review all of it. That being said, is there the possibility to split this into smaller PRs? Special thanks for all the READMEs that ease the understanding of your changes a lot. I think I'll have a lot of questions nevertheless. :)
Hi @david-gibbs-ig , thanks for all your work. At a first glance the changes look good to me. 👍 However, please keep in mind that these are a lot of changes and I will need more time to review all of it. That being said, is there the possibility to split this into smaller PRs? Special thanks for all the READMEs that ease the understanding of your changes a lot. I think I'll have a lot of questions nevertheless. :)
Hi @chrjohn
I have submitted https://github.com/quickfix-j/quickfixj/pull/493 which includes the FTC code generation part of the work. In the overall work most of the code is new and there are very few changes. The big changes are "structural". I can make time to walk you through it as I appreciate, it is a lot.
- quickfixj-base can be clean built without building all the quickfixj-messages.
- Once you have built the quickfixj-messages once, the quickfixj-core module can be clean built without rebuilding the quickfixj-messages. The dependency on the application messages are runtime dependencies.
I think the only way to review this is to check out the branch and build it, comparing it to an independent build of master in another working directory - and using the PR diff as an aid along the way.
The work tries as far as possible to preserve the original QFJ processing and usage for FIX50SP2 and earlier. This is so users can elect if and when they want to start using FIX Latest and little needs to be changed until the users can derive some benefit from using FIX Latest.
I think that the structure is not elegant but it is still worth working with. I think that the biggest benefit for QFJ users is to be able to use and maintain custom type-safe message classes more easily.
Once we're in a position that the project is productively maintainable and can work with FIX Latest as well as legacy FIX, there is doubtless room for some further refactoring.
hth
Just tried to resolve the conflicts, hopefully I didn't mess something up...
Just tried to resolve the conflicts, hopefully I didn't mess something up...
No, I think it's exactly the same as I did. Will look at the errors when I get a chance.
This is a test failure I have seen, I don't understand why the output seems different, possibly changes I made to XML dependencies. I'll look to see how the test expectations compare to master. I might have gone with what was in master when I merged master in...
[INFO] Tests run: 70, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.119 s - in quickfix.SessionTest
[INFO]
[INFO] Results:
[INFO]
Error: Failures:
Error: MessageTest.shouldConvertToXMLWithIndent:1979 expected:<...ncoding="ISO-8859-1"[?>
possibly changes I made to XML dependencies
I'd suspect the same.
XML errors resolved.
possibly changes I made to XML dependencies
I'd suspect the same.
XML errors resolved.
What was the problem? It seems an additional attribute somehow was added?
What was the problem? It seems an additional attribute somehow was added?
The output of Message.toXML(...) must express the field values as CDATA
i.e. <field tag="1"><![CDATA[test-account]]></field></field> .
The "toXML" code in QFJ deliberately treats the fields as CDATA, this make good sense. See line 408 of Message (in quickfixj-base in this branch, or quickfixj-core in master);
408 : final CDATASection value = document.createCDATASection(field.getObject().toString());
At some point on this branch the field data was not being out as CDATA, the test expectations are now the same as on master and the XML processing is as expected.
Just making a note here that I need to look a changes to SerialisationTest in quickfixj-core and whitespace in mina/ssl tests.
SerialisationTest was moved to quickfixj-messages/quickfixj-messages-all. mina/ssl tests had only whitespace (LF) changes, checked them out from master.
I tried to build with -Pminimal-fix-latest but got The requested profile "minimal-fix-latest" could not be activated because it does not exist.
I tried to build with
-Pminimal-fix-latestbut gotThe requested profile "minimal-fix-latest" could not be activated because it does not exist.
Will have a look asap
I think it was a problem on my end. I did a fresh clone of your repo and branch and it worked. Edit: but actually I need to check again if I didn't miss that message. :)
But I get a lot of JavaDoc errors and the build fails because of that (when I omit the -Dmaven.javadoc.skip=true option).
Just a question: by which criterion did you decide what goes into quickfixj-base? Just wondering whether it would be good to have that as a separate PR. I could do that if you like.
But I get a lot of JavaDoc errors and the build fails because of that (when I omit the
-Dmaven.javadoc.skip=trueoption). Just a question: by which criterion did you decide what goes intoquickfixj-base? Just wondering whether it would be good to have that as a separate PR. I could do that if you like.
quickfixj-base is meant to be only the immediate dependencies of the generated code and there are some other important and stable things that do not depend on the generated messages. Of course the way it is written, it still depends on some fields that are in std headers so these alone are generated for this module. One of the things that is suboptimal is that many tests must remain in quickfixj-core, for example it may be possible to pull some tests out of MessageTest in core, ones that don't require all the message builds. As you know this is all so that core->messages->base. For a QFJ developer, this means you can do mvn clean install quickfixj-base; mvn clean install quickfixj-messages; and then iterate more quickly over builds of core without needing to rebuild those modules. This is motivated by "Stable dependencies principal". Similarly quickfixj-core now only has test dependencies on quickfixj-messages. This is so that the message generation (and FIX specs) can be verified by the "home project" and prove that these "home" artefacts are demonstrably safe to use, but allows for much "easier" generation of custom message libraries by allowing them to be dropped in at run time . Custom libs can even published on maven central without conflicts with qfj namespace.
I don't think we can refactor base independently from much of the rest of the PR. It would still depend on the fields and messages in the existing way of generating code. Once we see that a lot of those changes are just moving things to base, there is not as much left to understand as it would first appear. Quite a lot of it is documents.
There are a number of changes related to preferring the enumeration names and similar from FIX Latest. I have tried to design the solution so that existing QFJ users could even quite easily work around that by omitting FIX Latest from their custom message builds. This PR proposed though that the home project though, prefers FIX Latest field names etc...
I did clean up some mvn errors in the existing code generation plugin, that can be independent. The generation of the QuickFIX/J Orchestration can be added independently but is of no real use.
I hope you don't get me wrong. I am all for this PR, I just need to look at it from the maintainer's perspective. :) I am just thinking how we could get this (or a new) PR a little more to a state which could be comprehended more easily (w.r.t. the changes to the structure of the project).
E.g. the history contains old changes (e.g. related to Travis) which are outdated now. I know this is my fault since it took me a long time to look more thoroughly at this PR. :-/ This (and generally the large number of commits) makes it very hard for me to make sure whether all changes done to the classes in core in the last months are also part of this PR.
P.S.: Please don't hate me. :)
Understood, I have tried to keep up but even with git it is not always easy. Core should be quite straightforward but I did struggle with the last merge. I think that we need to have an extra step to compare checked out branches on the file system, just to be certain some things have not gone backwards.
@chrjohn I think we should build some examples of Client Applications that build cut down/simple examples of custom message builds as discussed in the docs. I proved the concept with work on FIX APIs I was doing for my former employer and but we can make it explicit and compact and visible for users to explore. It can have integration tests of one kind or another. The examples could be in a multi-module project in a new repo that should be separate from the quickfix/j repo. It can look like :
- Here is a custom orchestration (derived from the published quickfix/j orchestration)
- Here is one way to generate and package your messages
- Here is another way to generate and package the messages
- Here is a build that uses the latest QuickFIX/J but builds using only using the FIX Dictionary and without FIX Latest, to provide an upgrade pathway for QuickFIX/J 2.n users. They can test QuickFIX/J 3.n against existing code with minimal changes.
I would suggest removing the existing examples from the home quickfixj repo and moving them to this new repo.
The tests provide a way to verify that new QFJ releases don't break this scheme.
@david-gibbs-ig I think those examples would help a lot! There are often questions dealing with customising data dictionaries and the build respectively.
I would suggest removing the existing examples from the home quickfixj repo and moving them to this new repo.
That is probably a good idea, too. I could do that. I think it shouldn't interfere with your changes (apart from minor changes you did that I just commented).
Edit:
In general I still have headaches about tests being separate from the tested class (e.g. Message in base but MessageTest in core. Maybe this needs to be split up in two separate tests then?
In general I still have headaches about tests being separate from the tested class (e.g.
MessageinbasebutMessageTestincore. Maybe this needs to be split up in two separate tests then?
Yes - I agree 100% . As a hasty check I commented out the Message package imports in MessageTest and I think there are a number of message tests that can be moved to a MessageTest(Base) in quickfixj-base.
@chrjohn I've moved lots of classes back to core to reduce the number of changes vs master, and to conform to the principle that quickfixj-base should contain, as far as possible, only the things that are dependencies of the generated code. When I did the last changes to split tests up where it may help, I could see that I had moved a number of classes on which the generated code does not depend.
@chrjohn Hi, I can't see the snyk report to see what the problem is, and I can't get snyk working on my forked repo. Can you tell me what the issue is please ?
Do you know which plugin triggers this?
It is a transitive dependency of "org.apache.maven:maven-core". scope of guava is now "provided" rather than "test" to match the expected scope of maven. At some point I changed the scope of maven things to "provided" as advised by warning messages but I guess I left "guava" as test. Reported security vulnerability should be resolved by managing the transitive dependency. Later I will conform if this is resolved in later versions of maven.
Please excuse the dumb question, am not in reach of my dev machine now. What is maven core plugin used for? Don't recall that we had that dependency before and also have a maven mojo for the code generator.