search

finos / legend-studio

Legend Studio
https://legend.finos.org
Apache License 2.0
87 stars 114 forks source link

Bug: Explorer Tree's Rename and Remove operations on PackageableElements causing text recompile failure, not cleaning up references #3432

Open jimnelson372 opened 4 weeks ago

jimnelson372 commented 4 weeks ago

Similar issues

How are you using Studio?

Legend Studio

Current and expected behavior

Problem with the Removing elements from the EXPLORER tree's context menus.

When experimenting with Legend Studio, latest version from docker and also master branch from github, I discovered that when using the context menu in the Explorer tree to remove some elements, Legend Studio would end up in the text mode reporting the following error:

image

I believe that message goes on to say "Redirected to text mode..." or something along those lines, as I end up in the text editor with one or more errors depending on element I removed and elements referring to it.

I will list the types of elements (packageableElements) and conditions in the "steps to reproduce" section.

Problem with renaming some elements from the EXPLORER tree's context menus.

However, as I was exploring this further, I discovered similar problems with some elements when simply renaming them. While in many cases any reference to them by other elements were automatically updated, in some cases I received the following message:

image

And just as with the first problem when removing some elements, I was redirected to text mode with many errors in the text.

As a programmer, I was able to figure out what to do to resolve the errors, but since the Graphics interface is intended to allow non-programmers to edit the elements and relationships without needing to understand Legend Pure in depth, my understanding is that for the feature allowing the removal of an element, Legend Studio should either automatically clean up references to removed elements, or it should warn the user that there are dependent elements and possibly not allow the element to be removed until all references to it are removed. For renaming, Studio should ensure all references are properly renamed.

Unique circumstances for the problems to occur

So far, it seems like these problems with renaming and removing occur with elements that are referenced by Mappings, by Associations, or by FileGenerations that are included in a Generation Specification.

Steps to reproduce

Test Pure code

I provided the Legend Pure script I have used to demonstrate the problem in the "Model data" section below. It is based on "Legend Showcase - Installer Demo" but without the Postscript db and with a FileGenerator and GenerationSpecification added in the "generator" package.

Unique circumstances for the problems to occur

So far, it seems like these problems with renaming and removing occur with elements that are referenced by Mappings, by Associations, or by FileGenerations that are included in a Generation Specification.

Reproducing the Remove problem when removing a File Generation element.

  1. Use the script provided in the "Model data" section and exit text mode.
  2. Expand the "generate" package. You'll see two elements, "genjson" and "MyGenerationSpecification".
  3. Click on "MyGenerationSpecification" and in the graphical view, in the lower section "File Generations" you'll find that "genjson" is listed.
  4. Back on the EXPLORER tree, right click on "genjson" to get its context menu and click on "Remove".
  5. You'll see the error message I wrote about, and you'll be in text mode.
  6. The problem, you'll see, is that generate::genjson is still in the file generation list, even though that element no longer exists in the script.
image
  1. In order to go back to the graphical mode and properly recompile the script, remove "generate::genjson" from between the brackets (leaving that list empty). It's easy enough to fix, but non-programmers shouldn't need to worry about this.
  2. Renaming of a File Generation element, on the other hand, works just fine.

Reproducing the Remove problem when removing a Database element

  1. Use the script provided in the "Model data" section and exit text mode.
  2. Expand the "database" package. You'll see the single database element: db1
  3. Right click on db1 to get its context menu and click on "Remove"
  4. You'll see the error message I wrote about, and you'll be in text mode.
  5. Here, the problem is that there is a Relational Mapping using tables and fields from that database. But the "Remove" did not clean up this dependency, nor did it warn the users of the dependency and prevent the database element from being removable. There error looks like:
image
  1. For a regular user to fix this problem, it's much more difficult than the File Generation problem. They would have to know how to edit out the mapping or know how to type a new database definition, etc. Legend Studio doesn't provide a means to undo the action (to my knowledge) when the problem was generated this way.

Reproduction the Rename problem when renaming a Database element

Unlike the Generation Specification rename working even though its remove has problems (first case above), with Mappings, even renaming the database element has problems.

  1. Use the script provided in the "Model data" section and exit text mode.
  2. Expand the "database" package. You'll see the single database element: db1
  3. Right click on db1 to get its context menu and click on "Rename"
  4. When the Rename Element dialog appears, change "database::db1" to "database::db2" and press "Rename".
  5. You'll get the rename error message and you'll be in text mode.
  6. The problem here is again with the Relational Mapping. The table mapping automatically now references "database::db2" but the element mappings are incorrectly still referencing "database::db1", which no longer exists.
image
  1. In this case, though not desirable, the user can fix the problem by using the text editing's Find/Replace feature, either renaming "database::db2" back to "database::db1" or the other way around.

Rather than have the complete steps for other cases, I'll just name them:

image

There may be additional cases, but the one's above are those I have discovered so far.

Model data

###FileGeneration
JsonSchema generate::genjson
{
  scopeElements: [database, domain, mapping, connection, diagram, generate];
}

###GenerationSpecification
GenerationSpecification generate::MyGenerationSpecification
{
  fileGenerations: [
    generate::genjson
  ];
}

###Diagram
Diagram diagram::all
{
  classView 0dee659c-6d6d-41d0-a326-dd0e63250732
  {
    class: domain::Firm;
    position: (639.0,202.0);
    rectangle: (134.32373046875,58.0);
  }
  classView 860aafab-2ae2-4714-8f4e-7d8924c6cec8
  {
    class: domain::Person;
    position: (644.0,327.0);
    rectangle: (124.521484375,58.0);
  }
  propertyView
  {
    property: domain::Person.firm;
    source: 860aafab-2ae2-4714-8f4e-7d8924c6cec8;
    target: 0dee659c-6d6d-41d0-a326-dd0e63250732;
    points: [(706.2607421875,356.0),(706.161865234375,231.0)];
  }
  propertyView
  {
    property: domain::Firm.employees;
    source: 0dee659c-6d6d-41d0-a326-dd0e63250732;
    target: 860aafab-2ae2-4714-8f4e-7d8924c6cec8;
    points: [(706.161865234375,231.0),(706.2607421875,356.0)];
  }
}

###Relational
Database database::db1
(
  Table PERSON
  (
    ID INTEGER PRIMARY KEY,
    FIRMID INTEGER,
    FIRST_NAME VARCHAR(200),
    LAST_NAME VARCHAR(200)
  )
  Table FIRM
  (
    ID INTEGER PRIMARY KEY,
    LEGAL_NAME VARCHAR(200)
  )

  Join Firm_Person(PERSON.FIRMID = FIRM.ID)
)

###Pure
Class domain::S_Person
{
  fullName: String[1];
}

Class domain::Person
{
  firstName: String[1];
  lastName: String[1];
}

Class domain::Firm
{
  legalName: String[1];
}

Class domain::S_Firm
{
  legalName: String[1];
}

Association domain::Firm_Person
{
  firm: domain::Firm[1];
  employees: domain::Person[*];
}

Association domain::S_Firm_S_Person
{
  firm: domain::S_Firm[1];
  employees: domain::S_Person[*];
}

###Mapping
Mapping mapping::relational::Firm_Person
(
  domain::Person: Relational
  {
    ~primaryKey
    (
      [database::db1]PERSON.ID
    )
    ~mainTable [database::db1]PERSON
    firstName: [database::db1]PERSON.FIRST_NAME,
    lastName: [database::db1]PERSON.LAST_NAME
  }
  domain::Firm: Relational
  {
    ~primaryKey
    (
      [database::db1]FIRM.ID
    )
    ~mainTable [database::db1]FIRM
    legalName: [database::db1]FIRM.LEGAL_NAME,
    employees[domain_Person]: [database::db1]@Firm_Person
  }

  MappingTests
  [
    test_1
    (
      query: |domain::Firm.all()->project(
  [
    x|$x.legalName,
    x|$x.employees.firstName,
    x|$x.employees.lastName
  ],
  [
    'Legal Name',
    'Employees/First Name',
    'Employees/Last Name'
  ]
);
      data:
      [
        <Relational, SQL, database::db1, 
          'drop table if exists FIRM;\n'+
          'create table FIRM(ID INTEGER, LEGAL_NAME VARCHAR(200));\n'+
          'insert into FIRM(ID, LEGAL_NAME) values(100, \'ACME Corp.\');\n'+
          'insert into FIRM(ID, LEGAL_NAME) values(200, \'Monsters Inc.\');\n'+
          'drop table if exists PERSON;\n'+
          'create table PERSON(ID INTEGER, FIRMID INTEGER, FIRST_NAME VARCHAR(200), LAST_NAME VARCHAR(200));\n'+
          'insert into PERSON(ID, FIRMID, FIRST_NAME, LAST_NAME) values(1, 100, \'Road\', \'Runner\');\n'+
          'insert into PERSON(ID, FIRMID, FIRST_NAME, LAST_NAME) values(2, 100, \'Wile\', \'Coyote\');\n'+
          'insert into PERSON(ID, FIRMID, FIRST_NAME, LAST_NAME) values(3, 200, \'Jake\', \'Sullivan\');\n'+
          'insert into PERSON(ID, FIRMID, FIRST_NAME, LAST_NAME) values(4, 200, \'Mike\', \'Wazwoski\');\n'
        >
      ];
      assert: '[{"values":["ACME Corp.","Road","Runner"]},{"values":["ACME Corp.","Wile","Coyote"]},{"values":["Monsters Inc.","Jake","Sullivan"]},{"values":["Monsters Inc.","Mike","Wazwoski"]}]';
    )
  ]
)

Mapping mapping::m2m::Firm_Person
(
  domain::Person: Pure
  {
    ~src domain::S_Person
    firstName: $src.fullName->substring(
  0,
  $src.fullName->indexOf(' ')
),
    lastName: $src.fullName->substring(
  $src.fullName->indexOf(' ') + 1,
  $src.fullName->length()
)
  }
  domain::Firm: Pure
  {
    ~src domain::S_Firm
    legalName: $src.legalName,
    employees[domain_Person]: $src.employees
  }

  MappingTests
  [
    test_1
    (
      query: |domain::Firm.all()->graphFetch(
  #{
    domain::Firm{
      legalName,
      employees{
        firstName,
        lastName
      }
    }
  }#
)->serialize(
  #{
    domain::Firm{
      legalName,
      employees{
        firstName,
        lastName
      }
    }
  }#
);
      data:
      [
        <Object, JSON, domain::S_Firm, '[{"employees":[{"fullName":"Road Runner"},{"fullName":"Wile Coyote"}],"legalName":"ACME Corp."},{"employees":[{"fullName":"Jake Sullivan"},{"fullName":"Mike Wazwoski"}],"legalName":"Monsters Inc."}]'>
      ];
      assert: '[{"legalName":"ACME Corp.","employees":[{"firstName":"Road","lastName":"Runner"},{"firstName":"Wile","lastName":"Coyote"}]},{"legalName":"Monsters Inc.","employees":[{"firstName":"Jake","lastName":"Sullivan"},{"firstName":"Mike","lastName":"Wazwoski"}]}]';
    )
  ]
)

###Connection
RelationalDatabaseConnection connection::h2
{
  store: database::db1;
  type: H2;
  specification: LocalH2
  {
    testDataSetupSqls: [
      'drop table if exists FIRM',
      'create table FIRM(ID INTEGER, LEGAL_NAME VARCHAR(200))',
      'insert into FIRM(ID, LEGAL_NAME) values(100, \'ACME Corp.\')',
      'insert into FIRM(ID, LEGAL_NAME) values(200, \'Monsters Inc.\')',
      'drop table if exists PERSON;',
      'create table PERSON(ID INTEGER, FIRMID INTEGER, FIRST_NAME VARCHAR(200), LAST_NAME VARCHAR(200))',
      'insert into PERSON(ID, FIRMID, FIRST_NAME, LAST_NAME) values(1, 100, \'Road\', \'Runner\')',
      'insert into PERSON(ID, FIRMID, FIRST_NAME, LAST_NAME) values(2, 100, \'Wile\', \'Coyote\')',
      'insert into PERSON(ID, FIRMID, FIRST_NAME, LAST_NAME) values(3, 200, \'Jake\', \'Sullivan\')',
      'insert into PERSON(ID, FIRMID, FIRST_NAME, LAST_NAME) values(4, 200, \'Mike\', \'Wazwoski\')'
      ];
  };
  auth: DefaultH2;
}

###Runtime
Runtime connection::runtime::h2runtime
{
  mappings:
  [
    mapping::m2m::Firm_Person
  ];
}

Environment

Legend:
    Using the Docker-Compose Installer with all the Legend components with snapshot releases.
    Also when stopping the Studio in Docker and running the latest master branch from my
       local Visual Code.

  System:
    OS: macOS 12.7.6
  Binaries:
    Node: 22.3.0 - /private/var/folders/68/6jc0j8dd08bc5q544_rs_tjh0000gn/T/xfs-854339c9/node
    Yarn: 4.2.1 - /private/var/folders/68/6jc0j8dd08bc5q544_rs_tjh0000gn/T/xfs-854339c9/yarn
    npm: 10.8.1 - /usr/local/bin/npm
  Browser: Tested with Brave

Possible solution and workaround

Original Attempted Solution

When I first discovered the problems above, I was testing File Generation elements. I made an initial attempt to add a fix for this, by modifying the deleteElement method in GraphEditFormModeState.ts to have a handlePostDeleteAction, where I checked if the element was of instance FileGenerationSpecification and if so, I called a new method generationSpecification_deleteGenerationElementFromAll which I added to DSL_Generation_GraphModifierHelper.ts. While this worked for that case (I still have it), I realized it was not a general solution.

Looking for a more general solution

I have not used Mobx observers, with action, flowResult, etc, before, so I am not able to determine if the problems are due to missing observer relationships. Also, I could not find methods in the Store classes to allow me to navigate through the element tree (though I have not looked at its use of the visitor pattern yet).

Today, I noticed that in PureModel.ts, the deleteElement method looks for DeadReferenceCleaner plugins. But as far as I can tell from debugging in the browser, there are no current DeadReferenceCleaner plugins.

Happy to help, if somone can point me in the right direction.

I mentioned above that I have a specific solution the the File Generation removal. I can create a Pull Request just for that, if requested. However, I would rather have a solution were the Graph tree is properly iterated to properly update the dependencies for renamed and removed elements. Or, alternatively for the case of removal, if there is a method to search for element dependencies, the Remove option can be disabled in the context menu.

I am primarily a Java developer, though I am familiar enough with React and Typescript to make the changes above (not submitted) and to debug the code. However, as I mentioned, I have not used Mobx before, so I was not able to determine all the parts of the code that observe the rename and delete operations on these elements.

In any case, I hope the information I've provided here is helpful.

Regards,

Jim Nelson jimnelson372

Contribution