archimatetool / archi

Archi: ArchiMate Modelling Tool
https://www.archimatetool.com
MIT License
944 stars 269 forks source link

BUG - Model-importer doesn't update some information (access type, influence strenght...) #746

Closed jbsarrodie closed 3 years ago

jbsarrodie commented 3 years ago

As discovered in #740

Here's a reproductible example of the bug:

This is because ModelImporter#updateObject() only updates Name, Documentation, Properties and Features. So the same issue exists in the following cases:

The agreed approach to solve it is to make ModelImporter#updateObject() more generic so that it updates all EStructuralFeature.

I'm working on it.

Phillipus commented 3 years ago

There are some useful refactorings in the specialization-modelimporter branch that you should build on, too. So maybe best to continue in this branch rather than working in master

jbsarrodie commented 3 years ago

There are some useful refactorings

Ok, I was not aware of it.

Phillipus commented 3 years ago

Some useful info:

Phillipus commented 3 years ago

Ok, I was not aware of it.

It might make it harder to merge everything.

Phillipus commented 3 years ago

I don't know if a generic Ecore type method will be able to do what we are doing in UpdatePropertiesCommand and UpdateFeaturesCommand where we are replacing all properties and features.

jbsarrodie commented 3 years ago

Just pushed a generic update of EAttribute (using commands)

I don't know if a generic Ecore type method will be able to do what we are doing in UpdatePropertiesCommand and UpdateFeaturesCommand where we are replacing all properties and features.

It won't and I don't think it makes sense to do it differently (your approach for properties and features is already generic).

Phillipus commented 3 years ago

My OCD is playing up with your git commit comments. Can you put a space between the commit message and any further comments? ;-) (Otherwise it shows on all one line in SmartGit)

jbsarrodie commented 3 years ago

My OCD is playing up with your git commit comments. Can you put a space between the commit message and any further comments? ;-) (Otherwise it shows on all one line in SmartGit)

No pb ;-)

BTW, I'm assuming that we'll squash these commits (the last one triggers my own OCD...)

Phillipus commented 3 years ago

BTW, I'm assuming that we'll squash these commits (the last one triggers my own OCD...)

Yes, I can tidy up anything as well.

I figured out how to do a commit but set the author to someone else.

jbsarrodie commented 3 years ago

I figured out how to do a commit but set the author to someone else.

Temporarily change your email?

Phillipus commented 3 years ago

Temporarily change your email?

No, it's something in SmartGit. Commit locally and then choose "Edit Author..."

Phillipus commented 3 years ago

I've updated the specialization-modelimporter branch.

TODO: Objects needs to refresh in Views and Properties tab if a Profile/Image has changed. This worked before because I was (wrongly) making a clone of the Profile and re-adding it which would force a refresh in the UI. Now I need to force a refresh if Profile is updated.

Phillipus commented 3 years ago

Problem now is if a Profile is renamed in the source model and target model was using it. That's not working...we end up with two Profiles with the same ID.

Phillipus commented 3 years ago
  1. Source model has Profile: "P1", "id123", "BusinessActor"
  2. Target model imports
  3. Source model changes name of "P1" to "P2"
  4. Target model imports and now has P1 and P2 with same ID

And:

  1. Source model has Profile: "P1", "id123", "BusinessActor"
  2. Target model imports
  3. Source model changes name of "P1" to "P2" and creates a new Profile "P1", "id456", "BusinessActor"
  4. Mess
jbsarrodie commented 3 years ago

Problem now is if a Profile is renamed in the source model and target model was using it. That's not working...we end up with two Profiles with the same ID.

I know, that's were I left before working on ModelImporter#updateObject(). This doesn't only affect model importer but also coArchi. And if we store images with the original file name instead of a hash, then it might happen that two images have the same path...

So I was trying to figure out the best way to solve this...

Phillipus commented 3 years ago

And if we store images with the original file name instead of a hash,

Actually, its not a hash, just a generated ID.

jbsarrodie commented 3 years ago
  • Source model has Profile: "P1", "id123", "BusinessActor"
  • Target model imports
  • Source model changes name of "P1" to "P2"
  • Target model imports and now has P1 and P2 with same ID

No, we first lookup Profiles by ID, and only by Name+Type if no ID matches. So it will get renamed (at least it was with my implementation).

And:

  1. Source model has Profile: "P1", "id123", "BusinessActor"
  2. Target model imports
  3. Source model changes name of "P1" to "P2" and creates a new Profile "P1", "id456", "BusinessActor"
  4. Mess

Same, matched by ID first wins.

The only issue I see is (assume same concept type for profiles):

  1. Source has profile "P1"
  2. Target has profile "P2"
  3. Import Source into Target
  4. Target now has "P1" and "P2"
  5. User rename "P1" to "P2" in Source
  6. Import again Source into Target
  7. Source "P1" is renamed to "P2, we now have two profiles named "P2" for the same concept type
jbsarrodie commented 3 years ago

Actually, its not a hash, just a generated ID.

Damn it autocorrect ;-) That's what I wanted to write

Phillipus commented 3 years ago

No, we first lookup Profiles by ID, and only by Name+Type if no ID matches.

OK, I'll refactor to do this and see what we get.

Phillipus commented 3 years ago

Now findObjectInTargetModel() checks for an existing object by its ID, then if not found by name + class

jbsarrodie commented 3 years ago

OK, I'll refactor to do this and see what we get. Now findObjectInTargetModel() checks for an existing object by its ID, then if not found by name + class

Good. I'll test later today.

The only issue I see is (assume same concept type for profiles):

  1. Source has profile "P1"
  2. Target has profile "P2"
  3. Import Source into Target
  4. Target now has "P1" and "P2"
  5. User rename "P1" to "P2" in Source
  6. Import again Source into Target
  7. Source "P1" is renamed to "P2, we now have two profiles named "P2" for the same concept type

For this, I think we could raise an error, or merge them and raise a warning (user can then decide to undo).

This also makes me think that it would wake sense to offer a way for users to "merge" specialization. For example one can decide to have two specialization SQL DataBase and NoSQL DataBase, and later decide to merge to only on DataBase. Not urgent now but for another release someday (jArchi will solve it in the meantime).

Phillipus commented 3 years ago

For this, I think we could raise an error, or merge them and raise a warning (user can then decide to undo).

Or rename one of them by adding a suffix or prefix P2 (2).

jbsarrodie commented 3 years ago

Or rename one of them by adding a suffix or prefix P2 (2).

That's another option.

In fact, maybe we should have a new option in the import wizard: Import and merge profiles by name:

Phillipus commented 3 years ago

Heads up - I've rebased the specialization-modelimporter branch on top of new changes in the specialization branch.

Phillipus commented 3 years ago

Thinking about your merge option ideas...why not import all Profiles by ID first and then if there are duplicates by name then merge these with a rule that the source is the winner or the target is the winner? We can't really "merge" because we have to choose which image to use.

Phillipus commented 3 years ago

Note: The specialization and specialization-modelimporter branches have been rebased onto the latest master branch.

jbsarrodie commented 3 years ago

Closing this issue as original bug has been solved