Open ggershaw opened 1 year ago
ggershaw
commented
1 year ago 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
commented
1 year ago @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
commented
1 year ago Hi,
Thanks for your response. I reported this as bug because the class generation does not work with the standard fix dictionary, which I would say is the reference as opposed to being there as a reference.
The modified fix file has a comment that notes the fix field name has been changed to allow for class generation. To me, this implies the standard fix dictionary is not supported and that is why I reported the bug. I and Coworkers had tried to generate venue specific classes and we couldn't do it with the fix file from the venue. We had to alter it to stop the generating duplicate method names.
Not sure who decides if this is indeed a bug, but that is why I reported it.
Best,
Geoff
Get Outlook for Androidhttps://aka.ms/AAb9ysg
From: david-gibbs-ig @.> Sent: Sunday, January 8, 2023 9:59:44 AM To: quickfix-j/quickfixj @.> Cc: Raleigh Fix Fan @.>; Mention @.> Subject: Re: [quickfix-j/quickfixj] quickfixj-codegenerator plugin generates classes which don't compile (Issue #584)
@ggershawhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fggershaw&data=05%7C01%7C%7Cd08fc074ff2841b63bbd08daf188fb99%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638087867879241715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ifYwikkxlvQjmp2%2BLflcXk9YFNxQMbd6Y4ZMq0Wm9M4%3D&reserved=0 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 referencehttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.github.com%2Fen%2Fget-started%2Fquickstart%2Fcontributing-to-projects&data=05%7C01%7C%7Cd08fc074ff2841b63bbd08daf188fb99%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638087867879241715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=61tZxj%2FRYjTGD%2BkLTgEqK3ViJSUUlixg0laYWgFSQAY%3D&reserved=0
— Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fquickfix-j%2Fquickfixj%2Fissues%2F584%23issuecomment-1374855844&data=05%7C01%7C%7Cd08fc074ff2841b63bbd08daf188fb99%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638087867879241715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=IccgfbWnnLIayMmssTW%2B7YRxgzerfsMI2wGP3ZrKEfY%3D&reserved=0, or unsubscribehttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAGD4VG4RBEFHOHZCAJ6LFJTWRLI6BANCNFSM6AAAAAAS3HHL6Y&data=05%7C01%7C%7Cd08fc074ff2841b63bbd08daf188fb99%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638087867879241715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Bd%2Bsns3%2BUgJqr%2BeKQzXRmEANcNqeeIeAS8hGONhMf2o%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>
david-gibbs-ig
commented
1 year ago @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 https://github.com/quickfix-j/quickfixj/pull/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
commented
1 year ago Hi,
That is what I observed. Someone added the word Data to the field name you mentioned. It does cause Java method names to conflict.
Best,
Geoff
Get Outlook for Androidhttps://aka.ms/AAb9ysg
From: david-gibbs-ig @.> Sent: Monday, January 9, 2023, 2:24 PM To: quickfix-j/quickfixj @.> Cc: Raleigh Fix Fan @.>; Mention @.> Subject: Re: [quickfix-j/quickfixj] quickfixj-codegenerator plugin generates classes which don't compile (Issue #584)
@ggershawhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fggershaw&data=05%7C01%7C%7C0cecb421895c497f773408daf2771fb2%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638088890703669297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=3auK7WnAWbOfQOY0Mv66i0j%2Fb2YfQ%2BHt1FsxQI71P8w%3D&reserved=0 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-dictgeneratorhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fquickfix-j%2Fquickfixj%2Ftree%2Fmaster%2Fquickfixj-dictgenerator&data=05%7C01%7C%7C0cecb421895c497f773408daf2771fb2%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638088890703669297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=9nSDECZhRXHnK%2F6lMvjfbFxe32QBPQxrSeyR7pObC%2Bw%3D&reserved=0 module. See comment: * This generator only works with the FPL 2008/09 repository files (http://fixprotocol.org/repository-2008https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ffixprotocol.org%2Frepository-2008&data=05%7C01%7C%7C0cecb421895c497f773408daf2771fb2%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638088890703669297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=xv8mdAx425L8W8A8WNyYOKNAbdSCZ1KXJ1GGgLOdPNY%3D&reserved=0).
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 ?
This is all a bit mucky. If you are interested in this topic you might like to look at the new FIX Orchestra projecthttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fquickfix-j%2Fquickfixj-orchestra&data=05%7C01%7C%7C0cecb421895c497f773408daf2771fb2%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638088890703669297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=mv%2BPTow750zWycrLQFszibyqMGEp1yc6SLgI25Z6Ny8%3D&reserved=0 . 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 #460https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fquickfix-j%2Fquickfixj%2Fpull%2F460&data=05%7C01%7C%7C0cecb421895c497f773408daf2771fb2%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638088890703669297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=TFJg61Qd7Ju%2BVYKu1kuQFKDg75QRUZouJmEv2Z2YCD8%3D&reserved=0 , 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).
— Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fquickfix-j%2Fquickfixj%2Fissues%2F584%23issuecomment-1376165741&data=05%7C01%7C%7C0cecb421895c497f773408daf2771fb2%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638088890703669297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=J3IXvF0TR6FQf7k7cyLRX1hCOa21LhHI5PA21jej9eQ%3D&reserved=0, or unsubscribehttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAGD4VG7Y2EZ3D5PIM3Y2EW3WRRQWPANCNFSM6AAAAAAS3HHL6Y&data=05%7C01%7C%7C0cecb421895c497f773408daf2771fb2%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638088890703669297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=owVCx%2FBpOk9%2Bee%2BBu3guSmsoIeOxTnmwQQ0JL%2Fq20Gc%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>
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.
`
`
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:
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 :)