Make association source and target reliable in CDB #2914
Conversation
scosta-obeo
force-pushed
the
sco/fix/cdb-edge-stability
branch
3 times, most recently
from
b1b60b3 to
9b745ef
Compare
...ties/src/org/polarsys/capella/core/data/information/properties/fields/NavigableCheckbox.java
Show resolved
Hide resolved
core/plugins/org.polarsys.capella.core.data.migration/plugin.xml
Outdated
Show resolved
Hide resolved
...ties/src/org/polarsys/capella/core/data/information/properties/fields/NavigableCheckbox.java
Outdated
Show resolved
Hide resolved
....ui.properties/src/org/polarsys/capella/core/ui/properties/fields/AbstractSemanticField.java
Outdated
Show resolved
Hide resolved
....ui.properties/src/org/polarsys/capella/core/ui/properties/fields/AbstractSemanticField.java
Outdated
Show resolved
Hide resolved
....ui.properties/src/org/polarsys/capella/core/ui/properties/fields/AbstractSemanticField.java
Outdated
Show resolved
Hide resolved
...ties/src/org/polarsys/capella/core/data/information/properties/fields/NavigableCheckbox.java
Outdated
Show resolved
Hide resolved
...on/src/org/polarsys/capella/core/data/migration/aird/AssociationCDBMigrationContributor.java
Show resolved
Hide resolved
core/plugins/org.polarsys.capella.core.sirius.analysis/description/common.odesign
Outdated
Show resolved
Hide resolved
...on/src/org/polarsys/capella/core/data/migration/aird/AssociationCDBMigrationContributor.java
Show resolved
Hide resolved
scosta-obeo
force-pushed
the
sco/fix/cdb-edge-stability
branch
from
9b745ef to
e63dc35
Compare
scosta-obeo
force-pushed
the
sco/fix/cdb-edge-stability
branch
from
e63dc35 to
aa8097b
Compare
scosta-obeo
force-pushed
the
sco/fix/cdb-edge-stability
branch
2 times, most recently
from
2abe891 to
3ef0360
Compare
| if (customFeatures.contains(sourceArrowFeatureName) && !customFeatures.contains(targetArrowFeatureName)) { | ||
| customFeatures.add(targetArrowFeatureName); | ||
| customFeatures.remove(sourceArrowFeatureName); | ||
| } else if (!customFeatures.contains(sourceArrowFeatureName) && customFeatures.contains(targetArrowFeatureName)) { |
There was a problem hiding this comment.
If we go there, we are going to compute again the contains for both. computing it once and using local variables would gain a little computation time
| EObject targetObject = targetNode.getTarget(); | ||
| return sourceObject == expectedTarget && targetObject == expectedSource; | ||
| } else { | ||
| // error |
There was a problem hiding this comment.
Maybe add something to some logs or throw an exception if the data are corrupted ?
Silently ignoring errors during migration is the best way to get strange bugs and data corruption visible months later
There was a problem hiding this comment.
I added a log message.
There was a problem hiding this comment.
I may be missing something but where do you check that the migration is not run twice on the same aird, for example when migrating to 7.0.1 and then again when 7.0.5 is out and the file is migrated again ?
There was a problem hiding this comment.
There is no marker for migration in Capella. In the context mentioned, the migration will indeed be launched twice. But the check carried out in the needMigration(DEdge dEdge) method will therefore allow no changes during the second migration.
There was a problem hiding this comment.
That's very resource (and time) expensive for the user if we no longer create old style DEdge objects. Would it be possible to bypass the whole migration by looking, for example, at the version of one of the meta models we know have increase in version 7.0.1 ?
There was a problem hiding this comment.
Is there some other migrations with this kind of check ? The problem seems global for all migration. No ?
For example, in Sirius, there is a common way to address this problematic.
There was a problem hiding this comment.
There is no common way defined in capella and that's an issue. In some other add-ons, we do it like in Sirius.
It could be implemented the same way here, couldn't it ?
There was a problem hiding this comment.
Yes, I'm agree but it's outside the scope of this issue. I suggest you open a new ticket.
There was a problem hiding this comment.
This is a new migration so it is in the scope of this ticket for this migration.
The refactoring to do it correctly in the other migrations will be new tickets.
There was a problem hiding this comment.
I've added version checking to the migration. There should be no more failed tests (except for BrokenLinksCheckTestCase which fails on master). You can resolve conversations if it's fine with you.
scosta-obeo
force-pushed
the
sco/fix/cdb-edge-stability
branch
from
3ef0360 to
0a5f8f4
Compare
|
With the lastest commit, there are still 11 failed tests compare to the single one on the master branch. They should be looked at before validating the PR. |
Yes, after analysis, the problem is that "DT_Association" has not the same source and target mappings list. With the new solution based on the ID of the source/target to determine with a deterministic way the source and the target, these lists must be the same. @scosta-obeo will push a new PR with a modification of the common.odesign. |
scosta-obeo
force-pushed
the
sco/fix/cdb-edge-stability
branch
from
0a5f8f4 to
cd65e6e
Compare
|
@scosta-obeo still 6 tests failing (+ 1 not because of this PR) |
scosta-obeo
force-pushed
the
sco/fix/cdb-edge-stability
branch
4 times, most recently
from
94f37e4 to
17d5715
Compare
This comment was marked as outdated.
This comment was marked as outdated.
scosta-obeo
force-pushed
the
sco/fix/cdb-edge-stability
branch
from
17d5715 to
2829828
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This commit merges two commands for navigability state of association in CDB to avoid diagram refresh (and unwanted behaviour) between them. Signed-off-by: Séraphin Costa <[email protected]>
This commit updates the CDB association edge mapping in the VSM (source/target mappings and styles) to make association source and target reliable in CDB, to always have the same source and target, and to prevent a change in property from changing the source or the target. This commit also adds a migration to reverse the association edge in the wrong direction in existing projects. This commit also adapts the tests ReconnectRelationshipGroup.ReconnectRelationshipGroupSA, ReconnectRelationshipGroup.ReconnectRelationshipGroupOA in org.polarsys.capella.test.diagram.tools.ju.cdb to these changes. Signed-off-by: Séraphin Costa <[email protected]>
Signed-off-by: Séraphin Costa <[email protected]>
This commit also: * Revert some unexpected changes (data intentionally corrupted for test Rule_I_43_ElementReferencesAirdOrProxyElement) * Adapt tests HideAssociationLabels, HideRoleNames and ShowModifiers because the source and target have been inverted and therefore also the labels. Signed-off-by: Séraphin Costa <[email protected]>
scosta-obeo
force-pushed
the
sco/fix/cdb-edge-stability
branch
from
2829828 to
ebeb224
Compare
There was a problem hiding this comment.
@lredor @scosta-obeo Looks better. Only 1 failed test left: org.polarsys.capella.test.migration.ju.testcases.basic.RelationStability.test
|
#2913 Merge two commands to avoid refresh between them
This commit merge two command for navigability state of association in
CDB to avoid diagram refresh (and unwanted behavior) between them.
#2913 Make association source and target reliable in CDB
This commit make association source and target reliable in CDB always
have the same source and target and to prevent a change in property from
changing the source or the target.
This commit also add migration to reverse the association edge in the
wrong direction in old project.
#2913 Add tests for relation edge migration in CDB
#2913 Migrate all data tests