From e0fb652618e22fe3bd5251319696c6fb5e3b1f56 Mon Sep 17 00:00:00 2001 From: Benedikt Jutz Date: Thu, 31 Oct 2024 09:54:42 +0100 Subject: [PATCH] Fix for Issue #71 createDeleteChanges (formerly createDeleteChange) creates, for each deleted object that has a container, a 1. RemoveReferenceChange, if this containment relationship is many(=multi)-valued, 2. ReplaceSingleReferenceChange, if the containment only allows one contained object at at time. --- .../composite/recording/ChangeRecorder.xtend | 39 ++++---- .../NotificationToEChangeConverter.xtend | 90 ++++++++++++++----- .../recording/ChangeRecorderTest.xtend | 6 +- ...angeDescription2RemoveEReferenceTest.xtend | 3 +- 4 files changed, 92 insertions(+), 46 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 afd000f3..01b645cb 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 @@ -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 xmi:id tag in an * {@link org.eclipse.emf.ecore.xmi.XMLResource XMLResource} if it is not stored in the element * but directly in the Resource. @@ -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: { - existingObjects += newValue - switch it { - UpdateReferenceEChange: existingObjects += affectedElement - } - } - } - ] - } + // Register any added object as existing, even if we are not recording + changes.forEach [ + switch it { + EObjectAddedEChange: { + existingObjects += newValue + switch it { + UpdateReferenceEChange: existingObjects += affectedElement + } + } + } + ] + } return changes } 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 b2889cdb..ae0c9509 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 @@ -15,6 +15,7 @@ import tools.vitruv.change.atomic.feature.reference.UpdateReferenceEChange import static extension edu.kit.ipd.sdq.commons.util.java.lang.IterableUtil.* import static extension tools.vitruv.change.composite.recording.EChangeCreationUtil.* +import java.util.LinkedList /** * Converts an EMF notification to an {@link EChange}. @@ -23,20 +24,46 @@ 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 createDeleteChange(EObject eObject) { - return createDeleteEObjectChange(eObject) + + /** + * Creates the required changes for the deletion of eObject: + *
    + *
  • 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.
    • + *
  • In any case, we also create a DeleteEObjectChange.
    • + *
+ */ + def List> createDeleteChanges(EObject eObject) { + var changes = new LinkedList> + + val containingFeature = eObject.eContainmentFeature + if (containingFeature !== null) { + var container = eObject.eContainer + + if (containingFeature.isMany) { + var indexOfEObject = containingFeature.eContents.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 +84,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 +99,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 +113,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 +138,14 @@ package final class NotificationToEChangeConverter { emptyList() } } - + private def Iterable> handleMoveAttribute(extension NotificationInfo notification) { #[ createRemoveAttributeChange(notifierModelElement, attribute, oldValue as Integer, newValue), createInsertAttributeChange(notifierModelElement, attribute, position, newValue) ] } - + private def Iterable> handleMoveReference(extension NotificationInfo notification) { #[ createRemoveReferenceChange(notifierModelElement, reference, newModelElementValue, oldValue as Integer), @@ -229,7 +266,8 @@ package final class NotificationToEChangeConverter { surroundWithCreateAndFeatureChangesIfNecessary() } - private def Iterable> handleMultiInsertReference(extension NotificationInfo notification) { + private def Iterable> handleMultiInsertReference( + extension NotificationInfo notification) { (newValue as List).flatMapFixedIndexed [ index, value | createInsertReferenceChange(notifierModelElement, reference, value, initialIndex + index). surroundWithCreateAndFeatureChangesIfNecessary() @@ -272,12 +310,17 @@ package final class NotificationToEChangeConverter { ] } - def private Iterable> allAdditiveChangesForChangeRelevantFeatures(EObjectAddedEChange change, EObject eObject) { + def private Iterable> allAdditiveChangesForChangeRelevantFeatures( + EObjectAddedEChange 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 +360,8 @@ package final class NotificationToEChangeConverter { emptyList() } - def private > addUnsetChangeIfNecessary(Iterable changes, NotificationInfo notification) { + def private > addUnsetChangeIfNecessary(Iterable changes, + NotificationInfo notification) { return if (notification.wasUnset) changes + List.of(createUnsetFeatureChange(notification.notifierModelElement, notification.structuralFeature)) diff --git a/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/recording/ChangeRecorderTest.xtend b/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/recording/ChangeRecorderTest.xtend index 382e150f..beec1b63 100644 --- a/tests/tools.vitruv.change.composite.tests/src/tools/vitruv/change/composite/recording/ChangeRecorderTest.xtend +++ b/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}") 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 1a7dbfb8..0b4e324d 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 @@ -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 }