Fix for Issue #71

bundles/tools.vitruv.change.composite/src/tools/vitruv/change/composite/recording/ChangeRecorder.xtend

@@ -38,7 +38,7 @@ import static extension tools.vitruv.change.atomic.EChangeUtil.*
  * that all objects that have been removed from their containment reference without being added to a new containment
  * reference while changes were being recorded have been deleted, resulting in an appropriate delete change.
  * The recorder considers resources being loaded as existing and does thus not produce changes for it.
- *
+ * 
  * Does not record changes of the <code>xmi:id</code> tag in an 
  * {@link org.eclipse.emf.ecore.xmi.XMLResource XMLResource} if it is not stored in the element 
  * but directly in the <code>Resource</code>.
@@ -75,7 +75,7 @@ class ChangeRecorder implements AutoCloseable {
 		// it can be potentially a reference to a third party model, for which no create shall be instantiated		
 		create = create && (addedObject.eResource === null || affectedObject === null ||
 			addedObject.eResource == affectedObject.eResource)
-		if (create) existingObjects += addedObject
+		if(create) existingObjects += addedObject
 		return create;
 	}
 
@@ -88,12 +88,11 @@ class ChangeRecorder implements AutoCloseable {
 	def void addToRecording(Notifier notifier) {
 		checkNotDisposed()
 		checkNotNull(notifier, "notifier")
-		checkArgument(notifier.isInOurResourceSet,
-			"cannot record changes in a different resource set!")
+		checkArgument(notifier.isInOurResourceSet, "cannot record changes in a different resource set!")
 
 		if (rootObjects += notifier) {
 			notifier.recursively [
-				if (it instanceof EObject) existingObjects += it
+				if(it instanceof EObject) existingObjects += it
 				addAdapter()
 			]
 		}
@@ -183,7 +182,7 @@ class ChangeRecorder implements AutoCloseable {
 				if (allElementsToDelete.values.exists[it.contains(element)]) {
 					return
 				}
-				var elementsToDelete = element.eAllContents.toList.reverse //delete from inner to outer
+				var elementsToDelete = element.eAllContents.toList.reverse // delete from inner to outer
 				elementsToDelete.forEach [ child |
 					if (allElementsToDelete.containsKey(child)) {
 						allElementsToDelete.remove(child)
@@ -192,8 +191,8 @@ class ChangeRecorder implements AutoCloseable {
 				elementsToDelete.add(element)
 				allElementsToDelete.put(element, elementsToDelete)
 			]
-			changes += allElementsToDelete.values.flatMap [ elementsToDelete | 
-				elementsToDelete.map [ converter.createDeleteChange(it) ]
+			changes += allElementsToDelete.values.flatMap [ elementsToDelete |
+				elementsToDelete.flatMap[converter.createDeleteChanges(it)]
 			].toList
 		}
 		return changes
@@ -315,18 +314,18 @@ class ChangeRecorder implements AutoCloseable {
 					converter.convert(new NotificationInfo(notification))
 				}
 			if (!changes.isEmpty) {
- 				// Register any added object as existing, even if we are not recording
- 				changes.forEach [
- 					switch it {
- 						EObjectAddedEChange<EObject>: {
- 							existingObjects += newValue
- 							switch it {
- 								UpdateReferenceEChange<EObject>: existingObjects += affectedElement
- 							}
- 						}
- 					}
- 				]
- 			}
+				// Register any added object as existing, even if we are not recording
+				changes.forEach [
+					switch it {
+						EObjectAddedEChange<EObject>: {
+							existingObjects += newValue
+							switch it {
+								UpdateReferenceEChange<EObject>: existingObjects += affectedElement
+							}
+						}
+					}
+				]
+			}
 			return changes
 		}
 

bundles/tools.vitruv.change.composite/src/tools/vitruv/change/composite/recording/NotificationToEChangeConverter.xtend

@@ -1,6 +1,9 @@
 package tools.vitruv.change.composite.recording
 
 import java.util.List
+import java.util.LinkedList
+
+import org.eclipse.emf.common.util.EList
 import org.eclipse.emf.common.util.URI
 import org.eclipse.emf.ecore.EAttribute
 import org.eclipse.emf.ecore.EObject
@@ -23,20 +26,48 @@ import static extension tools.vitruv.change.composite.recording.EChangeCreationU
 @FinalFieldsConstructor
 package final class NotificationToEChangeConverter {
 	extension val TypeInferringAtomicEChangeFactory changeFactory = TypeInferringAtomicEChangeFactory.instance
-	
+
 	val (EObject, EObject)=>boolean isCreateChange
-
-	def EChange<EObject> createDeleteChange(EObject eObject) {
-		return createDeleteEObjectChange(eObject)
+
+	/**
+	 * Creates the required changes for the deletion of eObject:
+	 * <ul>
+	 * 	<li>If eObject is contained by another object o2, we first create a RemoveReferenceChange
+	 *  or ReplaceSingleReferenceChange, dependent on whether the containment relation of o2 may
+	 *  also contain other elements, or not. </li>
+	 *  <li>In any case, we also create a DeleteEObjectChange.</li>
+	 * </ul>
+	 */
+	def List<EChange<EObject>> createDeleteChanges(EObject eObject) {
+		val changes = new LinkedList<EChange<EObject>>
+
+		val containingFeature = eObject.eContainmentFeature
+		if (containingFeature !== null) {
+			val container = eObject.eContainer
+
+			if (containingFeature.isMany) {
+				// Reflection to get to the actual list where eObject is contained
+				val containmentListForEObject = container.eGet(containingFeature) as EList<? extends EObject>
+				val indexOfEObject = containmentListForEObject.indexOf(eObject)
+				changes.add(createRemoveReferenceChange(container, containingFeature, eObject, indexOfEObject))
+			}
+			else {
+				changes.add(createReplaceSingleReferenceChange(container, eObject.eContainmentFeature, eObject, null))
+			}
+		}
+
+		changes.add(createDeleteEObjectChange(eObject))
+		changes
 	}
-	
+
 	private def String convertExceptionMessage(EventType eventType, String notificationType) {
 		String.format("Event type {} for {} Notifications unexpected.")
 	}
+
 	final String ATTRIBUTE_TYPE = "Attribute";
 	final String REFERENCE_TYPE = "Reference";
 	final String RESOURCE_CONTENTS_TYPE = "Resource Contents"
-	
+
 	/** 
 	 * Converts the given notification to a list of {@link EChange}s.
 	 * @param n the notification to convert
@@ -57,8 +88,10 @@ package final class NotificationToEChangeConverter {
 					case REMOVE: handleRemoveAttribute(notification)
 					case REMOVE_MANY: handleMultiRemoveAttribute(notification)
 					case MOVE: handleMoveAttribute(notification)
-					case RESOLVE: throw new IllegalArgumentException(convertExceptionMessage(EventType.RESOLVE, ATTRIBUTE_TYPE))
-					case REMOVING_ADAPTER: throw new IllegalArgumentException(convertExceptionMessage(EventType.REMOVING_ADAPTER, ATTRIBUTE_TYPE))
+					case RESOLVE: throw new IllegalArgumentException(
+						convertExceptionMessage(EventType.RESOLVE, ATTRIBUTE_TYPE))
+					case REMOVING_ADAPTER: throw new IllegalArgumentException(
+						convertExceptionMessage(EventType.REMOVING_ADAPTER, ATTRIBUTE_TYPE))
 					default: throw new IllegalArgumentException("Unexpected event type " + eventType)
 				}
 			case notification.isReferenceNotification:
@@ -70,8 +103,10 @@ package final class NotificationToEChangeConverter {
 					case REMOVE: handleRemoveReference(notification)
 					case REMOVE_MANY: handleMultiRemoveReference(notification)
 					case MOVE: handleMoveReference(notification)
-					case RESOLVE: throw new IllegalArgumentException(convertExceptionMessage(EventType.RESOLVE, REFERENCE_TYPE))
-					case REMOVING_ADAPTER: throw new IllegalArgumentException(convertExceptionMessage(EventType.REMOVING_ADAPTER, REFERENCE_TYPE))
+					case RESOLVE: throw new IllegalArgumentException(
+						convertExceptionMessage(EventType.RESOLVE, REFERENCE_TYPE))
+					case REMOVING_ADAPTER: throw new IllegalArgumentException(
+						convertExceptionMessage(EventType.REMOVING_ADAPTER, REFERENCE_TYPE))
 					default: throw new IllegalArgumentException("Unexpected event type " + eventType)
 				}
 			case notifier instanceof Resource:
@@ -82,17 +117,23 @@ package final class NotificationToEChangeConverter {
 							case ADD_MANY: handleMultiInsertRootChange(notification)
 							case REMOVE: handleRemoveRootChange(notification)
 							case REMOVE_MANY: handleMultiRemoveRootChange(notification)
-							case SET: throw new IllegalArgumentException(convertExceptionMessage(EventType.SET, RESOURCE_CONTENTS_TYPE))
-							case UNSET: throw new IllegalArgumentException(convertExceptionMessage(EventType.UNSET, RESOURCE_CONTENTS_TYPE))
-							case MOVE: throw new IllegalArgumentException(convertExceptionMessage(EventType.MOVE, RESOURCE_CONTENTS_TYPE))
-							case RESOLVE: throw new IllegalArgumentException(convertExceptionMessage(EventType.RESOLVE, RESOURCE_CONTENTS_TYPE))
-							case REMOVING_ADAPTER: throw new IllegalArgumentException(convertExceptionMessage(EventType.REMOVING_ADAPTER, RESOURCE_CONTENTS_TYPE))
+							case SET: throw new IllegalArgumentException(
+								convertExceptionMessage(EventType.SET, RESOURCE_CONTENTS_TYPE))
+							case UNSET: throw new IllegalArgumentException(
+								convertExceptionMessage(EventType.UNSET, RESOURCE_CONTENTS_TYPE))
+							case MOVE: throw new IllegalArgumentException(
+								convertExceptionMessage(EventType.MOVE, RESOURCE_CONTENTS_TYPE))
+							case RESOLVE: throw new IllegalArgumentException(
+								convertExceptionMessage(EventType.RESOLVE, RESOURCE_CONTENTS_TYPE))
+							case REMOVING_ADAPTER: throw new IllegalArgumentException(
+								convertExceptionMessage(EventType.REMOVING_ADAPTER, RESOURCE_CONTENTS_TYPE))
 							default: throw new IllegalArgumentException("Unexpected event type " + eventType)
 						}
 					case Resource.RESOURCE__URI:
 						switch (eventTypeEnum) {
 							case SET: handleSetUriChange(notification)
-							default: throw new IllegalArgumentException("Unexpected event type " + eventType + " for Resource URI Notification.")
+							default: throw new IllegalArgumentException("Unexpected event type " + eventType +
+								" for Resource URI Notification.")
 						}
 					default:
 						emptyList()
@@ -101,14 +142,14 @@ package final class NotificationToEChangeConverter {
 				emptyList()
 		}
 	}
-	
+
 	private def Iterable<? extends EChange<EObject>> handleMoveAttribute(extension NotificationInfo notification) {
 		#[
 			createRemoveAttributeChange(notifierModelElement, attribute, oldValue as Integer, newValue),
 			createInsertAttributeChange(notifierModelElement, attribute, position, newValue)
 		]
 	}
-	
+
 	private def Iterable<? extends EChange<EObject>> handleMoveReference(extension NotificationInfo notification) {
 		#[
 			createRemoveReferenceChange(notifierModelElement, reference, newModelElementValue, oldValue as Integer),
@@ -229,7 +270,8 @@ package final class NotificationToEChangeConverter {
 			surroundWithCreateAndFeatureChangesIfNecessary()
 	}
 
-	private def Iterable<? extends EChange<EObject>> handleMultiInsertReference(extension NotificationInfo notification) {
+	private def Iterable<? extends EChange<EObject>> handleMultiInsertReference(
+		extension NotificationInfo notification) {
 		(newValue as List<EObject>).flatMapFixedIndexed [ index, value |
 			createInsertReferenceChange(notifierModelElement, reference, value, initialIndex + index).
 				surroundWithCreateAndFeatureChangesIfNecessary()
@@ -272,12 +314,17 @@ package final class NotificationToEChangeConverter {
 		]
 	}
 
-	def private Iterable<? extends EChange<EObject>> allAdditiveChangesForChangeRelevantFeatures(EObjectAddedEChange<EObject> change, EObject eObject) {
+	def private Iterable<? extends EChange<EObject>> allAdditiveChangesForChangeRelevantFeatures(
+		EObjectAddedEChange<EObject> change, EObject eObject) {
 		change.newValue.walkChangeRelevantFeatures(
 			[object, attribute|createAdditiveEChangeForAttribute(object, attribute)],
-			[object, reference|if (reference.isContainment) createAdditiveEChangeForReferencedObject(object, reference, [referencedObject | isCreateChange.apply(object, referencedObject)])]
+			[ object, reference |
+				if(reference.isContainment) createAdditiveEChangeForReferencedObject(object, reference, [ referencedObject |
+					isCreateChange.apply(object, referencedObject)
+				])
+			]
 		) + change.newValue.walkChangeRelevantFeatures(null) [ object, reference |
-			if (!reference.isContainment) createAdditiveEChangeForReferencedObject(object, reference, [false])
+			if(!reference.isContainment) createAdditiveEChangeForReferencedObject(object, reference, [false])
 		]
 	}
 
@@ -317,7 +364,8 @@ package final class NotificationToEChangeConverter {
 			emptyList()
 	}
 
-	def private <T extends EChange<EObject>> addUnsetChangeIfNecessary(Iterable<T> changes, NotificationInfo notification) {
+	def private <T extends EChange<EObject>> addUnsetChangeIfNecessary(Iterable<T> changes,
+		NotificationInfo notification) {
 		return if (notification.wasUnset)
 			changes +
 				List.of(createUnsetFeatureChange(notification.notifierModelElement, notification.structuralFeature))

tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/recording/ChangeRecorderTest.xtend

@@ -194,7 +194,9 @@ class ChangeRecorderTest {
 			resource.contents.clear()
 		]
 
-		assertThat(changeRecorder.change, hasEChanges(RemoveRootEObject, DeleteEObject, DeleteEObject, DeleteEObject, DeleteEObject))
+		assertThat(changeRecorder.change, hasEChanges(
+			RemoveRootEObject, RemoveEReference, DeleteEObject, RemoveEReference, DeleteEObject, ReplaceSingleValuedEReference, DeleteEObject, DeleteEObject
+		))
 	}
 
 	@DisplayName("adds an object set as containment to the recording")
@@ -370,7 +372,7 @@ class ChangeRecorderTest {
 		record [
 			resource.delete(emptyMap)
 		]
-		assertThat(changeRecorder.change, hasEChanges(RemoveRootEObject, DeleteEObject, DeleteEObject))
+		assertThat(changeRecorder.change, hasEChanges(RemoveRootEObject, ReplaceSingleValuedEReference, DeleteEObject, DeleteEObject))
 	}
 
 	@ParameterizedTest(name="while isRecording={0}")

tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/reference/ChangeDescription2RemoveEReferenceTest.xtend

@@ -145,10 +145,11 @@ class ChangeDescription2RemoveEReferenceTest extends ChangeDescription2ChangeTra
 		]
 
 		// assert
-		result.assertChangeCount(5)
+		result.assertChangeCount(6)
 			.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)
+			.assertReplaceSingleValuedEReference(containedRoot, ROOT__RECURSIVE_ROOT, innerContainedRoot, null, true, false, false)
 			.assertDeleteEObjectAndContainedElements(containedRoot)
 			.assertEmpty
 	}