From e49509e7bac0b1917d9583c8b0cf48c386166b2a Mon Sep 17 00:00:00 2001 From: alperozturk Date: Tue, 16 Dec 2025 12:04:20 +0100 Subject: [PATCH 1/3] fix: upsert single file Signed-off-by: alperozturk --- .../providers/FileContentProvider.java | 74 ++++++++++++------- 1 file changed, 46 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/com/owncloud/android/providers/FileContentProvider.java b/app/src/main/java/com/owncloud/android/providers/FileContentProvider.java index 19e5fc0b2f4a..c4c4c90d0697 100644 --- a/app/src/main/java/com/owncloud/android/providers/FileContentProvider.java +++ b/app/src/main/java/com/owncloud/android/providers/FileContentProvider.java @@ -42,6 +42,7 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; +import androidx.sqlite.db.SimpleSQLiteQuery; import androidx.sqlite.db.SupportSQLiteDatabase; import androidx.sqlite.db.SupportSQLiteOpenHelper; import androidx.sqlite.db.SupportSQLiteQuery; @@ -234,34 +235,7 @@ private Uri insert(SupportSQLiteDatabase db, Uri uri, ContentValues values) { switch (mUriMatcher.match(uri)) { case ROOT_DIRECTORY: case SINGLE_FILE: - String where = ProviderTableMeta.FILE_PATH + "=? AND " + ProviderTableMeta.FILE_ACCOUNT_OWNER + "=?"; - - String remotePath = values.getAsString(ProviderTableMeta.FILE_PATH); - String accountName = values.getAsString(ProviderTableMeta.FILE_ACCOUNT_OWNER); - String[] whereArgs = {remotePath, accountName}; - - Cursor doubleCheck = query(db, uri, PROJECTION_FILE_PATH_AND_OWNER, where, whereArgs, null); - // ugly patch; serious refactoring is needed to reduce work in - // FileDataStorageManager and bring it to FileContentProvider - if (!doubleCheck.moveToFirst()) { - doubleCheck.close(); - long rowId = db.insert(ProviderTableMeta.FILE_TABLE_NAME, SQLiteDatabase.CONFLICT_REPLACE, values); - if (rowId > 0) { - return ContentUris.withAppendedId(ProviderTableMeta.CONTENT_URI_FILE, rowId); - } else { - throw new SQLException(ERROR + uri); - } - } else { - // file is already inserted; race condition, let's avoid a duplicated entry - Uri insertedFileUri = ContentUris.withAppendedId( - ProviderTableMeta.CONTENT_URI_FILE, - doubleCheck.getLong(doubleCheck.getColumnIndexOrThrow(ProviderTableMeta._ID)) - ); - doubleCheck.close(); - - return insertedFileUri; - } - + return upsertSingleFile(db, uri, values); case SHARES: Uri insertedShareUri; long idShares = db.insert(ProviderTableMeta.OCSHARES_TABLE_NAME, SQLiteDatabase.CONFLICT_REPLACE, values); @@ -343,6 +317,50 @@ private Uri insert(SupportSQLiteDatabase db, Uri uri, ContentValues values) { } } + private Uri upsertSingleFile(SupportSQLiteDatabase db, Uri uri, ContentValues values) { + String filePath = values.getAsString(ProviderTableMeta.FILE_PATH); + String accountOwner = values.getAsString(ProviderTableMeta.FILE_ACCOUNT_OWNER); + + String where = ProviderTableMeta.FILE_PATH + "=? AND " + ProviderTableMeta.FILE_ACCOUNT_OWNER + "=?"; + String[] whereArgs = {filePath, accountOwner}; + + // Try insert first, ignore conflict + long rowId = db.insert( + ProviderTableMeta.FILE_TABLE_NAME, + SQLiteDatabase.CONFLICT_IGNORE, + values); + + if (rowId <= 0) { + // Already exists: update + int count = db.update( + ProviderTableMeta.FILE_TABLE_NAME, + SQLiteDatabase.CONFLICT_NONE, + values, + where, + whereArgs); + + if (count == 0) { + throw new SQLException("Failed to update existing file: " + uri); + } + + try (Cursor cursor = db.query( + new SimpleSQLiteQuery( + "SELECT " + ProviderTableMeta._ID + + " FROM " + ProviderTableMeta.FILE_TABLE_NAME + + " WHERE " + where, + whereArgs + ))) { + if (cursor.moveToFirst()) { + rowId = cursor.getLong(0); + } else { + throw new SQLException("Failed to fetch ID after update: " + uri); + } + } + } + + return ContentUris.withAppendedId(ProviderTableMeta.CONTENT_URI_FILE, rowId); + } + private void updateFilesTableAccordingToShareInsertion(SupportSQLiteDatabase db, ContentValues newShare) { ContentValues fileValues = new ContentValues(); Integer shareTypeValue = newShare.getAsInteger(ProviderTableMeta.OCSHARES_SHARE_TYPE); From f2803b89b3153671617c0493bad4f46280078b11 Mon Sep 17 00:00:00 2001 From: alperozturk Date: Tue, 16 Dec 2025 12:43:33 +0100 Subject: [PATCH 2/3] add tests for upsert file Signed-off-by: alperozturk --- .../providers/FileContentProviderTests.kt | 157 ++++++++++++++++++ .../providers/FileContentProvider.java | 2 +- 2 files changed, 158 insertions(+), 1 deletion(-) create mode 100644 app/src/androidTest/java/com/owncloud/android/providers/FileContentProviderTests.kt diff --git a/app/src/androidTest/java/com/owncloud/android/providers/FileContentProviderTests.kt b/app/src/androidTest/java/com/owncloud/android/providers/FileContentProviderTests.kt new file mode 100644 index 000000000000..6064af2d56fe --- /dev/null +++ b/app/src/androidTest/java/com/owncloud/android/providers/FileContentProviderTests.kt @@ -0,0 +1,157 @@ +/* + * Nextcloud - Android Client + * + * SPDX-FileCopyrightText: 2025 Alper Ozturk + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +package com.owncloud.android.providers + +import android.content.ContentValues +import android.content.ContentUris +import android.net.Uri +import androidx.sqlite.db.SimpleSQLiteQuery +import androidx.sqlite.db.SupportSQLiteDatabase +import com.owncloud.android.db.ProviderMeta +import io.mockk.Runs +import io.mockk.every +import io.mockk.just +import io.mockk.mockk +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test + +class FileContentProviderTests { + + private lateinit var provider: FileContentProvider + private lateinit var db: SupportSQLiteDatabase + + @Before + fun setup() { + provider = FileContentProvider() + db = mockk() + } + + @Test + fun insertNewFileShouldReturnNewId() { + val values = ContentValues().apply { + put(ProviderMeta.ProviderTableMeta.FILE_PATH, "/path/to/file.txt") + put(ProviderMeta.ProviderTableMeta.FILE_ACCOUNT_OWNER, "user@example.com") + } + + // Mock insert to return new ID + every { db.insert(any(), any(), any()) } returns 42L + + val result: Uri = provider.upsertSingleFile( + db, + ProviderMeta.ProviderTableMeta.CONTENT_URI_FILE, + values + ) + + assertEquals( + ContentUris.withAppendedId(ProviderMeta.ProviderTableMeta.CONTENT_URI_FILE, 42), + result + ) + } + + @Test + fun updateExistingFileShouldReturnSameId() { + val values = ContentValues().apply { + put(ProviderMeta.ProviderTableMeta.FILE_PATH, "/path/to/file.txt") + put(ProviderMeta.ProviderTableMeta.FILE_ACCOUNT_OWNER, "user@example.com") + } + + // Simulate insert conflict + every { db.insert(ProviderMeta.ProviderTableMeta.FILE_TABLE_NAME, any(), values) } returns -1L + + // Simulate update returning 1 row affected + every { + db.update( + ProviderMeta.ProviderTableMeta.FILE_TABLE_NAME, + any(), + values, + any(), + any() + ) + } returns 1 + + // Mock cursor to return ID 99 + val cursor = mockk() + every { cursor.moveToFirst() } returns true + every { cursor.getLong(0) } returns 99L + every { cursor.close() } just Runs + + every { db.query(any()) } returns cursor + + val result: Uri = provider.upsertSingleFile( + db, + ProviderMeta.ProviderTableMeta.CONTENT_URI_FILE, + values + ) + + assertEquals(ContentUris.withAppendedId(ProviderMeta.ProviderTableMeta.CONTENT_URI_FILE, 99), result) + + cursor.close() + } + + @Test + fun testConcurrentUpserts() = runBlocking { + val values = ContentValues().apply { + put(ProviderMeta.ProviderTableMeta.FILE_PATH, "/path/to/file.txt") + put(ProviderMeta.ProviderTableMeta.FILE_ACCOUNT_OWNER, "user@example.com") + } + + // shared state to simulate race + val inserted = mutableListOf() + + // mock insert: fail first call, succeed second + every { db.insert(any(), any(), any()) } answers { + synchronized(inserted) { + if (inserted.isEmpty()) { + // first thread "fails" insert which means already existing file id will be returned + inserted.add(-1L) + -1L + } else { + // second thread "succeeds" it will update existing one + inserted.add(42L) + 42L + } + } + } + + // mock update only one row should be affected + every { db.update(any(), any(), any(), any(), any()) } returns 1 + + // mock query existing file id will return 99 + val cursor = mockk() + every { cursor.moveToFirst() } returns true + every { cursor.getLong(0) } returns 99L + every { cursor.close() } just Runs + every { db.query(any()) } returns cursor + + // launch two coroutines simulating concurrent threads + val results = mutableListOf() + coroutineScope { + val job1 = + async { + results.add(provider.upsertSingleFile(db, ProviderMeta.ProviderTableMeta.CONTENT_URI_FILE, values)) + } + val job2 = + async { + results.add(provider.upsertSingleFile(db, ProviderMeta.ProviderTableMeta.CONTENT_URI_FILE, values)) + } + awaitAll(job1, job2) + } + + // both URIs should be correct (one updated, one inserted) + assertTrue(results.contains(ContentUris.withAppendedId(ProviderMeta.ProviderTableMeta.CONTENT_URI_FILE, 99))) + assertTrue(results.contains(ContentUris.withAppendedId(ProviderMeta.ProviderTableMeta.CONTENT_URI_FILE, 42))) + + cursor.close() + } +} diff --git a/app/src/main/java/com/owncloud/android/providers/FileContentProvider.java b/app/src/main/java/com/owncloud/android/providers/FileContentProvider.java index c4c4c90d0697..47ecdab72fcd 100644 --- a/app/src/main/java/com/owncloud/android/providers/FileContentProvider.java +++ b/app/src/main/java/com/owncloud/android/providers/FileContentProvider.java @@ -317,7 +317,7 @@ private Uri insert(SupportSQLiteDatabase db, Uri uri, ContentValues values) { } } - private Uri upsertSingleFile(SupportSQLiteDatabase db, Uri uri, ContentValues values) { + public Uri upsertSingleFile(SupportSQLiteDatabase db, Uri uri, ContentValues values) { String filePath = values.getAsString(ProviderTableMeta.FILE_PATH); String accountOwner = values.getAsString(ProviderTableMeta.FILE_ACCOUNT_OWNER); From a6916c574a5c965093e70482f9a253ace6a1de02 Mon Sep 17 00:00:00 2001 From: alperozturk Date: Tue, 16 Dec 2025 13:14:16 +0100 Subject: [PATCH 3/3] add tests for upsert file Signed-off-by: alperozturk --- .../com/owncloud/android/providers/FileContentProviderTests.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/app/src/androidTest/java/com/owncloud/android/providers/FileContentProviderTests.kt b/app/src/androidTest/java/com/owncloud/android/providers/FileContentProviderTests.kt index 6064af2d56fe..8027feb9bd08 100644 --- a/app/src/androidTest/java/com/owncloud/android/providers/FileContentProviderTests.kt +++ b/app/src/androidTest/java/com/owncloud/android/providers/FileContentProviderTests.kt @@ -26,6 +26,7 @@ import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test +@Suppress("MagicNumber") class FileContentProviderTests { private lateinit var provider: FileContentProvider