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

Migrate to UUIDs #47

Merged
merged 32 commits into from
Apr 5, 2023
Merged

Migrate to UUIDs #47

merged 32 commits into from
Apr 5, 2023

Conversation

JanWittler
Copy link
Contributor

@JanWittler JanWittler commented Oct 28, 2022

This PR replaces hierarchical IDs (HIDs) with UUIDs for the internal consistency preservation. A UUID uniquely identifies an element but instead of a HID cannot be generated on the fly and is permanent. As such, any element must be registered to the UUIDResolver explicitly. Since elements are identified based on their identity and not their hierarchical position anymore, when copying a Resource, the UUIDResolver cannot be reused but a new one has to be created, matching the elements of the original resolver. As this case happens frequently, UUIDResolver::resolveResources is introduced.
This PR is introduced to enable the VirtualModel to identify all its elements and correspondences based on permanent identifiers, such that versioning, variants, and distributed functionalities are easier to support.

Since HIDs are still needed (in views), currently there are two similar class structures for HIDs and UUIDs. As next step, we should come up with one single interface to hide both functionalities. From my experience working on this PR, I suggest to aim for hiding almost all the currently exposed IDResolver logic and instead only provide functionality to assign IDs to a change or to resolve a change with the appropriate IDs assigned. As such, we should investigate whether changes can be made generic over the type of content (EObject, HID, UUID) they store (see #58).

Changes in detail

  • introduced UUIDResolver and matching classes for assigning UUIDs to changes and applying and resolving changes containing UUIDs
  • removed (UU)ID assignment from ChangeRecorder to reduce the occasions where an IDResolver has to be exposed
  • Extend ChangeRecordingModelRepository with UUIDResolver
  • Adapt ChangePublishingTestView to load the needed UUIDs into its own resolver when loading a resource

Follow-up tasks

@JanWittler JanWittler force-pushed the uuid branch 2 times, most recently from c350785 to 563356b Compare November 8, 2022 12:43
@JanWittler JanWittler requested a review from tsaglam November 10, 2022 12:08
@JanWittler

This comment was marked as outdated.

@JanWittler

This comment was marked as outdated.

@JanWittler
Copy link
Contributor Author

Missing tests for UUID resolver were added and code suggestions implemented.
I created issues for the follow-up tasks:

As such, this PR (and its counterparts in the other repositories) is ready to merge. @vitruv-tools/maintainers

Copy link
Contributor

@h4uges h4uges left a comment

Choose a reason for hiding this comment

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

Reviewed the changes of the implementation code (no tests)

Copy link
Contributor

@h4uges h4uges left a comment

Choose a reason for hiding this comment

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

Reviewed the tests.

@TomWerm TomWerm requested review from HansMartinA and removed request for HansMartinA March 16, 2023 15:28
@TomWerm TomWerm dismissed HansMartinA’s stale review March 16, 2023 15:33

Suggestions are implemented, force pushing seems to cause some problems.

Jan Wittler and others added 26 commits April 3, 2023 12:34
added UUID resolver (de)serialization
support registration of read-only EObjects in UUID resolver
@JanWittler JanWittler requested a review from a team as a code owner April 3, 2023 10:34
@TomWerm TomWerm merged commit b9cb0d1 into main Apr 5, 2023
@TomWerm TomWerm deleted the uuid branch April 5, 2023 07:20
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.

4 participants