#1687 fix: restoring key map groups would sometimes fail due to improper breadth first traversal of the group tree

Author: sds100

CHANGELOG.md

@@ -18,6 +18,7 @@
 - #1683 key event actions work in Minecraft and other apps again.
 - Export log files as .txt instead of .zip files.
 - #1684 Removed the redundant and broken refresh devices button when configuring a key event action because they are automatically refreshed anyway.
+- #1687 restoring key map groups would sometimes fail.
 
 ## [3.0.1](https://github.com/sds100/KeyMapper/releases/tag/v3.0.1)
 

app/src/main/java/io/github/sds100/keymapper/backup/BackupManager.kt

@@ -54,7 +54,9 @@ import io.github.sds100.keymapper.util.Error
 import io.github.sds100.keymapper.util.Result
 import io.github.sds100.keymapper.util.State
 import io.github.sds100.keymapper.util.Success
+import io.github.sds100.keymapper.util.TreeNode
 import io.github.sds100.keymapper.util.UuidGenerator
+import io.github.sds100.keymapper.util.breadFirstTraversal
 import io.github.sds100.keymapper.util.onFailure
 import io.github.sds100.keymapper.util.then
 import kotlinx.coroutines.CoroutineScope
@@ -473,55 +475,11 @@ class BackupManagerImpl(
 
                 // Group parents must be restored first so an SqliteConstraintException
                 // is not thrown when restoring a child group.
-                val groupsToRestoreMap = backupContent.groups.associateBy { it.uid }.toMutableMap()
-                val groupRestoreQueue = LinkedList<GroupEntity>()
+                val groupRestoreTrees = buildGroupTrees(backupContent.groups)
 
-                // Order the groups into a queue such that a parent is always before a child.
-                for (group in backupContent.groups) {
-                    if (groupsToRestoreMap.containsKey(group.uid)) {
-                        groupRestoreQueue.addFirst(group)
-                    }
-
-                    var parent = groupsToRestoreMap[group.parentUid]
-
-                    while (parent != null) {
-                        groupRestoreQueue.addFirst(parent)
-                        groupsToRestoreMap.remove(parent.uid)
-                        parent = groupsToRestoreMap[parent.parentUid]
-                    }
-                }
-
-                for (group in groupRestoreQueue) {
-                    // Set the last opened date to now so that the imported group
-                    // shows as the most recent.
-                    var modifiedGroup = group.copy(lastOpenedDate = currentTime)
-
-                    // If the group's parent wasn't backed up or doesn't exist
-                    // then set it the parent to the root group
-                    if (!groupUids.contains(group.parentUid)) {
-                        modifiedGroup = modifiedGroup.copy(parentUid = null)
-                    }
-
-                    val siblings =
-                        groupRepository.getGroupsByParent(modifiedGroup.parentUid).first()
-
-                    modifiedGroup = RepositoryUtils.saveUniqueName(
-                        modifiedGroup,
-                        saveBlock = { renamedGroup ->
-                            // Do not rename the group with a (1) if it is the same UID. Just overwrite the name.
-                            if (siblings.any { sibling -> sibling.uid != renamedGroup.uid && sibling.name == renamedGroup.name }) {
-                                throw IllegalStateException("Non unique group name")
-                            }
-                        },
-                        renameBlock = { entity, suffix ->
-                            entity.copy(name = "${entity.name} $suffix")
-                        },
-                    )
-
-                    if (existingGroupUids.contains(modifiedGroup.uid)) {
-                        groupRepository.update(modifiedGroup)
-                    } else {
-                        groupRepository.insert(modifiedGroup)
+                for (tree in groupRestoreTrees) {
+                    tree.breadFirstTraversal { group ->
+                        restoreGroup(group, currentTime, groupUids, existingGroupUids)
                     }
                 }
             }
@@ -615,6 +573,91 @@ class BackupManagerImpl(
         }
     }
 
+    private suspend fun restoreGroup(
+        group: GroupEntity,
+        currentTime: Long,
+        groupUids: Set<String>,
+        existingGroupUids: Set<String>,
+    ) {
+        // Set the last opened date to now so that the imported group
+        // shows as the most recent.
+        var modifiedGroup = group.copy(lastOpenedDate = currentTime)
+
+        // If the group's parent wasn't backed up or doesn't exist
+        // then set it the parent to the root group
+        if (!groupUids.contains(group.parentUid)) {
+            modifiedGroup = modifiedGroup.copy(parentUid = null)
+        }
+
+        val siblings =
+            groupRepository.getGroupsByParent(modifiedGroup.parentUid).first()
+
+        modifiedGroup = RepositoryUtils.saveUniqueName(
+            modifiedGroup,
+            saveBlock = { renamedGroup ->
+                // Do not rename the group with a (1) if it is the same UID. Just overwrite the name.
+                if (siblings.any { sibling -> sibling.uid != renamedGroup.uid && sibling.name == renamedGroup.name }) {
+                    throw IllegalStateException("Non unique group name")
+                }
+            },
+            renameBlock = { entity, suffix ->
+                entity.copy(name = "${entity.name} $suffix")
+            },
+        )
+
+        if (existingGroupUids.contains(modifiedGroup.uid)) {
+            groupRepository.update(modifiedGroup)
+        } else {
+            groupRepository.insert(modifiedGroup)
+        }
+    }
+
+    /**
+     * Converts the group relationships into trees. This first finds all the root groups which
+     * have no parent. Then it loops over all the other groups indefinitely until they have been
+     * added to their parent. If the parent does not exist while looping then it is skipped and
+     * processed in the next iteration.
+     *
+     * @return A list of the root nodes for all the group trees.
+     */
+    private fun buildGroupTrees(groups: List<GroupEntity>): List<TreeNode<GroupEntity>> {
+        if (groups.isEmpty()) {
+            return emptyList()
+        }
+
+        val nodeMap = mutableMapOf<String, TreeNode<GroupEntity>>()
+        val rootNodes = mutableListOf<TreeNode<GroupEntity>>()
+
+        val groupQueue = LinkedList<GroupEntity>()
+
+        for (group in groups) {
+            if (group.parentUid == null) {
+                val node = TreeNode(group)
+                nodeMap[group.uid] = node
+                rootNodes.add(node)
+            } else {
+                groupQueue.add(group)
+            }
+        }
+
+        while (groupQueue.isNotEmpty()) {
+            val groupsToRemove = mutableListOf<GroupEntity>()
+
+            for (group in groupQueue) {
+                if (nodeMap.containsKey(group.parentUid)) {
+                    val node = TreeNode(group)
+                    nodeMap[group.uid] = node
+                    nodeMap[group.parentUid]!!.children.add(node)
+                    groupsToRemove.add(group)
+                }
+            }
+
+            groupQueue.removeAll(groupsToRemove.toSet())
+        }
+
+        return rootNodes
+    }
+
     private suspend fun appendKeyMapsInRepository(keyMaps: List<KeyMapEntity>) = withContext(dispatchers.default()) {
         val randomUids = keyMaps.map { it.copy(uid = UUID.randomUUID().toString()) }
         keyMapRepository.insert(*randomUids.toTypedArray())

app/src/main/java/io/github/sds100/keymapper/util/TreeNode.kt

@@ -0,0 +1,16 @@
+package io.github.sds100.keymapper.util
+
+data class TreeNode<T>(val value: T, val children: MutableList<TreeNode<T>> = mutableListOf())
+
+inline fun <T> TreeNode<T>.breadFirstTraversal(
+    action: (T) -> Unit,
+) {
+    val queue = ArrayDeque<TreeNode<T>>()
+    queue.add(this)
+
+    while (queue.isNotEmpty()) {
+        val currentNode = queue.removeFirst()
+        action(currentNode.value)
+        queue.addAll(currentNode.children)
+    }
+}

app/src/test/java/io/github/sds100/keymapper/BackupManagerTest.kt

@@ -136,6 +136,61 @@ class BackupManagerTest {
         Dispatchers.resetMain()
     }
 
+    /**
+     * Issue #1655. If the list of groups in the backup has a child before the parent then the
+     * parent must be restored first. Otherwise the SqliteConstraintException will be thrown.
+     */
+    @Test
+    fun `restore groups breadth first so parents exist before children are restored with child first in the backup`() = runTest(testDispatcher) {
+        val parentGroup1 = GroupEntity(
+            uid = "parent_group_1_uid",
+            name = "parent_group_1_name",
+            parentUid = null,
+            lastOpenedDate = 0L,
+        )
+
+        val parentGroup2 = GroupEntity(
+            uid = "parent_group_2_uid",
+            name = "parent_group_2_name",
+            parentUid = null,
+            lastOpenedDate = 0L,
+        )
+
+        val childGroup = GroupEntity(
+            uid = "child_group_uid",
+            name = "child_group_name",
+            parentUid = parentGroup1.uid,
+            lastOpenedDate = 0L,
+        )
+
+        val grandChildGroup = GroupEntity(
+            uid = "grand_child_group_uid",
+            name = "grand_child_group_name",
+            parentUid = childGroup.uid,
+            lastOpenedDate = 0L,
+        )
+
+        val backupContent = BackupContent(
+            appVersion = Constants.VERSION_CODE,
+            dbVersion = AppDatabase.DATABASE_VERSION,
+            groups = listOf(childGroup, grandChildGroup, parentGroup1),
+        )
+
+        inOrder(mockGroupRepository) {
+            backupManager.restore(
+                RestoreType.REPLACE,
+                backupContent,
+                emptyList(),
+                currentTime = 0L,
+            )
+
+            verify(mockGroupRepository).insert(parentGroup1)
+            verify(mockGroupRepository).insert(childGroup)
+            verify(mockGroupRepository).insert(grandChildGroup)
+            verify(mockGroupRepository, never()).update(any())
+        }
+    }
+
     /**
      * Issue #1655. If the list of groups in the backup has a child before the parent then the
      * parent must be restored first. Otherwise the SqliteConstraintException will be thrown.
@@ -184,10 +239,10 @@ class BackupManagerTest {
                 currentTime = 0L,
             )
 
+            verify(mockGroupRepository).insert(parentGroup2)
             verify(mockGroupRepository).insert(parentGroup1)
             verify(mockGroupRepository).insert(childGroup)
             verify(mockGroupRepository).insert(grandChildGroup)
-            verify(mockGroupRepository).insert(parentGroup2)
             verify(mockGroupRepository, never()).update(any())
         }
     }