moosetechnology / FamixSQL

A Metamodel for SQL
MIT License
1 stars 2 forks source link

Error trying to visualize the SQL model using SQLRSTableGroupVizualisation #10

Open jeffsantos opened 1 year ago

jeffsantos commented 1 year ago

Hi. Thanks for the great job with this package. I tried to use it in a standard Moose 10 image, but I got some problems. As I managed those troubles, I decided to share what I found with you.

I followed the instructions in the README and I could connect and generate a model from an ODBC database (SQLServer, in my case). But I noticed an error trying to generate the visualization as described in the README. The SQLRSTableGroupVizualisation>>extractAssociationFor:with: in the main branch calls for messages that do not exist in the target objects. I think the API of the SQL-Model objects has changed, and this method was not updated since then. Looking at the current version of the SQL-Model classes, I could find a running version for it:

extractAssociationFor: nodeColumn with: allNodes

    | ref |
    ref := (nodeColumn rawModel incomingForeignKeyReferences
                    collect: [ :foreignRef |
                        foreignRef foreignKeyConstraint columnReferences collect: [ 
                            :columnReference | columnReference column ] ]) flattened.
    nodeColumn dependenciesToNodes:
        (allNodes select: [ :aColumnNode | 
             ref includes: aColumnNode rawModel ])

But, It was not enough. The class HSimpleVisualizationBuilder that comes distributed in Moose 10 (package Hierarchical-Roassal3) also needs a change. SQLRSTableGroupVizualisation>>build starts a sequence of messages that reaches HSimpleVisualizationBuilder>>canvasController:. The version of this method in Moose 10 has a block that gives an error in the sequence of calls that starts with SQLRSTableGroupVizualisation. RSControlConfiguration does not understand noZoomToFitOnExtendChanged message. So, I changed this block as following:

From:

[ i configuration noZoomToFitOnExtendChanged ]

To:

[ i zoomToFitInteraction noZoomToFitOnExtendChanged ]

The last is the way it is implemented in Moose 11. But with Moose 11, I got errors in other parts of the SQL-Model package. Which version of Moose are you using to develop FamixSQL? This is a change that is out of the context of the FamixSQL package, but I do not find other way to manage that.

The SQLRSTableGroupVizualisation is also used by the MCD Tables inspector, so the above solution also solves the model visualization on this inspector. And there is another minor issue: The FamixSQLTableGroup>>inspectionTable method is duplicated in the image: it is in the class in the SQL-Model package, and as an extension in the SQL-Model-Extension package.

I am not sure if this is the best way to report. Maybe it should be better to separate each point into a different issue. But I decided to put it all together, since I need all of them to manage the visualization of the model.

Summarizing: with Moose 10 and the changes above, I got the right visualization of the generated model. I have forked your project and applied the changes. The change of the HSimpleVisualizationBuilder is not in the commits, since I made it direct in my image. You can check in https://github.com/jeffsantos/FamixSQL.

NicolasAnquetil commented 1 year ago

hello. Thanks for reporting this and taking the time to correct things

Some precisions:

Will keep in touch

jeffsantos commented 1 year ago

Hi Nicolas, thanks for your prompt answer. I have been in the Pharo/Moose/Gtoolkit world recently (about a year) but I understand everything you point about the complexity of the projects. I am also a professor/research (in Brazil) so I really know what you mean. My intent usage of FamixSQL (and all Pharo ecosystem) is in academic projects and teaching. So I will keep my eyes on the evolution of things and I will try to contribute in any way as I can.

badetitou commented 1 year ago

Hello @jeffsantos

I have merged your work in #11, thanks again for your contribution

I also looked at the problem with Roassal. The API of Roassal for HNode changed from the version I have on Moose10 and Moose11. It is a breaking change on the node styler API (the stuff that draws the rectangle representing the tables and columns).

I found ways to fix the bug for Moose10 or Moose11, but not one convenient way to have the fix for both. I suppose that we will have to create two versions of the visualization For now, I decided to change the visualization by removing the usage of this styler. So it should work in both 10 and 11 versions.

I also renamed the class SQLRSTableGroupBuilder since it is actually a builder looking at its hierarchy (It was not correctly done previously, more a hacky thing)

Please don't hesitate to push PR to this repository. We'll also have to find time to write proper documentation on the moose website https://modularmoose.org/

jeffsantos commented 1 year ago

Hi Benoit. Thanks for the merge. I've synced my fork with the upstream, so I can continue following your progress. Anything I catch that I could solve, I will push via PR.

I tried the synced version on two fresh images of Moose, 10 and 11. The new version is working on 11 now. But is not on Moose 10, the visualization still fails. I noticed that now, the renamed SQLRSTableGroupBuilder inherits from HSimpleVisualizationBuilder, so the build message still reaches the canvasController method. That is the point of the code that gives the error, as I described in my first message. But, as SQLRSTableGroupBuilder now inherits from HSimpleVisualizationBuilder, a temporary solution may be to override the canvasController on it, change the problematic block.

canvasController: aCanvas

    ^ RSCanvasController new
          noLegend;
          in: [ :i | 
              aCanvas newAnimation
                  duration: 500 milliSeconds;
                  onStepDo: [ aCanvas signalUpdate ];
                  when: RSAnimationEndEvent
                  do: [ i zoomToFitInteraction noZoomToFitOnExtendChanged ]
                  for: self.
              (i interactions at: RSDraggableCanvasInteraction) left.
              i configuration
                  maxScale: 3;
                  minScale: 0.5
              "useBasicZoom;" ];
          yourself

I committed and pushed this to my fork, tested it over Moose 10 and 11 e now it is ok on both. For sure, it is not a good design, but at least it avoids the error temporarily. If you think it's worth, I can send a new PR. To contribute a best solution, I will need to understand better the Hierarchical-Roassal package. I will definitely try to do that, but it may take me a while.

badetitou commented 1 year ago

Thanks @jeffsantos , I'll have a look at what you've done. If it works, it is already super great.

You'll notice that I made a more complete page for the FamixSQL documentation here https://modularmoose.org/moose-wiki/Users/famix-sql/getting-started-with-famixsql.html

I'm trying to have all the documentation at the same place (else it is too hard to manage)

jeffsantos commented 1 year ago

Great! The documentation will be super helpful. I will check it. I'll also follow here for any updates about the fix. Thanks.