Merge pull request #54 from vitruv-tools/fix-insert-delete-change

Add delete changes at end of change sequence in `ChangeRecorder`

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<EChange> 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<EObject, Iterable<EObject>> 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<EChange> insertChanges(
-		List<EChange> changes,
-		(EChange)=>List<EChange> inserter
-	) {
-		var List<EChange> 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() {

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<EChange> 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)
 	}
 
 	/** 

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)
 	}
 }

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
 	}
 

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»!''')
 		}
 	}
 

tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/util/CompoundEChangeAssertHelper.xtend

@@ -1,13 +1,13 @@
 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
 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<? extends EChange> assertRemoveAndDeleteNonRoot(
-			Iterable<? extends EChange> 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<? extends EChange> assertCreateAndReplaceAndDeleteNonRoot(Iterable<? extends EChange> 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<? extends EChange> assertCreateAndReplaceNonRoot(Iterable<? extends EChange> changes, EObject expectedNewValue,
+		EObject affectedEObject, EStructuralFeature affectedFeature, boolean wasUnset) {
+		assertCreateAndReplaceNonRoot(changes, expectedNewValue, affectedEObject, null, affectedFeature, wasUnset)
 	}
 
 	def static Iterable<? extends EChange> assertCreateAndReplaceNonRoot(Iterable<? extends EChange> 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<? extends EChange> assertReplaceAndDeleteNonRoot(Iterable<? extends EChange> changes, EObject expectedOldValue,
 		EObject affectedEObject, EStructuralFeature affectedFeature, boolean isUnset) {
+		return changes
+			.assertReplaceSingleValuedEReference(affectedEObject, affectedFeature, expectedOldValue, null, true, false, isUnset)
+			.assertDeleteEObjectAndContainedElements(expectedOldValue)
+	}
+
+	def static Iterable<? extends EChange> assertDeleteEObjectAndContainedElements(Iterable<? extends EChange> 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<? extends EChange> assertRemoveAndDeleteRootEObject(Iterable<? extends EChange> changes, EObject expectedOldValue, String uri, Resource resource) {
 		changes.assertSizeGreaterEquals(2)
 		return changes.assertRemoveRootEObject(expectedOldValue, uri, resource)
-			.assertDeleteEObject(expectedOldValue)
+			.assertDeleteEObjectAndContainedElements(expectedOldValue)
 	}
 }