Skip to content

Commit

Permalink
Merge pull request #1020 from modelix/fix/model-synchronizer-nodes-wi…
Browse files Browse the repository at this point in the history 
…thout-id

MODELIX-1008 Sync with subtree skipping fails syncing new node to MPS
  • Loading branch information
@mhuster23
mhuster23 authored Aug 28, 2024
2 parents 4c12d51 + 07d8f1e commit 6c5f870
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 9 deletions.
17 changes: 11 additions & 6 deletions bulk-model-sync-lib/src/commonMain/kotlin/org/modelix/model/sync/bulk/ModelSynchronizer.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import org.modelix.model.api.INodeReference
import org.modelix.model.api.IReferenceLink
import org.modelix.model.api.IReplaceableNode
import org.modelix.model.api.IRole
import org.modelix.model.api.PNodeAdapter
import org.modelix.model.api.isChildRoleOrdered
import org.modelix.model.api.remove
import org.modelix.model.data.NodeData
Expand Down Expand Up @@ -121,8 +122,6 @@ class ModelSynchronizer(

val allExpectedNodesDoNotExist by lazy {
sourceNodes.all { sourceNode ->
val originalId = sourceNode.originalId()
checkNotNull(originalId) { "Specified node '$sourceNode' has no ID." }
nodeAssociation.resolveTarget(sourceNode) == null
}
}
Expand All @@ -132,8 +131,6 @@ class ModelSynchronizer(
targetParent.addNewChildren(role, -1, sourceNodes.map { it.getConceptReference() })
.zip(sourceNodes)
.forEach { (newChild, sourceChild) ->
val expectedId = sourceChild.originalId()
checkNotNull(expectedId) { "Specified node '$sourceChild' has no ID." }
nodeAssociation.associate(sourceChild, newChild)
synchronizeNode(sourceChild, newChild)
}
Expand All @@ -153,7 +150,7 @@ class ModelSynchronizer(

sourceNodes.forEachIndexed { indexInImport, expected ->
val existingChildren = targetParent.getChildren(role).toList()
val expectedId = checkNotNull(expected.originalId()) { "Specified node '$expected' has no id" }
val expectedId = checkNotNull(expected.originalIdOrFallback()) { "Specified node '$expected' has no id" }
// newIndex is the index on which to import the expected child.
// It might be -1 if the child does not exist and should be added at the end.
val newIndex = if (isOrdered) {
Expand All @@ -165,7 +162,7 @@ class ModelSynchronizer(
// Reusable indexing would be possible if we switch from
// a depth-first import to a breadth-first import.)
existingChildren
.indexOfFirst { existingChild -> existingChild.originalId() == expected.originalId() }
.indexOfFirst { existingChild -> existingChild.originalId() == expectedId }
}
// existingChildren.getOrNull handles `-1` as needed by returning `null`.
val nodeAtIndex = existingChildren.getOrNull(newIndex)
Expand Down Expand Up @@ -288,3 +285,11 @@ class ModelSynchronizer(
fun needsSynchronization(node: INode): Boolean
}
}

private fun INode.originalIdOrFallback(): String? {
val originalRef = getOriginalReference()
if (originalRef != null) return originalRef

if (this is PNodeAdapter) return reference.serialize()
return null
}
70 changes: 69 additions & 1 deletion ...model-sync-lib/src/commonTest/kotlin/org/modelix/model/sync/bulk/ModelSynchronizerTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,94 @@

package org.modelix.model.sync.bulk

import org.modelix.model.ModelFacade
import org.modelix.model.api.IBranch
import org.modelix.model.api.INode
import org.modelix.model.api.IProperty
import org.modelix.model.api.PBranch
import org.modelix.model.api.PNodeAdapter
import org.modelix.model.api.addNewChild
import org.modelix.model.api.getDescendants
import org.modelix.model.api.getRootNode
import org.modelix.model.client.IdGenerator
import org.modelix.model.data.ModelData
import org.modelix.model.data.NodeData.Companion.ID_PROPERTY_KEY
import org.modelix.model.lazy.CLTree
import org.modelix.model.lazy.ObjectStoreCache
import org.modelix.model.operations.AddNewChildOp
import org.modelix.model.operations.AddNewChildrenOp
import org.modelix.model.operations.OTBranch
import org.modelix.model.persistent.MapBasedStore
import org.modelix.model.test.RandomModelChangeGenerator
import kotlin.js.JsName
import kotlin.random.Random
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertTrue

class ModelSynchronizerTest : AbstractModelSyncTest() {
open class ModelSynchronizerTest : AbstractModelSyncTest() {

@Test
@JsName("can_handle_added_child_without_original_id_without_existing_sibling")
fun `can handle added child without original id (without existing sibling)`() {
val sourceBranch = createLocalBranch().apply {
runWrite {
getRootNode().apply {
setPropertyValue(ID_PROPERTY_KEY, "root")
addNewChild("test")
}
}
}.toOTBranch()

val targetBranch = createLocalBranch().apply {
runWrite {
getRootNode().setPropertyValue(ID_PROPERTY_KEY, "root")
}
}.toOTBranch()

runTest(sourceBranch, targetBranch) {
assertEquals(1, targetBranch.getRootNode().allChildren.count())
assertEquals(1, targetBranch.getNumOfUsedOperationsByType()[AddNewChildrenOp::class])
}
}

@Test
@JsName("can_handle_added_child_without_original_id_with_existing_sibling")
fun `can handle added child without original id (with existing sibling)`() {
val sourceBranch = createLocalBranch().apply {
runWrite {
getRootNode().apply {
setPropertyValue(ID_PROPERTY_KEY, "root")
addNewChild("test").setPropertyValue(ID_PROPERTY_KEY, "sibling")
addNewChild("test")
}
}
}.toOTBranch()

val targetBranch = createLocalBranch().apply {
runWrite {
getRootNode().apply {
setPropertyValue(ID_PROPERTY_KEY, "root")
addNewChild("test").setPropertyValue(ID_PROPERTY_KEY, "sibling")
}
}
}.toOTBranch()

runTest(sourceBranch, targetBranch) {
assertEquals(2, targetBranch.getRootNode().allChildren.count())
assertEquals(1, targetBranch.getNumOfUsedOperationsByType()[AddNewChildOp::class])
}
}

private fun createLocalBranch() = ModelFacade.toLocalBranch(ModelFacade.newLocalTree())

override fun runTest(initialData: ModelData, expectedData: ModelData, assertions: OTBranch.() -> Unit) {
val sourceBranch = createOTBranchFromModel(expectedData)
val targetBranch = createOTBranchFromModel(initialData)
runTest(sourceBranch, targetBranch, assertions)
}

private fun runTest(sourceBranch: OTBranch, targetBranch: OTBranch, assertions: OTBranch.() -> Unit) {
sourceBranch.runRead {
val sourceRoot = sourceBranch.getRootNode()

Expand Down Expand Up @@ -138,3 +202,7 @@ class ModelSynchronizerTest : AbstractModelSyncTest() {
}
}
}

private fun IBranch.toOTBranch(): OTBranch {
return OTBranch(this, IdGenerator.getInstance(1), ObjectStoreCache(MapBasedStore()))
}
3 changes: 1 addition & 2 deletions ...c/jvmTest/kotlin/org/modelix/model/sync/bulk/ModelSynchronizerWithInvalidationTreeTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,11 @@ import org.modelix.model.lazy.CLTree
import org.modelix.model.lazy.ObjectStoreCache
import org.modelix.model.operations.OTBranch
import org.modelix.model.persistent.MapBasedStore
import org.modelix.model.sync.bulk.ModelSynchronizerTest.BasicAssociation
import org.modelix.model.test.RandomModelChangeGenerator
import kotlin.random.Random
import kotlin.test.assertTrue

class ModelSynchronizerWithInvalidationTreeTest : AbstractModelSyncTest() {
class ModelSynchronizerWithInvalidationTreeTest : ModelSynchronizerTest() {

override fun runTest(initialData: ModelData, expectedData: ModelData, assertions: OTBranch.() -> Unit) {
val sourceBranch = createOTBranchFromModel(expectedData)
Expand Down

0 comments on commit 6c5f870

Please sign in to comment.