Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

track cascade deletion in change recorder #48

Merged
merged 1 commit into from
Nov 8, 2022

Conversation

JanWittler
Copy link
Contributor

This PR adapts the ChangeRecorder to create delete changes not only for the element actually deleted, but for all elements that are cascade deleted by it.
As an example, if I have an element a which contains element b and I delete a, both a and b are deleted since a is the container of b. As of now, we didn't explicitly track the deletion of b.

As a conceptual motivation we can argue that our change sequence should describe the entire model state change and thus make cascade deletions explicit. From a more practical motivation, we need these changes to replay change sequences backwards. Currently, due to the use of hierarchical IDs this problem is made transparent, i.e. the tests still succeed although some element IDs cannot be resolved. However, as soon as we migrate to UUIDs this is not the case anymore as no UUID is registered for a cascade deleted element when replaying changes backwards.

@JanWittler
Copy link
Contributor Author

The case studies tests had to be adjusted as described in vitruv-tools/Vitruv-CaseStudies#253

I think disabling part of those tests is reasonable for the scope of this PR, as the bug detected there is not introduced by the changes in this PR but only uncovered by them.

Copy link
Member

@tsaglam tsaglam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting change, simple in its implementation but conceptually complex. I would agree with your notion that a change sequence should be explicit and thus complete regarding deletions. On a related note, I think some of the issues Atilla had in his BA might have stemmed from this problem. So in summary: Great change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants