Support Propagating Changes to Multiple Resources

[jGleitz]

This seems to work with all tests, ~except for one: OperationRequiredRoleMappingTransformationTest#testChangeOperationRequiredRole~

Changes:

• VitruviusChanges don’t store a URI any more, but derive it from the resolved EChange
• Change propagation trusts the ChangePropagationSpecification regarding which domain was modified
• Copying a change is now an instance method (better OO, imo)
• Simplify the inheritance hierarchy of VitruviusChanges

[jGleitz]

corresponding application change: https://github.com/vitruv-tools/Vitruv-Applications-ComponentBasedSystems/pull/118

[jGleitz]

I fixed the failing test. This was actually an error in the production code, however, I don’t know why we didn’t discover it sooner.

[jGleitz]

I updated this branch to match the latest master. It is ready for review.

[jGleitz]

This PR contains a weird race condition where models are not saved if you don’t debug through the code slowly enough.

[HeikoKlare]

That sounds really strange. I hope that the race condition is actually "only" an issue of this PR and was not introduced by a recent one, which we did not recognize before merging it? At least, those PRs changed synchronization behavior and placement of the save operation for the VirtualModel.

[jGleitz]

It seems to affect only the Commonalities, which could mean that the IntermediateResourceBridgeI hack broke.

[jGleitz]

‘Fixed’ it by mimicking the old behaviour more closely. Somehow this error occurred when recording separately for every ChangePropagationSpecification

was not introduced by a recent one, which we did not recognize before merging it?

No, it was specific to this PR. I still don’t understand why the issue sometimes did not occur when debugging, though.

[jGleitz]

1. The tests failed because sometimes changes for inverse references ended up in the resulting changes of change propagation. I am not quite sure why, but I have also not fixed it.
2. The problem I ‘fixed’ as per comment https://github.com/vitruv-tools/Vitruv/pull/412#issuecomment-777095982 was actually that the ResourceRepository did not create copies of the lists it hands out. I fixed that and went back to trusting ChangePropagationSpecifications regarding which domain was changed
3. I first simplified AtomicEmfChangeRecorder because it contained quite some convoluted code.
4. Then I removed AtomicEmfChangeRecorder’s functionality of removing create changes that are followed by a delete. I had multiple reasons for this change:
   1. I found the logic very hard to understand. So much so that I suspected it of having a bug, without being able to figure out whether it was really a bug or intended behaviour.
   2. I suspected the whole approach to be wrong (what happens to modifications between the create and the delete?
   3. For all the reasons I could think of that would require this logic, change recording was the wrong spot to do it. If for example, this is an optimization for change propagation, the VSUM should apply it before propagating the changes.
   4. Nothing broke when I removed the logic.

All tests pass now, as far as I can see.

[jGleitz]

Okay, so now I have pushed two more changes:

• make CompositeChangeimmutable. Together with the polymorphic copy method, this brings us one step closer to immutable changes
• Remove ConcreteChangeWithFixedVuri since it is not needed for normal domains. Introduce a similar class in the Java domain instead.

This means that this PR now also requires a change to the Java domain: https://github.com/vitruv-tools/Vitruv-Domains-ComponentBasedSystems/pull/86

All in all, this PR has gotten pretty big. I didn’t anticipate that supporting changes to multiple resources would result in so many changes to the framework. Neither was I planning to do yet another big refactoring. But hey, I think the framework improved quite a bit with this.

This should hopefully be the final state and ready for review.

[jGleitz]

:+1:

[jGleitz]

:angry:. I made sure to check all tests, but forgot about the transitive change tests. The errors are cryptic and I have no idea what’s going wrong. The Commonalities and Mappings Tests work fine, though.

[jGleitz]

After rebasing on #419, this PR only fails the Transactional tests. A lot of those failures are related to the TUID mechanism that causes bad unloads. So I propose to wait with this PR until we migrated entirely to UUIDs.

[jGleitz]

I merged this branch with master and it is now ready for review.

[jGleitz]

Regarding whether composite changes should take a List or a Collection: I used the most general interface possible (Iterable is not possible because we'll just size when copying).

The collection will be copied and internally will always be a List. The current state allows to pass on something like a LinkedHashSet. That would not be possible if a List was required.

So I don't object to changing to List, but I also don't see the need.

[HeikoKlare]

To me, it'a bit weird that one can pass an unordered collection to a change which then becomes an order assigned, which is then also used to compare the changes. If an order is important (and in my opinion it is important for changes), it should already be clarified when passing it to a composite change. We may also pass an Iterable instead of a List to cover your issue with LinkedHashSet. I do not understand you point why this should not be possible. We take the Iterable and internally store it in a List, like we do now for the passed Collection?

[jGleitz]

I do not understand you point why this should not be possible.

It’s possible but more cumbersome. The nice thing about List#copyOf is that it doesn’t create a copy if the list is already read-only. But List#copyOf only takes a Collection.

To me, it'a bit weird that one can pass an unordered collection to a change which then becomes an order assigned, which is then also used to compare the changes.

We may also pass an Iterable instead of a List

Those two statements are somewhat contradictory since Collection inherits from Iterable. So in both cases, Collection and Iterable, we’d just make a copy of the collection in the order of iteration.

[jGleitz]

I changed the constructors to take a List. The factories still take an Iterable. Should I change this too?

[HeikoKlare]

For me, it's fine to accept an Iterable, so you can keep it as it is.

[jGleitz]

@HeikoKlare I pushed some more cleanup:

• Remove EChange#getInvolvedObjects and ConcreteChange#affectedObjectsEquals
• Remove ChangedResourceTracker
• Simplify applying changes in ChangePropagator

[HeikoKlare]

Perfect, thanks! All changes are fine for me. I make a final run of the applications against these changes and merge them afterwards.