UUID resolver fails to resolve not cleaned up correspondence

[JanWittler]

Whenever an object with a correspondence is deleted but there is no reaction to delete the corresponding object, the correspondence is not cleaned up (similar to #425). If following the corresponding object is changed, the correspondence is still there and a dead UUID is passed to the UUIDResolver resulting in an IllegalStateException (No EObject could be found for UUID: <UUID>).

A common situation for this in our current applications is the UML Java case. Whenever an UML property is created, a Java attribute and a getter and a setter are generated. When deleting these getter or setter, the corresponding UML property is not deleted. If later on the UML property triggers some reaction, the correspondence with the dead UUID is found and the framework crashes.

Example test code

In context of ```tools.vitruv.applications.umljava.tests.uml2java.UmlToJavaAttributeTest``` Note: I had to change the type of ```uAttr``` to ```pAttr``` otherwise some URI resolving error got thrown. Furthermore it was not possible to use the object returned from the correspondence model as this would throw the error ```cannot record changes in a different resource than that of our UUID resolver!``` ```Xtend @Test def void testDeleteSetter() { val javaFilePath = Path.of(JavaPersistenceHelper.buildJavaFilePath(CLASS_NAME + ".java", #[])) resourceAt(getUri(javaFilePath)).record [ val jCompilationUnit = contents.head as CompilationUnit val jClass = jCompilationUnit.classifiers.head val jClassMethod = jClass.members.filter[name == "setAttributName"].head EcoreUtil.delete(jClassMethod) ] propagate uAttr.name = ATTRIBUTE_RENAME propagate // -> CRASH } ```

[HeikoKlare]

To me, this looks like an issue of the actual Reactions and not of the framework. When objects are deleted, the Reactions should remove the according correspondences. We could also add some mechanism to the framework that removes correspondences for removed objects, but I am not convinced that this would be a good design choice, because it can easily lead to people not cleaning up properly.

Regarding the example: If I understand correctly, you change a name of an attribute that has been removed by the previous change propagation. Then, it's not surprising that this crashed. In fact, when changing both models that are kept consistent, it is important to not change both of them without reloading them after performing change propagation. Since the UML model has changed because of propagation changes to the Java model, updating some outdated UML elements will not work properly.

[jGleitz]

We could also add some mechanism to the framework that removes correspondences for removed objects, but I am not convinced that this would be a good design choice, because it can easily lead to people not cleaning up properly.

At first, I also thought that we should add such a mechanism (i.e. remove dangling correspondences). However, after giving it some thought, I think we should not do this. Correspondences serve a purpose even for deleted objects: First, we use them when handling a delete change to delete the corresponding object. But even after that, they might serve a purpose to determine whether an object was mapped to another object in the past.

But if the above is true, then I think this bug is valid: If I understand it correctly, Vitruv currently doesn’t allow changing objects whose corresponding object has been deleted. But per the above argumentation, having correspondences where one object is deleted is valid. And then the framework should support it.

On the other hand, correspondences to deleted objects also feel weird: Either they are correspondences with no second object (that validates a basic assumption, i.e. that correspondences are always pairs) or the correspondence has a dangling UUID (this validates the implicit assumption that a dangling UUID is always an error). We could take this as a reason not to allow correspondences to deleted objects. But then I think Vitruv should clean up correspondences automatically. On

[JanWittler]

When objects are deleted, the Reactions should remove the according correspondences.

I see your point, however it feels more natural to me to automatically deal with dead correspondences (in whatever way). Another point for an automatic treatment of this is the one-directional case. If we don't have reactions from target domain to source domain, a change in the target domain will not trigger any reactions, thus we can't fix it by defining reactions.

If I understand correctly, you change a name of an attribute that has been removed by the previous change propagation.

Actually the intended behaviour is to delete the setter which should not trigger a change to the uml property (as the java field is still there). Thus the rename is valid. However this assumes some reactions changes which I currently only have locally, so my test case was a bit bad chosen. Sorry for that.

But even after that, they might serve a purpose to determine whether an object was mapped to another object in the past.

This might be a use case (even though I can't think of any specific scenario right now). However, if we allow dead correspondences, they might accumulate which will fill up our correspondence model. At least we don't need to store the UUID of the deleted object anymore.

But if the above is true, then I think this bug is valid: If I understand it correctly, Vitruv currently doesn’t allow changing objects whose corresponding object has been deleted.

Correct.

[HeikoKlare]

Some clarification first: Vitruv does support resolving correspondences for deleted objects. More precisely, during one change propagation (i.e., until VirtualModel.propagateChanges returns), correspondences to deleted objects can still be resolved. This is intended, because they may be multiple transformations that need to react to the changes and still consider the correspondences (maybe even readd the object). UUIDs are only removed (and thus correspondences become unresolvable) after such a transaction finished. And that is on purpose and unavoidable: After a change propagation transaction, only the elements persisted in the models are still relevant. Anything else would not make any sense, because you can also throw away and reload the model, but objects not persisted in any model will be lost then, so you transformations cannot access them anyway.

In consequence, the current behavior is, from my point of view, completely as intended. I absolutely agree that we do not want to have "dead" correspondences, but still it is the responsibility of the Reactions to remove them. I do not think it is valid to let the framework throw away dead correspondences and potentially hide missing cleanup operations. But I think would be more reasonable is to check after a transaction whether there are "dead" correspondences and then throw an exception (like we did when elements not having a UUID were accessed). This would better ensure that transformations are written properly.

Thus the rename is valid.

Although modifying some probably outdated view models is dangerous, the change should be actually be propagated correctly then. It fails, however, because the transformations missed to remove the correspondences when the getter and setter were removed. So fixing them should resolve the problem, shouldn't it?

Another point for an automatic treatment of this is the one-directional case. If we don't have reactions from target domain to source domain, a change in the target domain will not trigger any reactions, thus we can't fix it by defining reactions.

If you modify the target model and are then not able to restore consistency because the transformation from target to source is missing, you are lost anyway, as you cannot achieve a consistent state anymore. Everytime a transformation from target to source is necessary to restore consistency, there is simply the necessity to have it. So I do not consider this a valid argument.

In summary, to me it seems like the best we could do it to ensure that no dead correspondences exist by checking after each transactions and throwing an exception if this is not the case. This enforces transformation delevopers to properly handle changes and cleanup the correspondences. Adding that check will probably lead to several failures in our current applications, but at least they would become easier to identify by #445 ;-)

If there are further arguments for different options, I am happy to discuss them. But right now I would not be in favor of simply removing dead correspondences without any notice.

[jGleitz]

potentially hide missing cleanup operations

You brought that up twice, and it’s unclear to me what kind of scenario you have in mind.

Possibility 1: We want to support correspondences to deleted objects, even after the delete change has been processed. This means that we even want to support correspondences to deleted objects after a VSUM reload, where the deleted object is no longer accessible in any way. This is what I described at the start of my first comment

Possibility 2: We don’t want to support the above, correspondences to deleted objects are only allowed while still processing the delete change. In that case, I don’t understand why we put the burden of deleting the correspondences on the transformations. Because the transformations have only one option: delete the correspondence. If it’s clear what needs to happen, why not just do it? The only kind of ‘missing cleanup operation’ I can think of would be forgetting to remove the corresponding object. However, that kind of ‘mistake’ should be covered by any basic test suite.

To conclude: I think we should clarify whether possibility 1 or possibility 2 is the intended behaviour for Vitruv. @HeikoKlare seems to advocate for possibility 2, because only then is throwing an exception valid. But if it’s possibility 2, I don’t understand which kind of bug @HeikoKlare has in mind that would be hidden if we removed dangling correspondences after the delete change has been processed.

[HeikoKlare]

This means that we even want to support correspondences to deleted objects after a VSUM reload, where the deleted object is no longer accessible in any way

I do not understand that. In that case, you can only do one thing: Take the object at the other end of the correspondence, which is not deleted, and find out that there is a correspondence pointing to any deleted object. I cannot imagine any situation in which that is useful. Maybe you can clarify where you expect this to be useful.

The only kind of ‘missing cleanup operation’ I can think of would be forgetting to remove the corresponding object.

I'm sorry for the imprecision. I can think of several scenarios in which this is important. First, considering our current tests the assumption that missing object removals are covered by test suites is not correct. We can, of course, say that this is a matter of improving tests, but we have no guarantee that tests will ever be complete in that regard (except for complete equivalence tests), which is why I would expect that behavior to improve transformations by design (even without proper test effort) and especially the ones that already exist. Second, correspondencens indicate that there are related elements that have to be updated on change of the other. If you properly remove a corresponding element without the correspondence, other transformations (in particular the one in the opposite direction) can easily misinterpret the existence of the correspondence as the information that there is still some element to be updated. Third, having transitive execution of transformations, it can easily happen that one transformation does not consider elements to already be existing and simply replaces/overwrites the existing ones. Only comparing for final equivalence, this does not lead to an error, because the result may be the same. However, there are unnecessary propagations, which could eventually also lead to an infinite number of overwrites (which would then, of course, lead to test error if proper tests are provided). This is something that cannot be detected in any other way than having correspondences to deleted objects that should actually still exist. I expect this situation to occur a very large number of times in the current commonalities case study, as many elements are overwritten by the same ones multiple times. And I have already faced that situation several times in our transformations.

From my point of view, it should be at least good practice (if not even necessary due to the arguments above) to remove a correspondence as soon as the mapping (i.e. the relation between two matching patterns of model elements kept consistent) for which it is used as a trace (or even witness) is not present anymore. I do not understand why it should be beneficial to tolerate that correspondences are dangling around, potentially leading to unintended element retrievals, instead of avoiding that situation by design. The missing removal of correspondences will, for the reasons discussed above, eventually lead to errors, such that it should not be something that we simply tolerate by providing a kind of garbage collection and thus even decreasing the motivation to properly remove correspondences within the transformations.

[JanWittler]

Third, having transitive execution of transformations, it can easily happen that one transformation does not consider elements to already be existing and simply replaces/overwrites the existing ones. Only comparing for final equivalence, this does not lead to an error, because the result may be the same. However, there are unnecessary propagations, which could eventually also lead to an infinite number of overwrites

I don't really understand your point here. To me, it sounds more like a general argument for using correspondences rather than equivalence matching.

This is something that cannot be detected in any other way than having correspondences to deleted objects that should actually still exist. I expect this situation to occur a very large number of times in the current commonalities case study

As the UUIDResolver currently crashes when trying to fetch a deleted object, right now we don't have any cases where a not cleaned up correspondence exists or leads to a problem. (Probably there a some cases where some exist but our tests don't uncover them.)

In general, I am totally on your side of not allowing correspondences with a deleted object. However, when it comes to the decision of either throwing an error when a dead correspondence is not removed or removing dead correspondences automatically, I don't get your arguments of not doing this automatically. I don't think we should argue whether some feature is reasonable by how well we can test it. Maybe it gets clearer for me if you detail your transitive execution argument.

I do not understand why it should be beneficial to tolerate that correspondences are dangling around, potentially leading to unintended element retrievals, instead of avoiding that situation by design.

Absolutely right! However, my conclusion from that argument would be to support automatic correspondences clean-up 😅 . Another point why it seems so unintuitive for me to delete correspondences for deleted objects manually: If I delete the target domain object in the reactions, its correspondences are automatically removed.

[HeikoKlare]

As the UUIDResolver currently crashes when trying to fetch a deleted object, right now we don't have any cases where a not cleaned up correspondence exists or leads to a problem.

This is not true. The UuidGeneratorAndResolver is able to resolve a UUID for a deleted object, as long as it still in the transaction of the same original change performed by the user. And this is also where the problem occurs.

1. A transformation may overwrite an existing object (accidentally): I faced this problem several times when moving from TUIDs to UUIDs and this issue still resides in the Commonalities language implementation and case study and potentially also other case studies multiple times. A transformation creates a target element anew although the (same) element already exists and is related by a correspondence. We could easily detect this situation when requiring that correspondences have to be removed, because in this case at the end of change propagation there is a dangling corresponding for the accidentally removed element. As an example, we had this situation with type references, which were aways created anew between UML and Java instead of considering whether the existing one already fits. Since the new element is simply placed into a single-valued containment reference, there is no explicit deletion of the old object that the transformations considered or even recognized. In addition, we know that the Commonalities create elements anew all the time, although the same elements already exist. This results in the same models, but with unnecessary (and finally faulty) element overwrites, which could easily be detected early by accidentally dangling correspondences.
2. A transformation does, by accident, remove an element but does not remove the correspondence. Then, the transformation for the opposite direction still sees the corresponcence and may perform the removal again. Although one may ignore this because the element is removed anyone, it is, first, unnecessary and unintended, and, second, can result in failures if the transformation performing the removal in the opposite direction has further side effects (although I have to admit that this should be detected by tests). In general, Reactions avoid this situation because, as you pointed out, correspondences are removed when deleting an object related by that correspondence. However, using Reactions is not mandatory.

All in all, not removing correspondences right when they should be removed leads to faulty or at least unnecessary transformation executions. Cleaning up afterwards means that we silently accept that transformations do unintended things meanwhile (which may lead to a correct result but especially makes debugging in situations in which this is not the case pretty hard because of many unintended transformation executions) . And I have to admit that I do not understand why we should accept that. Requiring correspondences to be removed properly does not require significant effort from the transformation developer but provides huge benefits due to the avoidance of unnecessary and faulty transformation executions that are then hard and cumbersome to debug.

To me, it sounds more like a general argument for using correspondences rather than equivalence matching.

If you delete the faulty correspondences, you cannot test for them ;-) But you can avoid that they exist by throwing an exception.

[HeikoKlare]

If I am not mistaken, this issue should be resolved implicitly by #462. Since we now use EObjects rather than UUIDs in correspondences, retrieving corresponding elements does not depend on the ability to retrieve a UUID. Having the elements at hand, you will still be able to resolve corresponding elements. Is that correct? Or is there still something to do w.r.t. this issue?

[JanWittler]

I assume you are right. At least there will be no problem with the UUID anymore, so this issue is outdated. I will test with the new logic whether there might still be a problem with not cleaned up correspondences. If so, I will create a new ticket based on the new EObject referencing logic.

[HeikoKlare]

Perfect, thank you!

[JanWittler]

I tested it again with the above test which now succeeds. The dead correspondence is now serialised as a correspondence with only one participant, so it will not appear anymore as it cannot be matched. This will make no harm to the system, however throwing a warning / error should still be considered as otherwise such incorrect cleanup is probably never detected.

<correspondences xsi:type="reactions:ReactionsCorrespondence" tag="setter">
    <leftEObjects href="../../model/model.uml#_3AE3QpH-EeuzpKsP_VPHYA"/>
  </correspondences>