fix: prevent pastes that would result in cycles (5bafbdd2) · Commits · e / os / Murena Maps

cardinal-android/app/src/main/java/earth/maps/cardinal/data/CutPasteRepository.kt

+10 −1

Original line number	Diff line number	Diff line
		@@ -18,15 +18,24 @@

		package earth.maps.cardinal.data

		import earth.maps.cardinal.data.room.ItemType
		import kotlinx.coroutines.flow.MutableStateFlow
		import javax.inject.Inject
		import javax.inject.Singleton

		/**
		* Represents an item in the clipboard with its ID and type
		*/
		data class ClipboardItem(
		val itemId: String,
		val itemType: ItemType
		)

		@Singleton
		class CutPasteRepository @Inject constructor() {

		/**
		* The set of list item UUIDs and ItemTypes in the clipboard.
		*/
		val clipboard: MutableStateFlow<Set<String>> = MutableStateFlow(emptySet())
		val clipboard: MutableStateFlow<Set<ClipboardItem>> = MutableStateFlow(emptySet())
		}
		No newline at end of file

cardinal-android/app/src/main/java/earth/maps/cardinal/data/room/SavedListRepository.kt

+68 −0

Original line number	Diff line number	Diff line
		@@ -288,6 +288,74 @@ class SavedListRepository @Inject constructor(
		return@mapLatest flows
		}

		/**
		* Checks if pasting the specified list items into the target list would create a cycle.
		* A cycle occurs when a list is being pasted into itself or any of its sublists.
		*
		* @param targetListId The ID of the list where items would be pasted
		* @param listIdsToPaste Set of list IDs that would be pasted
		* @return true if pasting would create a cycle, false otherwise
		*/
		suspend fun wouldCreateCycle(
		targetListId: String,
		listIdsToPaste: Set<String>
		): Boolean = withContext(Dispatchers.IO) {
		try {
		// If no lists are being pasted, no cycle can be created
		if (listIdsToPaste.isEmpty()) {
		return@withContext false
		}

		// Check if any of the lists to paste is the target list itself
		if (listIdsToPaste.contains(targetListId)) {
		return@withContext true
		}

		// For each list being pasted, check if the target list is in its hierarchy
		for (listId in listIdsToPaste) {
		if (isListInHierarchy(targetListId, listId)) {
		return@withContext true
		}
		}

		false
		} catch (e: Exception) {
		Log.e(TAG, "Error checking for cycle", e)
		false // Default to allowing paste if there's an error
		}
		}

		/**
		* Recursively checks if targetListId is in the hierarchy of parentListId.
		* This means targetListId is either parentListId itself or one of its descendants.
		*
		* @param targetListId The list ID we're looking for
		* @param parentListId The list ID to start searching from
		* @return true if targetListId is in the hierarchy of parentListId
		*/
		private suspend fun isListInHierarchy(
		targetListId: String,
		parentListId: String
		): Boolean {
		// Base case: if we found the target list
		if (targetListId == parentListId) {
		return true
		}

		// Get all child lists of the parent list
		val childLists = listItemDao.getItemsInList(parentListId)
		.filter { it.itemType == ItemType.LIST }

		// Recursively check each child list
		for (childListItem in childLists) {
		if (isListInHierarchy(targetListId, childListItem.itemId)) {
		return true
		}
		}

		return false
		}

		companion object {
		private const val TAG = "SavedListRepository"
		}

cardinal-android/app/src/main/java/earth/maps/cardinal/ui/saved/ManagePlacesScreen.kt

+5 −4

Original line number	Diff line number	Diff line
		@@ -77,6 +77,7 @@ import androidx.navigation.NavController
		import earth.maps.cardinal.R.dimen
		import earth.maps.cardinal.R.drawable
		import earth.maps.cardinal.R.string
		import earth.maps.cardinal.data.ClipboardItem
		import earth.maps.cardinal.data.room.ListContent
		import earth.maps.cardinal.data.room.ListContentItem
		import earth.maps.cardinal.data.room.PlaceContent
		@@ -453,7 +454,7 @@ private fun EmptyListContent() {
		private fun ListContentGrid(
		viewModel: ManagePlacesViewModel,
		content: List<Flow<ListContent?>>,
		clipboard: Set<String>,
		clipboard: Set<ClipboardItem>,
		selectedItems: Set<String>,
		onItemClick: (ListContent) -> Unit,
		onEditClick: (ListContent) -> Unit,
		@@ -483,7 +484,7 @@ private fun ListContentGrid(
		items(lists) { item ->
		ListItem(
		item = item,
		isInClipboard = clipboard.contains(item.id),
		isInClipboard = clipboard.any { it.itemId == item.id },
		isSelected = selectedItems.contains(item.id),
		onSelectionChange = { viewModel.toggleSelection(item.id) },
		onClick = { onItemClick(item) },
		@@ -505,7 +506,7 @@ private fun ListContentGrid(
		items(pinnedPlaces) { item ->
		PlaceItem(
		item = item,
		isInClipboard = clipboard.contains(item.id),
		isInClipboard = clipboard.any { it.itemId == item.id },
		isSelected = selectedItems.contains(item.id),
		onSelectionChange = { viewModel.toggleSelection(item.id) },
		onClick = { onItemClick(item) },
		@@ -530,7 +531,7 @@ private fun ListContentGrid(
		items(unpinnedPlaces) { item ->
		PlaceItem(
		item = item,
		isInClipboard = clipboard.contains(item.id),
		isInClipboard = clipboard.any { it.itemId == item.id },
		isSelected = selectedItems.contains(item.id),
		onSelectionChange = { viewModel.toggleSelection(item.id) },
		onClick = { onItemClick(item) },

cardinal-android/app/src/main/java/earth/maps/cardinal/ui/saved/ManagePlacesViewModel.kt

+33 −6

Original line number	Diff line number	Diff line
		@@ -21,6 +21,7 @@ package earth.maps.cardinal.ui.saved
		import androidx.lifecycle.ViewModel
		import androidx.lifecycle.viewModelScope
		import dagger.hilt.android.lifecycle.HiltViewModel
		import earth.maps.cardinal.data.ClipboardItem
		import earth.maps.cardinal.data.CutPasteRepository
		import earth.maps.cardinal.data.Place
		import earth.maps.cardinal.data.room.ItemType
		@@ -92,7 +93,7 @@ class ManagePlacesViewModel @Inject constructor(
		list?.let { savedListRepository.getListContent(it.id) } ?: flowOf(null)
		}

		val clipboard: Flow<Set<String>> = cutPasteRepository.clipboard
		val clipboard: Flow<Set<ClipboardItem>> = cutPasteRepository.clipboard

		suspend fun setInitialList(listId: String?) {
		if (listId != null) {
		@@ -174,18 +175,44 @@ class ManagePlacesViewModel @Inject constructor(
		}

		fun cutSelected() {
		val newClipboard = _selectedItems.value.toSet()
		cutPasteRepository.clipboard.value = newClipboard
		viewModelScope.launch {
		val currentListId = _currentListId.value ?: return@launch
		val itemsResult = savedListRepository.getItemsInList(currentListId)
		if (itemsResult.isFailure) return@launch
		val items = itemsResult.getOrNull() ?: return@launch

		val clipboardItems = _selectedItems.value.mapNotNull { itemId ->
		val item = items.find { it.itemId == itemId } ?: return@mapNotNull null
		ClipboardItem(itemId, item.itemType)
		}.toSet()

		cutPasteRepository.clipboard.value = clipboardItems
		clearSelection()
		}
		}

		fun pasteSelected() {
		val currentListId = currentListId.value ?: return
		viewModelScope.launch {
		val clipboardItems = cutPasteRepository.clipboard.value

		// Check if any lists in the clipboard would create a cycle
		val listIdsToCheck = clipboardItems
		.filter { it.itemType == ItemType.LIST }
		.map { it.itemId }
		.toSet()

		val wouldCreateCycle = savedListRepository.wouldCreateCycle(currentListId, listIdsToCheck)

		if (wouldCreateCycle) {
		// Don't paste if it would create a cycle
		return@launch
		}

		val itemsInListCount = listItemDao.getItemsInList(currentListId).size

		cutPasteRepository.clipboard.value.forEachIndexed { index, id ->
		listItemDao.moveItem(id, currentListId, itemsInListCount + index)
		clipboardItems.forEachIndexed { index, clipboardItem ->
		listItemDao.moveItem(clipboardItem.itemId, currentListId, itemsInListCount + index)
		}
		cutPasteRepository.clipboard.value = emptySet()
		}

cardinal-android/app/src/test/java/earth/maps/cardinal/data/room/SavedListRepositoryTest.kt

0 → 100644

+385 −0

Original line number	Diff line number	Diff line
		/*
		* Cardinal Maps
		* Copyright (C) 2025 Cardinal Maps Authors
		*
		* This program is free software: you can redistribute it and/or modify
		* it under the terms of the GNU General Public License as published by
		* the Free Software Foundation, either version 3 of the License, or
		* (at your option) any later version.
		*
		* This program is distributed in the hope that it will be useful,
		* but WITHOUT ANY WARRANTY; without even the implied warranty of
		* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
		* GNU General Public License for more details.
		*
		* You should have received a copy of the GNU General Public License
		* along with this program. If not, see <https://www.gnu.org/licenses/>.
		*/

		package earth.maps.cardinal.data.room

		import android.content.Context
		import io.mockk.coEvery
		import io.mockk.every
		import io.mockk.mockk
		import io.mockk.mockkStatic
		import kotlinx.coroutines.test.runTest
		import org.junit.Assert.assertFalse
		import org.junit.Assert.assertTrue
		import org.junit.Before
		import org.junit.Test
		import org.junit.runner.RunWith
		import org.robolectric.RobolectricTestRunner
		import java.util.UUID

		@RunWith(RobolectricTestRunner::class)
		class SavedListRepositoryTest {

		private lateinit var repository: SavedListRepository
		private val mockContext = mockk<Context>(relaxed = true)
		private val mockDatabase = mockk<AppDatabase>(relaxed = false)
		private val mockListDao = mockk<SavedListDao>(relaxed = false)
		private val mockListItemDao = mockk<ListItemDao>(relaxed = false)
		private val mockPlaceDao = mockk<SavedPlaceDao>(relaxed = false)

		// Test data
		private val rootList = SavedList(
		id = "root-list-id",
		name = "Saved Places",
		description = null,
		isRoot = true,
		isCollapsed = false,
		createdAt = System.currentTimeMillis(),
		updatedAt = System.currentTimeMillis()
		)

		private val parentList = SavedList(
		id = "parent-list-id",
		name = "Parent List",
		description = "Parent Description",
		isRoot = false,
		isCollapsed = false,
		createdAt = System.currentTimeMillis(),
		updatedAt = System.currentTimeMillis()
		)

		private val childList = SavedList(
		id = "child-list-id",
		name = "Child List",
		description = "Child Description",
		isRoot = false,
		isCollapsed = false,
		createdAt = System.currentTimeMillis(),
		updatedAt = System.currentTimeMillis()
		)

		private val grandchildList = SavedList(
		id = "grandchild-list-id",
		name = "Grandchild List",
		description = "Grandchild Description",
		isRoot = false,
		isCollapsed = false,
		createdAt = System.currentTimeMillis(),
		updatedAt = System.currentTimeMillis()
		)

		private val unrelatedList = SavedList(
		id = "unrelated-list-id",
		name = "Unrelated List",
		description = "Unrelated Description",
		isRoot = false,
		isCollapsed = false,
		createdAt = System.currentTimeMillis(),
		updatedAt = System.currentTimeMillis()
		)

		@Before
		fun setup() {
		every { mockDatabase.savedListDao() } returns mockListDao
		every { mockDatabase.listItemDao() } returns mockListItemDao
		every { mockDatabase.savedPlaceDao() } returns mockPlaceDao

		repository = SavedListRepository(mockContext, mockDatabase)
		}

		@Test
		fun `wouldCreateCycle should return false when no lists are being pasted`() = runTest {
		// Given
		val targetListId = "target-list-id"
		val listIdsToPaste = emptySet<String>()

		// When
		val result = repository.wouldCreateCycle(targetListId, listIdsToPaste)

		// Then
		assertFalse(result)
		}

		@Test
		fun `wouldCreateCycle should return true when pasting list into itself`() = runTest {
		// Given
		val targetListId = parentList.id
		val listIdsToPaste = setOf(parentList.id)

		// When
		val result = repository.wouldCreateCycle(targetListId, listIdsToPaste)

		// Then
		assertTrue(result)
		}

		@Test
		fun `wouldCreateCycle should return true when pasting parent into child`() = runTest {
		// Given - set up hierarchy: parent -> child
		val parentToChildListItem = ListItem(
		listId = parentList.id,
		itemId = childList.id,
		itemType = ItemType.LIST,
		position = 0,
		addedAt = System.currentTimeMillis()
		)

		coEvery { mockListItemDao.getItemsInList(parentList.id) } returns listOf(parentToChildListItem)
		coEvery { mockListItemDao.getItemsInList(childList.id) } returns emptyList()

		// When
		val result = repository.wouldCreateCycle(childList.id, setOf(parentList.id))

		// Then
		assertTrue(result)
		}

		@Test
		fun `wouldCreateCycle should return true when pasting grandparent into grandchild`() = runTest {
		// Given - set up hierarchy: parent -> child -> grandchild
		val parentToChildListItem = ListItem(
		listId = parentList.id,
		itemId = childList.id,
		itemType = ItemType.LIST,
		position = 0,
		addedAt = System.currentTimeMillis()
		)

		val childToGrandchildListItem = ListItem(
		listId = childList.id,
		itemId = grandchildList.id,
		itemType = ItemType.LIST,
		position = 0,
		addedAt = System.currentTimeMillis()
		)

		coEvery { mockListItemDao.getItemsInList(parentList.id) } returns listOf(parentToChildListItem)
		coEvery { mockListItemDao.getItemsInList(childList.id) } returns listOf(childToGrandchildListItem)
		coEvery { mockListItemDao.getItemsInList(grandchildList.id) } returns emptyList()

		// When
		val result = repository.wouldCreateCycle(grandchildList.id, setOf(parentList.id))

		// Then
		assertTrue(result)
		}

		@Test
		fun `wouldCreateCycle should return false when pasting child into parent`() = runTest {
		// Given - set up hierarchy: parent -> child
		val parentToChildListItem = ListItem(
		listId = parentList.id,
		itemId = childList.id,
		itemType = ItemType.LIST,
		position = 0,
		addedAt = System.currentTimeMillis()
		)

		coEvery { mockListItemDao.getItemsInList(parentList.id) } returns listOf(parentToChildListItem)
		coEvery { mockListItemDao.getItemsInList(childList.id) } returns emptyList()

		// When
		val result = repository.wouldCreateCycle(parentList.id, setOf(childList.id))

		// Then
		assertFalse(result)
		}

		@Test
		fun `wouldCreateCycle should return false when pasting unrelated list`() = runTest {
		// Given - set up hierarchy: parent -> child
		val parentToChildListItem = ListItem(
		listId = parentList.id,
		itemId = childList.id,
		itemType = ItemType.LIST,
		position = 0,
		addedAt = System.currentTimeMillis()
		)

		coEvery { mockListItemDao.getItemsInList(parentList.id) } returns listOf(parentToChildListItem)
		coEvery { mockListItemDao.getItemsInList(childList.id) } returns emptyList()
		coEvery { mockListItemDao.getItemsInList(unrelatedList.id) } returns emptyList()

		// When
		val result = repository.wouldCreateCycle(parentList.id, setOf(unrelatedList.id))

		// Then
		assertFalse(result)
		}

		@Test
		fun `wouldCreateCycle should return true when any list in set would create cycle`() = runTest {
		// Given - set up hierarchy: parent -> child
		val parentToChildListItem = ListItem(
		listId = parentList.id,
		itemId = childList.id,
		itemType = ItemType.LIST,
		position = 0,
		addedAt = System.currentTimeMillis()
		)

		coEvery { mockListItemDao.getItemsInList(parentList.id) } returns listOf(parentToChildListItem)
		coEvery { mockListItemDao.getItemsInList(childList.id) } returns emptyList()
		coEvery { mockListItemDao.getItemsInList(unrelatedList.id) } returns emptyList()

		// When - trying to paste both child (would create cycle) and unrelated (wouldn't create cycle)
		val result = repository.wouldCreateCycle(childList.id, setOf(parentList.id, unrelatedList.id))

		// Then
		assertTrue(result)
		}

		@Test
		fun `wouldCreateCycle should return false when no lists in set would create cycle`() = runTest {
		// Given - set up hierarchy: parent -> child
		val parentToChildListItem = ListItem(
		listId = parentList.id,
		itemId = childList.id,
		itemType = ItemType.LIST,
		position = 0,
		addedAt = System.currentTimeMillis()
		)

		coEvery { mockListItemDao.getItemsInList(parentList.id) } returns listOf(parentToChildListItem)
		coEvery { mockListItemDao.getItemsInList(childList.id) } returns emptyList()
		coEvery { mockListItemDao.getItemsInList(unrelatedList.id) } returns emptyList()

		// When - trying to paste unrelated lists only
		val result = repository.wouldCreateCycle(parentList.id, setOf(unrelatedList.id))

		// Then
		assertFalse(result)
		}

		@Test
		fun `wouldCreateCycle should handle complex hierarchy correctly`() = runTest {
		// Given - set up complex hierarchy: parent -> child1, child2 -> grandchild1, grandchild2
		val parentToChild1ListItem = ListItem(
		listId = parentList.id,
		itemId = "child1-list-id",
		itemType = ItemType.LIST,
		position = 0,
		addedAt = System.currentTimeMillis()
		)

		val parentToChild2ListItem = ListItem(
		listId = parentList.id,
		itemId = "child2-list-id",
		itemType = ItemType.LIST,
		position = 1,
		addedAt = System.currentTimeMillis()
		)

		val child1ToGrandchild1ListItem = ListItem(
		listId = "child1-list-id",
		itemId = "grandchild1-list-id",
		itemType = ItemType.LIST,
		position = 0,
		addedAt = System.currentTimeMillis()
		)

		val child2ToGrandchild2ListItem = ListItem(
		listId = "child2-list-id",
		itemId = "grandchild2-list-id",
		itemType = ItemType.LIST,
		position = 0,
		addedAt = System.currentTimeMillis()
		)

		coEvery { mockListItemDao.getItemsInList(parentList.id) } returns listOf(parentToChild1ListItem, parentToChild2ListItem)
		coEvery { mockListItemDao.getItemsInList("child1-list-id") } returns listOf(child1ToGrandchild1ListItem)
		coEvery { mockListItemDao.getItemsInList("child2-list-id") } returns listOf(child2ToGrandchild2ListItem)
		coEvery { mockListItemDao.getItemsInList("grandchild1-list-id") } returns emptyList()
		coEvery { mockListItemDao.getItemsInList("grandchild2-list-id") } returns emptyList()

		// When - trying to paste parent into grandchild (should create cycle)
		val result1 = repository.wouldCreateCycle("grandchild1-list-id", setOf(parentList.id))

		// When - trying to paste child1 into child2 (should not create cycle)
		val result2 = repository.wouldCreateCycle("child2-list-id", setOf("child1-list-id"))

		// When - trying to paste grandchild1 into parent (should not create cycle)
		val result3 = repository.wouldCreateCycle(parentList.id, setOf("grandchild1-list-id"))

		// Then
		assertTrue(result1) // parent -> child1 -> grandchild1, so pasting parent into grandchild1 creates cycle
		assertFalse(result2) // child1 and child2 are siblings, no cycle
		assertFalse(result3) // grandchild1 is descendant of parent, but pasting it into parent doesn't create cycle
		}

		@Test
		fun `wouldCreateCycle should handle empty lists correctly`() = runTest {
		// Given
		val targetListId = "target-list-id"
		val listIdsToPaste = setOf("list1-id", "list2-id")

		// Mock all lists as empty
		coEvery { mockListItemDao.getItemsInList(any()) } returns emptyList()

		// When
		val result = repository.wouldCreateCycle(targetListId, listIdsToPaste)

		// Then
		assertFalse(result)
		}

		@Test
		fun `wouldCreateCycle should handle database errors gracefully`() = runTest {
		// Given
		val targetListId = parentList.id
		val listIdsToPaste = setOf(childList.id)

		// Mock database to throw exception
		coEvery { mockListItemDao.getItemsInList(any()) } throws RuntimeException("Database error")

		// When
		val result = repository.wouldCreateCycle(targetListId, listIdsToPaste)

		// Then - should return false (allow paste) when there's an error
		assertFalse(result)
		}

		@Test
		fun `wouldCreateCycle should handle mixed item types correctly`() = runTest {
		// Given - list contains both lists and places
		val parentToChildListItem = ListItem(
		listId = parentList.id,
		itemId = childList.id,
		itemType = ItemType.LIST,
		position = 0,
		addedAt = System.currentTimeMillis()
		)

		val parentToPlaceListItem = ListItem(
		listId = parentList.id,
		itemId = "place-id",
		itemType = ItemType.PLACE,
		position = 1,
		addedAt = System.currentTimeMillis()
		)

		coEvery { mockListItemDao.getItemsInList(parentList.id) } returns listOf(parentToChildListItem, parentToPlaceListItem)
		coEvery { mockListItemDao.getItemsInList(childList.id) } returns emptyList()

		// When
		val result = repository.wouldCreateCycle(childList.id, setOf(parentList.id))

		// Then - should still detect cycle even with mixed item types
		assertTrue(result)
		}
		}
		No newline at end of file