From 45270257c01276b61e43640dd942b84e7f033d96 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 19 Dec 2025 16:44:43 +0000 Subject: [PATCH 1/9] Initial plan From ec03871930b0d45adb1690423d62754121ca0e33 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 19 Dec 2025 17:01:29 +0000 Subject: [PATCH 2/9] Optimize data structures and add security validation - Optimize WebDavFile children lookup from O(N) to O(1) using HashMap - Add path traversal prevention in parseDocumentId - Add comprehensive unit tests for Account, WebDavFile, and WebDavProvider Co-authored-by: MinecraftFuns <25814618+MinecraftFuns@users.noreply.github.com> --- .../joefang/webdav/provider/WebDavCache.kt | 4 +- .../org/joefang/webdav/provider/WebDavFile.kt | 157 +++++++++- .../joefang/webdav/provider/WebDavProvider.kt | 49 ++- .../org/joefang/webdav/data/AccountTest.kt | 223 ++++++++++++++ .../joefang/webdav/provider/WebDavFileTest.kt | 278 ++++++++++++++++++ .../webdav/provider/WebDavProviderTest.kt | 197 +++++++++++++ 6 files changed, 901 insertions(+), 7 deletions(-) create mode 100644 app/src/test/java/org/joefang/webdav/data/AccountTest.kt create mode 100644 app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt create mode 100644 app/src/test/java/org/joefang/webdav/provider/WebDavProviderTest.kt diff --git a/app/src/main/java/org/joefang/webdav/provider/WebDavCache.kt b/app/src/main/java/org/joefang/webdav/provider/WebDavCache.kt index 814b347..1400e04 100644 --- a/app/src/main/java/org/joefang/webdav/provider/WebDavCache.kt +++ b/app/src/main/java/org/joefang/webdav/provider/WebDavCache.kt @@ -30,8 +30,8 @@ class WebDavCache (private val context: Context, private val dao: CacheDao) { if (path.parent != null) { val parentFile = cache[path.parent] if (parentFile != null) { - // only return the cached metadata if the given path does not refer to a directory - val childFile = parentFile.children.find { f -> f.path == path } + // Use O(1) HashMap lookup instead of O(N) linear search + val childFile = parentFile.findChildByPath(path) if (childFile != null && !childFile.isDirectory) { return childFile } diff --git a/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt b/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt index 4e805b5..042c613 100644 --- a/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt +++ b/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt @@ -11,6 +11,14 @@ import java.text.SimpleDateFormat import java.util.Date import java.util.Locale +/** + * Represents a file or directory on a WebDAV server. + * + * This class maintains a tree structure of files where directories + * can have children. For efficient lookup operations, children are + * stored both in a list (for ordered iteration) and a HashMap + * (for O(1) path-based lookup). + */ class WebDavFile( var path: Path, var isDirectory: Boolean = false, @@ -18,7 +26,18 @@ class WebDavFile( var isPending: Boolean = false ) { var parent: WebDavFile? = null - var children: MutableList = ArrayList() + + // Use a backing list for ordered iteration and a HashMap for O(1) lookup + private val childrenList: MutableList = ArrayList() + private val childrenByPath: MutableMap = HashMap() + + /** + * Returns a mutable list view of children for backward compatibility. + * For better performance when looking up by path, use [findChildByPath]. + */ + val children: MutableList + get() = ChildrenListWrapper(childrenList, childrenByPath) + val writable: Boolean = true var etag: String? = null @@ -84,4 +103,140 @@ class WebDavFile( return "application/octet-stream" } + + /** + * Finds a child file by its path in O(1) time. + * @param childPath The path of the child to find + * @return The child file, or null if not found + */ + fun findChildByPath(childPath: Path): WebDavFile? { + return childrenByPath[childPath] + } + + /** + * A wrapper around the children list that keeps the HashMap in sync. + * This ensures backward compatibility with existing code that uses + * children as a MutableList while providing O(1) lookup by path. + */ + private class ChildrenListWrapper( + private val backingList: MutableList, + private val pathIndex: MutableMap + ) : MutableList by backingList { + + override fun add(element: WebDavFile): Boolean { + pathIndex[element.path] = element + return backingList.add(element) + } + + override fun add(index: Int, element: WebDavFile) { + pathIndex[element.path] = element + backingList.add(index, element) + } + + override fun addAll(elements: Collection): Boolean { + elements.forEach { pathIndex[it.path] = it } + return backingList.addAll(elements) + } + + override fun addAll(index: Int, elements: Collection): Boolean { + elements.forEach { pathIndex[it.path] = it } + return backingList.addAll(index, elements) + } + + override fun remove(element: WebDavFile): Boolean { + pathIndex.remove(element.path) + return backingList.remove(element) + } + + override fun removeAt(index: Int): WebDavFile { + val element = backingList.removeAt(index) + pathIndex.remove(element.path) + return element + } + + override fun removeAll(elements: Collection): Boolean { + elements.forEach { pathIndex.remove(it.path) } + return backingList.removeAll(elements) + } + + override fun retainAll(elements: Collection): Boolean { + val pathsToKeep = elements.map { it.path }.toSet() + pathIndex.keys.retainAll(pathsToKeep) + return backingList.retainAll(elements) + } + + override fun clear() { + pathIndex.clear() + backingList.clear() + } + + override fun set(index: Int, element: WebDavFile): WebDavFile { + val old = backingList[index] + pathIndex.remove(old.path) + pathIndex[element.path] = element + return backingList.set(index, element) + } + + override fun iterator(): MutableIterator { + return IndexSyncIterator(backingList.iterator(), pathIndex) + } + + override fun listIterator(): MutableListIterator { + return IndexSyncListIterator(backingList.listIterator(), pathIndex) + } + + override fun listIterator(index: Int): MutableListIterator { + return IndexSyncListIterator(backingList.listIterator(index), pathIndex) + } + + private class IndexSyncIterator( + private val delegate: MutableIterator, + private val pathIndex: MutableMap + ) : MutableIterator { + private var current: WebDavFile? = null + + override fun hasNext(): Boolean = delegate.hasNext() + override fun next(): WebDavFile { + current = delegate.next() + return current!! + } + override fun remove() { + current?.let { pathIndex.remove(it.path) } + delegate.remove() + } + } + + private class IndexSyncListIterator( + private val delegate: MutableListIterator, + private val pathIndex: MutableMap + ) : MutableListIterator { + private var current: WebDavFile? = null + + override fun hasNext(): Boolean = delegate.hasNext() + override fun hasPrevious(): Boolean = delegate.hasPrevious() + override fun next(): WebDavFile { + current = delegate.next() + return current!! + } + override fun nextIndex(): Int = delegate.nextIndex() + override fun previous(): WebDavFile { + current = delegate.previous() + return current!! + } + override fun previousIndex(): Int = delegate.previousIndex() + override fun add(element: WebDavFile) { + pathIndex[element.path] = element + delegate.add(element) + } + override fun remove() { + current?.let { pathIndex.remove(it.path) } + delegate.remove() + } + override fun set(element: WebDavFile) { + current?.let { pathIndex.remove(it.path) } + pathIndex[element.path] = element + delegate.set(element) + } + } + } } diff --git a/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt b/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt index e60fd59..8c168fa 100644 --- a/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt +++ b/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt @@ -377,7 +377,8 @@ class WebDavProvider : DocumentsProvider() { val file = if (isRoot) { resFile } else { - resFile.children.find { f -> f.path == doc.path } + // Use O(1) HashMap lookup instead of O(N) linear search + resFile.findChildByPath(doc.path) } if (file != null) { @@ -474,19 +475,59 @@ class WebDavProvider : DocumentsProvider() { } companion object { + /** + * Parses a document ID into account ID and path components. + * + * Document ID format: /{accountId}/{path...} + * Example: /1/documents/file.txt -> (1, /documents/file.txt) + * + * Security: This method validates the input to prevent: + * - Path traversal attacks (../) + * - Invalid account IDs + * - Malformed document IDs + * + * @throws IllegalArgumentException if the document ID is invalid + */ fun parseDocumentId(documentId: String): Pair { + // Validate basic structure + if (documentId.isEmpty() || !documentId.startsWith("/")) { + throw IllegalArgumentException("Invalid document ID: '$documentId' (must start with /)") + } + val parts = documentId.split("/") if (parts.size < 3) { throw IllegalArgumentException("Invalid document ID: '$documentId'") } val id = try { - parts[1].toLong() + val accountIdStr = parts[1] + // Validate account ID is a positive long + if (accountIdStr.isEmpty() || accountIdStr.any { !it.isDigit() }) { + throw NumberFormatException("Account ID must be a positive integer") + } + accountIdStr.toLong().also { + if (it < 0) throw NumberFormatException("Account ID must be non-negative") + } } catch (e: NumberFormatException) { - throw IllegalArgumentException("Invalid document ID: '$documentId' (Bad account ID: ${parts[1]}") + throw IllegalArgumentException("Invalid document ID: '$documentId' (Bad account ID: ${parts[1]})") } - val path = Paths.get(parts.drop(2).joinToString("/", prefix = "/")) + val pathStr = parts.drop(2).joinToString("/", prefix = "/") + + // Security: Check for path traversal attempts + if (pathStr.contains("/../") || pathStr.contains("/./") || + pathStr.endsWith("/..") || pathStr.endsWith("/.") || + pathStr == "/.." || pathStr == "/.") { + throw IllegalArgumentException("Invalid document ID: '$documentId' (Path traversal detected)") + } + + val path = Paths.get(pathStr).normalize() + + // Additional security check: ensure normalized path doesn't escape root + if (!path.toString().startsWith("/")) { + throw IllegalArgumentException("Invalid document ID: '$documentId' (Path must be absolute)") + } + return Pair(id, path) } diff --git a/app/src/test/java/org/joefang/webdav/data/AccountTest.kt b/app/src/test/java/org/joefang/webdav/data/AccountTest.kt new file mode 100644 index 0000000..79c7709 --- /dev/null +++ b/app/src/test/java/org/joefang/webdav/data/AccountTest.kt @@ -0,0 +1,223 @@ +package org.joefang.webdav.data + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +class AccountTest { + + @Test + fun `getResolvedHeaders returns empty list for NONE profile`() { + val account = Account( + id = 1, + name = "Test Account", + url = "https://example.com/webdav/", + headerProfile = HeaderProfile.NONE + ) + + val headers = account.getResolvedHeaders() + + assertTrue(headers.isEmpty()) + } + + @Test + fun `getResolvedHeaders returns Cloudflare headers for CLOUDFLARE profile`() { + val account = Account( + id = 1, + name = "Test Account", + url = "https://example.com/webdav/", + headerProfile = HeaderProfile.CLOUDFLARE, + cfAccessClientId = SecretString("test-client-id"), + cfAccessClientSecret = SecretString("test-client-secret") + ) + + val headers = account.getResolvedHeaders() + + assertEquals(2, headers.size) + + val clientIdHeader = headers.find { it.name == Account.CF_ACCESS_CLIENT_ID_HEADER } + val clientSecretHeader = headers.find { it.name == Account.CF_ACCESS_CLIENT_SECRET_HEADER } + + assertEquals("test-client-id", clientIdHeader?.value) + assertEquals("test-client-secret", clientSecretHeader?.value) + assertTrue(clientIdHeader?.isSecret == true) + assertTrue(clientSecretHeader?.isSecret == true) + assertTrue(clientIdHeader?.enabled == true) + assertTrue(clientSecretHeader?.enabled == true) + } + + @Test + fun `getResolvedHeaders returns partial Cloudflare headers when only clientId is set`() { + val account = Account( + id = 1, + name = "Test Account", + url = "https://example.com/webdav/", + headerProfile = HeaderProfile.CLOUDFLARE, + cfAccessClientId = SecretString("test-client-id"), + cfAccessClientSecret = null + ) + + val headers = account.getResolvedHeaders() + + assertEquals(1, headers.size) + assertEquals(Account.CF_ACCESS_CLIENT_ID_HEADER, headers[0].name) + assertEquals("test-client-id", headers[0].value) + } + + @Test + fun `getResolvedHeaders returns empty list for CLOUDFLARE profile with no credentials`() { + val account = Account( + id = 1, + name = "Test Account", + url = "https://example.com/webdav/", + headerProfile = HeaderProfile.CLOUDFLARE, + cfAccessClientId = null, + cfAccessClientSecret = null + ) + + val headers = account.getResolvedHeaders() + + assertTrue(headers.isEmpty()) + } + + @Test + fun `getResolvedHeaders returns custom headers for CUSTOM profile`() { + val customHeaders = listOf( + CustomHeader(name = "X-Custom-1", value = "value1", enabled = true), + CustomHeader(name = "X-Custom-2", value = "value2", enabled = true), + CustomHeader(name = "X-Disabled", value = "value3", enabled = false) + ) + + val account = Account( + id = 1, + name = "Test Account", + url = "https://example.com/webdav/", + headerProfile = HeaderProfile.CUSTOM, + customHeaders = customHeaders + ) + + val headers = account.getResolvedHeaders() + + // Only enabled headers should be returned + assertEquals(2, headers.size) + assertTrue(headers.any { it.name == "X-Custom-1" && it.value == "value1" }) + assertTrue(headers.any { it.name == "X-Custom-2" && it.value == "value2" }) + } + + @Test + fun `getResolvedHeaders returns empty list for CUSTOM profile with null customHeaders`() { + val account = Account( + id = 1, + name = "Test Account", + url = "https://example.com/webdav/", + headerProfile = HeaderProfile.CUSTOM, + customHeaders = null + ) + + val headers = account.getResolvedHeaders() + + assertTrue(headers.isEmpty()) + } + + @Test + fun `getResolvedHeaders returns empty list for CUSTOM profile with empty customHeaders`() { + val account = Account( + id = 1, + name = "Test Account", + url = "https://example.com/webdav/", + headerProfile = HeaderProfile.CUSTOM, + customHeaders = emptyList() + ) + + val headers = account.getResolvedHeaders() + + assertTrue(headers.isEmpty()) + } + + @Test + fun `hasError returns false when no credentials have errors`() { + val account = Account( + id = 1, + name = "Test Account", + url = "https://example.com/webdav/", + username = SecretString("user"), + password = SecretString("pass") + ) + + assertEquals(false, account.hasError) + } + + @Test + fun `hasError returns true when username has error`() { + val account = Account( + id = 1, + name = "Test Account", + url = "https://example.com/webdav/", + username = SecretString(error = Exception("Decryption failed")), + password = SecretString("pass") + ) + + assertEquals(true, account.hasError) + } + + @Test + fun `hasError returns true when cfAccessClientId has error`() { + val account = Account( + id = 1, + name = "Test Account", + url = "https://example.com/webdav/", + cfAccessClientId = SecretString(error = Exception("Decryption failed")) + ) + + assertEquals(true, account.hasError) + } + + @Test + fun `rootPath extracts path from URL correctly`() { + // Note: rootPath uses Paths.get which may not preserve trailing slash + // depending on the platform. The test validates the path segments are correct. + val account = Account( + id = 1, + name = "Test Account", + url = "https://example.com/webdav/files/" + ) + + val path = account.rootPath + assertTrue(path.toString().startsWith("/webdav/files")) + } + + @Test + fun `rootPath handles URL without trailing slash`() { + val account = Account( + id = 1, + name = "Test Account", + url = "https://example.com/webdav" + ) + + val path = account.rootPath + assertTrue(path.toString().startsWith("/webdav")) + } + + @Test + fun `byId extension function finds account by id`() { + val accounts = listOf( + Account(id = 1, name = "Account 1", url = "https://example1.com/"), + Account(id = 2, name = "Account 2", url = "https://example2.com/"), + Account(id = 3, name = "Account 3", url = "https://example3.com/") + ) + + val found = accounts.byId(2) + + assertEquals("Account 2", found.name) + assertEquals(2L, found.id) + } + + @Test(expected = NoSuchElementException::class) + fun `byId extension function throws when id not found`() { + val accounts = listOf( + Account(id = 1, name = "Account 1", url = "https://example1.com/") + ) + + accounts.byId(999) + } +} diff --git a/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt b/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt new file mode 100644 index 0000000..610a16e --- /dev/null +++ b/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt @@ -0,0 +1,278 @@ +package org.joefang.webdav.provider + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Assert.assertFalse +import org.junit.Test +import java.nio.file.Paths + +class WebDavFileTest { + + @Test + fun `children list add and retrieve works correctly`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child1 = WebDavFile(Paths.get("/documents/file1.txt")) + val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + + parent.children.add(child1) + parent.children.add(child2) + + assertEquals(2, parent.children.size) + assertTrue(parent.children.contains(child1)) + assertTrue(parent.children.contains(child2)) + } + + @Test + fun `findChildByPath returns child in O(1) time`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child1 = WebDavFile(Paths.get("/documents/file1.txt")) + val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + val child3 = WebDavFile(Paths.get("/documents/file3.txt")) + + parent.children.add(child1) + parent.children.add(child2) + parent.children.add(child3) + + // Find using O(1) HashMap lookup + val found = parent.findChildByPath(Paths.get("/documents/file2.txt")) + + assertNotNull(found) + assertEquals(child2, found) + } + + @Test + fun `findChildByPath returns null for non-existent path`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child = WebDavFile(Paths.get("/documents/file.txt")) + + parent.children.add(child) + + val found = parent.findChildByPath(Paths.get("/documents/nonexistent.txt")) + + assertNull(found) + } + + @Test + fun `children remove updates HashMap correctly`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child1 = WebDavFile(Paths.get("/documents/file1.txt")) + val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + + parent.children.add(child1) + parent.children.add(child2) + + // Remove child1 + parent.children.remove(child1) + + // Verify removal from both list and HashMap + assertEquals(1, parent.children.size) + assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + } + + @Test + fun `children clear removes all from HashMap`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child1 = WebDavFile(Paths.get("/documents/file1.txt")) + val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + + parent.children.add(child1) + parent.children.add(child2) + + parent.children.clear() + + assertEquals(0, parent.children.size) + assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) + assertNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + } + + @Test + fun `children addAll updates HashMap correctly`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val children = listOf( + WebDavFile(Paths.get("/documents/file1.txt")), + WebDavFile(Paths.get("/documents/file2.txt")), + WebDavFile(Paths.get("/documents/file3.txt")) + ) + + parent.children.addAll(children) + + assertEquals(3, parent.children.size) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file3.txt"))) + } + + @Test + fun `children removeAll updates HashMap correctly`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child1 = WebDavFile(Paths.get("/documents/file1.txt")) + val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + val child3 = WebDavFile(Paths.get("/documents/file3.txt")) + + parent.children.addAll(listOf(child1, child2, child3)) + parent.children.removeAll(listOf(child1, child3)) + + assertEquals(1, parent.children.size) + assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + assertNull(parent.findChildByPath(Paths.get("/documents/file3.txt"))) + } + + @Test + fun `children set updates HashMap correctly`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child1 = WebDavFile(Paths.get("/documents/file1.txt")) + val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + + parent.children.add(child1) + + // Replace child1 with child2 + parent.children[0] = child2 + + assertEquals(1, parent.children.size) + assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + } + + @Test + fun `children iterator remove updates HashMap correctly`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child1 = WebDavFile(Paths.get("/documents/file1.txt")) + val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + + parent.children.add(child1) + parent.children.add(child2) + + val iterator = parent.children.iterator() + while (iterator.hasNext()) { + val file = iterator.next() + if (file.path == Paths.get("/documents/file1.txt")) { + iterator.remove() + } + } + + assertEquals(1, parent.children.size) + assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + } + + @Test + fun `children removeAt updates HashMap correctly`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child1 = WebDavFile(Paths.get("/documents/file1.txt")) + val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + + parent.children.add(child1) + parent.children.add(child2) + + parent.children.removeAt(0) + + assertEquals(1, parent.children.size) + assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + } + + @Test + fun `WebDavFile name property returns filename`() { + val file = WebDavFile(Paths.get("/documents/file.txt")) + + assertEquals("file.txt", file.name) + } + + @Test + fun `WebDavFile name property returns slash for root`() { + val file = WebDavFile(Paths.get("/")) + + assertEquals("/", file.name) + } + + @Test + fun `WebDavFile decodedName decodes URL-encoded names`() { + val file = WebDavFile(Paths.get("/documents/file%20name.txt")) + + assertEquals("file name.txt", file.decodedName) + } + + @Test + fun `WebDavFile davPath returns correct WebDavPath for file`() { + val file = WebDavFile(Paths.get("/documents/file.txt"), isDirectory = false) + + val davPath = file.davPath + + assertEquals(Paths.get("/documents/file.txt"), davPath.path) + assertFalse(davPath.isDirectory) + } + + @Test + fun `WebDavFile davPath returns correct WebDavPath for directory`() { + val dir = WebDavFile(Paths.get("/documents"), isDirectory = true) + + val davPath = dir.davPath + + assertEquals(Paths.get("/documents"), davPath.path) + assertTrue(davPath.isDirectory) + } + + @Test + fun `WebDavFile toString returns path string`() { + val file = WebDavFile(Paths.get("/documents/file.txt")) + + assertEquals("/documents/file.txt", file.toString()) + } + + @Test + fun `WebDavFile parent can be set`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child = WebDavFile(Paths.get("/documents/file.txt")) + + child.parent = parent + + assertEquals(parent, child.parent) + } + + @Test + fun `WebDavFile writable defaults to true`() { + val file = WebDavFile(Paths.get("/documents/file.txt")) + + assertTrue(file.writable) + } + + @Test + fun `WebDavFile isPending defaults to false`() { + val file = WebDavFile(Paths.get("/documents/file.txt")) + + assertFalse(file.isPending) + } + + @Test + fun `WebDavFile isPending can be set to true`() { + val file = WebDavFile(Paths.get("/documents/file.txt"), isPending = true) + + assertTrue(file.isPending) + } + + @Test + fun `performance test for findChildByPath with many children`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + + // Add 1000 children + repeat(1000) { i -> + parent.children.add(WebDavFile(Paths.get("/documents/file$i.txt"))) + } + + // Find the 500th child - should be O(1) + val startTime = System.nanoTime() + val found = parent.findChildByPath(Paths.get("/documents/file500.txt")) + val endTime = System.nanoTime() + + assertNotNull(found) + assertEquals(Paths.get("/documents/file500.txt"), found?.path) + + // HashMap lookup should be very fast (under 1ms) + val durationMs = (endTime - startTime) / 1_000_000 + assertTrue("HashMap lookup took too long: ${durationMs}ms", durationMs < 10) + } +} diff --git a/app/src/test/java/org/joefang/webdav/provider/WebDavProviderTest.kt b/app/src/test/java/org/joefang/webdav/provider/WebDavProviderTest.kt new file mode 100644 index 0000000..103bf02 --- /dev/null +++ b/app/src/test/java/org/joefang/webdav/provider/WebDavProviderTest.kt @@ -0,0 +1,197 @@ +package org.joefang.webdav.provider + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Assert.fail +import org.junit.Test +import java.nio.file.Paths + +class WebDavProviderTest { + + @Test + fun `parseDocumentId parses valid document ID correctly`() { + val (accountId, path) = WebDavProvider.parseDocumentId("/1/documents/file.txt") + + assertEquals(1L, accountId) + assertEquals(Paths.get("/documents/file.txt"), path) + } + + @Test + fun `parseDocumentId parses root path correctly`() { + val (accountId, path) = WebDavProvider.parseDocumentId("/1/") + + assertEquals(1L, accountId) + assertEquals(Paths.get("/"), path) + } + + @Test + fun `parseDocumentId parses nested path correctly`() { + val (accountId, path) = WebDavProvider.parseDocumentId("/123/a/b/c/d/file.txt") + + assertEquals(123L, accountId) + assertEquals(Paths.get("/a/b/c/d/file.txt"), path) + } + + @Test + fun `parseDocumentId handles large account ID`() { + val (accountId, path) = WebDavProvider.parseDocumentId("/9223372036854775807/test") + + assertEquals(Long.MAX_VALUE, accountId) + assertEquals(Paths.get("/test"), path) + } + + @Test + fun `parseDocumentId throws for empty string`() { + try { + WebDavProvider.parseDocumentId("") + fail("Expected IllegalArgumentException") + } catch (e: IllegalArgumentException) { + assertTrue(e.message?.contains("must start with /") == true) + } + } + + @Test + fun `parseDocumentId throws for missing leading slash`() { + try { + WebDavProvider.parseDocumentId("1/documents/file.txt") + fail("Expected IllegalArgumentException") + } catch (e: IllegalArgumentException) { + assertTrue(e.message?.contains("must start with /") == true) + } + } + + @Test + fun `parseDocumentId throws for invalid account ID with letters`() { + try { + WebDavProvider.parseDocumentId("/abc/documents/file.txt") + fail("Expected IllegalArgumentException") + } catch (e: IllegalArgumentException) { + assertTrue(e.message?.contains("Bad account ID") == true) + } + } + + @Test + fun `parseDocumentId throws for negative account ID`() { + try { + WebDavProvider.parseDocumentId("/-1/documents/file.txt") + fail("Expected IllegalArgumentException") + } catch (e: IllegalArgumentException) { + assertTrue(e.message?.contains("Bad account ID") == true) + } + } + + @Test + fun `parseDocumentId throws for empty account ID`() { + try { + WebDavProvider.parseDocumentId("//documents/file.txt") + fail("Expected IllegalArgumentException") + } catch (e: IllegalArgumentException) { + assertTrue(e.message?.contains("Bad account ID") == true) + } + } + + @Test + fun `parseDocumentId throws for too few path segments`() { + try { + WebDavProvider.parseDocumentId("/1") + fail("Expected IllegalArgumentException") + } catch (e: IllegalArgumentException) { + assertTrue(e.message?.contains("Invalid document ID") == true) + } + } + + // Security tests for path traversal prevention + + @Test + fun `parseDocumentId throws for path traversal with double dots in middle`() { + try { + WebDavProvider.parseDocumentId("/1/documents/../../../etc/passwd") + fail("Expected IllegalArgumentException for path traversal") + } catch (e: IllegalArgumentException) { + assertTrue(e.message?.contains("Path traversal detected") == true) + } + } + + @Test + fun `parseDocumentId throws for path traversal with double dots at end`() { + try { + WebDavProvider.parseDocumentId("/1/documents/..") + fail("Expected IllegalArgumentException for path traversal") + } catch (e: IllegalArgumentException) { + assertTrue(e.message?.contains("Path traversal detected") == true) + } + } + + @Test + fun `parseDocumentId throws for path traversal with single dot`() { + try { + WebDavProvider.parseDocumentId("/1/documents/./hidden") + fail("Expected IllegalArgumentException for path traversal") + } catch (e: IllegalArgumentException) { + assertTrue(e.message?.contains("Path traversal detected") == true) + } + } + + @Test + fun `parseDocumentId throws for just double dots path`() { + try { + WebDavProvider.parseDocumentId("/1/..") + fail("Expected IllegalArgumentException for path traversal") + } catch (e: IllegalArgumentException) { + assertTrue(e.message?.contains("Path traversal detected") == true) + } + } + + @Test + fun `parseDocumentId throws for just single dot path`() { + try { + WebDavProvider.parseDocumentId("/1/.") + fail("Expected IllegalArgumentException for path traversal") + } catch (e: IllegalArgumentException) { + assertTrue(e.message?.contains("Path traversal detected") == true) + } + } + + @Test + fun `parseDocumentId allows filenames containing dots`() { + // This should NOT be considered path traversal + val (accountId, path) = WebDavProvider.parseDocumentId("/1/documents/file.name.txt") + + assertEquals(1L, accountId) + assertEquals(Paths.get("/documents/file.name.txt"), path) + } + + @Test + fun `parseDocumentId allows directory names with dots`() { + // Directory names like ".config" should be allowed + val (accountId, path) = WebDavProvider.parseDocumentId("/1/.config/settings") + + assertEquals(1L, accountId) + assertEquals(Paths.get("/.config/settings"), path) + } + + @Test + fun `parseDocumentId allows hidden files starting with dot`() { + val (accountId, path) = WebDavProvider.parseDocumentId("/1/documents/.hidden") + + assertEquals(1L, accountId) + assertEquals(Paths.get("/documents/.hidden"), path) + } + + @Test + fun `parseDocumentId handles URL-encoded paths`() { + val (accountId, path) = WebDavProvider.parseDocumentId("/1/documents/file%20name.txt") + + assertEquals(1L, accountId) + assertEquals(Paths.get("/documents/file%20name.txt"), path) + } + + @Test + fun `parseDocumentId normalizes redundant slashes`() { + val (accountId, path) = WebDavProvider.parseDocumentId("/1/documents//file.txt") + + assertEquals(1L, accountId) + // Path normalization should handle this + assertTrue(path.toString().contains("file.txt")) + } +} From db8aba146b36c5afb4ee36d452e5fe23fe5d4681 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 19 Dec 2025 17:05:08 +0000 Subject: [PATCH 3/9] Fix path traversal validation to allow legitimate dotfiles - Use path segment analysis instead of string pattern matching - Allow filenames like 'file..backup' while blocking '../' traversal - Fix error message to say 'non-negative integer' for consistency Co-authored-by: MinecraftFuns <25814618+MinecraftFuns@users.noreply.github.com> --- .../org/joefang/webdav/provider/WebDavProvider.kt | 11 ++++++----- .../org/joefang/webdav/provider/WebDavProviderTest.kt | 9 +++++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt b/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt index 8c168fa..3f84157 100644 --- a/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt +++ b/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt @@ -501,9 +501,9 @@ class WebDavProvider : DocumentsProvider() { val id = try { val accountIdStr = parts[1] - // Validate account ID is a positive long + // Validate account ID is a non-negative long (only digits allowed) if (accountIdStr.isEmpty() || accountIdStr.any { !it.isDigit() }) { - throw NumberFormatException("Account ID must be a positive integer") + throw NumberFormatException("Account ID must be a non-negative integer") } accountIdStr.toLong().also { if (it < 0) throw NumberFormatException("Account ID must be non-negative") @@ -515,9 +515,10 @@ class WebDavProvider : DocumentsProvider() { val pathStr = parts.drop(2).joinToString("/", prefix = "/") // Security: Check for path traversal attempts - if (pathStr.contains("/../") || pathStr.contains("/./") || - pathStr.endsWith("/..") || pathStr.endsWith("/.") || - pathStr == "/.." || pathStr == "/.") { + // We check for ".." and "." as standalone path segments, not as parts of filenames + // This allows legitimate files like ".config" or "file..backup" while blocking "../" + val pathSegments = pathStr.split("/").filter { it.isNotEmpty() } + if (pathSegments.any { it == ".." || it == "." }) { throw IllegalArgumentException("Invalid document ID: '$documentId' (Path traversal detected)") } diff --git a/app/src/test/java/org/joefang/webdav/provider/WebDavProviderTest.kt b/app/src/test/java/org/joefang/webdav/provider/WebDavProviderTest.kt index 103bf02..f8ed057 100644 --- a/app/src/test/java/org/joefang/webdav/provider/WebDavProviderTest.kt +++ b/app/src/test/java/org/joefang/webdav/provider/WebDavProviderTest.kt @@ -178,6 +178,15 @@ class WebDavProviderTest { assertEquals(Paths.get("/documents/.hidden"), path) } + @Test + fun `parseDocumentId allows filenames with double dots in name`() { + // Filenames like "file..backup" or "test..txt" should be allowed + val (accountId, path) = WebDavProvider.parseDocumentId("/1/documents/file..backup") + + assertEquals(1L, accountId) + assertEquals(Paths.get("/documents/file..backup"), path) + } + @Test fun `parseDocumentId handles URL-encoded paths`() { val (accountId, path) = WebDavProvider.parseDocumentId("/1/documents/file%20name.txt") From e343b713a7b5aabf133f5f3e2181253ca1be0693 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 19 Dec 2025 17:09:21 +0000 Subject: [PATCH 4/9] Address code review feedback - Use idiomatic Kotlin .also{} pattern instead of !! operator - Replace flaky timing-based performance test with correctness test Co-authored-by: MinecraftFuns <25814618+MinecraftFuns@users.noreply.github.com> --- .../org/joefang/webdav/provider/WebDavFile.kt | 15 +++---------- .../joefang/webdav/provider/WebDavFileTest.kt | 22 +++++++++---------- 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt b/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt index 042c613..f874519 100644 --- a/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt +++ b/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt @@ -196,10 +196,7 @@ class WebDavFile( private var current: WebDavFile? = null override fun hasNext(): Boolean = delegate.hasNext() - override fun next(): WebDavFile { - current = delegate.next() - return current!! - } + override fun next(): WebDavFile = delegate.next().also { current = it } override fun remove() { current?.let { pathIndex.remove(it.path) } delegate.remove() @@ -214,15 +211,9 @@ class WebDavFile( override fun hasNext(): Boolean = delegate.hasNext() override fun hasPrevious(): Boolean = delegate.hasPrevious() - override fun next(): WebDavFile { - current = delegate.next() - return current!! - } + override fun next(): WebDavFile = delegate.next().also { current = it } override fun nextIndex(): Int = delegate.nextIndex() - override fun previous(): WebDavFile { - current = delegate.previous() - return current!! - } + override fun previous(): WebDavFile = delegate.previous().also { current = it } override fun previousIndex(): Int = delegate.previousIndex() override fun add(element: WebDavFile) { pathIndex[element.path] = element diff --git a/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt b/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt index 610a16e..453d38b 100644 --- a/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt +++ b/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt @@ -255,24 +255,22 @@ class WebDavFileTest { } @Test - fun `performance test for findChildByPath with many children`() { + fun `findChildByPath works correctly with many children`() { val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - // Add 1000 children + // Add 1000 children to verify HashMap handles many entries repeat(1000) { i -> parent.children.add(WebDavFile(Paths.get("/documents/file$i.txt"))) } - // Find the 500th child - should be O(1) - val startTime = System.nanoTime() - val found = parent.findChildByPath(Paths.get("/documents/file500.txt")) - val endTime = System.nanoTime() + // Verify we can find children at various positions + assertNotNull(parent.findChildByPath(Paths.get("/documents/file0.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file500.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file999.txt"))) + assertNull(parent.findChildByPath(Paths.get("/documents/file1000.txt"))) - assertNotNull(found) - assertEquals(Paths.get("/documents/file500.txt"), found?.path) - - // HashMap lookup should be very fast (under 1ms) - val durationMs = (endTime - startTime) / 1_000_000 - assertTrue("HashMap lookup took too long: ${durationMs}ms", durationMs < 10) + // Verify the correct children are found + assertEquals(Paths.get("/documents/file500.txt"), + parent.findChildByPath(Paths.get("/documents/file500.txt"))?.path) } } From b28bb1013ec29efa1d189cc023810886916b6be8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 20 Dec 2025 00:27:25 +0000 Subject: [PATCH 5/9] Replace ArrayList+HashMap with LinkedHashMap for O(1) operations - Use LinkedHashMap for children collection with O(1) add/remove/lookup - Make path immutable (val) to ensure HashMap key stability - Add explicit children management API (addChild, removeChild, etc.) - Create comprehensive documentation in docs/DATA_STRUCTURES.md - Update all usages in WebDavProvider, WebDavClient, WebDavCache - Expand test coverage to 80 tests Co-authored-by: MinecraftFuns <25814618+MinecraftFuns@users.noreply.github.com> --- .../joefang/webdav/provider/WebDavClient.kt | 2 +- .../org/joefang/webdav/provider/WebDavFile.kt | 224 +++++----- .../joefang/webdav/provider/WebDavProvider.kt | 32 +- .../joefang/webdav/provider/WebDavFileTest.kt | 383 ++++++++++++------ docs/DATA_STRUCTURES.md | 169 ++++++++ 5 files changed, 555 insertions(+), 255 deletions(-) create mode 100644 docs/DATA_STRUCTURES.md diff --git a/app/src/main/java/org/joefang/webdav/provider/WebDavClient.kt b/app/src/main/java/org/joefang/webdav/provider/WebDavClient.kt index 7776dfd..4c1b8dd 100644 --- a/app/src/main/java/org/joefang/webdav/provider/WebDavClient.kt +++ b/app/src/main/java/org/joefang/webdav/provider/WebDavClient.kt @@ -161,7 +161,7 @@ class WebDavClient( val file = WebDavFile(desc) if (file.path != path.path) { file.parent = root - root.children.add(file) + root.addChild(file) } } diff --git a/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt b/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt index f874519..354120d 100644 --- a/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt +++ b/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt @@ -14,29 +14,40 @@ import java.util.Locale /** * Represents a file or directory on a WebDAV server. * - * This class maintains a tree structure of files where directories - * can have children. For efficient lookup operations, children are - * stored both in a list (for ordered iteration) and a HashMap - * (for O(1) path-based lookup). + * This class maintains a tree structure of files where directories can have children. + * + * ## Data Structure Choice: LinkedHashMap + * + * Children are stored in a [LinkedHashMap] keyed by [Path], which provides: + * - **O(1) lookup** by path via [findChildByPath] or [containsChild] + * - **O(1) insertion** at the end (preserves insertion order) + * - **O(1) deletion** by key/path + * - **Ordered iteration** in insertion order (important for consistent UI display) + * + * This is optimal for our access patterns which require: + * - Fast path-based lookups (cache validation, document resolution) + * - Adding children when listing directories + * - Removing children when files are deleted + * - Iterating over children for directory listings + * + * We do NOT require indexed access (e.g., `children[5]`), so LinkedHashMap is ideal. + * See `docs/DATA_STRUCTURES.md` for detailed analysis. + * + * @property path The immutable path of this file. Immutability ensures cache consistency. */ class WebDavFile( - var path: Path, + val path: Path, var isDirectory: Boolean = false, var contentType: String? = null, var isPending: Boolean = false ) { var parent: WebDavFile? = null - // Use a backing list for ordered iteration and a HashMap for O(1) lookup - private val childrenList: MutableList = ArrayList() - private val childrenByPath: MutableMap = HashMap() - /** - * Returns a mutable list view of children for backward compatibility. - * For better performance when looking up by path, use [findChildByPath]. + * Children stored in a LinkedHashMap for O(1) lookup, insertion, and deletion. + * Iteration preserves insertion order for consistent directory listings. */ - val children: MutableList - get() = ChildrenListWrapper(childrenList, childrenByPath) + private val childrenMap: LinkedHashMap = LinkedHashMap() val writable: Boolean = true @@ -104,130 +115,83 @@ class WebDavFile( return "application/octet-stream" } + // ==================== Children Management API ==================== + // All operations below are O(1) thanks to LinkedHashMap + /** - * Finds a child file by its path in O(1) time. + * Returns the number of children. O(1). + */ + val childCount: Int + get() = childrenMap.size + + /** + * Checks if this file has any children. O(1). + */ + fun hasChildren(): Boolean = childrenMap.isNotEmpty() + + /** + * Finds a child file by its path. O(1). * @param childPath The path of the child to find * @return The child file, or null if not found */ - fun findChildByPath(childPath: Path): WebDavFile? { - return childrenByPath[childPath] + fun findChildByPath(childPath: Path): WebDavFile? = childrenMap[childPath] + + /** + * Checks if a child with the given path exists. O(1). + */ + fun containsChild(childPath: Path): Boolean = childrenMap.containsKey(childPath) + + /** + * Adds a child file. O(1). + * If a child with the same path already exists, it will be replaced. + * @param child The child file to add + */ + fun addChild(child: WebDavFile) { + childrenMap[child.path] = child } /** - * A wrapper around the children list that keeps the HashMap in sync. - * This ensures backward compatibility with existing code that uses - * children as a MutableList while providing O(1) lookup by path. + * Adds multiple children. O(n) where n is the number of children to add. */ - private class ChildrenListWrapper( - private val backingList: MutableList, - private val pathIndex: MutableMap - ) : MutableList by backingList { - - override fun add(element: WebDavFile): Boolean { - pathIndex[element.path] = element - return backingList.add(element) - } - - override fun add(index: Int, element: WebDavFile) { - pathIndex[element.path] = element - backingList.add(index, element) - } - - override fun addAll(elements: Collection): Boolean { - elements.forEach { pathIndex[it.path] = it } - return backingList.addAll(elements) - } - - override fun addAll(index: Int, elements: Collection): Boolean { - elements.forEach { pathIndex[it.path] = it } - return backingList.addAll(index, elements) - } - - override fun remove(element: WebDavFile): Boolean { - pathIndex.remove(element.path) - return backingList.remove(element) - } - - override fun removeAt(index: Int): WebDavFile { - val element = backingList.removeAt(index) - pathIndex.remove(element.path) - return element - } - - override fun removeAll(elements: Collection): Boolean { - elements.forEach { pathIndex.remove(it.path) } - return backingList.removeAll(elements) - } - - override fun retainAll(elements: Collection): Boolean { - val pathsToKeep = elements.map { it.path }.toSet() - pathIndex.keys.retainAll(pathsToKeep) - return backingList.retainAll(elements) - } - - override fun clear() { - pathIndex.clear() - backingList.clear() - } - - override fun set(index: Int, element: WebDavFile): WebDavFile { - val old = backingList[index] - pathIndex.remove(old.path) - pathIndex[element.path] = element - return backingList.set(index, element) - } - - override fun iterator(): MutableIterator { - return IndexSyncIterator(backingList.iterator(), pathIndex) - } - - override fun listIterator(): MutableListIterator { - return IndexSyncListIterator(backingList.listIterator(), pathIndex) - } - - override fun listIterator(index: Int): MutableListIterator { - return IndexSyncListIterator(backingList.listIterator(index), pathIndex) - } - - private class IndexSyncIterator( - private val delegate: MutableIterator, - private val pathIndex: MutableMap - ) : MutableIterator { - private var current: WebDavFile? = null - - override fun hasNext(): Boolean = delegate.hasNext() - override fun next(): WebDavFile = delegate.next().also { current = it } - override fun remove() { - current?.let { pathIndex.remove(it.path) } - delegate.remove() - } - } - - private class IndexSyncListIterator( - private val delegate: MutableListIterator, - private val pathIndex: MutableMap - ) : MutableListIterator { - private var current: WebDavFile? = null - - override fun hasNext(): Boolean = delegate.hasNext() - override fun hasPrevious(): Boolean = delegate.hasPrevious() - override fun next(): WebDavFile = delegate.next().also { current = it } - override fun nextIndex(): Int = delegate.nextIndex() - override fun previous(): WebDavFile = delegate.previous().also { current = it } - override fun previousIndex(): Int = delegate.previousIndex() - override fun add(element: WebDavFile) { - pathIndex[element.path] = element - delegate.add(element) - } - override fun remove() { - current?.let { pathIndex.remove(it.path) } - delegate.remove() - } - override fun set(element: WebDavFile) { - current?.let { pathIndex.remove(it.path) } - pathIndex[element.path] = element - delegate.set(element) - } - } + fun addChildren(children: Collection) { + children.forEach { childrenMap[it.path] = it } + } + + /** + * Removes a child by reference. O(1). + * @return true if the child was removed, false if not found + */ + fun removeChild(child: WebDavFile): Boolean = childrenMap.remove(child.path) != null + + /** + * Removes a child by path. O(1). + * @return The removed child, or null if not found + */ + fun removeChildByPath(childPath: Path): WebDavFile? = childrenMap.remove(childPath) + + /** + * Removes all children. O(1). + */ + fun clearChildren() { + childrenMap.clear() } + + /** + * Returns an iterator over the children in insertion order. + * Modifications during iteration are supported via the iterator's remove() method. + */ + fun childrenIterator(): MutableIterator = childrenMap.values.iterator() + + /** + * Returns all children as a collection. + * The returned collection is a view backed by the map; changes to the map + * are reflected in the collection. For a snapshot, use [childrenSnapshot]. + */ + fun children(): Collection = childrenMap.values + + /** + * Returns a snapshot (copy) of all children as a list. + * Use this when you need to iterate while potentially modifying the children. + */ + fun childrenSnapshot(): List = childrenMap.values.toList() } diff --git a/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt b/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt index 3f84157..192b217 100644 --- a/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt +++ b/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt @@ -126,7 +126,7 @@ class WebDavProvider : DocumentsProvider() { cache.setFileMeta(account, parentFile) result.apply { - for (file in parentFile.children) { + for (file in parentFile.children()) { if (!file.isPending) { includeFile(this, account, file) } @@ -227,8 +227,8 @@ class WebDavProvider : DocumentsProvider() { val callback = WebDavWriteProxyCallback(clients.get(account), file, onSuccess = { newFile -> file.parent?.let { - it.children.remove(file) - it.children.add(newFile) + it.removeChild(file) + it.addChild(newFile) val notifyUri = buildDocumentUri(account, it) mustGetContext().contentResolver.notifyChange(notifyUri, null, 0) @@ -236,7 +236,7 @@ class WebDavProvider : DocumentsProvider() { writeProxies.remove(documentId) }, onFail = { - file.parent?.children?.remove(file) + file.parent?.removeChild(file) writeProxies.remove(documentId) } ) @@ -268,7 +268,7 @@ class WebDavProvider : DocumentsProvider() { if (res.isSuccessful) { val file = WebDavFile(path, true, contentType = mimeType) file.parent = dir - dir.children.add(file) + dir.addChild(file) cache.setFileMeta(account, file) val notifyUri = buildDocumentUri(account, file.parent!!) @@ -279,7 +279,7 @@ class WebDavProvider : DocumentsProvider() { } else { val file = WebDavFile(path, false, contentType = mimeType, isPending = true) file.parent = dir - dir.children.add(file) + dir.addChild(file) resDocumentId = buildDocumentId(account, file) } @@ -302,7 +302,7 @@ class WebDavProvider : DocumentsProvider() { if (res.isSuccessful) { cache.removeFileMeta(account, file.path) - file.parent?.children?.remove(file) + file.parent?.removeChild(file) val notifyUri = buildDocumentUri(account, file.path.parent) mustGetContext().contentResolver.notifyChange(notifyUri, null, 0) @@ -328,13 +328,25 @@ class WebDavProvider : DocumentsProvider() { clients.get(account).move(file.davPath, WebDavPath(newPath, file.isDirectory)) } if (res.isSuccessful) { - file.path = newPath + // Create a new file with the new path (path is immutable for HashMap key stability) + val renamedFile = WebDavFile(newPath, file.isDirectory, file.contentType, file.isPending) + renamedFile.parent = file.parent + renamedFile.etag = file.etag + renamedFile.contentLength = file.contentLength + renamedFile.quotaUsedBytes = file.quotaUsedBytes + renamedFile.quotaAvailableBytes = file.quotaAvailableBytes + renamedFile.lastModified = file.lastModified + + // Update parent's children: remove old, add new + file.parent?.removeChild(file) + file.parent?.addChild(renamedFile) + if (file.isDirectory) { cache.removeFileMeta(account, oldPath) - cache.setFileMeta(account, file) + cache.setFileMeta(account, renamedFile) } - val notifyUri = buildDocumentUri(account, file.path.parent) + val notifyUri = buildDocumentUri(account, newPath.parent) mustGetContext().contentResolver.notifyChange(notifyUri, null, 0) return buildDocumentId(account, newPath) diff --git a/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt b/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt index 453d38b..2984b81 100644 --- a/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt +++ b/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt @@ -8,22 +8,111 @@ import org.junit.Assert.assertFalse import org.junit.Test import java.nio.file.Paths +/** + * Comprehensive tests for WebDavFile, especially the LinkedHashMap-based children management. + * + * These tests verify: + * - O(1) operations (add, remove, lookup) + * - Insertion order preservation + * - Iterator correctness (including modification during iteration) + * - Edge cases and challenging access patterns + */ class WebDavFileTest { + // ==================== Basic Operations ==================== + @Test - fun `children list add and retrieve works correctly`() { + fun `addChild adds child and findChildByPath retrieves it`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child = WebDavFile(Paths.get("/documents/file.txt")) + + parent.addChild(child) + + assertEquals(1, parent.childCount) + assertEquals(child, parent.findChildByPath(Paths.get("/documents/file.txt"))) + } + + @Test + fun `addChildren adds multiple children`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val children = listOf( + WebDavFile(Paths.get("/documents/file1.txt")), + WebDavFile(Paths.get("/documents/file2.txt")), + WebDavFile(Paths.get("/documents/file3.txt")) + ) + + parent.addChildren(children) + + assertEquals(3, parent.childCount) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file3.txt"))) + } + + @Test + fun `removeChild removes child`() { val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) val child1 = WebDavFile(Paths.get("/documents/file1.txt")) val child2 = WebDavFile(Paths.get("/documents/file2.txt")) - parent.children.add(child1) - parent.children.add(child2) + parent.addChild(child1) + parent.addChild(child2) + + val removed = parent.removeChild(child1) - assertEquals(2, parent.children.size) - assertTrue(parent.children.contains(child1)) - assertTrue(parent.children.contains(child2)) + assertTrue(removed) + assertEquals(1, parent.childCount) + assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) } - + + @Test + fun `removeChild returns false for non-existent child`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child = WebDavFile(Paths.get("/documents/file.txt")) + + val removed = parent.removeChild(child) + + assertFalse(removed) + } + + @Test + fun `removeChildByPath removes child and returns it`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val child = WebDavFile(Paths.get("/documents/file.txt")) + + parent.addChild(child) + + val removed = parent.removeChildByPath(Paths.get("/documents/file.txt")) + + assertEquals(child, removed) + assertEquals(0, parent.childCount) + } + + @Test + fun `removeChildByPath returns null for non-existent path`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + + val removed = parent.removeChildByPath(Paths.get("/documents/nonexistent.txt")) + + assertNull(removed) + } + + @Test + fun `clearChildren removes all children`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + parent.addChild(WebDavFile(Paths.get("/documents/file1.txt"))) + parent.addChild(WebDavFile(Paths.get("/documents/file2.txt"))) + parent.addChild(WebDavFile(Paths.get("/documents/file3.txt"))) + + parent.clearChildren() + + assertEquals(0, parent.childCount) + assertFalse(parent.hasChildren()) + } + + // ==================== O(1) Lookup Tests ==================== + @Test fun `findChildByPath returns child in O(1) time`() { val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) @@ -31,149 +120,235 @@ class WebDavFileTest { val child2 = WebDavFile(Paths.get("/documents/file2.txt")) val child3 = WebDavFile(Paths.get("/documents/file3.txt")) - parent.children.add(child1) - parent.children.add(child2) - parent.children.add(child3) + parent.addChild(child1) + parent.addChild(child2) + parent.addChild(child3) - // Find using O(1) HashMap lookup val found = parent.findChildByPath(Paths.get("/documents/file2.txt")) assertNotNull(found) assertEquals(child2, found) } - + @Test fun `findChildByPath returns null for non-existent path`() { val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - val child = WebDavFile(Paths.get("/documents/file.txt")) - - parent.children.add(child) + parent.addChild(WebDavFile(Paths.get("/documents/file.txt"))) val found = parent.findChildByPath(Paths.get("/documents/nonexistent.txt")) assertNull(found) } - + @Test - fun `children remove updates HashMap correctly`() { + fun `containsChild returns true for existing child`() { val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - val child1 = WebDavFile(Paths.get("/documents/file1.txt")) - val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + parent.addChild(WebDavFile(Paths.get("/documents/file.txt"))) - parent.children.add(child1) - parent.children.add(child2) + assertTrue(parent.containsChild(Paths.get("/documents/file.txt"))) + } + + @Test + fun `containsChild returns false for non-existent child`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - // Remove child1 - parent.children.remove(child1) + assertFalse(parent.containsChild(Paths.get("/documents/nonexistent.txt"))) + } + + // ==================== Insertion Order Preservation ==================== + + @Test + fun `children iteration preserves insertion order`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + val paths = listOf( + Paths.get("/documents/a.txt"), + Paths.get("/documents/b.txt"), + Paths.get("/documents/c.txt"), + Paths.get("/documents/d.txt") + ) - // Verify removal from both list and HashMap - assertEquals(1, parent.children.size) - assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) - assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + paths.forEach { parent.addChild(WebDavFile(it)) } + + val iteratedPaths = parent.children().map { it.path } + + assertEquals(paths, iteratedPaths) } - + @Test - fun `children clear removes all from HashMap`() { + fun `insertion order preserved after removal and re-addition`() { val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - val child1 = WebDavFile(Paths.get("/documents/file1.txt")) - val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + parent.addChild(WebDavFile(Paths.get("/documents/a.txt"))) + parent.addChild(WebDavFile(Paths.get("/documents/b.txt"))) + parent.addChild(WebDavFile(Paths.get("/documents/c.txt"))) - parent.children.add(child1) - parent.children.add(child2) + // Remove 'b' and re-add it - it should go to the end + parent.removeChildByPath(Paths.get("/documents/b.txt")) + parent.addChild(WebDavFile(Paths.get("/documents/b.txt"))) - parent.children.clear() + val iteratedPaths = parent.children().map { it.path.fileName.toString() } - assertEquals(0, parent.children.size) - assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) - assertNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + assertEquals(listOf("a.txt", "c.txt", "b.txt"), iteratedPaths) } - + + // ==================== Iterator Tests ==================== + @Test - fun `children addAll updates HashMap correctly`() { + fun `childrenIterator iterates in insertion order`() { val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - val children = listOf( - WebDavFile(Paths.get("/documents/file1.txt")), - WebDavFile(Paths.get("/documents/file2.txt")), - WebDavFile(Paths.get("/documents/file3.txt")) - ) + parent.addChild(WebDavFile(Paths.get("/documents/file1.txt"))) + parent.addChild(WebDavFile(Paths.get("/documents/file2.txt"))) + parent.addChild(WebDavFile(Paths.get("/documents/file3.txt"))) + + val names = mutableListOf() + val iterator = parent.childrenIterator() + while (iterator.hasNext()) { + names.add(iterator.next().name) + } + + assertEquals(listOf("file1.txt", "file2.txt", "file3.txt"), names) + } + + @Test + fun `childrenIterator remove works correctly`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + parent.addChild(WebDavFile(Paths.get("/documents/file1.txt"))) + parent.addChild(WebDavFile(Paths.get("/documents/file2.txt"))) + parent.addChild(WebDavFile(Paths.get("/documents/file3.txt"))) - parent.children.addAll(children) + val iterator = parent.childrenIterator() + while (iterator.hasNext()) { + val file = iterator.next() + if (file.name == "file2.txt") { + iterator.remove() + } + } - assertEquals(3, parent.children.size) + assertEquals(2, parent.childCount) + assertNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) assertNotNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) - assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) assertNotNull(parent.findChildByPath(Paths.get("/documents/file3.txt"))) } - + @Test - fun `children removeAll updates HashMap correctly`() { + fun `childrenSnapshot allows safe iteration during modification`() { val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - val child1 = WebDavFile(Paths.get("/documents/file1.txt")) - val child2 = WebDavFile(Paths.get("/documents/file2.txt")) - val child3 = WebDavFile(Paths.get("/documents/file3.txt")) - - parent.children.addAll(listOf(child1, child2, child3)) - parent.children.removeAll(listOf(child1, child3)) + parent.addChild(WebDavFile(Paths.get("/documents/file1.txt"))) + parent.addChild(WebDavFile(Paths.get("/documents/file2.txt"))) + parent.addChild(WebDavFile(Paths.get("/documents/file3.txt"))) + + // Take a snapshot and modify original during iteration + for (child in parent.childrenSnapshot()) { + if (child.name == "file2.txt") { + parent.removeChild(child) + } + } - assertEquals(1, parent.children.size) - assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) - assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) - assertNull(parent.findChildByPath(Paths.get("/documents/file3.txt"))) + assertEquals(2, parent.childCount) + assertNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) } - + + // ==================== Edge Cases ==================== + @Test - fun `children set updates HashMap correctly`() { + fun `addChild replaces existing child with same path`() { val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - val child1 = WebDavFile(Paths.get("/documents/file1.txt")) - val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + val child1 = WebDavFile(Paths.get("/documents/file.txt")) + child1.contentLength = 100L + val child2 = WebDavFile(Paths.get("/documents/file.txt")) + child2.contentLength = 200L - parent.children.add(child1) + parent.addChild(child1) + parent.addChild(child2) // Same path, replaces child1 - // Replace child1 with child2 - parent.children[0] = child2 + assertEquals(1, parent.childCount) + assertEquals(200L, parent.findChildByPath(Paths.get("/documents/file.txt"))?.contentLength) + } + + @Test + fun `hasChildren returns false for empty parent`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - assertEquals(1, parent.children.size) - assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) - assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + assertFalse(parent.hasChildren()) } - + @Test - fun `children iterator remove updates HashMap correctly`() { + fun `hasChildren returns true for non-empty parent`() { val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - val child1 = WebDavFile(Paths.get("/documents/file1.txt")) - val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + parent.addChild(WebDavFile(Paths.get("/documents/file.txt"))) - parent.children.add(child1) - parent.children.add(child2) + assertTrue(parent.hasChildren()) + } + + @Test + fun `childCount is accurate after multiple operations`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - val iterator = parent.children.iterator() - while (iterator.hasNext()) { - val file = iterator.next() - if (file.path == Paths.get("/documents/file1.txt")) { - iterator.remove() - } + assertEquals(0, parent.childCount) + + parent.addChild(WebDavFile(Paths.get("/documents/a.txt"))) + assertEquals(1, parent.childCount) + + parent.addChild(WebDavFile(Paths.get("/documents/b.txt"))) + parent.addChild(WebDavFile(Paths.get("/documents/c.txt"))) + assertEquals(3, parent.childCount) + + parent.removeChildByPath(Paths.get("/documents/b.txt")) + assertEquals(2, parent.childCount) + + parent.clearChildren() + assertEquals(0, parent.childCount) + } + + // ==================== Large Scale Tests ==================== + + @Test + fun `operations work correctly with many children`() { + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + + // Add 1000 children + repeat(1000) { i -> + parent.addChild(WebDavFile(Paths.get("/documents/file$i.txt"))) } - assertEquals(1, parent.children.size) - assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) - assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + assertEquals(1000, parent.childCount) + + // Verify lookup at various positions + assertNotNull(parent.findChildByPath(Paths.get("/documents/file0.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file500.txt"))) + assertNotNull(parent.findChildByPath(Paths.get("/documents/file999.txt"))) + assertNull(parent.findChildByPath(Paths.get("/documents/file1000.txt"))) + + // Remove some and verify + parent.removeChildByPath(Paths.get("/documents/file500.txt")) + assertEquals(999, parent.childCount) + assertNull(parent.findChildByPath(Paths.get("/documents/file500.txt"))) } - + @Test - fun `children removeAt updates HashMap correctly`() { + fun `insertion order preserved with large number of children`() { val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - val child1 = WebDavFile(Paths.get("/documents/file1.txt")) - val child2 = WebDavFile(Paths.get("/documents/file2.txt")) + val expectedOrder = (0 until 100).map { Paths.get("/documents/file$it.txt") } - parent.children.add(child1) - parent.children.add(child2) + expectedOrder.forEach { parent.addChild(WebDavFile(it)) } - parent.children.removeAt(0) + val actualOrder = parent.children().map { it.path } - assertEquals(1, parent.children.size) - assertNull(parent.findChildByPath(Paths.get("/documents/file1.txt"))) - assertNotNull(parent.findChildByPath(Paths.get("/documents/file2.txt"))) + assertEquals(expectedOrder, actualOrder) + } + + // ==================== Path Immutability Tests ==================== + + @Test + fun `path is immutable`() { + val file = WebDavFile(Paths.get("/documents/file.txt")) + + // path is now 'val' so this should not compile if attempted + // file.path = Paths.get("/other/path.txt") // Compile error + + assertEquals(Paths.get("/documents/file.txt"), file.path) } + + // ==================== WebDavFile Properties Tests ==================== @Test fun `WebDavFile name property returns filename`() { @@ -253,24 +428,4 @@ class WebDavFileTest { assertTrue(file.isPending) } - - @Test - fun `findChildByPath works correctly with many children`() { - val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) - - // Add 1000 children to verify HashMap handles many entries - repeat(1000) { i -> - parent.children.add(WebDavFile(Paths.get("/documents/file$i.txt"))) - } - - // Verify we can find children at various positions - assertNotNull(parent.findChildByPath(Paths.get("/documents/file0.txt"))) - assertNotNull(parent.findChildByPath(Paths.get("/documents/file500.txt"))) - assertNotNull(parent.findChildByPath(Paths.get("/documents/file999.txt"))) - assertNull(parent.findChildByPath(Paths.get("/documents/file1000.txt"))) - - // Verify the correct children are found - assertEquals(Paths.get("/documents/file500.txt"), - parent.findChildByPath(Paths.get("/documents/file500.txt"))?.path) - } } diff --git a/docs/DATA_STRUCTURES.md b/docs/DATA_STRUCTURES.md new file mode 100644 index 0000000..fe31303 --- /dev/null +++ b/docs/DATA_STRUCTURES.md @@ -0,0 +1,169 @@ +# Data Structures for WebDavFile Children Collection + +This document analyzes the data structure options for storing child files in `WebDavFile` and justifies the choice of `LinkedHashMap`. + +## Requirements Analysis + +Based on the codebase analysis, `WebDavFile.children` is accessed in the following ways: + +| Operation | Location | Frequency | +|-----------|----------|-----------| +| **Add child** | `WebDavClient.kt`, `WebDavProvider.kt` | Every directory listing, file creation | +| **Remove child** | `WebDavProvider.kt` | File deletion, move operations | +| **Iterate all children** | `WebDavProvider.kt` | Directory listings in UI | +| **Lookup by path** | `WebDavCache.kt`, `WebDavProvider.kt` | Cache validation, document resolution | +| **Check existence** | `WebDavCache.kt` | Cache validation | + +Notably, **indexed access** (e.g., `children[5]`) is **NOT** used in production code. + +## Data Structure Comparison + +### Option 1: ArrayList Only + +```kotlin +private val children: MutableList = ArrayList() +``` + +| Operation | Complexity | +|-----------|------------| +| Add (at end) | O(1) amortized | +| Remove by reference | **O(N)** - must search then shift | +| Lookup by path | **O(N)** - linear search | +| Iterate | O(N) | +| Index access | O(1) | + +**Verdict:** Poor for lookup-heavy workloads. Path lookups during cache validation become O(N). + +### Option 2: ArrayList + HashMap (Previous Implementation) + +```kotlin +private val childrenList: MutableList = ArrayList() +private val childrenByPath: MutableMap = HashMap() +``` + +| Operation | Complexity | +|-----------|------------| +| Add (at end) | O(1) | +| Remove by reference | **O(N)** - ArrayList still shifts | +| Lookup by path | O(1) | +| Iterate | O(N) | +| Index access | O(1) | + +**Verdict:** Good lookup performance, but: +- Requires wrapper class to keep both structures in sync +- O(N) removal due to ArrayList shifting +- Two data structures = more memory, more complexity + +### Option 3: LinkedHashMap (Current Implementation) ✓ + +```kotlin +private val childrenMap: LinkedHashMap = LinkedHashMap() +``` + +| Operation | Complexity | +|-----------|------------| +| Add | O(1) | +| Remove by key | O(1) | +| Lookup by path | O(1) | +| Iterate | O(N) - insertion order preserved | +| Index access | **O(N)** - not directly supported | + +**Verdict:** Optimal for our access patterns: +- All operations we actually use are O(1) +- Insertion order preserved for consistent UI display +- Single data structure = simpler code, less memory +- No wrapper classes needed + +### Option 4: TreeMap + +```kotlin +private val childrenMap: TreeMap = TreeMap() +``` + +| Operation | Complexity | +|-----------|------------| +| Add | O(log N) | +| Remove by key | O(log N) | +| Lookup by path | O(log N) | +| Iterate | O(N) - sorted order | + +**Verdict:** Slower than HashMap. We don't need sorted order (insertion order is fine). + +## Why LinkedHashMap is Optimal + +1. **O(1) for all operations we use:** + - Adding children (directory listings) + - Removing children (deletions) + - Path-based lookups (cache validation) + +2. **Insertion order preservation:** + - Children appear in the order they were added + - Consistent UI behavior for directory listings + +3. **Simple implementation:** + - No wrapper classes needed + - Single data structure to maintain + - Clean, idiomatic Kotlin API + +4. **Memory efficient:** + - One LinkedHashMap vs ArrayList + HashMap + - LinkedHashMap uses ~same memory as HashMap + linked list overhead + +## API Design + +The new API explicitly reflects the data structure choice: + +```kotlin +// O(1) operations +fun findChildByPath(path: Path): WebDavFile? +fun containsChild(path: Path): Boolean +fun addChild(child: WebDavFile) +fun removeChild(child: WebDavFile): Boolean +fun removeChildByPath(path: Path): WebDavFile? +fun clearChildren() + +// Properties +val childCount: Int +fun hasChildren(): Boolean + +// Iteration (O(N), preserves insertion order) +fun children(): Collection +fun childrenIterator(): MutableIterator +fun childrenSnapshot(): List // Safe for modification during iteration +``` + +## Path Immutability + +The `path` property is now immutable (`val` instead of `var`) to prevent cache corruption: + +```kotlin +class WebDavFile( + val path: Path, // Immutable - ensures HashMap key stability + ... +) +``` + +This is critical because `LinkedHashMap` uses `path` as the key. If the path changed after insertion, the file would become "lost" in the map under the old hash. + +## Performance Characteristics + +For a typical directory with N children: + +| Operation | Time | Memory | +|-----------|------|--------| +| List directory | O(N) network + O(N) insertion | O(N) | +| Find file in directory | O(1) | - | +| Delete file | O(1) | - | +| Cache validation | O(1) lookup | - | + +## Conclusion + +`LinkedHashMap` is the optimal choice for our access patterns because: + +1. All production operations are O(1) +2. Insertion order is preserved for consistent UI +3. Simpler implementation with less code +4. Memory efficient +5. Immutable keys prevent cache corruption + +The only trade-off is O(N) indexed access, which we don't use. From 677a96f3127e821726a27679fb98e3e3391e4c24 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 20 Dec 2025 00:31:34 +0000 Subject: [PATCH 6/9] Add copyWithNewPath method for safe file renaming - Add copyWithNewPath() to WebDavFile for safe property copying - Update WebDavProvider.renameDocument to use the new method - Add tests for copyWithNewPath method - Total: 82 unit tests Co-authored-by: MinecraftFuns <25814618+MinecraftFuns@users.noreply.github.com> --- .../org/joefang/webdav/provider/WebDavFile.kt | 20 +++++++++ .../joefang/webdav/provider/WebDavProvider.kt | 10 +---- .../joefang/webdav/provider/WebDavFileTest.kt | 45 +++++++++++++++++++ 3 files changed, 67 insertions(+), 8 deletions(-) diff --git a/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt b/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt index 354120d..78bc07e 100644 --- a/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt +++ b/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt @@ -115,6 +115,26 @@ class WebDavFile( return "application/octet-stream" } + // ==================== Copy Method ==================== + + /** + * Creates a copy of this file with a new path. + * All properties are copied except children (which are not transferred during rename). + * + * @param newPath The path for the new file + * @return A new WebDavFile with the same properties but a different path + */ + fun copyWithNewPath(newPath: Path): WebDavFile { + return WebDavFile(newPath, isDirectory, contentType, isPending).also { + it.parent = parent + it.etag = etag + it.contentLength = contentLength + it.quotaUsedBytes = quotaUsedBytes + it.quotaAvailableBytes = quotaAvailableBytes + it.lastModified = lastModified + } + } + // ==================== Children Management API ==================== // All operations below are O(1) thanks to LinkedHashMap diff --git a/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt b/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt index 192b217..03a097f 100644 --- a/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt +++ b/app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt @@ -328,14 +328,8 @@ class WebDavProvider : DocumentsProvider() { clients.get(account).move(file.davPath, WebDavPath(newPath, file.isDirectory)) } if (res.isSuccessful) { - // Create a new file with the new path (path is immutable for HashMap key stability) - val renamedFile = WebDavFile(newPath, file.isDirectory, file.contentType, file.isPending) - renamedFile.parent = file.parent - renamedFile.etag = file.etag - renamedFile.contentLength = file.contentLength - renamedFile.quotaUsedBytes = file.quotaUsedBytes - renamedFile.quotaAvailableBytes = file.quotaAvailableBytes - renamedFile.lastModified = file.lastModified + // Create a copy with the new path (using copyWithNewPath to ensure all fields are copied) + val renamedFile = file.copyWithNewPath(newPath) // Update parent's children: remove old, add new file.parent?.removeChild(file) diff --git a/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt b/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt index 2984b81..1a3ef50 100644 --- a/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt +++ b/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt @@ -348,6 +348,51 @@ class WebDavFileTest { assertEquals(Paths.get("/documents/file.txt"), file.path) } + // ==================== Copy Method Tests ==================== + + @Test + fun `copyWithNewPath creates file with new path and copies all properties`() { + val original = WebDavFile(Paths.get("/documents/old.txt"), isDirectory = false) + original.contentType = "text/plain" + original.isPending = true + original.etag = "abc123" + original.contentLength = 1024L + original.quotaUsedBytes = 500L + original.quotaAvailableBytes = 9500L + original.lastModified = java.util.Date(1234567890000L) + + val parent = WebDavFile(Paths.get("/documents"), isDirectory = true) + original.parent = parent + + val copy = original.copyWithNewPath(Paths.get("/documents/new.txt")) + + // Verify new path + assertEquals(Paths.get("/documents/new.txt"), copy.path) + + // Verify all other properties are copied + assertEquals(original.isDirectory, copy.isDirectory) + assertEquals(original.contentType, copy.contentType) + assertEquals(original.isPending, copy.isPending) + assertEquals(original.etag, copy.etag) + assertEquals(original.contentLength, copy.contentLength) + assertEquals(original.quotaUsedBytes, copy.quotaUsedBytes) + assertEquals(original.quotaAvailableBytes, copy.quotaAvailableBytes) + assertEquals(original.lastModified, copy.lastModified) + assertEquals(original.parent, copy.parent) + } + + @Test + fun `copyWithNewPath does not copy children`() { + val original = WebDavFile(Paths.get("/documents"), isDirectory = true) + original.addChild(WebDavFile(Paths.get("/documents/file1.txt"))) + original.addChild(WebDavFile(Paths.get("/documents/file2.txt"))) + + val copy = original.copyWithNewPath(Paths.get("/backup")) + + assertEquals(0, copy.childCount) + assertEquals(2, original.childCount) + } + // ==================== WebDavFile Properties Tests ==================== @Test From 81b9e58d11285367b3c45eeee3c766b4e66a5db6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 20 Dec 2025 00:34:46 +0000 Subject: [PATCH 7/9] Improve documentation for children API and tests - Add ConcurrentModificationException warning to children() method - Enhance test class KDoc with specific test categories Co-authored-by: MinecraftFuns <25814618+MinecraftFuns@users.noreply.github.com> --- .../java/org/joefang/webdav/provider/WebDavFile.kt | 8 +++++--- .../org/joefang/webdav/provider/WebDavFileTest.kt | 11 +++++++---- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt b/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt index 78bc07e..db7b10f 100644 --- a/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt +++ b/app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt @@ -203,9 +203,11 @@ class WebDavFile( fun childrenIterator(): MutableIterator = childrenMap.values.iterator() /** - * Returns all children as a collection. - * The returned collection is a view backed by the map; changes to the map - * are reflected in the collection. For a snapshot, use [childrenSnapshot]. + * Returns all children as a collection view. + * + * **Warning:** This is a view backed by the map. If children are modified while + * iterating (except via the iterator's remove method), a ConcurrentModificationException + * may be thrown. For safe iteration during modification, use [childrenSnapshot]. */ fun children(): Collection = childrenMap.values diff --git a/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt b/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt index 1a3ef50..1dd8092 100644 --- a/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt +++ b/app/src/test/java/org/joefang/webdav/provider/WebDavFileTest.kt @@ -12,10 +12,13 @@ import java.nio.file.Paths * Comprehensive tests for WebDavFile, especially the LinkedHashMap-based children management. * * These tests verify: - * - O(1) operations (add, remove, lookup) - * - Insertion order preservation - * - Iterator correctness (including modification during iteration) - * - Edge cases and challenging access patterns + * - **O(1) operations**: add, remove, lookup by path + * - **Insertion order preservation**: children iterate in order they were added + * - **Iterator correctness**: safe removal during iteration via iterator.remove() + * - **Snapshot iteration**: safe iteration while modifying the collection + * - **Large-scale operations**: 1000+ children to verify performance + * - **Edge cases**: duplicate paths, empty collections, path immutability + * - **Copy semantics**: copyWithNewPath for safe file renaming */ class WebDavFileTest { From 5614cfa402710fa48805846fdc237db359be0249 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 20 Dec 2025 16:07:23 +0000 Subject: [PATCH 8/9] Document threading model and duplicate handling behavior - Add section explaining why ConcurrentModificationException cannot occur - Document the DocumentsProvider threading model (Binder threads, runBlocking) - Explain that original ArrayList had same concurrency characteristics - Document improved duplicate path handling (overwrite vs accumulate) - Explain pending file behavior and edge cases Co-authored-by: MinecraftFuns <25814618+MinecraftFuns@users.noreply.github.com> --- docs/DATA_STRUCTURES.md | 82 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/docs/DATA_STRUCTURES.md b/docs/DATA_STRUCTURES.md index fe31303..b251f59 100644 --- a/docs/DATA_STRUCTURES.md +++ b/docs/DATA_STRUCTURES.md @@ -156,6 +156,86 @@ For a typical directory with N children: | Delete file | O(1) | - | | Cache validation | O(1) lookup | - | +## Threading Model and Concurrency Safety + +### Why Concurrent Modification Cannot Occur + +The `WebDavFile` children collection is accessed exclusively through Android's `DocumentsProvider` +framework, which uses a specific threading model: + +1. **Binder Thread Access**: All `DocumentsProvider` methods (`queryChildDocuments`, `createDocument`, + `deleteDocument`, etc.) are called on Binder threads by the Android framework. + +2. **Synchronous Execution**: Each method uses `runBlocking` to block until completion: + ```kotlin + val res = runBlocking(Dispatchers.IO) { + clients.get(account).propFind(WebDavPath(parentPath, true)) + } + // Iteration happens AFTER the blocking call completes + for (file in parentFile.children()) { ... } + ``` + +3. **Sequential Request Handling**: The DocumentsProvider framework serializes requests for the + same document/directory, meaning one operation completes before another begins. + +4. **Read-Only Iteration Pattern**: All iteration over `children()` in production code only + **reads** the collection (e.g., calling `includeFile()`). No modifications occur during iteration. + +### Comparison with Original ArrayList Implementation + +The original `ArrayList`-based implementation had the **same concurrency characteristics**: +- No synchronization was used +- Iteration was never modified during traversal +- The framework's threading model prevented concurrent access + +The switch to `LinkedHashMap` maintains this safety model. The +`ConcurrentModificationException` warning in the documentation is a general best-practice +note, but cannot occur in our actual usage patterns. + +### When to Use `childrenSnapshot()` + +Use `childrenSnapshot()` only if you need to: +1. Iterate over children while potentially adding/removing children in the same loop +2. Store a stable list that won't change if the parent is refreshed + +In current production code, this is never needed because: +- `queryChildDocuments`: Iterates to populate UI cursor (read-only) +- `createDocument`: Adds a single child (no iteration) +- `deleteDocument`: Removes a single child (no iteration) + +## Duplicate Path Handling (Overwrite Behavior) + +### Behavior Change from ArrayList + +**Original ArrayList behavior:** +- Calling `children.add(file)` twice with the same path would create **duplicate entries** +- This could lead to inconsistent state where the same file appears twice in directory listings + +**New LinkedHashMap behavior:** +- Calling `addChild(file)` with an existing path **replaces** the old entry +- This is actually **more correct** behavior for a file system model + +### Impact on "Pending" Files + +When creating a file upload: +1. `createDocument` creates a "pending" file with `isPending = true` +2. If the path already exists (rare edge case), the old file is replaced +3. On upload success: The pending file is replaced with the real file +4. On upload failure: The pending file is removed via `removeChild()` + +**Edge case analysis:** +- **Duplicate upload attempts:** If user uploads `file.txt` twice rapidly, the second pending + file replaces the first. This is correct - only one upload should be in progress. +- **Overwriting existing file:** Not a concern because: + - `createDocument` is for **new** files (Android's file picker) + - `openDocumentWrite` is for **existing** files (writes to existing entry) +- **Upload failure:** If upload fails, `removeChild(file)` removes only the pending file. + Since we store by path (not object identity), this correctly removes the pending entry. + +**Improvement over original:** The original ArrayList approach could accumulate duplicate +entries if the same file was uploaded multiple times without refresh. LinkedHashMap prevents +this by design. + ## Conclusion `LinkedHashMap` is the optimal choice for our access patterns because: @@ -165,5 +245,7 @@ For a typical directory with N children: 3. Simpler implementation with less code 4. Memory efficient 5. Immutable keys prevent cache corruption +6. Automatic duplicate handling prevents inconsistent state +7. Thread-safe under the DocumentsProvider threading model The only trade-off is O(N) indexed access, which we don't use. From e4f4f18987621249dc531cb714e6c99f9fc1021e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 20 Dec 2025 16:09:58 +0000 Subject: [PATCH 9/9] Refine documentation wording to be more defensive - Use "current codebase (December 2024)" instead of absolute claims - Change "cannot occur" to "should not occur during normal operation" - Add note about using childrenSnapshot() for future code changes - Frame duplicate upload handling as "design decision" not "correctness" Co-authored-by: MinecraftFuns <25814618+MinecraftFuns@users.noreply.github.com> --- docs/DATA_STRUCTURES.md | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/docs/DATA_STRUCTURES.md b/docs/DATA_STRUCTURES.md index b251f59..7fb00ac 100644 --- a/docs/DATA_STRUCTURES.md +++ b/docs/DATA_STRUCTURES.md @@ -178,31 +178,36 @@ framework, which uses a specific threading model: 3. **Sequential Request Handling**: The DocumentsProvider framework serializes requests for the same document/directory, meaning one operation completes before another begins. -4. **Read-Only Iteration Pattern**: All iteration over `children()` in production code only - **reads** the collection (e.g., calling `includeFile()`). No modifications occur during iteration. +4. **Read-Only Iteration Pattern**: In the current codebase (as of December 2024), all iteration + over `children()` only **reads** the collection (e.g., calling `includeFile()`). No modifications + occur during iteration in the existing code paths. ### Comparison with Original ArrayList Implementation The original `ArrayList`-based implementation had the **same concurrency characteristics**: - No synchronization was used -- Iteration was never modified during traversal +- Iteration was not modified during traversal in existing code paths - The framework's threading model prevented concurrent access The switch to `LinkedHashMap` maintains this safety model. The -`ConcurrentModificationException` warning in the documentation is a general best-practice -note, but cannot occur in our actual usage patterns. +`ConcurrentModificationException` warning in the documentation is a defensive best-practice +note. Under the current threading model and usage patterns, it should not occur during +normal operation. ### When to Use `childrenSnapshot()` -Use `childrenSnapshot()` only if you need to: +Use `childrenSnapshot()` if you need to: 1. Iterate over children while potentially adding/removing children in the same loop 2. Store a stable list that won't change if the parent is refreshed -In current production code, this is never needed because: +In the current codebase (December 2024), `childrenSnapshot()` is not required because: - `queryChildDocuments`: Iterates to populate UI cursor (read-only) - `createDocument`: Adds a single child (no iteration) - `deleteDocument`: Removes a single child (no iteration) +**Note:** If future code changes introduce iteration with modification, use `childrenSnapshot()` +or the iterator's `remove()` method. + ## Duplicate Path Handling (Overwrite Behavior) ### Behavior Change from ArrayList @@ -213,7 +218,7 @@ In current production code, this is never needed because: **New LinkedHashMap behavior:** - Calling `addChild(file)` with an existing path **replaces** the old entry -- This is actually **more correct** behavior for a file system model +- This is more appropriate behavior for a file system model where paths are unique ### Impact on "Pending" Files @@ -225,8 +230,8 @@ When creating a file upload: **Edge case analysis:** - **Duplicate upload attempts:** If user uploads `file.txt` twice rapidly, the second pending - file replaces the first. This is correct - only one upload should be in progress. -- **Overwriting existing file:** Not a concern because: + file replaces the first. This is a design decision: we show the most recent pending state. +- **Overwriting existing file:** Not a typical concern because: - `createDocument` is for **new** files (Android's file picker) - `openDocumentWrite` is for **existing** files (writes to existing entry) - **Upload failure:** If upload fails, `removeChild(file)` removes only the pending file.