Improve closing of content provider in verify account owner test (#1838)

* Optimize imports * Remove the ignore annotation * Move provider use out of verify method Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Remove unnecessary provider.use blocks Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Add spaces Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Rename lambda param provider to client in LocalDataStore implementations Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Enhance kdoc Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Improve provider client usage Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Replace calling apply with assignment Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Remove whitespace Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Add nullable returns even though they never return null Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> * Apply WillNotClose annotation to client parameter instead of method Signed-off-by: Sunik Kupfer <kupfer@bitfire.at> --------- Signed-off-by: Sunik Kupfer <kupfer@bitfire.at>
2025-12-23 23:17:50 -05:00 · 2025-12-02 10:34:39 +01:00
parent 88928792af
commit 2d10cbb07d
9 changed files with 69 additions and 66 deletions
--- a/app/src/androidTest/kotlin/at/bitfire/davdroid/repository/AccountRepositoryTest.kt
+++ b/app/src/androidTest/kotlin/at/bitfire/davdroid/repository/AccountRepositoryTest.kt
@@ -174,7 +174,7 @@ class AccountRepositoryTest {
        accountRepository.rename(account.name, newName)

        val newAccount = accountRepository.fromName(newName)
-        coVerify { localAddressBookStore.updateAccount(account, newAccount) }
+        coVerify { localAddressBookStore.updateAccount(account, newAccount, any()) }
    }

    @Test
@@ -182,7 +182,7 @@ class AccountRepositoryTest {
        accountRepository.rename(account.name, newName)

        val newAccount = accountRepository.fromName(newName)
-        coVerify { localCalendarStore.updateAccount(account, newAccount) }
+        coVerify { localCalendarStore.updateAccount(account, newAccount, any()) }
    }

    @Test
@@ -191,7 +191,8 @@ class AccountRepositoryTest {
        every { tasksAppManager.getDataStore() } returns mockDataStore
        accountRepository.rename(account.name, newName)

-        coVerify { mockDataStore.updateAccount(account, accountRepository.fromName(newName)) }
+        val newAccount = accountRepository.fromName(newName)
+        coVerify { mockDataStore.updateAccount(account, newAccount, any()) }
    }

    @Test
--- a/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalCalendarStoreTest.kt
+++ b/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalCalendarStoreTest.kt
@@ -21,7 +21,6 @@ import dagger.hilt.android.testing.HiltAndroidTest
 import org.junit.After
 import org.junit.Assert.assertEquals
 import org.junit.Before
-import org.junit.Ignore
 import org.junit.Rule
 import org.junit.Test
 import org.junit.rules.TestRule
@@ -62,30 +61,30 @@ class LocalCalendarStoreTest {
    }


-    @Ignore("Sometimes failing, see https://github.com/bitfireAT/davx5-ose/issues/1828")
    @Test
    fun testUpdateAccount_updatesOwnerAccount() {
        // Verify initial state
-        verifyOwnerAccountIs("InitialAccountName")
+        verifyOwnerAccountIs(provider, "InitialAccountName")

        // Rename account
        val oldAccount = account
        account = TestAccount.rename(account, "ChangedAccountName")

        // Update account name in local calendar
-        localCalendarStore.updateAccount(oldAccount, account)
+        localCalendarStore.updateAccount(oldAccount, account, provider)

        // Verify [Calendar.OWNER_ACCOUNT] of local calendar was updated
-        verifyOwnerAccountIs("ChangedAccountName")
+        verifyOwnerAccountIs(provider, "ChangedAccountName")
+
    }


    // helpers

-    private fun createCalendarForAccount(account: Account): Uri {
-        var uri: Uri? = null
-        provider.use { providerClient ->
-            val values = contentValuesOf(
+    private fun createCalendarForAccount(account: Account): Uri =
+         provider.insert(
+            Calendars.CONTENT_URI.asSyncAdapter(account),
+            contentValuesOf(
                Calendars.ACCOUNT_NAME to account.name,
                Calendars.ACCOUNT_TYPE to account.type,
                Calendars.OWNER_ACCOUNT to account.name,
@@ -94,17 +93,10 @@ class LocalCalendarStoreTest {
                Calendars._SYNC_ID to 999,
                Calendars.CALENDAR_DISPLAY_NAME to "displayName",
            )
+        )!!.asSyncAdapter(account)

-            uri = providerClient.insert(
-                Calendars.CONTENT_URI.asSyncAdapter(account),
-                values
-            )!!.asSyncAdapter(account)
-        }
-        return uri!!
-    }
-
-    private fun verifyOwnerAccountIs(expectedOwnerAccount: String) = provider.use {
-        it.query(
+    private fun verifyOwnerAccountIs(provider: ContentProviderClient, expectedOwnerAccount: String) {
+        provider.query(
            calendarUri,
            arrayOf(Calendars.OWNER_ACCOUNT),
            "${Calendars.ACCOUNT_NAME}=?",
--- a/app/src/androidTest/kotlin/at/bitfire/davdroid/sync/SyncerTest.kt
+++ b/app/src/androidTest/kotlin/at/bitfire/davdroid/sync/SyncerTest.kt
@@ -194,7 +194,7 @@ class SyncerTest {
        }

        override fun create(
-            provider: ContentProviderClient,
+            client: ContentProviderClient,
            fromCollection: Collection
        ): LocalTestCollection? {
            throw NotImplementedError()
@@ -202,13 +202,13 @@ class SyncerTest {

        override fun getAll(
            account: Account,
-            provider: ContentProviderClient
+            client: ContentProviderClient
        ): List<LocalTestCollection> {
            throw NotImplementedError()
        }

        override fun update(
-            provider: ContentProviderClient,
+            client: ContentProviderClient,
            localCollection: LocalTestCollection,
            fromCollection: Collection
        ) {
@@ -219,7 +219,7 @@ class SyncerTest {
            throw NotImplementedError()
        }

-        override fun updateAccount(oldAccount: Account, newAccount: Account) {
+        override fun updateAccount(oldAccount: Account, newAccount: Account, client: ContentProviderClient?) {
            throw NotImplementedError()
        }

--- a/app/src/main/kotlin/at/bitfire/davdroid/repository/AccountRepository.kt
+++ b/app/src/main/kotlin/at/bitfire/davdroid/repository/AccountRepository.kt
@@ -218,22 +218,27 @@ class AccountRepository @Inject constructor(

            try {
                // update address books
-                localAddressBookStore.get().updateAccount(oldAccount, newAccount)
+                localAddressBookStore.get().updateAccount(oldAccount, newAccount, null)
            } catch (e: Exception) {
                logger.log(Level.WARNING, "Couldn't change address books to renamed account", e)
            }

            try {
                // update calendar events
-                localCalendarStore.get().updateAccount(oldAccount, newAccount)
+                val store = localCalendarStore.get()
+                store.acquireContentProvider(true)?.use { client ->
+                    store.updateAccount(oldAccount, newAccount, client)
+                }
            } catch (e: Exception) {
                logger.log(Level.WARNING, "Couldn't change calendars to renamed account", e)
            }

            try {
                // update account_name of local tasks
-                val dataStore = tasksAppManager.get().getDataStore()
-                dataStore?.updateAccount(oldAccount, newAccount)
+                val store = tasksAppManager.get().getDataStore()
+                store?.acquireContentProvider(true)?.use { client ->
+                    store.updateAccount(oldAccount, newAccount, client)
+                }
            } catch (e: Exception) {
                logger.log(Level.WARNING, "Couldn't change task lists to renamed account", e)
            }
--- a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBookStore.kt
+++ b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBookStore.kt
@@ -87,7 +87,7 @@ class LocalAddressBookStore @Inject constructor(
            /* return */ null
    }

-    override fun create(provider: ContentProviderClient, fromCollection: Collection): LocalAddressBook? {
+    override fun create(client: ContentProviderClient, fromCollection: Collection): LocalAddressBook? {
        val service = serviceRepository.getBlocking(fromCollection.serviceId) ?: throw IllegalArgumentException("Couldn't fetch DB service from collection")
        val account = Account(service.accountName, context.getString(R.string.account_type))

@@ -98,7 +98,7 @@ class LocalAddressBookStore @Inject constructor(
            id = fromCollection.id
        ) ?: return null

-        val addressBook = localAddressBookFactory.create(account, addressBookAccount, provider)
+        val addressBook = localAddressBookFactory.create(account, addressBookAccount, client)

        // update settings
        addressBook.updateSyncFrameworkSettings()
@@ -125,12 +125,12 @@ class LocalAddressBookStore @Inject constructor(
        return addressBookAccount
    }

-    override fun getAll(account: Account, provider: ContentProviderClient): List<LocalAddressBook> =
+    override fun getAll(account: Account, client: ContentProviderClient): List<LocalAddressBook> =
        getAddressBookAccounts(account).map { addressBookAccount ->
-            localAddressBookFactory.create(account, addressBookAccount, provider)
+            localAddressBookFactory.create(account, addressBookAccount, client)
        }

-    override fun update(provider: ContentProviderClient, localCollection: LocalAddressBook, fromCollection: Collection) {
+    override fun update(client: ContentProviderClient, localCollection: LocalAddressBook, fromCollection: Collection) {
        var currentAccount = localCollection.addressBookAccount
        logger.log(Level.INFO, "Updating local address book $currentAccount from collection $fromCollection")

@@ -167,8 +167,9 @@ class LocalAddressBookStore @Inject constructor(
     *
     * @param oldAccount    The old account
     * @param newAccount    The new account
+     * @param client        content provider client (not needed/does not exist for address books)
     */
-    override fun updateAccount(oldAccount: Account, newAccount: Account) {
+    override fun updateAccount(oldAccount: Account, newAccount: Account, client: ContentProviderClient?) {
        val accountManager = AccountManager.get(context)
        accountManager.getAccountsByType(context.getString(R.string.account_type_address_book))
            .filter { addressBookAccount ->
--- a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalCalendarStore.kt
+++ b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalCalendarStore.kt
@@ -26,6 +26,7 @@ import at.bitfire.synctools.storage.calendar.AndroidCalendarProvider
 import dagger.hilt.android.qualifiers.ApplicationContext
 import java.util.logging.Level
 import java.util.logging.Logger
+import javax.annotation.WillNotClose
 import javax.inject.Inject

 class LocalCalendarStore @Inject constructor(
@@ -138,7 +139,9 @@ class LocalCalendarStore @Inject constructor(
        return values
    }

-    override fun updateAccount(oldAccount: Account, newAccount: Account) {
+    override fun updateAccount(oldAccount: Account, newAccount: Account, @WillNotClose client: ContentProviderClient?) {
+        if (client == null)
+            return
        val values = contentValuesOf(
            // Account name to be changed
            Calendars.ACCOUNT_NAME to newAccount.name,
@@ -147,9 +150,7 @@ class LocalCalendarStore @Inject constructor(
            Calendars.OWNER_ACCOUNT to newAccount.name
        )
        val uri = Calendars.CONTENT_URI.asSyncAdapter(oldAccount)
-        context.contentResolver.acquireContentProviderClient(CalendarContract.AUTHORITY)?.use {
-            it.update(uri, values, "${Calendars.ACCOUNT_NAME}=?", arrayOf(oldAccount.name))
-        }
+        client.update(uri, values, "${Calendars.ACCOUNT_NAME}=?", arrayOf(oldAccount.name))
    }

    override fun delete(localCollection: LocalCalendar) {
--- a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalDataStore.kt
+++ b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalDataStore.kt
@@ -7,6 +7,7 @@ package at.bitfire.davdroid.resource
 import android.accounts.Account
 import android.content.ContentProviderClient
 import at.bitfire.davdroid.db.Collection
+import javax.annotation.WillNotClose

 /**
 * Represents a local data store for a specific collection type.
@@ -76,7 +77,8 @@ interface LocalDataStore<T: LocalCollection<*>> {
     *
     * @param oldAccount The old account.
     * @param newAccount The new account.
+     * @param client Content provider client for the local data store type or *null* when not needed for that data type.
     */
-    fun updateAccount(oldAccount: Account, newAccount: Account)
+    fun updateAccount(oldAccount: Account, newAccount: Account, @WillNotClose client: ContentProviderClient?)

 }
--- a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalJtxCollectionStore.kt
+++ b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalJtxCollectionStore.kt
@@ -18,11 +18,11 @@ import at.bitfire.davdroid.repository.PrincipalRepository
 import at.bitfire.davdroid.settings.AccountSettings
 import at.bitfire.davdroid.util.DavUtils.lastSegment
 import at.bitfire.ical4android.JtxCollection
-import at.bitfire.ical4android.TaskProvider
 import at.techbee.jtx.JtxContract
 import at.techbee.jtx.JtxContract.asSyncAdapter
 import dagger.hilt.android.qualifiers.ApplicationContext
 import java.util.logging.Logger
+import javax.annotation.WillNotClose
 import javax.inject.Inject

 class LocalJtxCollectionStore @Inject constructor(
@@ -46,7 +46,7 @@ class LocalJtxCollectionStore @Inject constructor(
            /* return */ null
    }

-    override fun create(provider: ContentProviderClient, fromCollection: Collection): LocalJtxCollection? {
+    override fun create(client: ContentProviderClient, fromCollection: Collection): LocalJtxCollection {
        val service = serviceDao.get(fromCollection.serviceId) ?: throw IllegalArgumentException("Couldn't fetch DB service from collection")
        val account = Account(service.accountName, context.getString(R.string.account_type))

@@ -63,8 +63,8 @@ class LocalJtxCollectionStore @Inject constructor(
            withColor = true
        )

-        val uri = JtxCollection.create(account, provider, values)
-        return LocalJtxCollection(account, provider, ContentUris.parseId(uri))
+        val uri = JtxCollection.create(account, client, values)
+        return LocalJtxCollection(account, client, ContentUris.parseId(uri))
    }

    private fun valuesFromCollection(info: Collection, account: Account, withColor: Boolean): ContentValues {
@@ -94,21 +94,21 @@ class LocalJtxCollectionStore @Inject constructor(
        }
    }

-    override fun getAll(account: Account, provider: ContentProviderClient): List<LocalJtxCollection> =
-        JtxCollection.find(account, provider, context, LocalJtxCollection.Factory, null, null)
+    override fun getAll(account: Account, client: ContentProviderClient): List<LocalJtxCollection> =
+        JtxCollection.find(account, client, context, LocalJtxCollection.Factory, null, null)

-    override fun update(provider: ContentProviderClient, localCollection: LocalJtxCollection, fromCollection: Collection) {
+    override fun update(client: ContentProviderClient, localCollection: LocalJtxCollection, fromCollection: Collection) {
        val accountSettings = accountSettingsFactory.create(localCollection.account)
        val values = valuesFromCollection(fromCollection, account = localCollection.account, withColor = accountSettings.getManageCalendarColors())
        localCollection.update(values)
    }

-    override fun updateAccount(oldAccount: Account, newAccount: Account) {
-        TaskProvider.acquire(context, TaskProvider.ProviderName.JtxBoard)?.use { provider ->
-            val values = contentValuesOf(JtxContract.JtxCollection.ACCOUNT_NAME to newAccount.name)
-            val uri = JtxContract.JtxCollection.CONTENT_URI.asSyncAdapter(oldAccount)
-            provider.client.update(uri, values, "${JtxContract.JtxCollection.ACCOUNT_NAME}=?", arrayOf(oldAccount.name))
-        }
+    override fun updateAccount(oldAccount: Account, newAccount: Account, @WillNotClose client: ContentProviderClient?) {
+        if (client == null)
+            return
+        val values = contentValuesOf(JtxContract.JtxCollection.ACCOUNT_NAME to newAccount.name)
+        val uri = JtxContract.JtxCollection.CONTENT_URI.asSyncAdapter(oldAccount)
+        client.update(uri, values, "${JtxContract.JtxCollection.ACCOUNT_NAME}=?", arrayOf(oldAccount.name))
    }

    override fun delete(localCollection: LocalJtxCollection) {
--- a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalTaskListStore.kt
+++ b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalTaskListStore.kt
@@ -28,6 +28,7 @@ import org.dmfs.tasks.contract.TaskContract.TaskLists
 import org.dmfs.tasks.contract.TaskContract.Tasks
 import java.util.logging.Level
 import java.util.logging.Logger
+import javax.annotation.WillNotClose

 class LocalTaskListStore @AssistedInject constructor(
    @Assisted private val providerName: TaskProvider.ProviderName,
@@ -56,13 +57,13 @@ class LocalTaskListStore @AssistedInject constructor(
            /* return */ null
    }

-    override fun create(provider: ContentProviderClient, fromCollection: Collection): LocalTaskList? {
+    override fun create(client: ContentProviderClient, fromCollection: Collection): LocalTaskList? {
        val service = serviceDao.get(fromCollection.serviceId) ?: throw IllegalArgumentException("Couldn't fetch DB service from collection")
        val account = Account(service.accountName, context.getString(R.string.account_type))

        logger.log(Level.INFO, "Adding local task list", fromCollection)
-        val uri = create(account, provider, providerName, fromCollection)
-        return DmfsTaskList.findByID(account, provider, providerName, LocalTaskList.Factory, ContentUris.parseId(uri))
+        val uri = create(account, client, providerName, fromCollection)
+        return DmfsTaskList.findByID(account, client, providerName, LocalTaskList.Factory, ContentUris.parseId(uri))
    }

    private fun create(account: Account, provider: ContentProviderClient, providerName: TaskProvider.ProviderName, fromCollection: Collection): Uri {
@@ -100,21 +101,21 @@ class LocalTaskListStore @AssistedInject constructor(
        return values
    }

-    override fun getAll(account: Account, provider: ContentProviderClient) =
-        DmfsTaskList.find(account, LocalTaskList.Factory, provider, providerName, null, null)
+    override fun getAll(account: Account, client: ContentProviderClient) =
+        DmfsTaskList.find(account, LocalTaskList.Factory, client, providerName, null, null)

-    override fun update(provider: ContentProviderClient, localCollection: LocalTaskList, fromCollection: Collection) {
+    override fun update(client: ContentProviderClient, localCollection: LocalTaskList, fromCollection: Collection) {
        logger.log(Level.FINE, "Updating local task list ${fromCollection.url}", fromCollection)
        val accountSettings = accountSettingsFactory.create(localCollection.account)
        localCollection.update(valuesFromCollectionInfo(fromCollection, withColor = accountSettings.getManageCalendarColors()))
    }

-    override fun updateAccount(oldAccount: Account, newAccount: Account) {
-        TaskProvider.acquire(context, providerName)?.use { provider ->
-            val values = contentValuesOf(Tasks.ACCOUNT_NAME to newAccount.name)
-            val uri = Tasks.getContentUri(providerName.authority)
-            provider.client.update(uri, values, "${Tasks.ACCOUNT_NAME}=?", arrayOf(oldAccount.name))
-        }
+    override fun updateAccount(oldAccount: Account, newAccount: Account, @WillNotClose client: ContentProviderClient?) {
+        if (client == null)
+            return
+        val values = contentValuesOf(Tasks.ACCOUNT_NAME to newAccount.name)
+        val uri = Tasks.getContentUri(providerName.authority)
+        client.update(uri, values, "${Tasks.ACCOUNT_NAME}=?", arrayOf(oldAccount.name))
    }

    override fun delete(localCollection: LocalTaskList) {