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 04055f375549..248ea0d7f681 100644 --- a/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java +++ b/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java @@ -112,6 +112,13 @@ public class FileDataStorageManager { private static final String JSON_EMPTY_ARRAY = "[]"; private static final long INTERNAL_TWO_WAY_SYNC_NEVER_SYNCED = 0L; + /** + * Serializes the "update if the row exists, insert otherwise" sequences of {@link #saveFile(OCFile)} and + * {@link #saveFolder(OCFile, List, Collection)}. The file table has no unique constraint on the remote path, so + * two concurrent writers (e.g. two refreshes of the same folder) would otherwise both insert the same new file. + */ + private static final Object FILE_TABLE_WRITE_LOCK = new Object(); + private final ContentResolver contentResolver; private final ContentProviderClient contentProviderClient; private final User user; @@ -572,6 +579,12 @@ public List getFolderImagesAndVideos(OCFile folder, boolean onlyOnDevice } public boolean saveFile(OCFile ocFile) { + synchronized (FILE_TABLE_WRITE_LOCK) { + return saveFileLocked(ocFile); + } + } + + private boolean saveFileLocked(OCFile ocFile) { Log_OC.d(TAG, "saving file " + ocFile.getFileName() + " into " + ocFile.getRemotePath()); boolean overridden = false; @@ -754,6 +767,12 @@ public void saveNewFile(OCFile newFile) { * @param filesToRemove */ public void saveFolder(OCFile folder, List updatedFiles, Collection filesToRemove) { + synchronized (FILE_TABLE_WRITE_LOCK) { + saveFolderLocked(folder, updatedFiles, filesToRemove); + } + } + + private void saveFolderLocked(OCFile folder, List updatedFiles, Collection filesToRemove) { Log_OC.d(TAG, "Saving folder " + folder.getRemotePath() + " with " + updatedFiles.size() + " children and " + filesToRemove.size() + " files to remove"); @@ -770,6 +789,7 @@ public void saveFolder(OCFile folder, List updatedFiles, Collection 0; } + /** + * Removes duplicate database entries, with the entries of their contents for folders. Local files are not + * touched: they belong to the entry that is kept for the same remote path. + */ + public void removeDuplicateEntries(Collection duplicates) { + for (OCFile duplicate : duplicates) { + Log_OC.w(TAG, "Removing duplicate entry " + duplicate.getFileId() + " of " + duplicate.getRemotePath()); + fileDao.deleteFolderWithDescendants(user.getAccountName(), duplicate.getFileId()); + } + } + private boolean removeLocalFolder(OCFile folder) { if (folder == null) { Log_OC.d(TAG, "removeLocalFolder: folder is null"); diff --git a/app/src/main/java/com/owncloud/android/operations/RefreshFolderOperation.java b/app/src/main/java/com/owncloud/android/operations/RefreshFolderOperation.java index dc27a5f236a3..49bfa7ecf475 100644 --- a/app/src/main/java/com/owncloud/android/operations/RefreshFolderOperation.java +++ b/app/src/main/java/com/owncloud/android/operations/RefreshFolderOperation.java @@ -58,6 +58,7 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; import androidx.localbroadcastmanager.content.LocalBroadcastManager; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; @@ -557,16 +558,24 @@ private void synchronizeData(List folderAndFiles) { // get current data about local contents of the folder to synchronize Map localFilesMap; + List duplicateLocalFiles = new ArrayList<>(); E2EVersion e2EVersion = fileDataStorageManager.getE2EEVersionObject(user); if (object instanceof DecryptedFolderMetadataFileV1 metadataFileV1) { - localFilesMap = prefillLocalFilesMap(metadataFileV1, fileDataStorageManager.getFolderContent(mLocalFolder, false)); + localFilesMap = prefillLocalFilesMap(metadataFileV1, + fileDataStorageManager.getFolderContent(mLocalFolder, false), + duplicateLocalFiles); } else { - localFilesMap = prefillLocalFilesMap(object, fileDataStorageManager.getFolderContent(mLocalFolder, false)); + localFilesMap = prefillLocalFilesMap(object, + fileDataStorageManager.getFolderContent(mLocalFolder, false), + duplicateLocalFiles); if (object instanceof DecryptedFolderMetadataFile metadataFile) { mLocalFolder.setE2eCounter(metadataFile.getMetadata().getCounter()); } } + // duplicate entries are never matched with a remote file, so they would stay forever + fileDataStorageManager.removeDuplicateEntries(duplicateLocalFiles); + // loop to update every child OCFile remoteFile; OCFile localFile; @@ -792,6 +801,19 @@ private void setLocalFileDataOnUpdatedFile(OCFile remoteFile, OCFile localFile, @NonNull @SuppressFBWarnings("OCP") public static Map prefillLocalFilesMap(Object metadata, List localFiles) { + return prefillLocalFilesMap(metadata, localFiles, new ArrayList<>()); + } + + /** + * Maps the remote path of every local file to the file. Local files with the same remote path as another one are + * duplicate database entries: only the one chosen by {@link #preferredLocalFile(OCFile, OCFile)} is mapped, the + * others are added to {@code duplicates}. + */ + @NonNull + @SuppressFBWarnings("OCP") + public static Map prefillLocalFilesMap(Object metadata, + List localFiles, + List duplicates) { Map localFilesMap = Maps.newHashMapWithExpectedSize(localFiles.size()); for (OCFile file : localFiles) { @@ -803,11 +825,32 @@ public static Map prefillLocalFilesMap(Object metadata, List folderAndFiles) throws OperationCancel } // get current data about local contents of the folder to synchronize - Map localFilesMap = RefreshFolderOperation.prefillLocalFilesMap(object,storageManager.getFolderContent(mLocalFolder, false)); + List duplicateLocalFiles = new ArrayList<>(); + Map localFilesMap = RefreshFolderOperation.prefillLocalFilesMap(object, + storageManager.getFolderContent(mLocalFolder, false), + duplicateLocalFiles); + + // duplicate entries are never matched with a remote file, so they would stay forever + storageManager.removeDuplicateEntries(duplicateLocalFiles); // loop to synchronize every child List updatedFiles = new ArrayList<>(folderAndFiles.size() - 1); diff --git a/app/src/test/java/com/owncloud/android/operations/PrefillLocalFilesMapTest.kt b/app/src/test/java/com/owncloud/android/operations/PrefillLocalFilesMapTest.kt new file mode 100644 index 000000000000..fb88ee976bd5 --- /dev/null +++ b/app/src/test/java/com/owncloud/android/operations/PrefillLocalFilesMapTest.kt @@ -0,0 +1,90 @@ +/* + * Nextcloud - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +package com.owncloud.android.operations + +import com.owncloud.android.datamodel.OCFile +import org.junit.Assert.assertEquals +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Test +import org.mockito.kotlin.doReturn +import org.mockito.kotlin.mock + +class PrefillLocalFilesMapTest { + + private fun localFile(id: Long, path: String, onDevice: Boolean = false): OCFile = mock { + on { fileId } doReturn id + on { remotePath } doReturn path + on { existsOnDevice() } doReturn onDevice + } + + @Test + fun withoutDuplicates_mapsEveryFile() { + val photos = localFile(1, "/Photos/") + val notes = localFile(2, "/Notes/") + val duplicates = mutableListOf() + + val map = RefreshFolderOperation.prefillLocalFilesMap(null, listOf(photos, notes), duplicates) + + assertEquals(mapOf("/Photos/" to photos, "/Notes/" to notes), map) + assertTrue(duplicates.isEmpty()) + } + + @Test + fun withDuplicates_keepsLowestIdAndReportsTheOthers() { + val newer = localFile(7, "/Photos/") + val oldest = localFile(3, "/Photos/") + val middle = localFile(5, "/Photos/") + val duplicates = mutableListOf() + + val map = RefreshFolderOperation.prefillLocalFilesMap(null, listOf(newer, oldest, middle), duplicates) + + assertSame(oldest, map["/Photos/"]) + assertEquals(setOf(newer, middle), duplicates.toSet()) + assertEquals(2, duplicates.size) + } + + @Test + fun withDuplicates_keepsEntryWithLocalCopy() { + val oldest = localFile(3, "/Documents/report.pdf") + val downloaded = localFile(9, "/Documents/report.pdf", onDevice = true) + val duplicates = mutableListOf() + + val map = RefreshFolderOperation.prefillLocalFilesMap(null, listOf(oldest, downloaded), duplicates) + + assertSame(downloaded, map["/Documents/report.pdf"]) + assertEquals(listOf(oldest), duplicates) + } + + @Test + fun encryptedFolder_sameKeyButDifferentRemotePath_isNoDuplicate() { + val first: OCFile = mock { + on { remotePath } doReturn "/secret/aaa" + on { parentRemotePath } doReturn "/secret/" + } + val second: OCFile = mock { + on { remotePath } doReturn "/secret/bbb" + on { parentRemotePath } doReturn "/secret/" + } + val duplicates = mutableListOf() + + RefreshFolderOperation.prefillLocalFilesMap(Any(), listOf(first, second), duplicates) + + assertTrue(duplicates.isEmpty()) + } + + @Test + fun withoutDuplicatesList_mapsTheSameEntry() { + val newer = localFile(7, "/zotero/") + val oldest = localFile(3, "/zotero/") + + val map = RefreshFolderOperation.prefillLocalFilesMap(null, listOf(newer, oldest)) + + assertSame(oldest, map["/zotero/"]) + } +}