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.

Sign up for GitHub

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

Jump to bottom

quickfixj-codegenerator plugin generates classes which don't compile #584

Open
ggershaw opened this issue Dec 11, 2022 · 5 comments
Open

quickfixj-codegenerator plugin generates classes which don't compile #584

ggershaw opened this issue Dec 11, 2022 · 5 comments
Labels
bug

Comments

@ggershaw
Copy link

Describe the bug
In 3.0.0-SNAPSHOT, the maven plugin generates code that doesn't compile when its run against the following dictionaries:
FIX44.xml
FIX50SP1.xml
FIX50SP2.xml

In each of the directories that contain the above files, there is another file called "fileName".modified.xml, where fileName is:
FIX44, FIX50SP1, or FIX50SP2. If you change the dictfile tag of the plugin's config to point to any of the below the source will be generated by the plugin, but that source will fail compilation :
FIX44.xml
FIX50SP1.xml
FIX50SP2.xml

To Reproduce

Change the dictFile tag found in the pom.xml of quickfixj-core to point to FIX50SP2.xml found in the same dir as FIX50SP2.modified.xml. Snippet of 1 problem execution is below. There are identical problems with 44 and 50sp1.

<execution> <id>fix50sp2</id> <goals> <goal>generate</goal> </goals> <configuration> <dictFile>../quickfixj-messages/quickfixj-messages-fix50sp2/src/main/resources/FIX50SP2.modified.xml</dictFile> <packaging>quickfix.fix50sp2</packaging> <fieldPackage>quickfix.field</fieldPackage> <decimal>${generator.decimal}</decimal> </configuration> </execution>

  1. run mvn clean install.
    the following error will be generated. There will be a total of 100 compilation errors. I think its 1 per class generated in the component pkg, but I'm not sure

method getSecurityXML() is already defined in class quickfixj.fix50sp2.component.Instrument

Steps to reproduce the behavior.
Or even better, a unit test or reproducer.

Expected behavior
Classes generated by plugin should be able to be compiled

system information:

  • OS: all
  • Java version any
  • QFJ Version : only tested on 2.3.1

Additional context
Diff FIX50SP2.modified.xml against FIX50SP2.xml and you can see someone has changed things to prevent the error by renaming the 1 of the generated offending methods. Very clever :)

For now, we could leave as it is, or remove the non *.modified.xml files to prevent confusion. We could add some comments in the pom.xml or readme to explain it.

In the long run it would be great to generate a method with a different name if the class already has the method. This would mean the plugin logic etc would need to change and that's beyond me at this point :)
@ggershaw ggershaw added the bug label Dec 11, 2022
@ggershaw
Copy link
Author

If you give me rights to create branch or create one for me, i'll remove the offending xml files leaving just the .modified.xml files where applicable. I'm brand new to github, as we use bitbucket @work.

@david-gibbs-ig
Copy link
Contributor

@ggershaw I noticed this bug report. I might think of it as a documentation defect rather than a bug. I'm not certain that the role of these files is documented.
The QuickFIX/J build generates files using only the modified xml files where they exist.
I think that the unmodified files are there for reference, and presumably for comparison with QuickFIX project assets. Perhaps this can cause unnecessary confusion.
P.S. If you want to contribute code and other changes in future the usual way is for you to Fork the QFJ repo, you can then make any changes on a branch of your repo. When you push your changes to your repo you will have an option to open a Pull Request for the original (upstream) repo. This is the general approach for git projects, please see this reference

@ggershaw
Copy link
Author

ggershaw commented Jan 9, 2023 via email

@david-gibbs-ig
Copy link
Contributor

@ggershaw I see, I think it's fields like SecurityXML that have caused you a problem. These comments are just my opinion I don't have any official responsibilities in this project.

The QuickFIX Dictionary is not an official FIX Trading Community document, I suspect it was generated from a legacy "FIX Repository file" by the org.quickfixj.dictgenerator from the quickfixj-dictgenerator module. See comment: * This generator only works with the FPL 2008/09 repository files (http://fixprotocol.org/repository-2008).

The format of the QuickFIX dictionary is I think common in the QuickFIX family of FIX Engines. If I understand you well, you were supplied a custom dictionary but encountered this incompatibility. I think the root cause is that in the original FIX repository the same name has been used for both a component and a field. Since the QuickFIX/J code generation does not add a suffix for the FIX structure type (component, group), if I recall correctly in Java you end up with a method name conflict on the class. Someone resolved this by adding "Data" to the field name. Does this fit the error you observed ?

<component name="SecurityXML" required="N"/>
<field number="1185" name="SecurityXML" type="XMLDATA"/>

This is all a bit mucky.
If you are interested in this topic you might like to look at the new FIX Orchestra project . This is part of ongoing work to build FIXLatest and add to QuickFIX/J the capability of building Messages and related classes from a FIX Orchestra Repository. It is based on work from the FIX Trading Community. I think I worked around the issue you described here, in a way that is not entirely consistent with the legacy code generation but that is more defensive. I don't think that I addressed this problem in the QuickFIX Dictionary generation since the Dictionary is meant to fit better with the legacy scheme. Components are de-normalised in the legacy code generation but it does make me think that we should have a test for this SecurityXML case - for both receiving and sending messages.

Please see also #460 , there are readme s that attempt to explain the benefit of proposed changes to the structure of QFJ to work with FIX Orchestra (together with FIXlatest).

@ggershaw
Copy link
Author

ggershaw commented Jan 12, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ggershaw @david-gibbs-ig