fix(mps-model-adapters): fix MPSNode.replaceNode

* correctly parse concept UID
* check preconditions before making changes
* remove child from parent before moving to new parent
* correctly call `SNode.insertChildBefore`

Author: Oleksandr Dzhychko

mps-model-adapters-plugin/src/test/kotlin/org/modelix/model/mpsadapters/ReplaceNodeTest.kt

@@ -1,43 +1,71 @@
 package org.modelix.model.mpsadapters
 
-import org.junit.Ignore
 import org.modelix.model.api.BuiltinLanguages
 import org.modelix.model.api.ConceptReference
-import org.modelix.model.api.INode
 import org.modelix.model.api.IReplaceableNode
 
-@Ignore("Replacing a node through MPS-model-adapters is broken. See MODELIX-920")
 class ReplaceNodeTest : MpsAdaptersTestBase("SimpleProject") {
 
-    fun testReplaceNode() {
-        readAction {
-            assertEquals(1, mpsProject.projectModules.size)
-        }
+    fun `test replace node`() = runCommandOnEDT {
+        val rootNode = getRootUnderTest()
+        val nodeToReplace = rootNode.allChildren.first() as IReplaceableNode
+        val oldProperties = nodeToReplace.getAllProperties().toSet()
+        check(oldProperties.isNotEmpty()) { "Test should replace node with properties." }
+        val oldReferences = nodeToReplace.getAllReferenceTargetRefs().toSet()
+        check(oldReferences.isNotEmpty()) { "Test should replace node with references." }
+        val oldChildren = nodeToReplace.allChildren.toList()
+        check(oldChildren.isNotEmpty()) { "Test should replace node with children." }
+        val newConcept = ConceptReference("mps:f3061a53-9226-4cc5-a443-f952ceaf5816/1083245097125")
+
+        val newNode = nodeToReplace.replaceNode(newConcept)
 
-        val repositoryNode: INode = MPSRepositoryAsNode(mpsProject.repository)
+        assertEquals(oldProperties, newNode.getAllProperties().toSet())
+        assertEquals(oldReferences, newNode.getAllReferenceTargetRefs().toSet())
+        assertEquals(oldChildren, newNode.allChildren.toList())
+        assertEquals(newConcept, newNode.getConceptReference())
+    }
 
-        runCommandOnEDT {
-            val module = repositoryNode.getChildren(BuiltinLanguages.MPSRepositoryConcepts.Repository.modules)
-                .single { it.getPropertyValue(BuiltinLanguages.jetbrains_mps_lang_core.INamedConcept.name) == "Solution1" }
-            val model = module.getChildren(BuiltinLanguages.MPSRepositoryConcepts.Module.models)
-                .single { it.getPropertyValue(BuiltinLanguages.jetbrains_mps_lang_core.INamedConcept.name) == "Solution1.model1" }
+    fun `test fail to replace node without parent`() = runCommandOnEDT {
+        val rootNode = getRootUnderTest()
+        val oldChildren = rootNode.allChildren.toList()
+        check(oldChildren.isNotEmpty()) { "Test should replace node with children." }
 
-            val rootNode = model.getChildren(BuiltinLanguages.MPSRepositoryConcepts.Model.rootNodes).single() as IReplaceableNode
+        val newConcept = ConceptReference(BuiltinLanguages.jetbrains_mps_lang_core.BaseConcept.getUID())
 
-            val oldProperties = rootNode.getAllProperties().toSet()
-            check(oldProperties.isNotEmpty()) { "Test should replace node with properties." }
-            val oldReferences = rootNode.getAllReferenceTargetRefs().toSet()
-            check(oldReferences.isNotEmpty()) { "Test should replace node with references." }
-            val oldChildren = rootNode.allChildren.toList()
-            check(oldChildren.isNotEmpty()) { "Test should replace node with children." }
+        val expectedMessage = "Cannot replace node `Class1` because it has no parent."
+        assertThrows(IllegalArgumentException::class.java, expectedMessage) {
+            rootNode.replaceNode(newConcept)
+        }
+        // Assert that precondition is check before children are deleted.
+        assertEquals(oldChildren, rootNode.allChildren.toList())
+    }
 
-            val newConcept = ConceptReference("mps:f3061a53-9226-4cc5-a443-f952ceaf5816/1083245097125")
-            val newNode = rootNode.replaceNode(newConcept)
+    fun `test fail to replace node with null concept`() = runCommandOnEDT {
+        val rootNode = getRootUnderTest()
+        val nodeToReplace = rootNode.allChildren.first() as IReplaceableNode
 
-            assertEquals(oldProperties, newNode.getAllProperties().toSet())
-            assertEquals(oldReferences, newNode.getAllReferenceTargetRefs().toSet())
-            assertEquals(oldChildren, newNode.allChildren.toList())
-            assertEquals(newConcept, newNode.getConceptReference())
+        val expectedMessage = "Cannot replace node `method1` with a null concept. Explicitly specify a concept (e.g., `BaseConcept`)."
+        assertThrows(IllegalArgumentException::class.java, expectedMessage) {
+            nodeToReplace.replaceNode(null)
         }
     }
+
+    fun `test fail to replace node with non mps concept`() = runCommandOnEDT {
+        val rootNode = getRootUnderTest()
+        val nodeToReplace = rootNode.allChildren.first() as IReplaceableNode
+        val newConcept = ConceptReference("notMpsConcept")
+
+        val expectedMessage = "Concept UID `notMpsConcept` cannot be parsed as MPS concept."
+        assertThrows(IllegalArgumentException::class.java, expectedMessage) {
+            nodeToReplace.replaceNode(newConcept)
+        }
+    }
+
+    private fun getRootUnderTest(): IReplaceableNode {
+        val repositoryNode = MPSRepositoryAsNode(mpsProject.repository)
+        val module = repositoryNode.getChildren(BuiltinLanguages.MPSRepositoryConcepts.Repository.modules)
+            .single { it.getPropertyValue(BuiltinLanguages.jetbrains_mps_lang_core.INamedConcept.name) == "Solution1" }
+        val model = module.getChildren(BuiltinLanguages.MPSRepositoryConcepts.Module.models).single()
+        return model.getChildren(BuiltinLanguages.MPSRepositoryConcepts.Model.rootNodes).single() as IReplaceableNode
+    }
 }

mps-model-adapters/src/main/kotlin/org/modelix/model/mpsadapters/MPSNode.kt

@@ -3,11 +3,9 @@ package org.modelix.model.mpsadapters
 import jetbrains.mps.lang.smodel.generator.smodelAdapter.SNodeOperations
 import jetbrains.mps.smodel.MPSModuleRepository
 import jetbrains.mps.smodel.adapter.MetaAdapterByDeclaration
-import jetbrains.mps.smodel.adapter.ids.SConceptId
 import jetbrains.mps.smodel.adapter.ids.SContainmentLinkId
 import jetbrains.mps.smodel.adapter.ids.SPropertyId
 import jetbrains.mps.smodel.adapter.ids.SReferenceLinkId
-import jetbrains.mps.smodel.adapter.structure.concept.SConceptAdapterById
 import jetbrains.mps.smodel.adapter.structure.link.SContainmentLinkAdapterById
 import jetbrains.mps.smodel.adapter.structure.property.SPropertyAdapterById
 import jetbrains.mps.smodel.adapter.structure.ref.SReferenceLinkAdapterById
@@ -62,20 +60,25 @@ data class MPSNode(val node: SNode) : IDefaultNodeAdapter, IReplaceableNode {
         }
 
     override fun replaceNode(concept: ConceptReference?): INode {
-        requireNotNull(concept) { "Can't replace $node with null concept. Use BaseConcept explicitly." }
+        requireNotNull(concept) { "Cannot replace node `$node` with a null concept. Explicitly specify a concept (e.g., `BaseConcept`)." }
+        val parent = requireNotNull(node.parent) { "Cannot replace node `$node` because it has no parent." }
+
+        val mpsConcept = MPSConcept.tryParseUID(concept.uid)
+        requireNotNull(mpsConcept) { "Concept UID `${concept.uid}` cannot be parsed as MPS concept." }
+        val sConcept = MetaAdapterByDeclaration.asInstanceConcept(mpsConcept.concept)
 
         val id = node.nodeId
         val model = checkNotNull(node.model) { "Node is not part of a model" }
-        val newNode = model.createNode(SConceptAdapterById(SConceptId.deserialize(concept.uid), ""), id)
+        val newNode = model.createNode(sConcept, id)
         node.properties.forEach { newNode.setProperty(it, node.getProperty(it)) }
         node.references.forEach { newNode.setReference(it.link, it.targetNodeReference) }
         node.children.forEach { child ->
             val link = checkNotNull(child.containmentLink) { "Containment link of child node not found" }
+            node.removeChild(child)
             newNode.addChild(link, child)
         }
 
-        val parent = checkNotNull(node.parent) { "Cannot replace node without a parent" }
-        parent.insertChildBefore(getMPSContainmentLink(getContainmentLink()), node, newNode)
+        parent.insertChildBefore(getMPSContainmentLink(getContainmentLink()), newNode, node)
         node.delete()
         return MPSNode(newNode)
     }