From ed1f5d9324764dc6332a80d1dcce51d0a4c89227 Mon Sep 17 00:00:00 2001 From: Jan Wittler Date: Tue, 27 Dec 2022 13:24:00 +0100 Subject: [PATCH 1/3] correct english in `ChangeAssertHelper` --- .../vitruv/change/composite/util/ChangeAssertHelper.xtend | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/util/ChangeAssertHelper.xtend b/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/util/ChangeAssertHelper.xtend index f4332839..f33d6ab4 100644 --- a/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/util/ChangeAssertHelper.xtend +++ b/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/util/ChangeAssertHelper.xtend @@ -33,11 +33,11 @@ class ChangeAssertHelper { static def assertOldValue(EChange eChange, Object oldValue) { if (oldValue instanceof EObject) { - assertThat("old value must be the same or a copy than the given old value", oldValue, + assertThat("old value must be the same or a copy as the given old value", oldValue, equalsDeeply((eChange as SubtractiveEChange).oldValue as EObject)) } else { assertEquals(oldValue, (eChange as SubtractiveEChange).oldValue, - "old value must be the same than the given old value") + "old value must be the same as the given old value") } } @@ -48,14 +48,14 @@ class ChangeAssertHelper { val newEObject = newValue as EObject var newEObjectInChange = newValueInChange as EObject assertThat( - '''new value in change '«newValueInChange»' must be the same than the given new value '«newValue»!''', + '''new value in change '«newValueInChange»' must be the same as the given new value '«newValue»!''', newEObject, equalsDeeply(newEObjectInChange) ) } else if (!condition) { assertNotNull(newValue) assertEquals(newValue, - newValueInChange, '''new value in change '«newValueInChange»' must be the same than the given new value '«newValue»!''') + newValueInChange, '''new value in change '«newValueInChange»' must be the same as the given new value '«newValue»!''') } } From c489bd820dcd44ce740bd60de7d4174a5ffd00d9 Mon Sep 17 00:00:00 2001 From: Jan Wittler Date: Tue, 27 Dec 2022 13:39:11 +0100 Subject: [PATCH 2/3] add delete changes to end of change sequence instead of inserting them right after the element is removed from its parent --- .../composite/recording/ChangeRecorder.xtend | 93 ++++++++----------- .../NotificationToEChangeConverter.xtend | 9 +- 2 files changed, 42 insertions(+), 60 deletions(-) diff --git a/bundles/tools.vitruv.change.composite/src/tools/vitruv/change/composite/recording/ChangeRecorder.xtend b/bundles/tools.vitruv.change.composite/src/tools/vitruv/change/composite/recording/ChangeRecorder.xtend index b1f3a745..eac8fad5 100644 --- a/bundles/tools.vitruv.change.composite/src/tools/vitruv/change/composite/recording/ChangeRecorder.xtend +++ b/bundles/tools.vitruv.change.composite/src/tools/vitruv/change/composite/recording/ChangeRecorder.xtend @@ -1,38 +1,40 @@ package tools.vitruv.change.composite.recording import java.util.ArrayList +import java.util.HashMap import java.util.HashSet import java.util.List +import java.util.Map import java.util.Set import org.eclipse.emf.common.notify.Adapter import org.eclipse.emf.common.notify.Notification import org.eclipse.emf.common.notify.Notifier import org.eclipse.emf.ecore.EObject +import org.eclipse.emf.ecore.EReference import org.eclipse.emf.ecore.resource.Resource import org.eclipse.emf.ecore.resource.ResourceSet import org.eclipse.xtend.lib.annotations.FinalFieldsConstructor -import tools.vitruv.change.composite.description.TransactionalChange -import tools.vitruv.change.composite.description.VitruviusChangeFactory +import tools.vitruv.change.atomic.EChange import tools.vitruv.change.atomic.EChangeIdManager +import tools.vitruv.change.atomic.eobject.DeleteEObject import tools.vitruv.change.atomic.eobject.EObjectAddedEChange import tools.vitruv.change.atomic.eobject.EObjectSubtractedEChange +import tools.vitruv.change.atomic.feature.reference.UpdateReferenceEChange +import tools.vitruv.change.atomic.id.IdResolver +import tools.vitruv.change.composite.description.TransactionalChange +import tools.vitruv.change.composite.description.VitruviusChangeFactory +import static com.google.common.base.Preconditions.checkArgument +import static com.google.common.base.Preconditions.checkNotNull import static com.google.common.base.Preconditions.checkState import static org.eclipse.emf.common.notify.Notification.* +import static org.eclipse.emf.ecore.resource.Resource.* +import static org.eclipse.emf.ecore.resource.ResourceSet.* import static extension org.eclipse.emf.ecore.util.EcoreUtil.* import static extension tools.vitruv.change.atomic.EChangeUtil.* -import org.eclipse.emf.ecore.EReference -import static com.google.common.base.Preconditions.checkNotNull -import static com.google.common.base.Preconditions.checkArgument -import static extension org.eclipse.emf.ecore.resource.Resource.RESOURCE__CONTENTS -import static extension org.eclipse.emf.ecore.resource.Resource.RESOURCE__IS_LOADED -import static extension org.eclipse.emf.ecore.resource.ResourceSet.RESOURCE_SET__RESOURCES -import static extension tools.vitruv.change.atomic.resolve.EChangeResolverAndApplicator.applyForward import static extension tools.vitruv.change.atomic.resolve.EChangeResolverAndApplicator.applyBackward -import tools.vitruv.change.atomic.id.IdResolver -import tools.vitruv.change.atomic.feature.reference.UpdateReferenceEChange -import tools.vitruv.change.atomic.EChange +import static extension tools.vitruv.change.atomic.resolve.EChangeResolverAndApplicator.applyForward /** * Records changes to model elements as a {@link TransactionalChange}. @@ -167,6 +169,11 @@ class ChangeRecorder implements AutoCloseable { change.applyForward(idResolver) } + /** + * Creates {@link DeleteEObject} changes for every element implicitly deleted in the change + * sequence and all of its contained elements. The delete changes are appended at the end + * of the list. Contained elements are deleted before their container. + */ def private postprocessRemovals(List changes) { if(changes.isEmpty) return changes @@ -183,51 +190,29 @@ class ChangeRecorder implements AutoCloseable { } } } - - return if (removedElements.isEmpty) { - changes - } else { - changes.insertChanges [ eChange | - switch (eChange) { - EObjectSubtractedEChange case eChange.isContainmentRemoval && - removedElements.contains(eChange.oldValue): - converter.createDeleteChanges(eChange) - default: - null + if (!removedElements.isEmpty) { + // removed elements may contain elements with parent - child relation. + // As a deleted parent removes all its children, filter out such children + // to avoid duplicated delete changes + val Map> allElementsToDelete = new HashMap + removedElements.forEach [ element | + if (allElementsToDelete.values.exists[it.contains(element)]) { + return } + var elementsToDelete = element.eAllContents.toList.reverse //delete from inner to outer + elementsToDelete.forEach [ child | + if (allElementsToDelete.containsKey(child)) { + allElementsToDelete.remove(child) + } + ] + elementsToDelete.add(element) + allElementsToDelete.put(element, elementsToDelete) ] + changes += allElementsToDelete.values.flatMap [ elementsToDelete | + elementsToDelete.map [ converter.createDeleteChange(it) ] + ].toList } - } - - /** - * Iterates over the {@code target} change tree and returns a modified tree, where all new changes - * provided by {@code inserter} have been inserted. - * - * @param inserter a function that receives a {@link ConcreteChange} and returns a change to insert directly - * after the received {@link ConcreteChange}. Can return {@code null} to not insert a change. - */ - def private static List insertChanges( - List changes, - (EChange)=>List inserter - ) { - var List resultEChanges = null - for (var k = 0; k < changes.size; k++) { - val eChange = changes.get(k) - resultEChanges?.add(eChange) - val additional = inserter.apply(eChange) - if (additional !== null) { - if (resultEChanges === null) { - resultEChanges = new ArrayList(changes.size + additional.size) - resultEChanges.addAll(changes.subList(0, k + 1)) - } - resultEChanges += additional - } - } - return if (resultEChanges !== null) { - resultEChanges - } else { - changes - } + return changes } def TransactionalChange getChange() { diff --git a/bundles/tools.vitruv.change.composite/src/tools/vitruv/change/composite/recording/NotificationToEChangeConverter.xtend b/bundles/tools.vitruv.change.composite/src/tools/vitruv/change/composite/recording/NotificationToEChangeConverter.xtend index f2feaf54..d6947791 100644 --- a/bundles/tools.vitruv.change.composite/src/tools/vitruv/change/composite/recording/NotificationToEChangeConverter.xtend +++ b/bundles/tools.vitruv.change.composite/src/tools/vitruv/change/composite/recording/NotificationToEChangeConverter.xtend @@ -10,7 +10,6 @@ import org.eclipse.xtend.lib.annotations.FinalFieldsConstructor import tools.vitruv.change.atomic.EChange import tools.vitruv.change.atomic.TypeInferringAtomicEChangeFactory import tools.vitruv.change.atomic.eobject.EObjectAddedEChange -import tools.vitruv.change.atomic.eobject.EObjectSubtractedEChange import tools.vitruv.change.atomic.feature.attribute.AttributeFactory import tools.vitruv.change.atomic.feature.reference.UpdateReferenceEChange @@ -28,11 +27,9 @@ package final class NotificationToEChangeConverter { extension val TypeInferringAtomicEChangeFactory changeFactory = TypeInferringAtomicEChangeFactory.instance val (EObject, EObject)=>boolean isCreateChange - - def List createDeleteChanges(EObjectSubtractedEChange change) { - var allDeletedElements = change.oldValue.eAllContents.toList.reverse //delete from inner to outer - allDeletedElements.add(change.oldValue) - return allDeletedElements.mapFixed[createDeleteEObjectChange] + + def EChange createDeleteChange(EObject eObject) { + return createDeleteEObjectChange(eObject) } /** From 4c8cf64455843f38479be47a8c290a814ff18bd2 Mon Sep 17 00:00:00 2001 From: Jan Wittler Date: Tue, 27 Dec 2022 13:45:12 +0100 Subject: [PATCH 3/3] adapt composite tests to new order of change recorder delete changes --- ...angeDescription2RemoveEReferenceTest.xtend | 38 +++++++++++++------ ...on2ReplaceSingleValuedEReferenceTest.xtend | 3 +- .../util/CompoundEChangeAssertHelper.xtend | 34 ++++++++--------- 3 files changed, 43 insertions(+), 32 deletions(-) diff --git a/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/reference/ChangeDescription2RemoveEReferenceTest.xtend b/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/reference/ChangeDescription2RemoveEReferenceTest.xtend index 9b47c9e2..fff02239 100644 --- a/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/reference/ChangeDescription2RemoveEReferenceTest.xtend +++ b/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/reference/ChangeDescription2RemoveEReferenceTest.xtend @@ -1,12 +1,17 @@ package tools.vitruv.change.composite.reference -import static extension tools.vitruv.change.composite.util.AtomicEChangeAssertHelper.* -import static extension tools.vitruv.change.composite.util.CompoundEChangeAssertHelper.* -import static allElementTypes.AllElementTypesPackage.Literals.* import org.junit.jupiter.api.Test -import static extension tools.vitruv.testutils.metamodels.AllElementTypesCreators.* +import tools.vitruv.change.atomic.eobject.DeleteEObject import tools.vitruv.change.composite.ChangeDescription2ChangeTransformationTest +import static allElementTypes.AllElementTypesPackage.Literals.* +import static tools.vitruv.change.composite.util.ChangeAssertHelper.* +import static tools.vitruv.testutils.metamodels.AllElementTypesCreators.* + +import static extension tools.vitruv.change.composite.util.AtomicEChangeAssertHelper.* +import static extension tools.vitruv.change.composite.util.CompoundEChangeAssertHelper.* +import static org.junit.jupiter.api.Assertions.assertTrue + class ChangeDescription2RemoveEReferenceTest extends ChangeDescription2ChangeTransformationTest { @Test @@ -66,7 +71,8 @@ class ChangeDescription2RemoveEReferenceTest extends ChangeDescription2ChangeTra // assert result.assertChangeCount(2) - .assertRemoveAndDeleteNonRoot(uniquePersistedRoot, ROOT__MULTI_VALUED_CONTAINMENT_EREFERENCE, nonRoot, 0) + .assertRemoveEReference(uniquePersistedRoot, ROOT__MULTI_VALUED_CONTAINMENT_EREFERENCE, nonRoot, 0, true) + .assertDeleteEObject(nonRoot) .assertEmpty } @@ -86,8 +92,9 @@ class ChangeDescription2RemoveEReferenceTest extends ChangeDescription2ChangeTra // assert result.assertChangeCount(3) - .assertRemoveAndDeleteNonRoot(uniquePersistedRoot, ROOT__MULTI_VALUED_UNSETTABLE_CONTAINMENT_EREFERENCE, nonRoot, 0) + .assertRemoveEReference(uniquePersistedRoot, ROOT__MULTI_VALUED_UNSETTABLE_CONTAINMENT_EREFERENCE, nonRoot, 0, true) .assertUnsetFeature(uniquePersistedRoot, ROOT__MULTI_VALUED_UNSETTABLE_CONTAINMENT_EREFERENCE) + .assertDeleteEObject(nonRoot) .assertEmpty } @@ -110,9 +117,10 @@ class ChangeDescription2RemoveEReferenceTest extends ChangeDescription2ChangeTra // assert result.assertChangeCount(4) - .assertReplaceAndDeleteNonRoot(containedRoot, uniquePersistedRoot, ROOT__RECURSIVE_ROOT, false) + .assertReplaceSingleValuedEReference(uniquePersistedRoot, ROOT__RECURSIVE_ROOT, containedRoot, null, true, false, false) .assertReplaceSingleValuedEReference(containedRoot, ROOT__SINGLE_VALUED_CONTAINMENT_EREFERENCE, nonRoot, null, true, false, false) .assertReplaceSingleValuedEReference(uniquePersistedRoot, ROOT__SINGLE_VALUED_CONTAINMENT_EREFERENCE, null, nonRoot, true, false, false) + .assertDeleteEObjectAndContainedElements(containedRoot) .assertEmpty } @@ -138,9 +146,10 @@ class ChangeDescription2RemoveEReferenceTest extends ChangeDescription2ChangeTra // assert result.assertChangeCount(5) - .assertReplaceAndDeleteNonRoot(containedRoot, uniquePersistedRoot, ROOT__RECURSIVE_ROOT, false) + .assertReplaceSingleValuedEReference(uniquePersistedRoot, ROOT__RECURSIVE_ROOT, containedRoot, null, true, false, false) .assertReplaceSingleValuedEReference(innerContainedRoot, ROOT__SINGLE_VALUED_CONTAINMENT_EREFERENCE, nonRoot, null, true, false, false) .assertReplaceSingleValuedEReference(uniquePersistedRoot, ROOT__SINGLE_VALUED_CONTAINMENT_EREFERENCE, null, nonRoot, true, false, false) + .assertDeleteEObjectAndContainedElements(containedRoot) .assertEmpty } @@ -160,9 +169,14 @@ class ChangeDescription2RemoveEReferenceTest extends ChangeDescription2ChangeTra ] // assert - result.assertChangeCount(4) - .assertRemoveAndDeleteNonRoot(uniquePersistedRoot, ROOT__MULTI_VALUED_CONTAINMENT_EREFERENCE, nonRoot2, 1) - .assertRemoveAndDeleteNonRoot(uniquePersistedRoot, ROOT__MULTI_VALUED_CONTAINMENT_EREFERENCE, nonRoot1, 0) - .assertEmpty + val deleteChanges = result.assertChangeCount(4) + .assertRemoveEReference(uniquePersistedRoot, ROOT__MULTI_VALUED_CONTAINMENT_EREFERENCE, nonRoot2, 1, true) + .assertRemoveEReference(uniquePersistedRoot, ROOT__MULTI_VALUED_CONTAINMENT_EREFERENCE, nonRoot1, 0, true) + .assertChangeCount(2) + // order of delete changes is random, thus use custom assertions + deleteChanges.forEach[assertType(it, DeleteEObject)] + val deletedObjects = deleteChanges.map[(it as DeleteEObject).affectedEObject] + assertTrue(deletedObjects.contains(nonRoot1), "deleted objects are missing " + nonRoot1) + assertTrue(deletedObjects.contains(nonRoot2), "deleted objects are missing " + nonRoot2) } } diff --git a/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/reference/ChangeDescription2ReplaceSingleValuedEReferenceTest.xtend b/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/reference/ChangeDescription2ReplaceSingleValuedEReferenceTest.xtend index 49b5bdba..cd8f633c 100644 --- a/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/reference/ChangeDescription2ReplaceSingleValuedEReferenceTest.xtend +++ b/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/reference/ChangeDescription2ReplaceSingleValuedEReferenceTest.xtend @@ -45,8 +45,9 @@ class ChangeDescription2ReplaceSingleValuedEReferenceTest extends ChangeDescript // assert result.assertChangeCount(4) - .assertCreateAndReplaceAndDeleteNonRoot(nonRoot, replaceNonRoot, ROOT__SINGLE_VALUED_CONTAINMENT_EREFERENCE, uniquePersistedRoot, true, false, false) + .assertCreateAndReplaceNonRoot(replaceNonRoot, uniquePersistedRoot, nonRoot, ROOT__SINGLE_VALUED_CONTAINMENT_EREFERENCE, false) .assertReplaceSingleValuedEAttribute(replaceNonRoot, IDENTIFIED__ID, null, replaceNonRoot.id, false, false) + .assertDeleteEObjectAndContainedElements(nonRoot) .assertEmpty } diff --git a/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/util/CompoundEChangeAssertHelper.xtend b/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/util/CompoundEChangeAssertHelper.xtend index e2d2bfb7..f9ec9fe3 100644 --- a/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/util/CompoundEChangeAssertHelper.xtend +++ b/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/util/CompoundEChangeAssertHelper.xtend @@ -1,5 +1,6 @@ package tools.vitruv.change.composite.util +import edu.kit.ipd.sdq.activextendannotations.Utility import org.eclipse.emf.ecore.EObject import org.eclipse.emf.ecore.EStructuralFeature import org.eclipse.emf.ecore.resource.Resource @@ -7,7 +8,6 @@ import tools.vitruv.change.atomic.EChange import static extension tools.vitruv.change.composite.util.AtomicEChangeAssertHelper.* import static extension tools.vitruv.change.composite.util.ChangeAssertHelper.* -import edu.kit.ipd.sdq.activextendannotations.Utility @Utility class CompoundEChangeAssertHelper { @@ -17,34 +17,30 @@ class CompoundEChangeAssertHelper { return changes.assertCreateEObject(expectedNewValue) .assertInsertEReference(affectedEObject, affectedFeature, expectedNewValue, expectedIndex, true, wasUnset) } - - def static Iterable assertRemoveAndDeleteNonRoot( - Iterable changes, EObject affectedEObject, EStructuralFeature affectedFeature, EObject expectedOldValue, int expectedOldIndex) { - changes.assertSizeGreaterEquals(2) - return changes.assertRemoveEReference(affectedEObject, affectedFeature, expectedOldValue, expectedOldIndex, true) - .assertDeleteEObject(expectedOldValue) - } - def static Iterable assertCreateAndReplaceAndDeleteNonRoot(Iterable changes, EObject expectedOldValue, - EObject expectedNewValue, EStructuralFeature affectedFeature, EObject affectedEObject, boolean isContainment, boolean wasUnset, boolean isUnset) { - changes.assertSizeGreaterEquals(3) - return changes.assertCreateEObject(expectedNewValue) - .assertReplaceSingleValuedEReference(affectedEObject, affectedFeature, expectedOldValue, expectedNewValue, isContainment, wasUnset, isUnset) - .assertDeleteEObject(expectedOldValue) + def static Iterable assertCreateAndReplaceNonRoot(Iterable changes, EObject expectedNewValue, + EObject affectedEObject, EStructuralFeature affectedFeature, boolean wasUnset) { + assertCreateAndReplaceNonRoot(changes, expectedNewValue, affectedEObject, null, affectedFeature, wasUnset) } def static Iterable assertCreateAndReplaceNonRoot(Iterable changes, EObject expectedNewValue, - EObject affectedEObject, EStructuralFeature affectedFeature, boolean wasUnset) { + EObject affectedEObject, EObject expectedOldValue, EStructuralFeature affectedFeature, boolean wasUnset) { changes.assertSizeGreaterEquals(2) return changes.assertCreateEObject(expectedNewValue) - .assertReplaceSingleValuedEReference(affectedEObject, affectedFeature, null, expectedNewValue, true, wasUnset, false) + .assertReplaceSingleValuedEReference(affectedEObject, affectedFeature, expectedOldValue, expectedNewValue, true, wasUnset, false) } def static Iterable assertReplaceAndDeleteNonRoot(Iterable changes, EObject expectedOldValue, EObject affectedEObject, EStructuralFeature affectedFeature, boolean isUnset) { + return changes + .assertReplaceSingleValuedEReference(affectedEObject, affectedFeature, expectedOldValue, null, true, false, isUnset) + .assertDeleteEObjectAndContainedElements(expectedOldValue) + } + + def static Iterable assertDeleteEObjectAndContainedElements(Iterable changes, EObject expectedOldValue) { val deletedContainedElements = expectedOldValue.eAllContents.toList.reverse // elements are deleted from inner to outer - changes.assertSizeGreaterEquals(2 + deletedContainedElements.size) - var filteredChanges = changes.assertReplaceSingleValuedEReference(affectedEObject, affectedFeature, expectedOldValue, null, true, false, isUnset) + changes.assertSizeGreaterEquals(1 + deletedContainedElements.size) + var filteredChanges = changes for (deletedContainedElement : deletedContainedElements) { filteredChanges = filteredChanges.assertDeleteEObject(deletedContainedElement) } @@ -60,7 +56,7 @@ class CompoundEChangeAssertHelper { def static Iterable assertRemoveAndDeleteRootEObject(Iterable changes, EObject expectedOldValue, String uri, Resource resource) { changes.assertSizeGreaterEquals(2) return changes.assertRemoveRootEObject(expectedOldValue, uri, resource) - .assertDeleteEObject(expectedOldValue) + .assertDeleteEObjectAndContainedElements(expectedOldValue) } } \ No newline at end of file