Merge Folders and Concepts

[Phillipus]

The first thing I'm tackling for the import/merge operation is Folders and their contents.

"source" = model that is imported and merged "target" = model that receives the imported model

What happens in the following cases?

• A folder exists in source but not in target
• A folder exist in both but has been moved
• Other cases?

How should we handle merging folders and their child objects:

• concepts
• sub-folders
• views

[jbsarrodie]

First we have to define what exists means. My definition would be that an object exists in the model with the same type and the same Id. This means that if an object exists with the same Id but different type, operation should be aborted.

We also have to restate some generic principles when merging model:

• When requesting a merge, user should be prompted to choose between two behaviors in case of "conflict" (in this context a conflict happens when we want to import an object from source which exists on target):
  1. Keep version from source
  2. Keep version from target
• The granularity of comparison is and will always be the object as a whole. We well never try to detect or manage changes beyond that (ie. don't try to merge properties...). If someone needs such feature, then s/he should use the collaboration plugin to benefit from the underlying merge algorithm implemented through Git.

What happens in the following cases?

• A folder exists in source but not in target

We simply create it in target.

• A folder exist in both but has been moved

This is in fact one case of: a folder exist in both but has been changed (name, documentation, properties or location changed).

If Keep version from target then do nothing.

If Keep version from source then it should be restored to its original state (ie. we should restore its name, documentation and properties, and it should be moved back to its original location). This might involve doing it in two passes (1. import folders, 2. move them in their original location).

• Other cases?

Can't see any for the moment

[Phillipus]

First we have to define what exists means. My definition would be that an object exists in the model with the same type and the same Id. This means that if an object exists with the same Id but different type, operation should be aborted.

That's already taken care of.

don't try to merge properties...

That's not hard to do if we want to do that...

[jbsarrodie]

That's not hard to do if we want to do that...

I prefer an all or nothing approach at the moment. My purpose here is to say that we'll not create a dialog box similar to the one we have in coArchi, to manage conflicts.

[Phillipus]

We could do a simple merge of Properties and Features. If key exists in target then set the value, if not create a new one.

[jbsarrodie]

We could do a simple merge of Properties and Features. If key exists in target then set the value, if not create a new one.

But keys are not unique. So better not to bother.

[Phillipus]

We now have a first working version that merges/updates Folders and concepts.

• User is asked to save model before proceeding if needed
• A wizard asks for the model file and whether to update target model from source model
• Folders are updated
• Concepts are updated
• Features are updated
• Properties are updated
• If option above is chosen then relationship source and target ends are updated if they have been changed in the source model
• The undo/redo stack is reset (this is a temporary safety measure until full undo/redo is implemented for the import)

Please test this before we proceed onto merging Views!

[jbsarrodie]

Please test this before we proceed onto merging Views!

Ok, I'll do later today

[jbsarrodie]

Testing now !

Here is model A:

Here is model B:

1st Test A is opened, B is closed. Merging B into A with "update" option set.

• Merge is as expected
• Dirty flag is not set on A, so when closing without prior saving, it is not saved and changes are lost.

2nd Test Same as 1st Test + Rename a Folder and one of the Capabilities, delete from other Capabilities then import B into A again, "update" option not set.

• Merge as expected: changed concepts are not updated, deleted concepts are "restored"

**3rd Test"" Same as 2nd Test but with "update" option set.

• Merge as expected: changed and deleted concepts are restore.

4st Test Same as 2nd Test, but in addition create a view based on a changed concept and add a relationship between changed concept and an element from model A.

• Merge as expected: no impact on new view or relationship

5st Test Same as 4st Test but with "update" option set.

• Merge as expected: changed and deleted concepts are restore. No impact on new view or relationship

6st Test Same as 2nd Test, but create a view containing two capabilities from B and their composition relationship, then change on end of the relationship so that the composition now targets the renamed concept (which is also a capability)

• Merge as expected: no impact on new view or relationship

7st Test Same as 4st Test but with "update" option set.

• Merge as expected: changed and deleted concepts are restore. No impact on new view except the automatic update of the relationship which was expected.

Perfect!

Remark: When managing views, Test #6 and #7 should be looked at, because we might have some edge cases:

• Model B as already been imported into A
• Some relationship's ends has been changed
• Let's assume these relationships were used in a view "V" coming from B, but remove since
• We merge again B into A with "update" option not set
• "V" should be restored (because this is a merge and "V" no more exist), but "V" contains a relationhip whose ends have changed, so this relationiship can no more exist in "V"
• So in fact we can't restore "V"
• What do we do? Either we abort the whole merge operation because we can't merge/restore "V" exactly as it was in B, or we raise a warning?

Another edge case:

• Model B as already been imported into A
• Some relationship's ends has been changed
• Let's assume these relationships were used in a view "V" which does not come from B (ie. created after the first merge)
• We merge again B into A with "update" option set
• Because "update" option is set, the relationiship is restored to its original ends.
• "V" is impacted because now the relationship is no more valid in the context of the view
• What do we do? Warning?

[Phillipus]

Dirty flag is not set on A, so when closing without prior saving, it is not saved and changes are lost.

This is a side-effect of resetting the Command Stack. It can only be "dirty" if there's something on the Command Stack. This is temporary until we have it all working and I can look at sorting out undo/redo.

I'll open a new issue for Views...

[jbsarrodie]

This is a side-effect of resetting the Command Stack.

That was my assumption. Just wanted to confirm.

[jbsarrodie]

Slight bug: when "update mode" is on, the model element itself is also updated which override its documentation and properties (but shouldn't).

[Phillipus]

Slight bug: when "update mode" is on, the model element itself is also updated which override its documentation and properties (but shouldn't).

But surely that means replace them?

        // Upate root model information
        if(replaceWithSource) {
            targetModel.setPurpose(importedModel.getPurpose());
            updateProperties(importedModel, targetModel);
            updateFeatures(importedModel, targetModel);
        }

[jbsarrodie]

But surely that means replace them?

Seems so, but we really don't want to do this for the model element itself :-)

[Phillipus]

Seems so, but we really don't want to do this for the model element itself :-)

OK, easy enough to take that out.

[Phillipus]

With reference to bug in #2:

This happens when the relationship involves another relationship. Maybe you first import elements and then only relationship, assuming that then relationships can be imported without issues, but two passes (or some other trick) should be used.

I could use a simple test case if you have one?

Import Concept:

1. If concept does not exist, clone a new one
2. If concept does exist, update it
3. If concept is a new one OR we have selected "update with source" AND it's a relationship Update Relationship Ends

Update Relationship Ends:

1. Find sourceEnd concept in target model. If not found, Import Concept. Set the end.
2. Find targetEnd concept in target model. If not found, Import Concept. Set the end.

So if a relationship end is another relationship end that has not been imported yet a new relationship is created. When the relationship is imported, it is just updated. So it should work, but clearly it isn't. Hmmm.....

BTW - I've set it to run the Model Integrity Checker after the import, so you don't have to save it.

[Phillipus]

Hmmm... I get a bad feeling about the logic in ConceptImporter...

[Phillipus]

I've created a new branch "test" with some changes to ConceptImporter. I'm not sure if this will solve the bug of missing ends of a relationship. The bug might lie elsewhere, but if you could test it... :-)

[jbsarrodie]

I've created a new branch "test" with some changes to ConceptImporter. I'm not sure if this will solve the bug of missing ends of a relationship. The bug might lie elsewhere, but if you could test it... :-)

You can remove this new branch: I've found the issue!

I was looking at your changes and noticed that the original logic was good. So as I didn't understand why it was failing I checked a bit more, and....

And I saw this:

    private void setRelationshipEnds(IArchimateRelationship importedRelationship, IArchimateRelationship targetRelationship) throws PorticoException {
        IArchimateConcept source = findObjectInTargetModel(importedRelationship.getSource());
        if(source == null) {
            source = importConcept(importedRelationship.getSource());
        }
        targetRelationship.setSource(source);

        IArchimateConcept target = findObjectInTargetModel(importedRelationship.getTarget());
        if(target == null) {
            source = importConcept(importedRelationship.getTarget());
        }
        targetRelationship.setTarget(target);
    }

You don't see? I'm helping you a bit:

        IArchimateConcept target = findObjectInTargetModel(importedRelationship.getTarget());
        if(target == null) {
            source = importConcept(importedRelationship.getTarget());
        }

We check target but we set source.

Replace with this and it works as expected:

        IArchimateConcept target = findObjectInTargetModel(importedRelationship.getTarget());
        if(target == null) {
            target = importConcept(importedRelationship.getTarget());
        }

So IMHO, no need to change the logic.

[Phillipus]

D'oh!

Thanks for that.

I spent all morning looking at this code thinking "it should work..." and I didn't see that. I'm glad you found it as I was quite pleased with this recursive method. ;-)

[jbsarrodie]

I spent all morning looking at this code thinking "it should work..."

I thought the same! So I created a really small model to recreate the issue and then noticed that the error was always about "target". I switched to debug mode and found it :-)

[jbsarrodie]

Now I can go back to selecting the right looper pedal for Christmas ;-)

[Phillipus]

Now I can go back to selecting the right looper pedal for Christmas ;-)

I have a Boss RC-3 https://www.boss.info/us/products/rc-3/

[jbsarrodie]

I have a Boss RC-3 https://www.boss.info/us/products/rc-3/

I find it too expensive. I've seen multi-effects pedals that include looper for same price or less:

• https://voxamps.com/product/lil-looper/ (dual looper)
• https://www.zoom-na.com/products/effects-preamps/guitar/zoom-g3xn-multi-effects-processor (includes amp simulation and much more)

[Phillipus]

I have a Boss multi-fx pedal with looper...but..it doesn't remember the loop(s) when powered off.

boolean saved = saveLoop(ILoop loop);
if(!saved) {
    throw new PedalOutTheWindowException();
}

[Phillipus]

Be careful - there's a bug where relations are duplicated when connections are created. The duplicates are phantom relations and don't show in the UI.

Under investigation...

[Phillipus]

Be careful - there's a bug where relations are duplicated when connections are created. The duplicates are phantom relations and don't show in the UI.

Fixed in master branch.

Now I have to fix it in the dev branch of Undo/Redo...

[jbsarrodie]

Be careful - there's a bug where relations are duplicated when connections are created. The duplicates are phantom relations and don't show in the UI.

Whoops, I used it in production. Any tips on how to discover those phantom relations? Do they show up after a model close/open or are they contained in a folder which is not "Relationships" ?

[jbsarrodie]

Whoops, I used it in production. Any tips on how to discover those phantom relations? Do they show up after a model close/open or are they contained in a folder which is not "Relationships" ?

I guess it's easier for me to simply rollback the commit (I use coArchi) and redo the model merge.

[Phillipus]

I used it in production.

:-0

Double relationships are (were) created. This will only show up if you then drag and drop a relationship or concepts onto a View and connections are created. But then you won't be able to save it because the Model Checker will see these as orphans. So if you didn't do that then you should be OK since the duplicate relation does not have a parent folder...

[Phillipus]

Something to consider now...

The structure of sub-folders and their objects are updated when importing even if you don't select "Update and replace from selected model".

So, if you import a model then re-arrange those imported objects and/or subfolders in the tree they will return to their original positions if you import again.

I think I did it this way to ensure that nothing was orphaned or went weird if the source folder structure changes.

But probably not the behaviour we want, right?

[Phillipus]

...when "update mode" is on, the model element itself is also updated which override its documentation and properties (but shouldn't).

What about top level folders (Strategy, Business, Application, etc.)? Do we want to replace the Documentation and Properties of these?

[jbsarrodie]

...when "update mode" is on, the model element itself is also updated which override its documentation and properties (but shouldn't).

What about top level folders (Strategy, Business, Application, etc.)? Do we want to replace the Documentation and Properties of these?

That's a good question. IMHO (based on my original use-cases) I think we shouldn't update top level folders. That's for the MVP. After that, we could imagine another option (usable only when update mode is checked) to allow update of model element and top level folder too. Something like:

[x] Allow update
[ ] Update model object and top level folders too

[Phillipus]

we could imagine another option (usable only when update mode is checked) to allow update of model element and top level folder too. Something like:

Already thought of that. Easy enough to implement. ;-)

[jbsarrodie]

So, if you import a model then re-arrange those imported objects and/or subfolders in the tree they will return to their original positions if you import again.

I think I did it this way to ensure that nothing was orphaned or went weird if the source folder structure changes.

But probably not the behaviour we want, right?

Yes, not the wanted behavior. We should:

• create folders if they don't exist
• create elements that don't exist, and in this case create them under the right folder (like in source)
• if an element already exists, then do nothing (ie. don't move it)

For me, moving concept in their "original positions" is already an "update" action...

[Phillipus]

For me, moving concept in their "original positions" is already an "update" action...

OK, so if update is selected then move concepts and sub-folders back to their original positions? Maybe this should be an additional option in the wizard?

[jbsarrodie]

For me, moving concept in their "original positions" is already an "update" action...

OK, so if update is selected then move concepts and sub-folders back to their original positions?

Yes.

Maybe this should be an additional option in the wizard?

No because the goal of "update" mode is exactly this: make sure that everyting that has been imported before but changed since, return to the right place with the right content. So this includes moving things to their original folders.

Model and top level folders are apart because they are forced to be in any model, so we know in advance that there will be "conflicts", but not because users did a first import and changed things.

[Phillipus]

That's a good question. IMHO (based on my original use-cases) I think we shouldn't update top level folders. That's for the MVP. After that, we could imagine another option (usable only when update mode is checked) to allow update of model element and top level folder too. Something like:

[x] Allow update
[ ] Update model object and top level folders too

Done.

[Phillipus]

Yes, not the wanted behavior. We should:

* create folders if they don't exist
* create elements that don't exist, and in this case create them under the right folder (like in source)
* if an element already exists, then do nothing (ie. don't move it)

For me, moving concept in their "original positions" is already an "update" action...

Done.

[jbsarrodie]

I've just tested and everything seems ok, so closing.

[jbsarrodie]

Reopening (I forgot that edge cases were described in this issue)

Edge case 1

• Model B as already been imported into A
• Some relationship's ends has been changed
• Let's assume these relationships were used in a view "V" coming from B, but remove since
• We merge again B into A with "update" option not set
• "V" should be restored (because this is a merge and "V" no more exist), but "V" contains a relationhip whose ends have changed, so this relationiship can no more exist in "V"
• So in fact we can't restore "V"

What do we do? Either we abort the whole merge operation because we can't merge/restore "V" exactly as it was in B, or we raise a warning?

From my tests, as of now, running this tests end up with the view being imported and the relationship ends moved back to their original elements (ie. relationship has been updated while update mode is off). I can confirm that relationship is not updated if no views from B contain it (expected behavior).

My thoughts:

• This behavior is somewhat aligned with what we have in coArchi (ie. restore some deleted object after a merge if this is needed to keep the model coherent).
• This should be logged as "Some relationships were forced-updated during the import of some views" and shown in a small dialog box at the end of the import operation
• In an ideal world, this "summary" dialog box should allow me to see the detailed log (which relationships' type, name and ends)

Edge case 2

• Model B as already been imported into A
• Some relationship's ends has been changed
• Let's assume these relationships were used in a view "V" which does not come from B (ie. created after the first merge)
• We merge again B into A with "update" option set
• Because "update" option is set, the relationship is restored to its original ends.
• "V" is impacted because now the relationship is no more valid in the context of the view

What do we do? Warning?

Tested, and of course, "V" is impacted as expected.

My thoughts:

• This should be logged as "Some local views were updated during the import of some relationships" and shown in a small dialog box at the end of the import operation
• In an ideal world, this "summary" dialog box should allow me to see the detailed log (which views and why)

In addition:

• A "summary" dialog could provide the number of elements, relationships, views and folders imported, updated...
• A detailed log could also provide the full list of elements, relationships, views and folders imported, updated... and whether they were existing or not.

[Phillipus]

This is in line with https://github.com/archimatetool/archi-portico-plugin/issues/2#issuecomment-565859209

[jbsarrodie]

This is in line with #2 (comment)

Yes, so I suggest to open a new issue for this (doing it atm).