Add support for specialization in model import feature

[jbsarrodie]

This issue is related to #740 but dedicated to model import feature.

I'll add here my test cases and potential ideas for improvements or fixes (if needed).

[jbsarrodie]

Test case 1

GIVEN a model A containing definitions for specializations and some views and elements using them, WHEN importing this model into an empty model B THEN model B contains the same definitions, elements and views than A

PASSED

Test case 2

GIVEN the same two models after Test 1 WHEN changing the name of some specializations and removing them from some concepts in model B, and importing A again with "Update existing objects and folders" unset, THEN model B should not be changed

PASSED

Test case 3

GIVEN the same two models after Test 2 WHEN importing A again with "Update existing objects and folders" set, THEN model B should be updated and definition and usage of specialization should be back at their original value.

PASSED

[Phillipus]

PASSED

Thank f** for that. :-)

[jbsarrodie]

Test case 4

GIVEN a model A containing definitions for a specialization S1 with an image set, some views and elements using it, and a model B containing a specialization S2 with the same name and type than S1 but without image defined, some view and elements using it WHEN importing A into B with "Update existing objects and folders" unset, THEN S2 should be seen as the same specialization as S1 (match by name+type) but not updated, and views from A should be imported with elements referencing S2 (without exhibiting image).

FAILED: S1 is imported but renamed "S1_1" I'm gonna work on this one to port some of the work I did in june/july onto your new code

BTW...

Test case 4.1

GIVEN a model A containing definitions for a specialization S1 with an image set, some views and elements using it, and a model B containing a specialization S2 with the same name and type than S1 but without image defined, some view and elements using it, and containing another specialization "S1_1" having the name of S1 with "_1" added at the end. WHEN importing A into B with "Update existing objects and folders" unset, THEN S2 should be seen as the same specialization as S1 (match by name+type) but not updated, and views from A should be imported with elements referencing S2 (without exhibiting image).

FAILED: S1 is imported but renamed "S1_1" leading to two specialisations with the same name (S1_1) in model B

Test case 5

GIVEN a model A containing definitions for a specialization S1 with an image set, some views and elements using it, and a model B containing the same definition (same name, same type) for one of the specialization (let's call it S) but without image defined, some view and elements using it WHEN importing A into B with "Update existing objects and folders" set, THEN S2 should be seen as the same specialization as S1 (match by name+type) and updated (image added). All elements (from A and B) should reference the same specialization S2

FAILED: S1 is imported but renamed "S1_1"

[Phillipus]

I'm gonna work on this one to port some of the work I did in june/july onto your new code

That's still in my code, but commented out.

In ModelImporter

[jbsarrodie]

That's still in my code, but commented out.

I've seen, but as you've refactored to merge both caches, I have to make sure it still works as expected without side effects.

[jbsarrodie]

Test case 4

GIVEN a model A containing definitions for a specialization S1 with an image set, some views and elements using it, and a model B containing a specialization S2 with the same name and type than S1 but without image defined, some view and elements using it WHEN importing A into B with "Update existing objects and folders" unset, THEN S2 should be seen as the same specialization as S1 (match by name+type) but not updated, and views from A should be imported with elements referencing S2 (without exhibiting image).

PASSED with commit 9c1b2477b89f73bbdd71bf3a08cacd5556dafa61

Test case 5

GIVEN a model A containing definitions for a specialization S1 with an image set, some views and elements using it, and a model B containing the same definition (same name, same type) for one of the specialization (let's call it S) but without image defined, some view and elements using it WHEN importing A into B with "Update existing objects and folders" set, THEN S2 should be seen as the same specialization as S1 (match by name+type) and updated (image added). All elements (from A and B) should reference the same specialization S2

PASSED with commit 9c1b2477b89f73bbdd71bf3a08cacd5556dafa61

[jbsarrodie]

Test case 6

GIVEN a model A containing definitions for a specialization S1, some views and elements using it, and a model B containing a specialization S2 with the same name but a different type than S1, some view and elements using it WHEN importing A into B with "Update existing objects and folders" unset, THEN there should be no conflicts between S1 and S2 that should both appear in the result.

PASSED (with commit 9c1b247)

[jbsarrodie]

This one might raise some questions or remarks:

Test case 7

GIVEN a model A containing the definition for a specialization S1 and an empty model B WHEN

• importing A into B, and
• in B, rename S1 to S1_OLD and create S2 with the same name and type as S1, and
• importing again A into B with "Update existing objects and folders" set,

THEN S2 should be seen as the same specialization as S1 (match by name+type) and updated if needed

FAILED (with commit 9c1b247) : because S1_OLD hase been created during a previous import, it shares the same internal id. And because findObjectInTargetModel() first checks by id, we end up with S1_OLD been "reset" to S1. At this point we have two specialization with the same type+name (because of S2) and UnduplicateProfileNamesCommand() rename one of them to S1_1 (and at this point we might exibit the same issue that in 4.1 if another specialization was already existing with this name).

So I think we should maybe not even cache profiles by Id, but only by type+name, which lead me to this potential change: only cache and retrieve objects using the key provided by getObjectKey(). This would make sure the method is the same (and unique for a type of object) everywhere.

@Phillipus I can easily implement it, but I'd like your feedback on this idea.

[jbsarrodie]

Test case 7

GIVEN a model A containing the definition for a specialization S1 and an empty model B WHEN

• importing A into B, and
• in B, rename S1 to S1_OLD and create S2 with the same name and type as S1, and
• importing again A into B with "Update existing objects and folders" set,

THEN S2 should be seen as the same specialization as S1 (match by name+type) and updated if needed

PASSED with new commit cf4855b1222f9ab0d193438cb9424d2e3840e050

[Phillipus]

So I think we should maybe not even cache profiles by Id, but only by type+name, which lead me to this potential change: only cache and retrieve objects using the key provided by getObjectKey(). This would make sure the method is the same (and unique for a type of object) everywhere.

Was there a reason I forgot why we decided to use id as well as name/class?

[jbsarrodie]

I've reached the point where I think it fully works as expected. I'm open to any other test case, so I'll keep this issue open for some time just in case...

[jbsarrodie]

Was there a reason I forgot why we decided to use id as well as name/class?

I guess that's simply because my first attempt at it was very conservative with an additional cache and you reproduced this by caching with both id and type+name.

And until yesterday, I wasn't sure about which approach to keep. But the more I test, the more I think users will expect the match to be only on type+name to avoid strange behavior.

[Phillipus]

OK, thanks for your extensive testing. 👍

[Phillipus]

But this version of ModelImporter does still check for id before checking name/type...

[jbsarrodie]

But this version of ModelImporter does still check for id before checking name/type...

??

(I updated findObjectInTargetModel() so that it uses getObjectKey())

[Phillipus]

We have:

// Get by ID
EObject foundObject = objectCache.get(eObject.getId());

// Try by other key
if(foundObject == null) {
    foundObject = objectCache.get(getObjectKey(eObject));
}

Don't you want simply the following?:

EObject foundObject = objectCache.get(getObjectKey(eObject));

[Phillipus]

D'oh! I've just got your recent commit...

[jbsarrodie]

I've reached the point where I think it fully works as expected. I'm open to any other test case, so I'll keep this issue open for some time just in case...

Seems really ok. Now closing this issue.