diff --git a/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java b/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java index dd892465424c..ae793c845797 100644 --- a/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java +++ b/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java @@ -44,6 +44,7 @@ import com.nextcloud.model.OfflineOperationType; import com.nextcloud.model.ShareeEntry; import com.nextcloud.utils.AppConstants; +import com.nextcloud.utils.ResultParser; import com.nextcloud.utils.e2ee.E2EVersionHelper; import com.nextcloud.utils.extensions.FileDataStorageManagerExtensionsKt; import com.nextcloud.utils.extensions.FileExtensionsKt; @@ -189,7 +190,9 @@ public void addCreateFileOfflineOperation(String[] localPaths, String[] remotePa entity.setModifiedAt(modificationTimestamp / AppConstants.MILLIS_PER_SECOND); entity.setFilename(new File(remotePath).getName()); - String parentPath = new File(remotePath).getParent() + OCFile.PATH_SEPARATOR; + addCreateFolderOfflineOperationsForMissingParents(remotePath); + + String parentPath = FileStorageUtils.getParentPath(remotePath); OCFile parentFile = getFileByDecryptedRemotePath(parentPath); if (parentFile != null) { @@ -201,6 +204,24 @@ public void addCreateFileOfflineOperation(String[] localPaths, String[] remotePa } } + private void addCreateFolderOfflineOperationsForMissingParents(String remotePath) { + String parentPath = FileStorageUtils.getParentPath(remotePath); + if (parentPath == null || OCFile.ROOT_PATH.equals(parentPath)) { + return; + } + + if (getFileByEncryptedRemotePath(parentPath) != null) { + return; + } + + addCreateFolderOfflineOperationsForMissingParents(parentPath); + + String grandParentPath = FileStorageUtils.getParentPath(parentPath); + OCFile grandParent = grandParentPath == null ? null : getFileByEncryptedRemotePath(grandParentPath); + Long grandParentId = grandParent == null ? null : grandParent.getFileId(); + addCreateFolderOfflineOperation(parentPath, new File(parentPath).getName(), grandParentId); + } + public OfflineOperationEntity getOfflineEntityFromOCFile(OCFile file) { return offlineOperationDao.getByPath(file.getDecryptedRemotePath()); } @@ -640,17 +661,11 @@ private boolean isInternalTwoWaySyncAndNeverSynced(OCFile folder) { } /** - * Ensures that an {@link OCFile} and all of its parent folders are stored locally. + * Saves an {@link OCFile} and ensures that all of its parent folders are stored locally. *

- * If the file has no parent ID and is not the root folder, this method recursively: - *

+ * Parent folders missing from local storage are fetched from the server and saved recursively. * - * @param ocFile the file to be saved together with its parent hierarchy + * @param ocFile the file to be saved together with its parent hierarchy * @param context Android context used for remote operations * * @return the same {@link OCFile} instance with a valid parent ID @@ -658,51 +673,45 @@ private boolean isInternalTwoWaySyncAndNeverSynced(OCFile folder) { * @throws RemoteOperationFailedException if a parent folder cannot be retrieved * from the server */ - public OCFile saveFileWithParent(OCFile ocFile, Context context) { - if (ocFile.getParentId() == 0 && !OCFile.ROOT_PATH.equals(ocFile.getRemotePath())) { - Log_OC.d(TAG, "saving file with parents: " + ocFile.getRemotePath()); - - String remotePath = ocFile.getRemotePath(); - String parentPath = remotePath.substring(0, remotePath.lastIndexOf(ocFile.getFileName())); - - OCFile parentFile = getFileByPath(parentPath); - OCFile returnFile; + public OCFile saveFileWithParent(OCFile ocFile, Context context) throws RemoteOperationFailedException { + boolean isParentMissing = ocFile.getParentId() == 0 && !OCFile.ROOT_PATH.equals(ocFile.getRemotePath()); + if (isParentMissing) { + OCFile parent = getOrFetchParentFolder(ocFile.getRemotePath(), context); + ocFile.setParentId(parent.getFileId()); + } - if (parentFile == null) { - Log_OC.d(TAG, "Parent not found locally, fetching: " + parentPath); + saveFile(ocFile); + return ocFile; + } - final var operation = new ReadFileRemoteOperation(parentPath); - final var result = operation.execute(getUser(), context); + private OCFile getOrFetchParentFolder(String remotePath, Context context) { + String parentPath = FileStorageUtils.getParentPath(remotePath); + OCFile localParent = getFileByEncryptedRemotePath(parentPath); + if (localParent != null) { + return localParent; + } - if (result.isSuccess() && result.getData().get(0) instanceof RemoteFile remoteFile) { - OCFile folder = FileStorageUtils.fillOCFile(remoteFile); - Log_OC.d(TAG, "Fetched parent folder: " + folder); - returnFile = saveFileWithParent(folder, context); - } else { - Exception exception = result.getException(); - String message = "Error during saving file with parents: " + ocFile.getRemotePath() + " / " - + result.getLogMessage(context); + Log_OC.d(TAG, "Parent not found locally, fetching: " + parentPath); + OCFile remoteParent = FileStorageUtils.fillOCFile(fetchRemoteFolder(parentPath, context)); + return saveFileWithParent(remoteParent, context); + } - Log_OC.e(TAG, message); + @SuppressWarnings("deprecation") + private RemoteFile fetchRemoteFolder(String remotePath, Context context) { + final var result = new ReadFileRemoteOperation(remotePath).execute(getUser(), context); + final var remoteFile = ResultParser.data(result, RemoteFile.class); + if (remoteFile != null) { + return remoteFile; + } - if (exception != null) { - throw new RemoteOperationFailedException(message, exception); - } else { - throw new RemoteOperationFailedException(message); - } - } - } else { - Log_OC.d(TAG, "parent file exists, calling saveFileWithParent: " + ocFile.getRemotePath()); - returnFile = saveFileWithParent(parentFile, context); - } + String message = "Cannot fetch folder: " + remotePath + " / " + result.getLogMessage(context); + Log_OC.e(TAG, message); - long parentId = returnFile.getFileId(); - Log_OC.d(TAG, "saving parent id of: " + ocFile.getRemotePath() + " with: " + parentId); - ocFile.setParentId(parentId); - saveFile(ocFile); + Exception exception = result.getException(); + if (exception == null) { + throw new RemoteOperationFailedException(message); } - - return ocFile; + throw new RemoteOperationFailedException(message, exception); } public static void clearTempEncryptedFolder(String accountName) { diff --git a/app/src/test/java/com/owncloud/android/datamodel/FileDataStorageManagerSaveFileWithParentTest.kt b/app/src/test/java/com/owncloud/android/datamodel/FileDataStorageManagerSaveFileWithParentTest.kt new file mode 100644 index 000000000000..4d24686b9660 --- /dev/null +++ b/app/src/test/java/com/owncloud/android/datamodel/FileDataStorageManagerSaveFileWithParentTest.kt @@ -0,0 +1,411 @@ +/* + * Nextcloud - Android Client + * + * SPDX-FileCopyrightText: 2026 Alper Ozturk + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +package com.owncloud.android.datamodel + +import android.content.ContentResolver +import android.content.Context +import android.text.TextUtils +import com.nextcloud.client.account.User +import com.nextcloud.client.database.NextcloudDatabase +import com.nextcloud.client.database.dao.OfflineOperationDao +import com.nextcloud.client.database.entity.OfflineOperationEntity +import com.nextcloud.model.OfflineOperationType +import com.owncloud.android.MainApp +import com.owncloud.android.lib.common.operations.RemoteOperationResult +import com.owncloud.android.lib.resources.files.ReadFileRemoteOperation +import com.owncloud.android.lib.resources.files.model.RemoteFile +import com.owncloud.android.operations.RemoteOperationFailedException +import com.owncloud.android.utils.MimeType +import com.owncloud.android.utils.MimeTypeUtil +import io.mockk.EqMatcher +import io.mockk.every +import io.mockk.mockk +import io.mockk.mockkConstructor +import io.mockk.mockkObject +import io.mockk.mockkStatic +import io.mockk.spyk +import io.mockk.unmockkAll +import io.mockk.verify +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import com.nextcloud.common.User as LibraryUser + +class FileDataStorageManagerSaveFileWithParentTest { + + companion object { + private const val ROOT_ID = 1L + private const val LOCAL_PARENT_ID = 42L + private const val FIRST_GENERATED_ID = 100L + private const val LOG_MESSAGE = "server unreachable" + private const val LOCAL_UPLOAD_PATH = "/storage/emulated/0/upload" + } + + private lateinit var manager: FileDataStorageManager + private lateinit var context: Context + private val localFiles = mutableMapOf() + private val savedFiles = mutableListOf() + private val queuedOperations = mutableListOf() + private var nextFileId = FIRST_GENERATED_ID + + @Before + fun setUp() { + mockkStatic(TextUtils::class) + every { TextUtils.isEmpty(any()) } answers { arg(0).isNullOrEmpty() } + + context = mockk(relaxed = true) + mockkObject(MainApp.Companion) + every { MainApp.getAppContext() } returns context + + mockkStatic(MimeTypeUtil::class) + every { MimeTypeUtil.getMimeTypeFromPath(any()) } returns "text/plain" + + val offlineOperationDao = mockk(relaxed = true) + every { offlineOperationDao.insert(any()) } answers { + queuedOperations.addAll( + args.flatMap { if (it is Array<*>) it.toList() else listOf(it) } + .filterIsInstance() + ) + } + + val db = mockk(relaxed = true) + every { db.offlineOperationDao() } returns offlineOperationDao + mockkObject(NextcloudDatabase.Companion) + every { NextcloudDatabase.getInstance(any()) } returns db + every { NextcloudDatabase.instance() } returns db + + mockkConstructor(ReadFileRemoteOperation::class) + + manager = spyk(FileDataStorageManager(mockk(relaxed = true), null as ContentResolver?)) + every { manager.getFileByEncryptedRemotePath(any()) } answers { localFiles[firstArg()] } + every { manager.getFileByDecryptedRemotePath(any()) } answers { localFiles[firstArg()] } + every { manager.saveFile(any()) } answers { + val file = firstArg() + if (file.fileId == -1L) { + file.fileId = nextFileId++ + } + savedFiles.add(file) + localFiles[file.remotePath] = file + false + } + } + + @After + fun tearDown() { + unmockkAll() + } + + @Test + fun `root folder is saved without resolving a parent`() { + val root = OCFile(OCFile.ROOT_PATH).apply { mimeType = MimeType.DIRECTORY } + + manager.saveFileWithParent(root, context) + + assertEquals(0L, root.parentId) + assertEquals(listOf(root), savedFiles) + verify(exactly = 0) { manager.getFileByEncryptedRemotePath(any()) } + verifyNoServerFetch() + } + + @Test + fun `file with a parent id is saved without resolving a parent`() { + val file = OCFile("/documents/report.pdf").apply { parentId = LOCAL_PARENT_ID } + + manager.saveFileWithParent(file, context) + + assertEquals(LOCAL_PARENT_ID, file.parentId) + assertEquals(listOf(file), savedFiles) + verify(exactly = 0) { manager.getFileByEncryptedRemotePath(any()) } + verifyNoServerFetch() + } + + @Test + fun `file gets the id of its locally stored parent and the same instance is returned`() { + givenLocalFolder("/documents/", LOCAL_PARENT_ID) + val file = OCFile("/documents/report.pdf") + + val result = manager.saveFileWithParent(file, context) + + assertSame(file, result) + assertEquals(LOCAL_PARENT_ID, file.parentId) + assertEquals(listOf(file), savedFiles) + verifyNoServerFetch() + } + + @Test + fun `file in root gets the id of the locally stored root`() { + givenLocalFolder(OCFile.ROOT_PATH, ROOT_ID) + val file = OCFile("/report.pdf") + + manager.saveFileWithParent(file, context) + + assertEquals(ROOT_ID, file.parentId) + verifyNoServerFetch() + } + + @Test + fun `folder path with trailing separator resolves its parent folder`() { + givenLocalFolder("/documents/", LOCAL_PARENT_ID) + val folder = OCFile("/documents/invoices/").apply { mimeType = MimeType.DIRECTORY } + + manager.saveFileWithParent(folder, context) + + assertEquals(LOCAL_PARENT_ID, folder.parentId) + verifyNoServerFetch() + } + + @Test + fun `encrypted file resolves its parent from the encrypted remote path`() { + givenLocalFolder("/A1B2C3/", LOCAL_PARENT_ID) + val file = OCFile("/A1B2C3/D4E5F6").apply { + isEncrypted = true + decryptedRemotePath = "/secret/photo.jpg" + } + + manager.saveFileWithParent(file, context) + + assertEquals(LOCAL_PARENT_ID, file.parentId) + verify(exactly = 0) { manager.getFileByEncryptedRemotePath("/secret/") } + verifyNoServerFetch() + } + + @Test + fun `missing parent is fetched from the server and saved before the file`() { + givenLocalFolder(OCFile.ROOT_PATH, ROOT_ID) + givenServerFolder("/documents/") + val file = OCFile("/documents/report.pdf") + + manager.saveFileWithParent(file, context) + + assertEquals(listOf("/documents/", "/documents/report.pdf"), savedFiles.map { it.remotePath }) + val fetchedParent = savedFiles.first() + assertEquals(ROOT_ID, fetchedParent.parentId) + assertEquals(fetchedParent.fileId, file.parentId) + } + + @Test + fun `all missing ancestors are fetched and saved from the top down`() { + givenLocalFolder(OCFile.ROOT_PATH, ROOT_ID) + givenServerFolder("/a/") + givenServerFolder("/a/b/") + val file = OCFile("/a/b/c.txt") + + manager.saveFileWithParent(file, context) + + assertEquals(listOf("/a/", "/a/b/", "/a/b/c.txt"), savedFiles.map { it.remotePath }) + val (folderA, folderB) = savedFiles + assertEquals(ROOT_ID, folderA.parentId) + assertEquals(folderA.fileId, folderB.parentId) + assertEquals(folderB.fileId, file.parentId) + } + + @Test + fun `missing root is fetched and saved so its child gets a valid parent id`() { + givenServerFolder(OCFile.ROOT_PATH) + val file = OCFile("/report.pdf") + + manager.saveFileWithParent(file, context) + + assertEquals(listOf(OCFile.ROOT_PATH, "/report.pdf"), savedFiles.map { it.remotePath }) + assertEquals(FIRST_GENERATED_ID, file.parentId) + } + + @Test + fun `failed fetch with an exception throws it as the cause and saves nothing`() { + val cause = IllegalStateException("timeout") + givenServerResult("/documents/", serverResult(isSuccess = false, exception = cause)) + + val thrown = assertThrows(RemoteOperationFailedException::class.java) { + manager.saveFileWithParent(OCFile("/documents/report.pdf"), context) + } + + assertSame(cause, thrown.cause) + assertTrue(thrown.message.orEmpty().contains("/documents/")) + assertTrue(savedFiles.isEmpty()) + } + + @Test + fun `failed fetch without an exception throws without a cause`() { + givenServerResult("/documents/", serverResult(isSuccess = false)) + + val thrown = assertThrows(RemoteOperationFailedException::class.java) { + manager.saveFileWithParent(OCFile("/documents/report.pdf"), context) + } + + assertNull(thrown.cause) + assertTrue(thrown.message.orEmpty().contains(LOG_MESSAGE)) + assertTrue(savedFiles.isEmpty()) + } + + @Test + fun `successful fetch with empty data throws`() { + givenServerResult("/documents/", serverResult(isSuccess = true, data = arrayListOf())) + + assertThrows(RemoteOperationFailedException::class.java) { + manager.saveFileWithParent(OCFile("/documents/report.pdf"), context) + } + assertTrue(savedFiles.isEmpty()) + } + + @Test + fun `successful fetch with null data throws`() { + givenServerResult("/documents/", serverResult(isSuccess = true, data = null)) + + assertThrows(RemoteOperationFailedException::class.java) { + manager.saveFileWithParent(OCFile("/documents/report.pdf"), context) + } + assertTrue(savedFiles.isEmpty()) + } + + @Test + fun `successful fetch without a remote file throws`() { + givenServerResult("/documents/", serverResult(isSuccess = true, data = arrayListOf("not a remote file"))) + + assertThrows(RemoteOperationFailedException::class.java) { + manager.saveFileWithParent(OCFile("/documents/report.pdf"), context) + } + assertTrue(savedFiles.isEmpty()) + } + + @Test + fun `offline file in root queues no create folder operation`() { + givenLocalFolder(OCFile.ROOT_PATH, ROOT_ID) + + queueOfflineUpload("/report.pdf") + + assertEquals(listOf("/report.pdf"), queuedOperations.map { it.path }) + assertEquals(ROOT_ID, queuedOperations.single().parentOCFileId) + verifyNoServerFetch() + } + + @Test + fun `offline file in a locally stored folder queues no create folder operation`() { + givenLocalFolder(OCFile.ROOT_PATH, ROOT_ID) + givenLocalFolder("/documents/", LOCAL_PARENT_ID) + + queueOfflineUpload("/documents/report.pdf") + + assertEquals(listOf("/documents/report.pdf"), queuedOperations.map { it.path }) + assertEquals(LOCAL_PARENT_ID, queuedOperations.single().parentOCFileId) + verifyNoServerFetch() + } + + @Test + fun `missing parent of an offline file is queued as a create folder operation before the file`() { + givenLocalFolder(OCFile.ROOT_PATH, ROOT_ID) + + queueOfflineUpload("/documents/report.pdf") + + val (folderOperation, fileOperation) = queuedOperations + assertEquals("/documents/", folderOperation.path) + assertEquals("documents", folderOperation.filename) + assertEquals(ROOT_ID, folderOperation.parentOCFileId) + assertTrue(folderOperation.type is OfflineOperationType.CreateFolder) + assertTrue(fileOperation.type is OfflineOperationType.CreateFile) + assertEquals(localFiles.getValue("/documents/").fileId, fileOperation.parentOCFileId) + verifyNoServerFetch() + } + + @Test + fun `all missing ancestors of an offline file are queued from the top down`() { + givenLocalFolder(OCFile.ROOT_PATH, ROOT_ID) + + queueOfflineUpload("/a/b/c.txt") + + assertEquals(listOf("/a/", "/a/b/", "/a/b/c.txt"), queuedOperations.map { it.path }) + val (folderA, folderB, file) = queuedOperations + assertEquals(ROOT_ID, folderA.parentOCFileId) + assertEquals(localFiles.getValue("/a/").fileId, folderB.parentOCFileId) + assertEquals(localFiles.getValue("/a/b/").fileId, file.parentOCFileId) + verifyNoServerFetch() + } + + @Test + fun `missing ancestors are stored as pending local folders`() { + givenLocalFolder(OCFile.ROOT_PATH, ROOT_ID) + + queueOfflineUpload("/a/b/c.txt") + + val folderA = localFiles.getValue("/a/") + val folderB = localFiles.getValue("/a/b/") + assertTrue(folderA.isFolder && folderB.isFolder) + assertEquals(ROOT_ID, folderA.parentId) + assertEquals(folderA.fileId, folderB.parentId) + assertEquals(folderB.fileId, localFiles.getValue("/a/b/c.txt").parentId) + } + + @Test + fun `only folders below the deepest locally stored folder are queued`() { + givenLocalFolder(OCFile.ROOT_PATH, ROOT_ID) + givenLocalFolder("/documents/", LOCAL_PARENT_ID) + + queueOfflineUpload("/documents/x/y/z.txt") + + assertEquals( + listOf("/documents/x/", "/documents/x/y/", "/documents/x/y/z.txt"), + queuedOperations.map { it.path } + ) + assertEquals(LOCAL_PARENT_ID, queuedOperations.first().parentOCFileId) + } + + @Test + fun `offline files sharing a missing parent queue that folder only once`() { + givenLocalFolder(OCFile.ROOT_PATH, ROOT_ID) + + queueOfflineUpload("/documents/first.pdf", "/documents/second.pdf") + + assertEquals( + listOf("/documents/", "/documents/first.pdf", "/documents/second.pdf"), + queuedOperations.map { it.path } + ) + verifyNoServerFetch() + } + + private fun queueOfflineUpload(vararg remotePaths: String) { + val localPaths = remotePaths.map { LOCAL_UPLOAD_PATH + it }.toTypedArray() + manager.addCreateFileOfflineOperation(localPaths, arrayOf(*remotePaths)) + } + + private fun givenLocalFolder(path: String, id: Long) { + localFiles[path] = OCFile(path).apply { + fileId = id + parentId = if (path == OCFile.ROOT_PATH) 0L else ROOT_ID + mimeType = MimeType.DIRECTORY + } + } + + private fun givenServerFolder(path: String) { + val remoteFolder = RemoteFile(path).apply { mimeType = MimeType.DIRECTORY } + givenServerResult(path, serverResult(isSuccess = true, data = arrayListOf(remoteFolder))) + } + + private fun givenServerResult(path: String, result: RemoteOperationResult) { + every { + constructedWith(EqMatcher(path)).execute(any(), any()) + } returns result + } + + private fun serverResult( + isSuccess: Boolean, + data: ArrayList? = null, + exception: Exception? = null + ): RemoteOperationResult = mockk(relaxed = true) { + every { this@mockk.isSuccess } returns isSuccess + every { this@mockk.data } returns data + every { this@mockk.exception } returns exception + every { getLogMessage(any()) } returns LOG_MESSAGE + } + + private fun verifyNoServerFetch() { + verify(exactly = 0) { anyConstructed().execute(any(), any()) } + } +}