quickfix-j / quickfixj

QuickFIX/J is a full featured messaging engine for the FIX protocol. - This is the official project repository.
http://www.quickfixj.org
Other
955 stars 611 forks source link

`DataDictionary.getVersion()` ignores service pack #258

Closed drekbour closed 3 years ago

drekbour commented 4 years ago

Describe the bug dataDictionary.getVersion() ignores the XML's @servicepack attribute. A FIX.5.0.SP2 dictionary XML will return "FIX.5.0". This feels wrong ...

To Reproduce This from your own quickfix.MessageTest: ...

    final DataDictionary dataDictionary = new DataDictionary("FIX44.xml");
    final Message message = new DefaultMessageFactory()
            .create(dataDictionary.getVersion(), "D");
    message.fromString(expectedMessageString, dataDictionary, false);

... is a useful shorthand (creating/using the correct MessageFactory requiring only a vendor-provided dictionary). It is likely to cause bugs if it was FIX.5.0SP2. If message is passed to downstream code, it is highly likely somewhere it will be tested/casted with

if (message instanceof quickfix.fix50sp2.ExecutionReport) ...

and fail since quickfix.fix50.ExecutionReport is binary incompatible.

Expected behavior OPTION1 getVersion() to return "FIX.5.0SP2" where appropriate. OPTION2 add String getFullVersion() OPTION3 Explicitly expose int getServicePack()

system information:

I'm happy to PR this little change if stakeholders approve a design option.

drekbour commented 4 years ago

To preempt a likely question: The issue encountered here is a generic test-harness loads pre-canned QFJ Messages then passes them to version-specific production code.

philipwhiuk commented 4 years ago

I like doing option 3 regardless. We'd need to agree what it should return for FIX.4.4 et al. Maybe it should return an Integer instead of int.

However whatever we do should ideally also handle EP builds and FIX Latest (or at least shouldn't make it harder for us to do so).

I'm not too worried about changing getVersion() personally.

chrjohn commented 4 years ago

Hi @drekbour , where is that @servicepack coming from? We don't have it in our data dictionary XMLs. Is it in the original FIX repository that can be downloaded from the FIX Protocol website? We could of course add that information manually to our dictionaries.

BTW, what is the official notation for the versions + servicepack? E.g. you write FIX5.0.SP2. In FixVersions we have:

https://github.com/quickfix-j/quickfixj/blob/3a9e1046c3a6e98da766c74ddb82cec651bb4221/quickfixj-core/src/main/java/quickfix/FixVersions.java#L36-L37

philipwhiuk commented 4 years ago

The FIX TC website don't provide the data dictionary (at least not for the EPs). You have to generate it from the fixRepository file format. When I needed a MiFID compliant DataDictionary I had to use the FIX Orchestra's tooling, io.fixprotocol.orchestra.quickfix.DataDictionaryGenerator and io.fixprotocol.orchestra.transformers.RepositoryXslTransformer

The converter doesn't report the servicePack attribute:

https://github.com/FIXTradingCommunity/fix-orchestra/blob/master/repository-quickfix/src/main/java/io/fixprotocol/orchestra/quickfix/DataDictionaryGenerator.java#L147

drekbour commented 4 years ago

you write FIX5.0.SP2

Fixed that. Convention (an ugly one!) is FIX.5.0SP2

I won't pretend to follow exactly where the canonical source of truth is but these are the examples I was looking at (actually I was given a vendor-customised dictionary that had it) examples:

chrjohn commented 4 years ago

@philipwhiuk : I know that these need to be generated. :) QFJ has its own dictionary generator: https://github.com/quickfix-j/quickfixj/tree/master/quickfixj-dictgenerator But it only works on the old 2010 FPL repository format. Good to know that Orchestra's generator works for QFJ. I never got around to try it out. IMHO we could delete QFJ's dictionary generator since I don't see any benefit in making it work with newer versions of the FPL repository when Orchestra has a better alternative. As you mentioned they also don't pass the servicepack attribute but I hope that it should be no problem to file an issue or create a PR there which will be accepted. Maybe they should also add extensionpack?!

@drekbour OK, QuickFIX seems to have its own generator. Looking at the FIX4.4 dictionary they seem to use 0 as servicepack number which would answer @philipwhiuk 's question.

I like doing option 3 regardless. We'd need to agree what it should return for FIX.4.4 et al.

Not that we need to do it the same way, but it's an option.

Looking at https://fiximate.fixtrading.org/ we can also see that the official notation seems to be e.g. FIX.5.0SP2_EP254 for an extension pack and otherwise e.g. FIX.5.0SP2.

philipwhiuk commented 4 years ago

Yes, you have to use 2010 as an intermediary format in the Orchestra approach. I should probably push the code as an example to a public repo in docs or something. I'll put a PR together.

Is there a DataDictionary XSD anywhere?

EDIT:

Here's some code:

String repository = "FIXRepository_FIX.5.0SP2_EP240/Unified/FixRepository.xml";
String xsl = "2010Repository/Repository2010to2016.xsl";
String intermediate = "Transformed.xml";
String outputDir = "spec";
RepositoryXslTransformer.main(new String[]{xsl,repository,intermediate});
DataDictionaryGenerator.main(new String[]{intermediate, outputDir});
Files.delete(new File(intermediate).toPath());
chrjohn commented 4 years ago

Hi @philipwhiuk , sorry totally forgot about this. No, there is no XSD for DataDictionaries. There was one planned a few years ago but it wasn't really finished. https://www.quickfixj.org/jira/browse/QFJ-819

chrjohn commented 3 years ago

FYI, I have created a PR to add servicepack and extensionpack to the Orchestra-generated data dictionaries: https://github.com/FIXTradingCommunity/fix-orchestra-quickfix/pull/2

chrjohn commented 3 years ago

Putting this to the next minor version milestone but will not change getVersion() for the time being (see #381 ) but provided getFullVersion(), getServicePack(), getExtensionPack() now. Also added constant for FIXLatest.